Patchwork [v5,02/12] libata: bind the Linux device tree to the ACPI device tree

login
register
mail settings
Submitter Lin Ming
Date June 18, 2012, 6:25 a.m.
Message ID <1340000766-129148-3-git-send-email-ming.m.lin@intel.com>
Download mbox | patch
Permalink /patch/165400/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Lin Ming - June 18, 2012, 6:25 a.m.
From: Matthew Garrett <mjg@redhat.com>

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@redhat.com>
Signed-off-by: Holger Macht <holger@homac.de>
Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---

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(-)
Dan Williams - June 20, 2012, 11:50 p.m.
On Sun, Jun 17, 2012 at 11:25 PM, Lin Ming <ming.m.lin@intel.com> wrote:
> From: Matthew Garrett <mjg@redhat.com>
>
> 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@redhat.com>
> Signed-off-by: Holger Macht <holger@homac.de>
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
>
> 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...

> +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.

--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lin Ming - June 21, 2012, 1:26 a.m.
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@intel.com> wrote:
> > From: Matthew Garrett <mjg@redhat.com>
> >
> > 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@redhat.com>
> > Signed-off-by: Holger Macht <holger@homac.de>
> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > ---
> >
> > 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)

Thanks,
Lin Ming

> 
> --
> Dan


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Williams - June 21, 2012, 3:59 a.m.
On Wed, Jun 20, 2012 at 6:26 PM, Lin Ming <ming.m.lin@intel.com> 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@intel.com> wrote:
>> > From: Matthew Garrett <mjg@redhat.com>
>> >
>> > 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@redhat.com>
>> > Signed-off-by: Holger Macht <holger@homac.de>
>> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
>> > ---
>> >
>> > 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;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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
@@ -39,6 +39,7 @@  int register_acpi_bus_type(struct acpi_bus_type *type)
 	}
 	return -ENODEV;
 }
+EXPORT_SYMBOL(register_acpi_bus_type);
 
 int unregister_acpi_bus_type(struct acpi_bus_type *type)
 {
@@ -54,6 +55,7 @@  int unregister_acpi_bus_type(struct acpi_bus_type *type)
 	}
 	return -ENODEV;
 }
+EXPORT_SYMBOL(unregister_acpi_bus_type);
 
 static struct acpi_bus_type *acpi_get_bus_type(struct bus_type *type)
 {
@@ -69,7 +71,6 @@  static struct acpi_bus_type *acpi_get_bus_type(struct bus_type *type)
 	up_read(&bus_type_sem);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(register_acpi_bus_type);
 
 static int acpi_find_bridge_device(struct device *dev, acpi_handle * handle)
 {
@@ -86,7 +87,6 @@  static int acpi_find_bridge_device(struct device *dev, acpi_handle * handle)
 	up_read(&bus_type_sem);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(unregister_acpi_bus_type);
 
 /* Get device's handler per its address under its parent */
 struct acpi_find_child {
diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index bb7c5f1..ae93593 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -47,6 +47,39 @@  static void ata_acpi_clear_gtf(struct ata_device *dev)
 	dev->gtf_cache = NULL;
 }
 
+static acpi_handle ap_acpi_handle(struct ata_port *ap)
+{
+	if (ap->flags & ATA_FLAG_ACPI_SATA)
+		return NULL;
+
+	/*
+	 * If acpi bind operation has already happened, we can get the handle
+	 * for the port by checking the corresponding scsi_host device's
+	 * firmware node, otherwise we will need to find out the handle from
+	 * its parent's acpi node.
+	 */
+	if (ap->scsi_host)
+		return DEVICE_ACPI_HANDLE(&ap->scsi_host->shost_gendev);
+	else
+		return acpi_get_child(DEVICE_ACPI_HANDLE(ap->host->dev),
+				ap->port_no);
+}
+
+static acpi_handle dev_acpi_handle(struct ata_device *dev)
+{
+	acpi_integer adr;
+	struct ata_port *ap = dev->link->ap;
+
+	if (ap->flags & ATA_FLAG_ACPI_SATA) {
+		if (!sata_pmp_attached(ap))
+			adr = SATA_ADR(ap->port_no, NO_PORT_MULT);
+		else
+			adr = SATA_ADR(ap->port_no, dev->link->pmp);
+		return acpi_get_child(DEVICE_ACPI_HANDLE(ap->host->dev), adr);
+	} else
+		return acpi_get_child(ap_acpi_handle(ap), dev->devno);
+}
+
 /**
  * ata_acpi_associate_sata_port - associate SATA port with ACPI objects
  * @ap: target SATA port
@@ -1018,3 +1051,95 @@  void ata_acpi_on_disable(struct ata_device *dev)
 {
 	ata_acpi_clear_gtf(dev);
 }
+
+static int compat_pci_ata(struct device *dev)
+{
+	struct pci_dev *pdev;
+
+	if (!is_pci_dev(dev))
+		return 0;
+
+	pdev = to_pci_dev(dev);
+
+	if ((pdev->class >> 8) != PCI_CLASS_STORAGE_SATA &&
+	    (pdev->class >> 8) != PCI_CLASS_STORAGE_IDE)
+		return 0;
+
+	return 1;
+}
+
+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;
+}
+
+static int ata_acpi_bind_device(struct device *dev, int channel, int id,
+				acpi_handle *handle)
+{
+	struct device *host = dev->parent->parent;
+	struct Scsi_Host *shost = dev_to_shost(host);
+	struct ata_port *ap = ata_shost_to_port(shost);
+	struct ata_device *ata_dev;
+
+	if (ap->flags & ATA_FLAG_ACPI_SATA)
+		ata_dev = &ap->link.device[channel];
+	else
+		ata_dev = &ap->link.device[id];
+
+	*handle = dev_acpi_handle(ata_dev);
+
+	if (!*handle)
+		return -ENODEV;
+
+	return 0;
+}
+
+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;
+
+		return ata_acpi_bind_device(dev, channel, id, handle);
+	} else
+		return -ENODEV;
+}
+
+static int ata_acpi_find_dummy(struct device *dev, acpi_handle *handle)
+{
+	return -ENODEV;
+}
+
+static struct acpi_bus_type ata_acpi_bus = {
+	.find_bridge = ata_acpi_find_dummy,
+	.find_device = ata_acpi_find_device,
+};
+
+int ata_acpi_register(void)
+{
+	return scsi_register_acpi_bus_type(&ata_acpi_bus);
+}
+
+void ata_acpi_unregister(void)
+{
+	scsi_unregister_acpi_bus_type(&ata_acpi_bus);
+}
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index cece3a4..5628bf6 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6513,6 +6513,8 @@  static int __init ata_init(void)
 
 	ata_parse_force_param();
 
+	ata_acpi_register();
+
 	rc = ata_sff_init();
 	if (rc) {
 		kfree(ata_force_tbl);
@@ -6539,6 +6541,7 @@  static void __exit ata_exit(void)
 	ata_release_transport(ata_scsi_transport_template);
 	libata_transport_exit();
 	ata_sff_exit();
+	ata_acpi_unregister();
 	kfree(ata_force_tbl);
 }
 
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 9d0fd0b..3df3362 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -119,6 +119,8 @@  extern void ata_acpi_on_resume(struct ata_port *ap);
 extern int ata_acpi_on_devcfg(struct ata_device *dev);
 extern void ata_acpi_on_disable(struct ata_device *dev);
 extern void ata_acpi_set_state(struct ata_port *ap, pm_message_t state);
+extern int ata_acpi_register(void);
+extern void ata_acpi_unregister(void);
 #else
 static inline void ata_acpi_associate_sata_port(struct ata_port *ap) { }
 static inline void ata_acpi_associate(struct ata_host *host) { }
@@ -129,6 +131,8 @@  static inline int ata_acpi_on_devcfg(struct ata_device *dev) { return 0; }
 static inline void ata_acpi_on_disable(struct ata_device *dev) { }
 static inline void ata_acpi_set_state(struct ata_port *ap,
 				      pm_message_t state) { }
+static inline int ata_acpi_register(void) { return 0; }
+static void ata_acpi_unregister(void) { }
 #endif
 
 /* libata-scsi.c */