| Message ID | 20090714205333.18933.73027.stgit@bob.kio (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tuesday 14 July 2009 02:53:33 pm Alex Chiang wrote: > We cannot simply call acpi_get_pci_dev() on any random ACPI handle > and hope that it works, because a PCI root bridge may not have > an associated struct pci_dev. > > This is allowed per the PCI specification, and is referred to as a > non-materialized bridge. > > So, depending on the type of PCI bridge that the handle points to, > use the appropriate interface to return the struct pci_bus correctly. > > Signed-off-by: Alex Chiang <achiang@hp.com> > --- > > drivers/pci/hotplug/acpiphp_glue.c | 27 ++++++++++++++++----------- > 1 files changed, 16 insertions(+), 11 deletions(-) > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > index 0cb0f83..fa4658b 100644 > --- a/drivers/pci/hotplug/acpiphp_glue.c > +++ b/drivers/pci/hotplug/acpiphp_glue.c > @@ -62,6 +62,21 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus); > static void acpiphp_set_hpp_values(acpi_handle handle, struct pci_bus *bus); > static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context); > > +static struct pci_bus *pci_bus_from_handle(acpi_handle handle) > +{ > + struct pci_bus *pbus; > + > + if (acpi_is_root_bridge(handle)) { > + struct acpi_pci_root *root = acpi_pci_find_root(handle); > + pbus = root->bus; > + } else { > + struct pci_dev *pdev = acpi_get_pci_dev(handle); > + pbus = pdev->subordinate; > + pci_dev_put(pdev); > + } > + return pbus; I worry that acpi_is_root_bridge() merely checks the device IDs of "handle", which isn't quite the same as checking whether the pci_root driver has claimed "handle". Are you confident that it's safe to move the pci_dev_put(), so it is released before configuring the bridge? What do you think about something like this (even though the get/put is still a little clunky): struct pci_dev *dev = NULL; root = acpi_pci_find_root(handle); if (root) bus = root->bus; else { dev = acpi_get_pci_dev(handle); if (dev) bus = pdev->subordinate; else { err("cannot get PCI domain and bus number for bridge\n"); return -EINVAL; } } pci_bus_size_bridges(bus); ... if (dev) pci_dev_put(dev); return 0; > +} > + > /* callback routine to check for the existence of a pci dock device */ > static acpi_status > is_pci_dock_device(acpi_handle handle, u32 lvl, void *context, void **rv) > @@ -1387,16 +1402,7 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus) > /* Program resources in newly inserted bridge */ > static int acpiphp_configure_bridge (acpi_handle handle) > { > - struct pci_dev *dev; > - struct pci_bus *bus; > - > - dev = acpi_get_pci_dev(handle); > - if (!dev) { > - err("cannot get PCI domain and bus number for bridge\n"); > - return -EINVAL; > - } > - > - bus = dev->bus; > + struct pci_bus *bus = pci_bus_from_handle(handle); > > pci_bus_size_bridges(bus); > pci_bus_assign_resources(bus); > @@ -1404,7 +1410,6 @@ static int acpiphp_configure_bridge (acpi_handle handle) > acpiphp_set_hpp_values(handle, bus); > pci_enable_bridges(bus); > acpiphp_configure_ioapics(handle); > - pci_dev_put(dev); > return 0; > } -- 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 <bjorn.helgaas@hp.com>: > On Tuesday 14 July 2009 02:53:33 pm Alex Chiang wrote: > > We cannot simply call acpi_get_pci_dev() on any random ACPI handle > > and hope that it works, because a PCI root bridge may not have > > an associated struct pci_dev. > > > > This is allowed per the PCI specification, and is referred to as a > > non-materialized bridge. > > > > So, depending on the type of PCI bridge that the handle points to, > > use the appropriate interface to return the struct pci_bus correctly. > > > > Signed-off-by: Alex Chiang <achiang@hp.com> > > --- > > > > drivers/pci/hotplug/acpiphp_glue.c | 27 ++++++++++++++++----------- > > 1 files changed, 16 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > > index 0cb0f83..fa4658b 100644 > > --- a/drivers/pci/hotplug/acpiphp_glue.c > > +++ b/drivers/pci/hotplug/acpiphp_glue.c > > @@ -62,6 +62,21 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus); > > static void acpiphp_set_hpp_values(acpi_handle handle, struct pci_bus *bus); > > static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context); > > > > +static struct pci_bus *pci_bus_from_handle(acpi_handle handle) > > +{ > > + struct pci_bus *pbus; > > + > > + if (acpi_is_root_bridge(handle)) { > > + struct acpi_pci_root *root = acpi_pci_find_root(handle); > > + pbus = root->bus; > > + } else { > > + struct pci_dev *pdev = acpi_get_pci_dev(handle); > > + pbus = pdev->subordinate; > > + pci_dev_put(pdev); > > + } > > + return pbus; > > I worry that acpi_is_root_bridge() merely checks the device IDs of > "handle", which isn't quite the same as checking whether the pci_root > driver has claimed "handle". Hm, I understand this concern in a theoretical sense, but could you explain more of what you're thinking about, and give me a concrete example of something that might go wrong here? > Are you confident that it's safe to move the pci_dev_put(), so it is > released before configuring the bridge? I'm confident that I'm not changing the lifetime assumptions in acpiphp_configure_bridge(), as the old code didn't seem to care either. Commit d6aa484c (acpiphp: convert to acpi_get_pci_dev) changed from pci_find_bus() to use acpi_get_pci_dev(), and pci_find_bus() does not elevate any reference counts. What I'm trying to fix here is that acpi_get_pci_dev() /might/ not work all the time, namely on machines that have both: a) hotpluggable root bridges b) non-materialized root bridges > What do you think about something like this (even though the get/put > is still a little clunky): > > struct pci_dev *dev = NULL; > > root = acpi_pci_find_root(handle); > if (root) > bus = root->bus; > else { > dev = acpi_get_pci_dev(handle); > if (dev) > bus = pdev->subordinate; > else { > err("cannot get PCI domain and bus number for bridge\n"); > return -EINVAL; > } > } > > pci_bus_size_bridges(bus); > ... > if (dev) > pci_dev_put(dev); > return 0; This seems like a good approach too, but I'd like to understand your concern about acpi_is_root_bridge() first. Thanks for the review. /ac -- 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 Tuesday 21 July 2009 01:57:55 pm Alex Chiang wrote: > * Bjorn Helgaas <bjorn.helgaas@hp.com>: > > On Tuesday 14 July 2009 02:53:33 pm Alex Chiang wrote: > > > We cannot simply call acpi_get_pci_dev() on any random ACPI handle > > > and hope that it works, because a PCI root bridge may not have > > > an associated struct pci_dev. > > > > > > This is allowed per the PCI specification, and is referred to as a > > > non-materialized bridge. > > > > > > So, depending on the type of PCI bridge that the handle points to, > > > use the appropriate interface to return the struct pci_bus correctly. > > > > > > Signed-off-by: Alex Chiang <achiang@hp.com> > > > --- > > > > > > drivers/pci/hotplug/acpiphp_glue.c | 27 ++++++++++++++++----------- > > > 1 files changed, 16 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > > > index 0cb0f83..fa4658b 100644 > > > --- a/drivers/pci/hotplug/acpiphp_glue.c > > > +++ b/drivers/pci/hotplug/acpiphp_glue.c > > > @@ -62,6 +62,21 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus); > > > static void acpiphp_set_hpp_values(acpi_handle handle, struct pci_bus *bus); > > > static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context); > > > > > > +static struct pci_bus *pci_bus_from_handle(acpi_handle handle) > > > +{ > > > + struct pci_bus *pbus; > > > + > > > + if (acpi_is_root_bridge(handle)) { > > > + struct acpi_pci_root *root = acpi_pci_find_root(handle); > > > + pbus = root->bus; > > > + } else { > > > + struct pci_dev *pdev = acpi_get_pci_dev(handle); > > > + pbus = pdev->subordinate; > > > + pci_dev_put(pdev); > > > + } > > > + return pbus; > > > > I worry that acpi_is_root_bridge() merely checks the device IDs of > > "handle", which isn't quite the same as checking whether the pci_root > > driver has claimed "handle". > > Hm, I understand this concern in a theoretical sense, but could > you explain more of what you're thinking about, and give me a > concrete example of something that might go wrong here? My concern is only theoretical -- I could imagine a PNP0A03 device in the namespace (so acpi_is_root_bridge() is true) that has not been claimed by the pci_root driver (so acpi_pci_find_root() returns NULL). I don't think this will happen in practice because pci_root can't be a module, but it's easier to analyze with just one check, since you can learn everything you need from acpi_pci_find_root() without also depending on acpi_is_root_bridge(). Bjorn -- 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 <bjorn.helgaas@hp.com>: > On Tuesday 21 July 2009 01:57:55 pm Alex Chiang wrote: > > * Bjorn Helgaas <bjorn.helgaas@hp.com>: > > > On Tuesday 14 July 2009 02:53:33 pm Alex Chiang wrote: > > > > +static struct pci_bus *pci_bus_from_handle(acpi_handle handle) > > > > +{ > > > > + struct pci_bus *pbus; > > > > + > > > > + if (acpi_is_root_bridge(handle)) { > > > > + struct acpi_pci_root *root = acpi_pci_find_root(handle); > > > > + pbus = root->bus; > > > > + } else { > > > > + struct pci_dev *pdev = acpi_get_pci_dev(handle); > > > > + pbus = pdev->subordinate; > > > > + pci_dev_put(pdev); > > > > + } > > > > + return pbus; > > > > > > I worry that acpi_is_root_bridge() merely checks the device IDs of > > > "handle", which isn't quite the same as checking whether the pci_root > > > driver has claimed "handle". > > > > Hm, I understand this concern in a theoretical sense, but could > > you explain more of what you're thinking about, and give me a > > concrete example of something that might go wrong here? > > My concern is only theoretical -- I could imagine a PNP0A03 device > in the namespace (so acpi_is_root_bridge() is true) that has not been > claimed by the pci_root driver (so acpi_pci_find_root() returns NULL). > > I don't think this will happen in practice because pci_root can't be > a module, but it's easier to analyze with just one check, since you > can learn everything you need from acpi_pci_find_root() without also > depending on acpi_is_root_bridge(). Ah, that makes sense. I can respin one more time using your suggestion (although I'll probably keep it factored out into a separate function). Thanks. /ac -- 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
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c index 0cb0f83..fa4658b 100644 --- a/drivers/pci/hotplug/acpiphp_glue.c +++ b/drivers/pci/hotplug/acpiphp_glue.c @@ -62,6 +62,21 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus); static void acpiphp_set_hpp_values(acpi_handle handle, struct pci_bus *bus); static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context); +static struct pci_bus *pci_bus_from_handle(acpi_handle handle) +{ + struct pci_bus *pbus; + + if (acpi_is_root_bridge(handle)) { + struct acpi_pci_root *root = acpi_pci_find_root(handle); + pbus = root->bus; + } else { + struct pci_dev *pdev = acpi_get_pci_dev(handle); + pbus = pdev->subordinate; + pci_dev_put(pdev); + } + return pbus; +} + /* callback routine to check for the existence of a pci dock device */ static acpi_status is_pci_dock_device(acpi_handle handle, u32 lvl, void *context, void **rv) @@ -1387,16 +1402,7 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus) /* Program resources in newly inserted bridge */ static int acpiphp_configure_bridge (acpi_handle handle) { - struct pci_dev *dev; - struct pci_bus *bus; - - dev = acpi_get_pci_dev(handle); - if (!dev) { - err("cannot get PCI domain and bus number for bridge\n"); - return -EINVAL; - } - - bus = dev->bus; + struct pci_bus *bus = pci_bus_from_handle(handle); pci_bus_size_bridges(bus); pci_bus_assign_resources(bus); @@ -1404,7 +1410,6 @@ static int acpiphp_configure_bridge (acpi_handle handle) acpiphp_set_hpp_values(handle, bus); pci_enable_bridges(bus); acpiphp_configure_ioapics(handle); - pci_dev_put(dev); return 0; }
We cannot simply call acpi_get_pci_dev() on any random ACPI handle and hope that it works, because a PCI root bridge may not have an associated struct pci_dev. This is allowed per the PCI specification, and is referred to as a non-materialized bridge. So, depending on the type of PCI bridge that the handle points to, use the appropriate interface to return the struct pci_bus correctly. Signed-off-by: Alex Chiang <achiang@hp.com> --- drivers/pci/hotplug/acpiphp_glue.c | 27 ++++++++++++++++----------- 1 files changed, 16 insertions(+), 11 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