On Thu, Mar 23, 2023 at 11:14 PM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
>
> Hello Pierre,
>
> On Tue, Mar 07, 2023 at 02:31:49PM -0500, Pierre Asselin wrote:
> > > Maybe the following patch helps (on top of v6.3-rc1):
> > >
> > > 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 &&
> > >
> >
> > It does indeed "fix" 6.3-rc1. Modulo locking issues, which I am
> > not qualified to evaluate.
>
> Thanks for your prompt test feedback.
>
> The locked variant could look as follows:
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 9531dd0fef50..fddca263ac40 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -518,13 +518,15 @@ static void acpi_bus_notify(acpi_handle handle, u32 type, void *data)
> if (!adev)
> goto err;
>
> - if (adev->dev.driver) {
> + device_lock(&adev->dev);
> + if (device_is_bound(&adev->dev)) {
> struct acpi_driver *driver = to_acpi_driver(adev->dev.driver);
>
> if (driver && driver->ops.notify &&
> (driver->flags & ACPI_DRIVER_ALL_NOTIFY_EVENTS))
> driver->ops.notify(adev, type);
> }
> + device_unlock(&adev->dev);
>
> if (!hotplug_event) {
> acpi_put_acpi_dev(adev);
>
> Pierre: If you want to test that, I suggest to also enable
> PROVE_LOCKING.
>
> Rafael: I don't know if this would work and I hope you're able to judge
> this better than I do. The change without the lock is for sure better
> than the status quo.
It is better than the status quo, but it is not ideal either.
It appears to me that it is a mistake to invoke the driver's
->notify() callback from acpi_bus_notify(), because there is no
synchronization between this and the driver probe/remove, so basically
all drivers setting ACPI_DRIVER_ALL_NOTIFY_EVENTS in flags need to
synchronize their ->notify() callbacks with probe and remove by hand.
The AC driver evidently doesn't do that and I have not looked at the
other ones yet, but chances are that they don't do that either.
However, this is readily avoidable if the handler installed by
acpi_device_install_notify_handler() is installed for all events.
I'll send a patch along these lines shortly.
> I did a similar conversion as the blamed commit for pci that got
> reverted for similiar reasons. See
> 68da4e0eaaab421f228eac57cbe7505b136464af. (Added Bjorn and Robert to
> Cc:) I think the pci code suffers from a similar race even after
> reverting my change.
It is very likely that there is a race, but it may be very hard to trigger.
> If someone is able to find the right fix for one of
> them, that might be transferable to the other?!
I don't think so.
