[PATCH 1/2] ACPI: scan: Move acpi_scan_dep_init() higher up in scan.c

[PATCH 1/2] ACPI: scan: Move acpi_scan_dep_init() higher up in scan.c

[Hans de Goede]

Move acpi_scan_dep_init() higher up in scan.c to avoid needing a forward
declaration in the next patch in this set.

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

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 1584c9e463bd..19f8fd6ea17a 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1655,6 +1655,20 @@ void acpi_device_add_finalize(struct acpi_device *device)
 	kobject_uevent(&device->dev.kobj, KOBJ_ADD);
 }
 
+static void acpi_scan_dep_init(struct acpi_device *adev)
+{
+	struct acpi_dep_data *dep;
+
+	mutex_lock(&acpi_dep_list_lock);
+
+	list_for_each_entry(dep, &acpi_dep_list, node) {
+		if (dep->consumer == adev->handle)
+			adev->dep_unmet++;
+	}
+
+	mutex_unlock(&acpi_dep_list_lock);
+}
+
 static int acpi_add_single_object(struct acpi_device **child,
 				  acpi_handle handle, int type,
 				  unsigned long long sta)
@@ -1906,20 +1920,6 @@ static u32 acpi_scan_check_dep(acpi_handle handle)
 	return count;
 }
 
-static void acpi_scan_dep_init(struct acpi_device *adev)
-{
-	struct acpi_dep_data *dep;
-
-	mutex_lock(&acpi_dep_list_lock);
-
-	list_for_each_entry(dep, &acpi_dep_list, node) {
-		if (dep->consumer == adev->handle)
-			adev->dep_unmet++;
-	}
-
-	mutex_unlock(&acpi_dep_list_lock);
-}
-
 static bool acpi_bus_scan_second_pass;
 
 static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
-- 
2.30.2

[PATCH 2/2] ACPI: scan: Fix _STA getting called on devices with unmet dependencies

[Hans de Goede]

Commit 71da201f38df ("ACPI: scan: Defer enumeration of devices with
_DEP lists") dropped the following 2 lines from acpi_init_device_object():

	/* Assume there are unmet deps until acpi_device_dep_initialize() runs */
	device->dep_unmet = 1;

Leaving the initial value of dep_unmet at the 0 from the kzalloc(). This
causes the acpi_bus_get_status() call in acpi_add_single_object() to
actually call _STA, even though there maybe unmet deps, leading to errors
like these:

[    0.123579] ACPI Error: No handler for Region [ECRM] (00000000ba9edc4c)
               [GenericSerialBus] (20170831/evregion-166)
[    0.123601] ACPI Error: Region GenericSerialBus (ID=9) has no handler
               (20170831/exfldio-299)
[    0.123618] ACPI Error: Method parse/execution failed
               \_SB.I2C1.BAT1._STA, AE_NOT_EXIST (20170831/psparse-550)

Fix this by moving the acpi_scan_dep_init() call done for devices added
during the second pass done by acpi_bus_scan() to inside
acpi_add_single_object(), so that dep_unmet is properly initialized
before the acpi_bus_get_status() call.

This re-fixes the issue initially fixed by
commit 63347db0affa ("ACPI / scan: Use acpi_bus_get_status() to initialize
ACPI_TYPE_DEVICE devs"), which introduced the removed
"device->dep_unmet = 1;" statement.

This issue was noticed; and the fix tested on a Dell Venue 10 Pro 5055.

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

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 19f8fd6ea17a..0d0176559bc2 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1671,7 +1671,8 @@ static void acpi_scan_dep_init(struct acpi_device *adev)
 
 static int acpi_add_single_object(struct acpi_device **child,
 				  acpi_handle handle, int type,
-				  unsigned long long sta)
+				  unsigned long long sta,
+				  bool init_dep)
 {
 	struct acpi_device_info *info = NULL;
 	struct acpi_device *device;
@@ -1693,9 +1694,13 @@ static int acpi_add_single_object(struct acpi_device **child,
 	 * that we can call acpi_bus_get_status() and use its quirk handling.
 	 * Note this must be done before the get power-/wakeup_dev-flags calls.
 	 */
-	if (type == ACPI_BUS_TYPE_DEVICE)
+	if (type == ACPI_BUS_TYPE_DEVICE) {
+		if (init_dep)
+			acpi_scan_dep_init(device);
+
 		if (acpi_bus_get_status(device) < 0)
 			acpi_set_device_status(device, 0);
+	}
 
 	acpi_bus_get_power_flags(device);
 	acpi_bus_get_wakeup_device_flags(device);
@@ -1952,13 +1957,11 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
 		}
 	}
 
-	acpi_add_single_object(&device, handle, type, sta);
+	acpi_add_single_object(&device, handle, type, sta, !check_dep);
 	if (!device)
 		return AE_CTRL_DEPTH;
 
 	acpi_scan_init_hotplug(device);
-	if (!check_dep)
-		acpi_scan_dep_init(device);
 
 out:
 	if (!*adev_p)
@@ -2221,7 +2224,7 @@ int acpi_bus_register_early_device(int type)
 	int result;
 
 	result = acpi_add_single_object(&device, NULL,
-					type, ACPI_STA_DEFAULT);
+					type, ACPI_STA_DEFAULT, false);
 	if (result)
 		return result;
 
@@ -2242,7 +2245,7 @@ static int acpi_bus_scan_fixed(void)
 
 		result = acpi_add_single_object(&device, NULL,
 						ACPI_BUS_TYPE_POWER_BUTTON,
-						ACPI_STA_DEFAULT);
+						ACPI_STA_DEFAULT, false);
 		if (result)
 			return result;
 
@@ -2259,7 +2262,7 @@ static int acpi_bus_scan_fixed(void)
 
 		result = acpi_add_single_object(&device, NULL,
 						ACPI_BUS_TYPE_SLEEP_BUTTON,
-						ACPI_STA_DEFAULT);
+						ACPI_STA_DEFAULT, false);
 		if (result)
 			return result;
 
-- 
2.30.2

Re: [PATCH 2/2] ACPI: scan: Fix _STA getting called on devices with unmet dependencies

[Rafael J. Wysocki]

On Sunday, March 28, 2021 1:20:00 PM CEST Hans de Goede wrote:
> Commit 71da201f38df ("ACPI: scan: Defer enumeration of devices with
> _DEP lists") dropped the following 2 lines from acpi_init_device_object():
> 
> 	/* Assume there are unmet deps until acpi_device_dep_initialize() runs */
> 	device->dep_unmet = 1;
> 
> Leaving the initial value of dep_unmet at the 0 from the kzalloc(). This
> causes the acpi_bus_get_status() call in acpi_add_single_object() to
> actually call _STA, even though there maybe unmet deps, leading to errors
> like these:
> 
> [    0.123579] ACPI Error: No handler for Region [ECRM] (00000000ba9edc4c)
>                [GenericSerialBus] (20170831/evregion-166)
> [    0.123601] ACPI Error: Region GenericSerialBus (ID=9) has no handler
>                (20170831/exfldio-299)
> [    0.123618] ACPI Error: Method parse/execution failed
>                \_SB.I2C1.BAT1._STA, AE_NOT_EXIST (20170831/psparse-550)
> 
> Fix this by moving the acpi_scan_dep_init() call done for devices added
> during the second pass done by acpi_bus_scan() to inside
> acpi_add_single_object(), so that dep_unmet is properly initialized
> before the acpi_bus_get_status() call.

I wonder why the change below can't be made instead.

The behavior would be closer to the original then AFAICS.

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

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1647,6 +1647,8 @@ void acpi_init_device_object(struct acpi
 	device_initialize(&device->dev);
 	dev_set_uevent_suppress(&device->dev, true);
 	acpi_init_coherency(device);
+	/* Assume there are unmet deps to start with. */
+	device->dep_unmet = 1;
 }
 
 void acpi_device_add_finalize(struct acpi_device *device)
@@ -1957,7 +1959,13 @@ static acpi_status acpi_bus_check_add(ac
 		return AE_CTRL_DEPTH;
 
 	acpi_scan_init_hotplug(device);
-	if (!check_dep)
+	/*
+	 * If check_dep is true at this point, the device has no dependencies,
+	 * or the creation of the device object would have been postponed above.
+	 */
+	if (check_dep)
+		device->dep_unmet = 0;
+	else
 		acpi_scan_dep_init(device);
 
 out:

Re: [PATCH 2/2] ACPI: scan: Fix _STA getting called on devices with unmet dependencies

[Hans de Goede]

Hi,

On 3/29/21 3:39 PM, Rafael J. Wysocki wrote:
> On Sunday, March 28, 2021 1:20:00 PM CEST Hans de Goede wrote:
>> Commit 71da201f38df ("ACPI: scan: Defer enumeration of devices with
>> _DEP lists") dropped the following 2 lines from acpi_init_device_object():
>>
>> 	/* Assume there are unmet deps until acpi_device_dep_initialize() runs */
>> 	device->dep_unmet = 1;
>>
>> Leaving the initial value of dep_unmet at the 0 from the kzalloc(). This
>> causes the acpi_bus_get_status() call in acpi_add_single_object() to
>> actually call _STA, even though there maybe unmet deps, leading to errors
>> like these:
>>
>> [    0.123579] ACPI Error: No handler for Region [ECRM] (00000000ba9edc4c)
>>                [GenericSerialBus] (20170831/evregion-166)
>> [    0.123601] ACPI Error: Region GenericSerialBus (ID=9) has no handler
>>                (20170831/exfldio-299)
>> [    0.123618] ACPI Error: Method parse/execution failed
>>                \_SB.I2C1.BAT1._STA, AE_NOT_EXIST (20170831/psparse-550)
>>
>> Fix this by moving the acpi_scan_dep_init() call done for devices added
>> during the second pass done by acpi_bus_scan() to inside
>> acpi_add_single_object(), so that dep_unmet is properly initialized
>> before the acpi_bus_get_status() call.
> 
> I wonder why the change below can't be made instead.
> 
> The behavior would be closer to the original then AFAICS.

Right the behavior would be closer to the code before the 2 fase scan
rework. But just actually making sure we have the right value in unmet_dep
a tiny bit earlier seems cleaner to me.

Note that the one acpi_add_single_object() call which actually sets the
new init_dep parameter to true and the previous place of calling
acpi_scan_dep_init() are very close together, here is the code before
this patch:

        acpi_add_single_object(&device, handle, type, sta, !check_dep);
        if (!device)
                return AE_CTRL_DEPTH;

        acpi_scan_init_hotplug(device);
        if (!check_dep)
                acpi_scan_dep_init();

So we are only doing the acpi_scan_dep_init() call a tiny bit earlier
and wrt which locks are being held when it gets called no changes are
made since it is still called as part of the call-graph below
acpi_bus_check_add(), I explicitly checked the locking situation because
that was my one worry with this patch.
		
And this new approach also has the advantage of not setting dep_unmet=1
(and never clearing it) for devices instantiated through:

acpi_bus_register_early_device()
acpi_bus_scan_fixed()
acpi_add_power_resource()

IOW while looking into fixing the regression of the errors being logged
again I tried to do a cleaner fix then last time.

With that said if you prefer the version you suggest let me know and I'll
post a single v2 patch doing things that way.

If you want to go with your suggestion, shall I then add a dep_unmet=0
statement to the 3 mentioned functions which leave it at 1 when going back
to the old way of handling this ?

Regards,

Hans






> 
> ---
>  drivers/acpi/scan.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -1647,6 +1647,8 @@ void acpi_init_device_object(struct acpi
>  	device_initialize(&device->dev);
>  	dev_set_uevent_suppress(&device->dev, true);
>  	acpi_init_coherency(device);
> +	/* Assume there are unmet deps to start with. */
> +	device->dep_unmet = 1;
>  }
>  
>  void acpi_device_add_finalize(struct acpi_device *device)
> @@ -1957,7 +1959,13 @@ static acpi_status acpi_bus_check_add(ac
>  		return AE_CTRL_DEPTH;
>  
>  	acpi_scan_init_hotplug(device);
> -	if (!check_dep)
> +	/*
> +	 * If check_dep is true at this point, the device has no dependencies,
> +	 * or the creation of the device object would have been postponed above.
> +	 */
> +	if (check_dep)
> +		device->dep_unmet = 0;
> +	else
>  		acpi_scan_dep_init(device);
>  
>  out:
> 
> 
> 

Re: [PATCH 2/2] ACPI: scan: Fix _STA getting called on devices with unmet dependencies

[Rafael J. Wysocki]

On Mon, Mar 29, 2021 at 4:46 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 3/29/21 3:39 PM, Rafael J. Wysocki wrote:
> > On Sunday, March 28, 2021 1:20:00 PM CEST Hans de Goede wrote:
> >> Commit 71da201f38df ("ACPI: scan: Defer enumeration of devices with
> >> _DEP lists") dropped the following 2 lines from acpi_init_device_object():
> >>
> >>      /* Assume there are unmet deps until acpi_device_dep_initialize() runs */
> >>      device->dep_unmet = 1;
> >>
> >> Leaving the initial value of dep_unmet at the 0 from the kzalloc(). This
> >> causes the acpi_bus_get_status() call in acpi_add_single_object() to
> >> actually call _STA, even though there maybe unmet deps, leading to errors
> >> like these:
> >>
> >> [    0.123579] ACPI Error: No handler for Region [ECRM] (00000000ba9edc4c)
> >>                [GenericSerialBus] (20170831/evregion-166)
> >> [    0.123601] ACPI Error: Region GenericSerialBus (ID=9) has no handler
> >>                (20170831/exfldio-299)
> >> [    0.123618] ACPI Error: Method parse/execution failed
> >>                \_SB.I2C1.BAT1._STA, AE_NOT_EXIST (20170831/psparse-550)
> >>
> >> Fix this by moving the acpi_scan_dep_init() call done for devices added
> >> during the second pass done by acpi_bus_scan() to inside
> >> acpi_add_single_object(), so that dep_unmet is properly initialized
> >> before the acpi_bus_get_status() call.
> >
> > I wonder why the change below can't be made instead.
> >
> > The behavior would be closer to the original then AFAICS.
>
> Right the behavior would be closer to the code before the 2 fase scan
> rework. But just actually making sure we have the right value in unmet_dep
> a tiny bit earlier seems cleaner to me.
>
> Note that the one acpi_add_single_object() call which actually sets the
> new init_dep parameter to true and the previous place of calling
> acpi_scan_dep_init() are very close together, here is the code before
> this patch:
>
>         acpi_add_single_object(&device, handle, type, sta, !check_dep);
>         if (!device)
>                 return AE_CTRL_DEPTH;
>
>         acpi_scan_init_hotplug(device);
>         if (!check_dep)
>                 acpi_scan_dep_init();
>
> So we are only doing the acpi_scan_dep_init() call a tiny bit earlier
> and wrt which locks are being held when it gets called no changes are
> made since it is still called as part of the call-graph below
> acpi_bus_check_add(), I explicitly checked the locking situation because
> that was my one worry with this patch.
>
> And this new approach also has the advantage of not setting dep_unmet=1
> (and never clearing it) for devices instantiated through:
>
> acpi_bus_register_early_device()
> acpi_bus_scan_fixed()
> acpi_add_power_resource()
>
> IOW while looking into fixing the regression of the errors being logged
> again I tried to do a cleaner fix then last time.
>
> With that said if you prefer the version you suggest let me know and I'll
> post a single v2 patch doing things that way.

I'd prefer to do the simple fix at this stage of the development
cycle, so yes, please.

I agree that it would be better to initialize dep_unmet properly in
acpi_add_single_object(), but I'd do that a bit differently.

> If you want to go with your suggestion, shall I then add a dep_unmet=0
> statement to the 3 mentioned functions which leave it at 1 when going back
> to the old way of handling this ?

No, I'll take care of this separately.

Cheers!

Re: [PATCH 1/2] ACPI: scan: Move acpi_scan_dep_init() higher up in scan.c

[Rafael J. Wysocki]

On Sun, Mar 28, 2021 at 1:20 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Move acpi_scan_dep_init() higher up in scan.c to avoid needing a forward
> declaration in the next patch in this set.
>
> Fixes: 71da201f38df ("ACPI: scan: Defer enumeration of devices with _DEP lists")

Well, this doesn't seem applicable here, as the patch shouldn't make a
practical difference.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/scan.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 1584c9e463bd..19f8fd6ea17a 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1655,6 +1655,20 @@ void acpi_device_add_finalize(struct acpi_device *device)
>         kobject_uevent(&device->dev.kobj, KOBJ_ADD);
>  }
>
> +static void acpi_scan_dep_init(struct acpi_device *adev)
> +{
> +       struct acpi_dep_data *dep;
> +
> +       mutex_lock(&acpi_dep_list_lock);
> +
> +       list_for_each_entry(dep, &acpi_dep_list, node) {
> +               if (dep->consumer == adev->handle)
> +                       adev->dep_unmet++;
> +       }
> +
> +       mutex_unlock(&acpi_dep_list_lock);
> +}
> +
>  static int acpi_add_single_object(struct acpi_device **child,
>                                   acpi_handle handle, int type,
>                                   unsigned long long sta)
> @@ -1906,20 +1920,6 @@ static u32 acpi_scan_check_dep(acpi_handle handle)
>         return count;
>  }
>
> -static void acpi_scan_dep_init(struct acpi_device *adev)
> -{
> -       struct acpi_dep_data *dep;
> -
> -       mutex_lock(&acpi_dep_list_lock);
> -
> -       list_for_each_entry(dep, &acpi_dep_list, node) {
> -               if (dep->consumer == adev->handle)
> -                       adev->dep_unmet++;
> -       }
> -
> -       mutex_unlock(&acpi_dep_list_lock);
> -}
> -
>  static bool acpi_bus_scan_second_pass;
>
>  static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
> --
> 2.30.2
>

Re: [PATCH 1/2] ACPI: scan: Move acpi_scan_dep_init() higher up in scan.c

[Hans de Goede]

Hi,

On 3/29/21 2:52 PM, Rafael J. Wysocki wrote:
> On Sun, Mar 28, 2021 at 1:20 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Move acpi_scan_dep_init() higher up in scan.c to avoid needing a forward
>> declaration in the next patch in this set.
>>
>> Fixes: 71da201f38df ("ACPI: scan: Defer enumeration of devices with _DEP lists")
> 
> Well, this doesn't seem applicable here, as the patch shouldn't make a
> practical difference.

Its a pre-req for the actual fix, hence the fixes tag so that it will get
cherry-pick into the same stable-series as the actual fix (assuming the
actual fix gets merged as is).

Regards,

Hans


> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/acpi/scan.c | 28 ++++++++++++++--------------
>>  1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index 1584c9e463bd..19f8fd6ea17a 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -1655,6 +1655,20 @@ void acpi_device_add_finalize(struct acpi_device *device)
>>         kobject_uevent(&device->dev.kobj, KOBJ_ADD);
>>  }
>>
>> +static void acpi_scan_dep_init(struct acpi_device *adev)
>> +{
>> +       struct acpi_dep_data *dep;
>> +
>> +       mutex_lock(&acpi_dep_list_lock);
>> +
>> +       list_for_each_entry(dep, &acpi_dep_list, node) {
>> +               if (dep->consumer == adev->handle)
>> +                       adev->dep_unmet++;
>> +       }
>> +
>> +       mutex_unlock(&acpi_dep_list_lock);
>> +}
>> +
>>  static int acpi_add_single_object(struct acpi_device **child,
>>                                   acpi_handle handle, int type,
>>                                   unsigned long long sta)
>> @@ -1906,20 +1920,6 @@ static u32 acpi_scan_check_dep(acpi_handle handle)
>>         return count;
>>  }
>>
>> -static void acpi_scan_dep_init(struct acpi_device *adev)
>> -{
>> -       struct acpi_dep_data *dep;
>> -
>> -       mutex_lock(&acpi_dep_list_lock);
>> -
>> -       list_for_each_entry(dep, &acpi_dep_list, node) {
>> -               if (dep->consumer == adev->handle)
>> -                       adev->dep_unmet++;
>> -       }
>> -
>> -       mutex_unlock(&acpi_dep_list_lock);
>> -}
>> -
>>  static bool acpi_bus_scan_second_pass;
>>
>>  static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
>> --
>> 2.30.2
>>
>