| Message ID | 20120918152323.48cb2703.izumi.taku@jp.fujitsu.com |
|---|---|
| State | Accepted |
| Headers | show |
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
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); > > > > >
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
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);
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