Patchwork [v3,5/8] ACPI, PCI: change acpi_pci_find_root implementation

login
register
mail settings
Submitter Taku Izumi
Date Sept. 18, 2012, 6:23 a.m.
Message ID <20120918152323.48cb2703.izumi.taku@jp.fujitsu.com>
Download mbox | patch
Permalink /patch/184620/
State Accepted
Headers show

Comments

Taku Izumi - Sept. 18, 2012, 6:23 a.m.
This patch changes the implementation of acpi_pci_find_root().

We can access acpi_pci_root without scanning acpi_pci_roots list.
If hostbridge hotplug is supported, acpi_pci_roots list will be
protected by mutex. We should not access acpi_pci_roots list 
if preventable to lessen deadlock risk.

Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
---
 drivers/acpi/pci_root.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - Sept. 19, 2012, 10:03 p.m.
On Tue, Sep 18, 2012 at 12:23 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
>
> This patch changes the implementation of acpi_pci_find_root().
>
> We can access acpi_pci_root without scanning acpi_pci_roots list.
> If hostbridge hotplug is supported, acpi_pci_roots list will be
> protected by mutex. We should not access acpi_pci_roots list
> if preventable to lessen deadlock risk.
>
> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> ---
>  drivers/acpi/pci_root.c |   13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> Index: Bjorn-next-0903/drivers/acpi/pci_root.c
> ===================================================================
> --- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
> +++ Bjorn-next-0903/drivers/acpi/pci_root.c
> @@ -265,12 +265,15 @@ static acpi_status acpi_pci_osc_support(
>  struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle)
>  {
>         struct acpi_pci_root *root;
> +       struct acpi_device *device;
>
> -       list_for_each_entry(root, &acpi_pci_roots, node) {
> -               if (root->device->handle == handle)
> -                       return root;
> -       }
> -       return NULL;
> +       if (acpi_bus_get_device(handle, &device) ||
> +           acpi_match_device_ids(device, root_device_ids))

What's the purpose of the acpi_match_device_ids() check?  It's not
obvious, so worth calling it out in the changelog, and maybe even a
comment in the code.

Nice to get rid of the list traversal.

> +               return NULL;
> +
> +       root = acpi_driver_data(device);
> +
> +       return root;
>  }
>  EXPORT_SYMBOL_GPL(acpi_pci_find_root);
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Taku Izumi - Sept. 21, 2012, 7:14 a.m.
On Wed, 19 Sep 2012 16:03:27 -0600
Bjorn Helgaas <bhelgaas@google.com> wrote:

> On Tue, Sep 18, 2012 at 12:23 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
> >
> > This patch changes the implementation of acpi_pci_find_root().
> >
> > We can access acpi_pci_root without scanning acpi_pci_roots list.
> > If hostbridge hotplug is supported, acpi_pci_roots list will be
> > protected by mutex. We should not access acpi_pci_roots list
> > if preventable to lessen deadlock risk.
> >
> > Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
> > ---
> >  drivers/acpi/pci_root.c |   13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > Index: Bjorn-next-0903/drivers/acpi/pci_root.c
> > ===================================================================
> > --- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
> > +++ Bjorn-next-0903/drivers/acpi/pci_root.c
> > @@ -265,12 +265,15 @@ static acpi_status acpi_pci_osc_support(
> >  struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle)
> >  {
> >         struct acpi_pci_root *root;
> > +       struct acpi_device *device;
> >
> > -       list_for_each_entry(root, &acpi_pci_roots, node) {
> > -               if (root->device->handle == handle)
> > -                       return root;
> > -       }
> > -       return NULL;
> > +       if (acpi_bus_get_device(handle, &device) ||
> > +           acpi_match_device_ids(device, root_device_ids))
> 
> What's the purpose of the acpi_match_device_ids() check?  It's not
> obvious, so worth calling it out in the changelog, and maybe even a
> comment in the code.

  My intention is to reject acpi_handle which doesn't represent 
  hostbrige. I think this is reasonable...

  Best regards,
  Taku Izumi

> 
> Nice to get rid of the list traversal.
> 
> > +               return NULL;
> > +
> > +       root = acpi_driver_data(device);
> > +
> > +       return root;
> >  }
> >  EXPORT_SYMBOL_GPL(acpi_pci_find_root);
> >
> >
>
Bjorn Helgaas - Sept. 21, 2012, 5:57 p.m.
On Fri, Sep 21, 2012 at 1:14 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
> On Wed, 19 Sep 2012 16:03:27 -0600
> Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>> On Tue, Sep 18, 2012 at 12:23 AM, Taku Izumi <izumi.taku@jp.fujitsu.com> wrote:
>> >
>> > This patch changes the implementation of acpi_pci_find_root().
>> >
>> > We can access acpi_pci_root without scanning acpi_pci_roots list.
>> > If hostbridge hotplug is supported, acpi_pci_roots list will be
>> > protected by mutex. We should not access acpi_pci_roots list
>> > if preventable to lessen deadlock risk.
>> >
>> > Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
>> > ---
>> >  drivers/acpi/pci_root.c |   13 ++++++++-----
>> >  1 file changed, 8 insertions(+), 5 deletions(-)
>> >
>> > Index: Bjorn-next-0903/drivers/acpi/pci_root.c
>> > ===================================================================
>> > --- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
>> > +++ Bjorn-next-0903/drivers/acpi/pci_root.c
>> > @@ -265,12 +265,15 @@ static acpi_status acpi_pci_osc_support(
>> >  struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle)
>> >  {
>> >         struct acpi_pci_root *root;
>> > +       struct acpi_device *device;
>> >
>> > -       list_for_each_entry(root, &acpi_pci_roots, node) {
>> > -               if (root->device->handle == handle)
>> > -                       return root;
>> > -       }
>> > -       return NULL;
>> > +       if (acpi_bus_get_device(handle, &device) ||
>> > +           acpi_match_device_ids(device, root_device_ids))
>>
>> What's the purpose of the acpi_match_device_ids() check?  It's not
>> obvious, so worth calling it out in the changelog, and maybe even a
>> comment in the code.
>
>   My intention is to reject acpi_handle which doesn't represent
>   hostbrige. I think this is reasonable...

Yes, I understand that part.  I was just trying to figure out why that
check is needed.  Is there a path where we can call
acpi_pci_find_root() with a handle that is *not* a PNP0A03 device?  I
looked at all the callers, and as far as I can tell, the supplied
handle is always for a host bridge device.

But I guess I don't object to the check.  This is just another case of
a lookup that in many cases is unnecessary because we should have the
struct acpi_pci_root * available already.

>> Nice to get rid of the list traversal.
>>
>> > +               return NULL;
>> > +
>> > +       root = acpi_driver_data(device);
>> > +
>> > +       return root;
>> >  }
>> >  EXPORT_SYMBOL_GPL(acpi_pci_find_root);
>> >
>> >
>>
>
>
> --
> Taku Izumi <izumi.taku@jp.fujitsu.com>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

Index: Bjorn-next-0903/drivers/acpi/pci_root.c
===================================================================
--- Bjorn-next-0903.orig/drivers/acpi/pci_root.c
+++ Bjorn-next-0903/drivers/acpi/pci_root.c
@@ -265,12 +265,15 @@  static acpi_status acpi_pci_osc_support(
 struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle)
 {
 	struct acpi_pci_root *root;
+	struct acpi_device *device;
 
-	list_for_each_entry(root, &acpi_pci_roots, node) {
-		if (root->device->handle == handle)
-			return root;
-	}
-	return NULL;
+	if (acpi_bus_get_device(handle, &device) ||
+	    acpi_match_device_ids(device, root_device_ids))
+		return NULL;
+
+	root = acpi_driver_data(device);
+
+	return root;
 }
 EXPORT_SYMBOL_GPL(acpi_pci_find_root);