[PATCH 5.11 regression-fix 0/1] ACPI: scan: Fix Battery devices sometimes never binding

[PATCH 5.11 regression-fix 0/1] ACPI: scan: Fix Battery devices sometimes never binding

[Hans de Goede]

Hi Rafael,

Sorry for the last minute 5.11 fix, but I only noticed this yesterday
(and wrote the fix today). I believe that this was covered up by the
broken acpi_bus_get_device() error handling which we debugged earlier :|

For details see the actual patch.

It would be nice if you can get this to Linus for 5.11. But I understand
this is cutting it pretty close to the release, so if it misses 5.11 then
this can be picked up by the linux-stable releases.

Regards,

Hans


Hans de Goede (1):
  ACPI: scan: Fix Battery devices sometimes never binding

 drivers/acpi/scan.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.29.2

[PATCH 1/1] ACPI: scan: Fix Battery devices sometimes never binding

[Hans de Goede]

With the new 2 step scanning process, which defers instantiating some
ACPI-devices based on their _DEP to the second step, the following may
happen:

1. During the first acpi_walk_namespace(acpi_bus_check_add) call
   acpi_scan_check_dep() gets called on the Battery ACPI dev handle and
   adds one or more deps for this handle to the acpi_dep_list

2. During the first acpi_bus_attach() call one or more of the suppliers of
   these deps get their driver attached and
   acpi_walk_dep_device_list(supplier_handle) gets called.

   At this point acpi_bus_get_device(dep->consumer) get called,
   but since the battery has DEPs it has not been instantiated during the
   first acpi_walk_namespace(acpi_bus_check_add), so the
   acpi_bus_get_device(dep->consumer) call fails.

   Before this commit, acpi_walk_dep_device_list() would now continue
   *without* removing the acpi_dep_data entry for this supplier,consumer
   pair from the acpi_dep_list.

3. During the second acpi_walk_namespace(acpi_bus_check_add) call
   an acpi_device gets instantiated for the battery and
   acpi_scan_dep_init() gets called to initialize its dep_unmet val.

   Before this commit, the dep_unmet count would include DEPs for
   suppliers for which acpi_walk_dep_device_list(supplier_handle)
   has already been called, so it will never become 0 and the
   ACPI battery driver will never get attached / bind.

Fix the ACPI battery driver never binding in this scenario by making
acpi_walk_dep_device_list() always remove matching acpi_dep_data
entries independent of the acpi_bus_get_device(dep->consumer) call
succeeding or not.

Fixes: 71da201f38df ("ACPI: scan: Defer enumeration of devices with _DEP lists")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/scan.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 5d7b2fcecf06..4ce54115e981 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2123,12 +2123,12 @@ void acpi_walk_dep_device_list(acpi_handle handle)
 	list_for_each_entry_safe(dep, tmp, &acpi_dep_list, node) {
 		if (dep->supplier == handle) {
 			acpi_bus_get_device(dep->consumer, &adev);
-			if (!adev)
-				continue;
 
-			adev->dep_unmet--;
-			if (!adev->dep_unmet)
-				acpi_bus_attach(adev, true);
+			if (adev) {
+				adev->dep_unmet--;
+				if (!adev->dep_unmet)
+					acpi_bus_attach(adev, true);
+			}
 
 			list_del(&dep->node);
 			kfree(dep);
-- 
2.29.2

Re: [PATCH 1/1] ACPI: scan: Fix Battery devices sometimes never binding

[Rafael J. Wysocki]

On Mon, Feb 1, 2021 at 5:35 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> With the new 2 step scanning process, which defers instantiating some
> ACPI-devices based on their _DEP to the second step, the following may
> happen:
>
> 1. During the first acpi_walk_namespace(acpi_bus_check_add) call
>    acpi_scan_check_dep() gets called on the Battery ACPI dev handle and
>    adds one or more deps for this handle to the acpi_dep_list
>
> 2. During the first acpi_bus_attach() call one or more of the suppliers of
>    these deps get their driver attached and
>    acpi_walk_dep_device_list(supplier_handle) gets called.
>
>    At this point acpi_bus_get_device(dep->consumer) get called,
>    but since the battery has DEPs it has not been instantiated during the
>    first acpi_walk_namespace(acpi_bus_check_add), so the
>    acpi_bus_get_device(dep->consumer) call fails.
>
>    Before this commit, acpi_walk_dep_device_list() would now continue
>    *without* removing the acpi_dep_data entry for this supplier,consumer
>    pair from the acpi_dep_list.

Yeah, I've overlooked the fact that the consumer needs to have a
struct acpi_device in order for the entry to be dropped from the list.
Sorry.

> 3. During the second acpi_walk_namespace(acpi_bus_check_add) call
>    an acpi_device gets instantiated for the battery and
>    acpi_scan_dep_init() gets called to initialize its dep_unmet val.
>
>    Before this commit, the dep_unmet count would include DEPs for
>    suppliers for which acpi_walk_dep_device_list(supplier_handle)
>    has already been called, so it will never become 0 and the
>    ACPI battery driver will never get attached / bind.
>
> Fix the ACPI battery driver never binding in this scenario by making
> acpi_walk_dep_device_list() always remove matching acpi_dep_data
> entries independent of the acpi_bus_get_device(dep->consumer) call
> succeeding or not.
>
> Fixes: 71da201f38df ("ACPI: scan: Defer enumeration of devices with _DEP lists")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied and thanks for fixing this!

> ---
>  drivers/acpi/scan.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 5d7b2fcecf06..4ce54115e981 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2123,12 +2123,12 @@ void acpi_walk_dep_device_list(acpi_handle handle)
>         list_for_each_entry_safe(dep, tmp, &acpi_dep_list, node) {
>                 if (dep->supplier == handle) {
>                         acpi_bus_get_device(dep->consumer, &adev);
> -                       if (!adev)
> -                               continue;
>
> -                       adev->dep_unmet--;
> -                       if (!adev->dep_unmet)
> -                               acpi_bus_attach(adev, true);
> +                       if (adev) {
> +                               adev->dep_unmet--;
> +                               if (!adev->dep_unmet)
> +                                       acpi_bus_attach(adev, true);
> +                       }
>
>                         list_del(&dep->node);
>                         kfree(dep);
> --
> 2.29.2
>