Re: [PATCH v5 02/12] libata: bind the Linux device tree to theACPI device tree
From: Lin Ming
Date: Thu Jun 21 2012 - 02:37:12 EST
On Wed, 2012-06-20 at 20:59 -0700, Dan Williams wrote:
> On Wed, Jun 20, 2012 at 6:26 PM, Lin Ming <ming.m.lin@xxxxxxxxx> wrote:
> > On Wed, 2012-06-20 at 16:50 -0700, Dan Williams wrote:
> >> On Sun, Jun 17, 2012 at 11:25 PM, Lin Ming <ming.m.lin@xxxxxxxxx> wrote:
> >> > From: Matthew Garrett <mjg@xxxxxxxxxx>
> >> >
> >> > Associate the ACPI device tree and libata devices.
> >> > This patch uses the generic ACPI glue framework to do so.
> >> >
> >> > Signed-off-by: Matthew Garrett <mjg@xxxxxxxxxx>
> >> > Signed-off-by: Holger Macht <holger@xxxxxxxx>
> >> > Signed-off-by: Lin Ming <ming.m.lin@xxxxxxxxx>
> >> > ---
> >> >
> >> > Changelog:
> >> >
> >> > - fixed crash reported at https://lkml.org/lkml/2012/2/24/2 (Aaron Lu)
> >> > - rename is_pci_ata to compat_pci_ata (Alan Cox)
> >> >
> >> > drivers/acpi/glue.c | 4 +-
> >> > drivers/ata/libata-acpi.c | 125 +++++++++++++++++++++++++++++++++++++++++++++
> >> > drivers/ata/libata-core.c | 3 ++
> >> > drivers/ata/libata.h | 4 ++
> >> > 4 files changed, 134 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> >> > index 1564e09..18d6812 100644
> >> > --- a/drivers/acpi/glue.c
> >> > +++ b/drivers/acpi/glue.c
> >> [..]
> >> > +static int ata_acpi_bind_host(struct device *dev, int host, acpi_handle *handle)
> >> > +{
> >> > + struct Scsi_Host *shost = dev_to_shost(dev);
> >> > + struct ata_port *ap = ata_shost_to_port(shost);
> >> > +
> >> > + if (ap->flags & ATA_FLAG_ACPI_SATA)
> >> > + return -ENODEV;
> >> > +
> >> > + *handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev->parent), ap->port_no);
> >> > +
> >> > + if (!*handle)
> >> > + return -ENODEV;
> >> > +
> >> > + return 0;
> >> > +}
> >>
> >> I think this patch should be squashed with patch 4, right? That is if
> >> we keep the hard-coded ->parent chasing...
> >
> > Right. Will merge patch 4.
> >
> >>
> >> > +static int ata_acpi_find_device(struct device *dev, acpi_handle *handle)
> >> > +{
> >> > + unsigned int host, channel, id, lun;
> >> > +
> >> > + if (sscanf(dev_name(dev), "host%u", &host) == 1) {
> >> > + if (!compat_pci_ata(dev->parent))
> >> > + return -ENODEV;
> >> > +
> >> > + return ata_acpi_bind_host(dev, host, handle);
> >> > + } else if (sscanf(dev_name(dev), "%d:%d:%d:%d",
> >> > + &host, &channel, &id, &lun) == 4) {
> >> > + if (!compat_pci_ata(dev->parent->parent->parent))
> >> > + return -ENODEV;
> >>
> >> ...this looks like it should be using a dev_to_ata_port() helper which
> >> like dev_to_shost() skips the need to remember just how many parents
> >> until we get back to our ata_port, scsi_host, etc.
> >
> > Oh, no.
> >
> > compat_pci_ata checks whether the controller(rather than shost or ata
> > port) is pci SATA/IDE controller.
> >
> > As in patch 4:
> > compat_pci_ata(dev->parent->parent->parent->parent)
> >
> > The "dev" is ata port, and dev->parent->parent->parent->parent points to
> > the controller dev.
> >
> > This looks ugly. How about add two helpers?
> > shost_to_controller(dev) and ata_port_to_controller(dev)
> >
>
> What about something like the following (untested)? Makes it mostly
> independent of the topology, gives some type safety, and drops some
> redundant work finding the ata_port.
>
> static int ata_acpi_find_device(struct device *dev, acpi_handle *handle)
> {
> struct ata_port *ap = dev_to_ata_port(dev);
>
> if (!ap)
> return -ENODEV;
>
> if (!compat_pci_ata(ap))
> return -ENODEV;
>
> if (scsi_is_host_device(dev))
> return ata_acpi_bind_host(ap, handle);
> else if (scsi_is_sdev_device(dev)) {
> struct scsi_device *sdev = to_scsi_device(dev);
>
> return ata_acpi_bind_device(ap, sdev->channel,
> sdev->id, handle);
> } else
> return -ENODEV;
> }
Good idea.
Below is the incremental patch on top of this series.
drivers/ata/libata-acpi.c | 56 +++++++++++++++++++++++++++------------------
drivers/ata/libata-core.c | 2 --
drivers/ata/libata.h | 2 ++
3 files changed, 36 insertions(+), 24 deletions(-)
diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 08edebf..4112eaa 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -1075,8 +1075,9 @@ void ata_acpi_unbind(struct ata_device *dev)
ata_acpi_unregister_power_resource(dev);
}
-static int compat_pci_ata(struct device *dev)
+static int compat_pci_ata(struct ata_port *ap)
{
+ struct device *dev = ap->tdev.parent;
struct pci_dev *pdev;
if (!is_pci_dev(dev))
@@ -1091,15 +1092,13 @@ static int compat_pci_ata(struct device *dev)
return 1;
}
-static int ata_acpi_bind_host(struct device *dev, int host, acpi_handle *handle)
+static int ata_acpi_bind_host(struct ata_port *ap, acpi_handle *handle)
{
- struct Scsi_Host *shost = dev_to_shost(dev);
- struct ata_port *ap = ata_shost_to_port(shost);
-
if (ap->flags & ATA_FLAG_ACPI_SATA)
return -ENODEV;
- *handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev->parent->parent), ap->port_no);
+ *handle = acpi_get_child(DEVICE_ACPI_HANDLE(ap->tdev.parent),
+ ap->port_no);
if (!*handle)
return -ENODEV;
@@ -1107,21 +1106,18 @@ static int ata_acpi_bind_host(struct device *dev, int host, acpi_handle *handle)
return 0;
}
-static int ata_acpi_bind_device(struct device *dev, int channel, int id,
+static int ata_acpi_bind_device(struct ata_port *ap, struct scsi_device *sdev,
acpi_handle *handle)
{
- struct scsi_device *sdev = to_scsi_device(dev);
- struct Scsi_Host *shost = sdev->host;
- struct ata_port *ap = ata_shost_to_port(shost);
struct ata_device *ata_dev;
acpi_status status;
struct acpi_device *acpi_dev;
struct acpi_device_power_state *states;
if (ap->flags & ATA_FLAG_ACPI_SATA)
- ata_dev = &ap->link.device[channel];
+ ata_dev = &ap->link.device[sdev->channel];
else
- ata_dev = &ap->link.device[id];
+ ata_dev = &ap->link.device[sdev->id];
*handle = ata_dev_acpi_handle(ata_dev);
@@ -1146,21 +1142,37 @@ static int ata_acpi_bind_device(struct device *dev, int channel, int id,
return 0;
}
+static int is_ata_port(const struct device *dev)
+{
+ return dev->type == &ata_port_type;
+}
+
+static struct ata_port *dev_to_ata_port(struct device *dev)
+{
+ while (!is_ata_port(dev)) {
+ if (!dev->parent)
+ return NULL;
+ dev = dev->parent;
+ }
+ return to_ata_port(dev);
+}
+
static int ata_acpi_find_device(struct device *dev, acpi_handle *handle)
{
- unsigned int host, channel, id, lun;
+ struct ata_port *ap = dev_to_ata_port(dev);
- if (sscanf(dev_name(dev), "host%u", &host) == 1) {
- if (!compat_pci_ata(dev->parent->parent))
- return -ENODEV;
+ if (!ap)
+ return -ENODEV;
+
+ if (!compat_pci_ata(ap))
+ return -ENODEV;
- return ata_acpi_bind_host(dev, host, handle);
- } else if (sscanf(dev_name(dev), "%d:%d:%d:%d",
- &host, &channel, &id, &lun) == 4) {
- if (!compat_pci_ata(dev->parent->parent->parent->parent))
- return -ENODEV;
+ if (scsi_is_host_device(dev))
+ return ata_acpi_bind_host(ap, handle);
+ else if (scsi_is_sdev_device(dev)) {
+ struct scsi_device *sdev = to_scsi_device(dev);
- return ata_acpi_bind_device(dev, channel, id, handle);
+ return ata_acpi_bind_device(ap, sdev, handle);
} else
return -ENODEV;
}
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index ba169c4..c14f88c 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5291,8 +5291,6 @@ static int ata_port_request_pm(struct ata_port *ap, pm_message_t mesg,
return rc;
}
-#define to_ata_port(d) container_of(d, struct ata_port, tdev)
-
static int ata_port_suspend_common(struct device *dev, pm_message_t mesg)
{
struct ata_port *ap = to_ata_port(dev);
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index b0ba924..44a7939 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -107,6 +107,8 @@ extern const char *sata_spd_string(unsigned int spd);
extern int ata_port_probe(struct ata_port *ap);
extern void __ata_port_probe(struct ata_port *ap);
+#define to_ata_port(d) container_of(d, struct ata_port, tdev)
+
/* libata-acpi.c */
#ifdef CONFIG_ATA_ACPI
extern unsigned int ata_acpi_gtf_filter;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/