Hello,
On Fri, Mar 24, 2023 at 10:09:55AM +0100, Uwe Kleine-König wrote:
> Commit d6fb6ee1820c ("ACPI: bus: Drop driver member of struct
> acpi_device") removed acpi_device::driver in favour of struct
> acpi_device::dev.driver.
>
> However there is a problem: While the two pointers are equivalent after
> the device is completely probed, there is a small time frame where
> acpi_device::dev->driver is already set but acpi_device::driver wasn't.
>
> The function acpi_bus_notify() used to have a check for
> acpi_device::driver that was changed to a check for adev->dev.driver in
> Commit d6fb6ee1820c.
>
> Pierre Asselin reports that starting with above change his laptop
> crashed during boot when on AC power. That's because acpi_bus_notify()
> is called in that small time frame (triggered by acpi_ac_add()) which
> results in a call to acpi_ac_notify while this function isn't ready yet.
>
> So in acpi_bus_notify() check for the device being bound (which becomes
> true only after the acpi probe call succeeds) instead of for
> acpi_device::dev.driver.
>
> Note that usually you have to hold the device lock to call
> device_is_bound(). I don't think this is the case here, so there likely
> is a race condition. The problem might be that the driver unbinds after
> the check but before adev->dev.driver is evaluated. However this race
> already existed before commit d6fb6ee1820c, so the change here is a net
> improvement even though it might not result in a completely correct
> handling.
>
> A similar check in the acpi sysfs code is also converted. This is less
> critical as it happens in code that is run when a sysfs file is
> accessed. That shouldn't happen for a device that isn't bound.
>
> Fixes: d6fb6ee1820c ("ACPI: bus: Drop driver member of struct acpi_device")
> Reported-by: Pierre Asselin <pa@xxxxxxxxx>
> Link: https://lore.kernel.org/linux-acpi/9f6cba7a8a57e5a687c934e8e406e28c.squirrel@xxxxxxxxxxxxxx
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> ---
> drivers/acpi/bus.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 9531dd0fef50..a5a8f82981ce 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -518,7 +518,7 @@ static void acpi_bus_notify(acpi_handle handle, u32 type, void *data)
> if (!adev)
> goto err;
>
> - if (adev->dev.driver) {
> + if (device_is_bound(&adev->dev)) {
> struct acpi_driver *driver = to_acpi_driver(adev->dev.driver);
>
> if (driver && driver->ops.notify &&
>
there is a hunk that I failed to add to the commit:
diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index 0fbfbaa8d8e3..2ee756d68052 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -376,7 +376,7 @@ eject_store(struct device *d, struct device_attribute *attr,
return -EINVAL;
if ((!acpi_device->handler || !acpi_device->handler->hotplug.enabled)
- && !d->driver)
+ && !device_is_bound(d))
return -ENODEV;
status = acpi_get_type(acpi_device->handle, ¬_used);
Rafael sait in the thread where Pierre reported the problem that he has
a different fix in mind. So I guess it's not worth to send a v2.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
