Re: [PATCH v4 01/35] acpi: Adjust functions installing bus event handlers

["Rafael J. Wysocki" <rafael@kernel.org>]

On Mon, Jun 5, 2023 at 10:44 AM Wilczynski, Michal
<michal.wilczynski@intel.com> wrote:
>
> On 6/2/2023 8:34 PM, Rafael J. Wysocki wrote:
> > On Thursday, June 1, 2023 3:17:19 PM CEST Michal Wilczynski wrote:
> >> Currently acpi_device_install_notify_handler() and
> >> acpi_device_remove_notify_handler() always install acpi_notify_device()
> >> as a function handler, and only then the real .notify callback gets
> >> called. This is not efficient and doesn't provide any real advantage.
> >>
> >> Introduce new acpi_device_install_event_handler() and
> >> acpi_device_remove_event_handler(). Those functions are replacing old
> >> installers, and after all drivers switch to the new model, old installers
> >> will be removed at the end of the patchset.
> >>
> >> Make new installer/removal function arguments to take function pointer as
> >> an argument instead of using .notify callback. Introduce new variable in
> >> struct acpi_device, as fixed events still needs to be handled by an
> >> intermediary that would schedule them for later execution. This is due to
> >> fixed hardware event handlers being executed in interrupt context.
> >>
> >> Make acpi_device_install_event_handler() and
> >> acpi_device_remove_event_handler() non-static, and export symbols. This
> >> will allow the drivers to call them directly, instead of relying on
> >> .notify callback.
> >>
> >> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
> >> ---
> >>  drivers/acpi/bus.c      | 59 ++++++++++++++++++++++++++++++++++++++++-
> >>  include/acpi/acpi_bus.h |  7 +++++
> >>  2 files changed, 65 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> >> index d161ff707de4..cf2c2bfe29a0 100644
> >> --- a/drivers/acpi/bus.c
> >> +++ b/drivers/acpi/bus.c
> >> @@ -535,7 +535,7 @@ static void acpi_notify_device_fixed(void *data)
> >>      struct acpi_device *device = data;
> >>
> >>      /* Fixed hardware devices have no handles */
> >> -    acpi_notify_device(NULL, ACPI_FIXED_HARDWARE_EVENT, device);
> >> +    device->fixed_event_notify(NULL, ACPI_FIXED_HARDWARE_EVENT, device);
> >>  }
> >>
> >>  static u32 acpi_device_fixed_event(void *data)
> >> @@ -550,11 +550,13 @@ static int acpi_device_install_notify_handler(struct acpi_device *device,
> >>      acpi_status status;
> >>
> >>      if (device->device_type == ACPI_BUS_TYPE_POWER_BUTTON) {
> >> +            device->fixed_event_notify = acpi_notify_device;
> >>              status =
> >>                  acpi_install_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
> >>                                                   acpi_device_fixed_event,
> >>                                                   device);
> >>      } else if (device->device_type == ACPI_BUS_TYPE_SLEEP_BUTTON) {
> >> +            device->fixed_event_notify = acpi_notify_device;
> >>              status =
> >>                  acpi_install_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
> >>                                                   acpi_device_fixed_event,
> >> @@ -579,9 +581,11 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device,
> >>      if (device->device_type == ACPI_BUS_TYPE_POWER_BUTTON) {
> >>              acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
> >>                                              acpi_device_fixed_event);
> >> +            device->fixed_event_notify = NULL;
> >>      } else if (device->device_type == ACPI_BUS_TYPE_SLEEP_BUTTON) {
> >>              acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
> >>                                              acpi_device_fixed_event);
> >> +            device->fixed_event_notify = NULL;
> >>      } else {
> >>              u32 type = acpi_drv->flags & ACPI_DRIVER_ALL_NOTIFY_EVENTS ?
> >>                              ACPI_ALL_NOTIFY : ACPI_DEVICE_NOTIFY;
> >> @@ -592,6 +596,59 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device,
> >>      acpi_os_wait_events_complete();
> >>  }
> >>
> >> +int acpi_device_install_event_handler(struct acpi_device *device,
> >> +                                  u32 type,
> >> +                                  void (*notify)(acpi_handle, u32, void*))
> >> +{
> >> +    acpi_status status;
> >> +
> >> +    if (!notify)
> >> +            return -EINVAL;
> >> +
> >> +    if (device->device_type == ACPI_BUS_TYPE_POWER_BUTTON) {
> >> +            device->fixed_event_notify = notify;
> >> +            status =
> >> +                acpi_install_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
> >> +                                                 acpi_device_fixed_event,
> >> +                                                 device);
> >> +    } else if (device->device_type == ACPI_BUS_TYPE_SLEEP_BUTTON) {
> >> +            device->fixed_event_notify = notify;
> >> +            status =
> >> +                acpi_install_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
> >> +                                                 acpi_device_fixed_event,
> >> +                                                 device);
> >> +    } else {
> >> +            status = acpi_install_notify_handler(device->handle, type,
> >> +                                                 notify,
> >> +                                                 device);
> >> +    }
> >> +
> >> +    if (ACPI_FAILURE(status))
> >> +            return -EINVAL;
> >> +    return 0;
> >> +}
> >> +EXPORT_SYMBOL(acpi_device_install_event_handler);
> >> +
> >> +void acpi_device_remove_event_handler(struct acpi_device *device,
> >> +                                  u32 type,
> >> +                                  void (*notify)(acpi_handle, u32, void*))
> >> +{
> >> +    if (device->device_type == ACPI_BUS_TYPE_POWER_BUTTON) {
> >> +            acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
> >> +                                            acpi_device_fixed_event);
> >> +            device->fixed_event_notify = NULL;
> >> +    } else if (device->device_type == ACPI_BUS_TYPE_SLEEP_BUTTON) {
> >> +            acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
> >> +                                            acpi_device_fixed_event);
> >> +            device->fixed_event_notify = NULL;
> >> +    } else {
> >> +            acpi_remove_notify_handler(device->handle, type,
> >> +                                       notify);
> >> +    }
> >> +    acpi_os_wait_events_complete();
> >> +}
> >> +EXPORT_SYMBOL(acpi_device_remove_event_handler);
> >> +
> >>  /* Handle events targeting \_SB device (at present only graceful shutdown) */
> >>
> >>  #define ACPI_SB_NOTIFY_SHUTDOWN_REQUEST 0x81
> >> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> >> index a6affc0550b0..7fb411438b6f 100644
> >> --- a/include/acpi/acpi_bus.h
> >> +++ b/include/acpi/acpi_bus.h
> >> @@ -387,6 +387,7 @@ struct acpi_device {
> >>      struct list_head physical_node_list;
> >>      struct mutex physical_node_lock;
> >>      void (*remove)(struct acpi_device *);
> >> +    void (*fixed_event_notify)(acpi_handle handle, u32 type, void *data);
>
>
> Hi,
> Thank for you review,
>
> > This is a rather confusing change, because ->remove() above is not a driver
> > callback, whereas the new one would be.
> >
> > Moreover, it is rather wasteful, because the only devices needing it are
> > buttons, so for all of the other ACPI device objects the new callback pointer
> > would always be NULL.
> >
> > Finally, it is not necessary even.
>
> I was thinking about resolving this somehow in compile-time, but I guess was a bit
> afraid of refactoring too much code - didn't want to break anything.
>
> >
> > The key observation here is that there are only 2 drivers handling power and
> > sleep buttons that use ACPI fixed events: the ACPI button driver (button.c in
> > drivers/acpi) and the "tiny power button" driver (tiny-power-button.c in
> > drivers/acpi).  All of the other drivers don't need the "fixed event notify"
> > thing and these two can be modified to take care of all of it by themselves.
> >
> > So if something like the below is done prior to the rest of your series, the
> > rest will be about acpi_install/remove_notify_handler() only and you won't
> > even need the wrapper routines any more: driver may just be switched over
> > to using the ACPICA functions directly.
>
> Sure, will get your patch, apply it before my series and fix individual drivers to use acpica
> functions directly.

I have posted this series which replaces it  in the meantime:
https://lore.kernel.org/linux-acpi/1847933.atdPhlSkOF@kreacher

Moreover, I think that there's still a reason to use the wrappers.

Namely, the unregistration part needs to call
acpi_os_wait_events_complete() after the notify handler has been
unregistered and it's better to avoid code duplication related to
that.

Also the registration wrapper can be something like:

int acpi_dev_install_notify_handler(struct acpi_device *adev,
acpi_notify_handler handler, u32 handler_type)
{
    if (ACPI_FAILURE(acpi_install_notify_handler(adev->handle,
handler_type, handler, adev)))
        return -ENODEV;

    return 0;
}

which would be simpler to use than the "raw"
acpi_install_notify_handler() and using it would avoid a tiny bit of
code duplication IMV.