diff mbox series

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

Message ID 20210328112000.12502-2-hdegoede@redhat.com
State New
Headers show
Series [1/2] ACPI: scan: Move acpi_scan_dep_init() higher up in scan.c | expand

Commit Message

Hans de Goede March 28, 2021, 11:20 a.m. UTC 
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(-)

Comments

Rafael J. Wysocki March 29, 2021, 1:39 p.m. UTC | #1 
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:
Hans de Goede March 29, 2021, 2:45 p.m. UTC | #2 
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:

> 

> 

>
Rafael J. Wysocki March 29, 2021, 2:56 p.m. UTC | #3 
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!
diff mbox series

Patch

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;