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:
>
>
>
