On Tue, Aug 6, 2013 at 5:00 PM, Rafael J. Wysocki <r...@sisk.pl> wrote: > From: Rafael J. Wysocki <rafael.j.wyso...@intel.com> > > In theory, under a given ACPI namespace node there should be only > one child device object with _ADR whose value matches a given bus > address exactly. In practice, however, there are systems in which > multiple child device objects under a given parent have _ADR matching > exactly the same address. In those cases we use _STA to determine > which of the multiple matching devices is enabled, since some systems > are known to indicate which ACPI device object to associate with the > given physical (usually PCI) device this way. > > Unfortunately, as it turns out, there are systems in which many > device objects under the same parent have _ADR matching exactly the > same bus address and none of them has _STA, in which case they all > should be regarded as enabled according to the spec. Still, if > those device objects are supposed to represent bridges (e.g. this > is the case for device objects corresponding to PCIe ports), we can > try harder and skip the ones that have no child device objects in the > ACPI namespace. With luck, we can avoid using device objects that we > are not expected to use this way. > > Although this only works for bridges whose children also have ACPI > namespace representation, it is sufficient to address graphics > adapter detection issues on some systems, so rework the code finding > a matching device ACPI handle for a given bus address to implement > this idea. > > Introduce a new function, acpi_find_child(), taking three arguments: > the ACPI handle of the device's parent, a bus address suitable for > the device's bus type and a bool indicating if the device is a > bridge and make it work as outlined above. Reimplement the function > currently used for this purpose, acpi_get_child(), as a call to > acpi_find_child() with the last argument set to 'false' and make > the PCI subsystem use acpi_find_child() with the bridge information > passed as the last argument to it. [Lan Tianyu notices that it is > not sufficient to use pci_is_bridge() for that, because the device's > subordinate pointer hasn't been set yet at this point, so use > hdr_type instead.] > > This change fixes a regression introduced inadvertently by commit > 33f767d (ACPI: Rework acpi_get_child() to be more efficient) which > overlooked the fact that for acpi_walk_namespace() "post-order" means > "after all children have been visited" rather than "on the way back", > so for device objects without children and for namespace walks of > depth 1, as in the acpi_get_child() case, the "post-order" callbacks > ordering is actually the same as the ordering of "pre-order" ones. > Since that commit changed the namespace walk in acpi_get_child() to > terminate after finding the first matching object instead of going > through all of them and returning the last one, it effectively > changed the result returned by that function in some rare cases and > that led to problems (the switch from a "pre-order" to a "post-order" > callback was supposed to prevent that from happening, but it was > ineffective). > > As it turns out, the systems where the change made by commit > 33f767d actually matters are those where there are multiple ACPI > device objects representing the same PCIe port (which effectively > is a bridge). Moreover, only one of them, and the one we are > expected to use, has child device objects in the ACPI namespace, > so the regression can be addressed as described above.
I hesitate to suggest making the changelog even longer, but it sounds like the user-visible effect of this patch is that it fixes some sort of video problem? > References: https://bugzilla.kernel.org/show_bug.cgi?id=60561 > Reported-by: Peter Wu <lekenst...@gmail.com> > Tested-by: Vladimir Lalov <m...@vlalov.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com> > Cc: 3.9+ <sta...@vger.kernel.org> # 3.9+ I'm OK with this as-is, and expect that you'll merge this via your tree. A few nits below, feel free to address or ignore at your discretion. If you need it: Acked-by: Bjorn Helgaas <bhelg...@google.com> > --- > drivers/acpi/glue.c | 99 > +++++++++++++++++++++++++++++++++++++++--------- > drivers/pci/pci-acpi.c | 14 ++++-- > include/acpi/acpi_bus.h | 6 ++ > 3 files changed, 97 insertions(+), 22 deletions(-) > > Index: linux-pm/drivers/acpi/glue.c > =================================================================== > --- linux-pm.orig/drivers/acpi/glue.c > +++ linux-pm/drivers/acpi/glue.c > @@ -79,34 +79,99 @@ static struct acpi_bus_type *acpi_get_bu > return ret; > } > > -static acpi_status do_acpi_find_child(acpi_handle handle, u32 lvl_not_used, > - void *addr_p, void **ret_p) > +static acpi_status acpi_dev_check(acpi_handle handle, u32 lvl_not_used, > + void *not_used, void **ret_p) "acpi_dev_check()" isn't very descriptive because it doesn't indicate what we're checking for. I guess it has to do with whether the handle has a corresponding acpi_device. > { > - unsigned long long addr, sta; > - acpi_status status; > + struct acpi_device *adev = NULL; > > - status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &addr); > - if (ACPI_SUCCESS(status) && addr == *((u64 *)addr_p)) { > + acpi_bus_get_device(handle, &adev); > + if (adev) { > *ret_p = handle; > - status = acpi_bus_get_status_handle(handle, &sta); > - if (ACPI_SUCCESS(status) && (sta & ACPI_STA_DEVICE_ENABLED)) > - return AE_CTRL_TERMINATE; > + return AE_CTRL_TERMINATE; > } > return AE_OK; > } > > -acpi_handle acpi_get_child(acpi_handle parent, u64 address) > +static bool acpi_dev_suitable(acpi_handle handle, bool is_bridge) Same comment for "acpi_dev_suitable()". It'd be nice to have a hint about what makes it suitable. I wish I understood better what's going on so I could suggest possibilities, but I really don't. > { > - void *ret = NULL; > + unsigned long long sta; > + acpi_status status; > > - if (!parent) > - return NULL; > + status = acpi_bus_get_status_handle(handle, &sta); > + if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_ENABLED)) > + return false; > + > + if (is_bridge) { > + void *test = NULL; > + > + /* Check if this object has at least one child device. */ > + acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1, > acpi_dev_check, > + NULL, NULL, &test); > + return !!test; > + } > + return true; > +} > > - acpi_walk_namespace(ACPI_TYPE_DEVICE, parent, 1, NULL, > - do_acpi_find_child, &address, &ret); > - return (acpi_handle)ret; > +struct find_child_context { > + u64 addr; > + bool is_bridge; > + acpi_handle ret; > + bool ret_checked; > +}; > + > +static acpi_status do_find_child(acpi_handle handle, u32 lvl_not_used, > + void *data, void **not_used) > +{ > + struct find_child_context *context = data; > + unsigned long long addr; > + acpi_status status; > + > + status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &addr); > + if (ACPI_FAILURE(status) || addr != context->addr) > + return AE_OK; > + > + if (!context->ret) { > + /* This is the first matching object. Save its handle. */ > + context->ret = handle; > + return AE_OK; > + } > + /* > + * There is one more matching object with the same _ADR value. That "more than one matching object ..."? > + * really shouldn't happen, so we are kind of beyond the scope of the > + * spec here. We have to choose which one to return, though. I don't know enough about ACPI to know whether multiple objects with the same _ADR are legal. I haven't seen an explicit prohibition in the spec, so it's one of those cases where I have to wonder if we're trying to force a square Linux peg into a round BIOS hole. The example at https://bugzilla.kernel.org/show_bug.cgi?id=60561#c16 is a case where two devices have the same _ADR but really don't seem to conflict with each other. > + * > + * First, check if the previously found object is good enough and > return > + * its handle if so. Second, check the same for the object that we've > + * just found. > + */ > + if (!context->ret_checked) { > + if (acpi_dev_suitable(context->ret, context->is_bridge)) > + return AE_CTRL_TERMINATE; > + else > + context->ret_checked = true; > + } > + if (acpi_dev_suitable(handle, context->is_bridge)) { > + context->ret = handle; > + return AE_CTRL_TERMINATE; > + } > + return AE_OK; > +} > + > +acpi_handle acpi_find_child(acpi_handle parent, u64 addr, bool is_bridge) > +{ > + if (parent) { > + struct find_child_context context = { > + .addr = addr, > + .is_bridge = is_bridge, > + }; > + > + acpi_walk_namespace(ACPI_TYPE_DEVICE, parent, 1, > do_find_child, > + NULL, &context, NULL); > + return context.ret; > + } > + return NULL; > } > -EXPORT_SYMBOL(acpi_get_child); > +EXPORT_SYMBOL_GPL(acpi_find_child); > > int acpi_bind_one(struct device *dev, acpi_handle handle) > { > Index: linux-pm/include/acpi/acpi_bus.h > =================================================================== > --- linux-pm.orig/include/acpi/acpi_bus.h > +++ linux-pm/include/acpi/acpi_bus.h > @@ -439,7 +439,11 @@ struct acpi_pci_root { > }; > > /* helper */ > -acpi_handle acpi_get_child(acpi_handle, u64); > +acpi_handle acpi_find_child(acpi_handle, u64, bool); > +static inline acpi_handle acpi_get_child(acpi_handle handle, u64 addr) > +{ > + return acpi_find_child(handle, addr, false); > +} The use of "get" in acpi_get_child() has nothing to do with refcounting, does it? In PCI, "get" typically implies that the function does acquire a reference and the caller is responsible for disposing of it. > int acpi_is_root_bridge(acpi_handle); > struct acpi_pci_root *acpi_pci_find_root(acpi_handle handle); > #define DEVICE_ACPI_HANDLE(dev) ((acpi_handle)ACPI_HANDLE(dev)) > Index: linux-pm/drivers/pci/pci-acpi.c > =================================================================== > --- linux-pm.orig/drivers/pci/pci-acpi.c > +++ linux-pm/drivers/pci/pci-acpi.c > @@ -309,13 +309,19 @@ void acpi_pci_remove_bus(struct pci_bus > /* ACPI bus type */ > static int acpi_pci_find_device(struct device *dev, acpi_handle *handle) > { > - struct pci_dev * pci_dev; > - u64 addr; > + struct pci_dev *pci_dev = to_pci_dev(dev); A blank line is typical here. > + /* > + * pci_is_bridge() is not suitable here, because pci_dev->subordinate > + * is set only after acpi_pci_find_device() has been called for the > + * given device. > + */ > + bool is_bridge = pci_dev->hdr_type == PCI_HEADER_TYPE_BRIDGE > + || pci_dev->hdr_type == PCI_HEADER_TYPE_CARDBUS; > + u64 addr; > > - pci_dev = to_pci_dev(dev); > /* Please ref to ACPI spec for the syntax of _ADR */ > addr = (PCI_SLOT(pci_dev->devfn) << 16) | PCI_FUNC(pci_dev->devfn); > - *handle = acpi_get_child(DEVICE_ACPI_HANDLE(dev->parent), addr); > + *handle = acpi_find_child(ACPI_HANDLE(dev->parent), addr, is_bridge); > if (!*handle) > return -ENODEV; > return 0; It's trivial, but I do agree with Bob's preference for not modifying the return value when returning failure, e.g., h = acpi_find_child(...); if (!h) return -ENODEV; *handle = h; return 0; That makes it clear that there's only one way to check for success. If we return -ENODEV and also set "*handle = NULL", a sloppy caller could check "*handle" for NULL, which would appear to work but isn't actually correct. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
