On Mon, Jun 5, 2023 at 10:44 AM Wilczynski, Michal
<michal.wilczynski@xxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >> 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.
