On Tuesday, July 23, 2013 10:47:13 AM tianyu.lan@xxxxxxxxx wrote: > From: Lan Tianyu <tianyu.lan@xxxxxxxxx> > > commit 33f767d7(ACPI: Rework acpi_get_child() to be more efficient) changes > the logic of choosing child in the do_acpi_find_child() when there are > multi child devices with same _ADR. Original, it returned the last one but > it return the first one after commit 33f767d7. I don't really think this is correct, because after that change we do a post-order walk, but we return the first handle found, while without it we would do a pre-order walk, but we would return the *last* handle found, which should be the same in both cases. Also as indicated here: https://bugzilla.kernel.org/show_bug.cgi?id=60561#c8 simply changing the walk ordering doesn't solve the problem. It looks like the other change made by commit 33f767d7 is at fualt here, which was to evaluate _ADR directly instead of using acpi_get_object_info(). That said I think we may try the patch below, but I'm going to rewrite the changelog a bit. Thanks, Rafael > This causes regression on the > Notebook P15SM that there is no ACPI handle for video card. This because > there are two ACPI nodes with same _ADR for associated PCIE port. One > contains video card's ACPI node and the other one doesn't(So called wrong > ACPI node.). The wrong one is the first child device on this machine. So > after commit 33f767d7, bbswitch can't find ACPI handle for video card. > Further more, both these device nodes don't have _STA. > > commit c7d9ca9(ACPI: add _STA evaluation at do_acpi_find_child()) also > changes the logic of choosing child. On some platforms, there are multiple > devices with the same "_ADR" and they are perceptively for WIN7, WIN8 and > Linux. If one of them is enabled, others will be disabled. The commit calls > acpi_bus_get_status_handle() to get device's status If a matching device object > exists, but is disabled, acpi_get_child() will continue to walk the namespace > in the hope of finding an enabled one. If one is found, its handle will be returned, > but otherwise the function will return the handle of the disabled object found. > For this case, Bios must provide _STA for these device nodes since this is designed > for OS to select which device node should be chosen. > > After commit c7d9ca9, the first enabled device will be chosen or if > all device nodes were disabled or acpi_bus_get_status_handle() always returned > error, the last one would be returned. But for device nodes without _STA, > according acpi spec "If a device object (including the processor object) does not > have an _STA object, then OSPM assumes that the device is present, enabled, shown > in the UI, and functioning." and so acpi_bus_get_status_handle() return 0x0F as > status for these devices without _STA. > > On the Notebook P15SM, both PCIE port device node with same _ADR don't have _STA > and are treated as ENABLED devices by do_acpi_find_child(). The first wrong device > node is returned. To workaround the issue, evaluate the _STA directly in the > do_acpi_find_child() and if all device nodes doesn't have _STA, the last one will > be chosen. This also will not affect the case for commit c7d9ca9. This have been > tested on Lenovo Y480 where commit 33f767d7 fixed one similar issue. > > Reference:https://bugzilla.kernel.org/show_bug.cgi?id=60561 > Reference:https://github.com/Bumblebee-Project/bbswitch/issues/65 > reference:https://github.com/Bumblebee-Project/bbswitch/issues/2#issuecomment-21101083 > Tested-by: Sergio Perez <Dagobertstaler@xxxxxxxxx> > Tested-by: Karol Herbst <git@xxxxxxxxxxxxxx> > Cc: <stable@xxxxxxxxxxxxxxx> #3.9+ > Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx> > --- > Hi Rafael: > This patch is based on the commit c7d9ca9 merged in v3.11 > and purposes to fix the regression introduced by 33f767d7 merged in v3.9. > So it's necessary to backport both commit c7d9ca9 and this patch to 3.9+ stable tree? > > drivers/acpi/glue.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c > index f680957..732c707 100644 > --- a/drivers/acpi/glue.c > +++ b/drivers/acpi/glue.c > @@ -87,7 +87,7 @@ static acpi_status do_acpi_find_child(acpi_handle handle, u32 lvl_not_used, > status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &addr); > if (ACPI_SUCCESS(status) && addr == *((u64 *)addr_p)) { > *ret_p = handle; > - status = acpi_bus_get_status_handle(handle, &sta); > + status = acpi_evaluate_integer(handle, "_STA", NULL, &sta); > if (ACPI_SUCCESS(status) && (sta & ACPI_STA_DEVICE_ENABLED)) > return AE_CTRL_TERMINATE; > } > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
