| Message ID | 20230925144842.586829-2-michal.wilczynski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Replace acpi_driver with platform_driver | expand |
On Mon, Sep 25, 2023 at 6:31 PM Michal Wilczynski <michal.wilczynski@intel.com> wrote: > > acpi_dev_install_notify_handler() and acpi_dev_remove_notify_handler() > are wrappers around ACPICA installers. They are meant to save some > duplicated code from drivers. However as we're moving towards drivers > operating on platform_device they become a bit inconvenient to use as > inside the driver code we mostly want to use driver data of platform > device instead of ACPI device. That's fair enough, but -> > Make notify handlers installer wrappers more generic, while still > saving some code that would be duplicated otherwise. > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> > --- > > Notes: > So one solution could be to just replace acpi_device with > platform_device as an argument in those functions. However I don't > believe this is a correct solution, as it is very often the case that > drivers declare their own private structures which gets allocated during > the .probe() callback, and become the heart of the driver. When drivers > do that it makes much more sense to just pass the private structure > to the notify handler instead of forcing user to dance with the > platform_device or acpi_device. > > drivers/acpi/ac.c | 6 +++--- > drivers/acpi/acpi_video.c | 6 +++--- > drivers/acpi/battery.c | 6 +++--- > drivers/acpi/bus.c | 14 ++++++-------- > drivers/acpi/hed.c | 6 +++--- > drivers/acpi/nfit/core.c | 6 +++--- > drivers/acpi/thermal.c | 6 +++--- > include/acpi/acpi_bus.h | 9 ++++----- > 8 files changed, 28 insertions(+), 31 deletions(-) > > diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c > index 225dc6818751..0b245f9f7ec8 100644 > --- a/drivers/acpi/ac.c > +++ b/drivers/acpi/ac.c > @@ -256,8 +256,8 @@ static int acpi_ac_add(struct acpi_device *device) > ac->battery_nb.notifier_call = acpi_ac_battery_notify; > register_acpi_notifier(&ac->battery_nb); > > - result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, > - acpi_ac_notify); > + result = acpi_dev_install_notify_handler(device->handle, ACPI_ALL_NOTIFY, > + acpi_ac_notify, device); > if (result) > goto err_unregister; > > @@ -306,7 +306,7 @@ static void acpi_ac_remove(struct acpi_device *device) > > ac = acpi_driver_data(device); > > - acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY, > + acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, > acpi_ac_notify); > power_supply_unregister(ac->charger); > unregister_acpi_notifier(&ac->battery_nb); > diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c > index 948e31f7ce6e..025c17890127 100644 > --- a/drivers/acpi/acpi_video.c > +++ b/drivers/acpi/acpi_video.c > @@ -2059,8 +2059,8 @@ static int acpi_video_bus_add(struct acpi_device *device) > > acpi_video_bus_add_notify_handler(video); > > - error = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, > - acpi_video_bus_notify); > + error = acpi_dev_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, > + acpi_video_bus_notify, device); > if (error) > goto err_remove; > > @@ -2092,7 +2092,7 @@ static void acpi_video_bus_remove(struct acpi_device *device) > > video = acpi_driver_data(device); > > - acpi_dev_remove_notify_handler(device, ACPI_DEVICE_NOTIFY, > + acpi_dev_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, > acpi_video_bus_notify); > > mutex_lock(&video_list_lock); > diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c > index 969bf81e8d54..45dae32a8646 100644 > --- a/drivers/acpi/battery.c > +++ b/drivers/acpi/battery.c > @@ -1213,8 +1213,8 @@ static int acpi_battery_add(struct acpi_device *device) > > device_init_wakeup(&device->dev, 1); > > - result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, > - acpi_battery_notify); > + result = acpi_dev_install_notify_handler(device->handle, ACPI_ALL_NOTIFY, > + acpi_battery_notify, device); > if (result) > goto fail_pm; > > @@ -1241,7 +1241,7 @@ static void acpi_battery_remove(struct acpi_device *device) > > battery = acpi_driver_data(device); > > - acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY, > + acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, > acpi_battery_notify); > > device_init_wakeup(&device->dev, 0); > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c > index f41dda2d3493..479fe888d629 100644 > --- a/drivers/acpi/bus.c > +++ b/drivers/acpi/bus.c > @@ -554,14 +554,13 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device, > acpi_os_wait_events_complete(); > } > > -int acpi_dev_install_notify_handler(struct acpi_device *adev, > - u32 handler_type, > - acpi_notify_handler handler) > +int acpi_dev_install_notify_handler(acpi_handle handle, u32 handler_type, > + acpi_notify_handler handler, void *context) > { > acpi_status status; > > - status = acpi_install_notify_handler(adev->handle, handler_type, > - handler, adev); > + status = acpi_install_notify_handler(handle, handler_type, > + handler, context); The wrapper now takes exactly the same arguments as the wrapped function, so what exactly is the point of having it? The return value type? > if (ACPI_FAILURE(status)) > return -ENODEV; > > @@ -569,11 +568,10 @@ int acpi_dev_install_notify_handler(struct acpi_device *adev, > } > EXPORT_SYMBOL_GPL(acpi_dev_install_notify_handler); > > -void acpi_dev_remove_notify_handler(struct acpi_device *adev, > - u32 handler_type, > +void acpi_dev_remove_notify_handler(acpi_handle handle, u32 handler_type, > acpi_notify_handler handler) > { > - acpi_remove_notify_handler(adev->handle, handler_type, handler); > + acpi_remove_notify_handler(handle, handler_type, handler); > acpi_os_wait_events_complete(); Here at least there is the extra workqueues synchronization point. That said, why exactly is it better to use acpi_handle instead of a struct acpi_device pointer? Realistically, in a platform driver you'll need the latter to obtain the former anyway.
Hi, Thanks for your review ! On 10/4/2023 9:09 PM, Rafael J. Wysocki wrote: > On Mon, Sep 25, 2023 at 6:31 PM Michal Wilczynski > <michal.wilczynski@intel.com> wrote: >> acpi_dev_install_notify_handler() and acpi_dev_remove_notify_handler() >> are wrappers around ACPICA installers. They are meant to save some >> duplicated code from drivers. However as we're moving towards drivers >> operating on platform_device they become a bit inconvenient to use as >> inside the driver code we mostly want to use driver data of platform >> device instead of ACPI device. > That's fair enough, but -> > >> Make notify handlers installer wrappers more generic, while still >> saving some code that would be duplicated otherwise. >> >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> >> --- >> >> Notes: >> So one solution could be to just replace acpi_device with >> platform_device as an argument in those functions. However I don't >> believe this is a correct solution, as it is very often the case that >> drivers declare their own private structures which gets allocated during >> the .probe() callback, and become the heart of the driver. When drivers >> do that it makes much more sense to just pass the private structure >> to the notify handler instead of forcing user to dance with the >> platform_device or acpi_device. >> >> drivers/acpi/ac.c | 6 +++--- >> drivers/acpi/acpi_video.c | 6 +++--- >> drivers/acpi/battery.c | 6 +++--- >> drivers/acpi/bus.c | 14 ++++++-------- >> drivers/acpi/hed.c | 6 +++--- >> drivers/acpi/nfit/core.c | 6 +++--- >> drivers/acpi/thermal.c | 6 +++--- >> include/acpi/acpi_bus.h | 9 ++++----- >> 8 files changed, 28 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c >> index 225dc6818751..0b245f9f7ec8 100644 >> --- a/drivers/acpi/ac.c >> +++ b/drivers/acpi/ac.c >> @@ -256,8 +256,8 @@ static int acpi_ac_add(struct acpi_device *device) >> ac->battery_nb.notifier_call = acpi_ac_battery_notify; >> register_acpi_notifier(&ac->battery_nb); >> >> - result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, >> - acpi_ac_notify); >> + result = acpi_dev_install_notify_handler(device->handle, ACPI_ALL_NOTIFY, >> + acpi_ac_notify, device); >> if (result) >> goto err_unregister; >> >> @@ -306,7 +306,7 @@ static void acpi_ac_remove(struct acpi_device *device) >> >> ac = acpi_driver_data(device); >> >> - acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY, >> + acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, >> acpi_ac_notify); >> power_supply_unregister(ac->charger); >> unregister_acpi_notifier(&ac->battery_nb); >> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c >> index 948e31f7ce6e..025c17890127 100644 >> --- a/drivers/acpi/acpi_video.c >> +++ b/drivers/acpi/acpi_video.c >> @@ -2059,8 +2059,8 @@ static int acpi_video_bus_add(struct acpi_device *device) >> >> acpi_video_bus_add_notify_handler(video); >> >> - error = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, >> - acpi_video_bus_notify); >> + error = acpi_dev_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, >> + acpi_video_bus_notify, device); >> if (error) >> goto err_remove; >> >> @@ -2092,7 +2092,7 @@ static void acpi_video_bus_remove(struct acpi_device *device) >> >> video = acpi_driver_data(device); >> >> - acpi_dev_remove_notify_handler(device, ACPI_DEVICE_NOTIFY, >> + acpi_dev_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, >> acpi_video_bus_notify); >> >> mutex_lock(&video_list_lock); >> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c >> index 969bf81e8d54..45dae32a8646 100644 >> --- a/drivers/acpi/battery.c >> +++ b/drivers/acpi/battery.c >> @@ -1213,8 +1213,8 @@ static int acpi_battery_add(struct acpi_device *device) >> >> device_init_wakeup(&device->dev, 1); >> >> - result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, >> - acpi_battery_notify); >> + result = acpi_dev_install_notify_handler(device->handle, ACPI_ALL_NOTIFY, >> + acpi_battery_notify, device); >> if (result) >> goto fail_pm; >> >> @@ -1241,7 +1241,7 @@ static void acpi_battery_remove(struct acpi_device *device) >> >> battery = acpi_driver_data(device); >> >> - acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY, >> + acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, >> acpi_battery_notify); >> >> device_init_wakeup(&device->dev, 0); >> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c >> index f41dda2d3493..479fe888d629 100644 >> --- a/drivers/acpi/bus.c >> +++ b/drivers/acpi/bus.c >> @@ -554,14 +554,13 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device, >> acpi_os_wait_events_complete(); >> } >> >> -int acpi_dev_install_notify_handler(struct acpi_device *adev, >> - u32 handler_type, >> - acpi_notify_handler handler) >> +int acpi_dev_install_notify_handler(acpi_handle handle, u32 handler_type, >> + acpi_notify_handler handler, void *context) >> { >> acpi_status status; >> >> - status = acpi_install_notify_handler(adev->handle, handler_type, >> - handler, adev); >> + status = acpi_install_notify_handler(handle, handler_type, >> + handler, context); > The wrapper now takes exactly the same arguments as the wrapped > function, so what exactly is the point of having it? The return value > type? I considered removing the wrapper altogether, but decided not to do so. One trivial advantage of leaving this wrapper is the return value type as you noticed, another is that the removal wrapper actually does something extra and removing it would result in duplicate code among the drivers. So I didn't really want to remove the 'install' wrapper but leave the 'remove' wrapper, as I think this might be confusing for the future reader. In my mind if something is removed by the wrapper it should also be installed by the wrapper. > >> if (ACPI_FAILURE(status)) >> return -ENODEV; >> >> @@ -569,11 +568,10 @@ int acpi_dev_install_notify_handler(struct acpi_device *adev, >> } >> EXPORT_SYMBOL_GPL(acpi_dev_install_notify_handler); >> >> -void acpi_dev_remove_notify_handler(struct acpi_device *adev, >> - u32 handler_type, >> +void acpi_dev_remove_notify_handler(acpi_handle handle, u32 handler_type, >> acpi_notify_handler handler) >> { >> - acpi_remove_notify_handler(adev->handle, handler_type, handler); >> + acpi_remove_notify_handler(handle, handler_type, handler); >> acpi_os_wait_events_complete(); > Here at least there is the extra workqueues synchronization point. > > That said, why exactly is it better to use acpi_handle instead of a > struct acpi_device pointer? I wanted to make the wrapper as close as possible to the wrapped function. This way it would be easier to remove it in the future i.e if we ever deem extra synchronization not worth it etc. What the ACPICA function need to install a wrapper is a handle not a pointer to a device. So there is no need for a middle man. > > Realistically, in a platform driver you'll need the latter to obtain > the former anyway. I don't want to introduce arbitrary limitations where they are not necessary. It is often the case that driver allocates it's own private struct using kmalloc family of functions, and that structure already contains everything that is needed to remove the handler, so why force ? There are already examples in the drivers that do that i.e in acpi_video the function acpi_video_dev_add_notify_handler() uses raw ACPICA handler to install a notify handler and it passes private structure there. So there is value in leaving the choice of an actual type to the user of the API. To summarize: I would say the wrappers are mostly unnecessary, but they actually save some duplicate code in the drivers, so I decided to leave them, as I don't want to introduce duplicate code if I can avoid that. Thanks ! Michał
On Thu, Oct 5, 2023 at 10:10 AM Wilczynski, Michal <michal.wilczynski@intel.com> wrote: > > Hi, > > Thanks for your review ! > > On 10/4/2023 9:09 PM, Rafael J. Wysocki wrote: > > On Mon, Sep 25, 2023 at 6:31 PM Michal Wilczynski > > <michal.wilczynski@intel.com> wrote: > >> acpi_dev_install_notify_handler() and acpi_dev_remove_notify_handler() > >> are wrappers around ACPICA installers. They are meant to save some > >> duplicated code from drivers. However as we're moving towards drivers > >> operating on platform_device they become a bit inconvenient to use as > >> inside the driver code we mostly want to use driver data of platform > >> device instead of ACPI device. > > That's fair enough, but -> > > > >> Make notify handlers installer wrappers more generic, while still > >> saving some code that would be duplicated otherwise. > >> > >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > >> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> > >> --- > >> > >> Notes: > >> So one solution could be to just replace acpi_device with > >> platform_device as an argument in those functions. However I don't > >> believe this is a correct solution, as it is very often the case that > >> drivers declare their own private structures which gets allocated during > >> the .probe() callback, and become the heart of the driver. When drivers > >> do that it makes much more sense to just pass the private structure > >> to the notify handler instead of forcing user to dance with the > >> platform_device or acpi_device. > >> > >> drivers/acpi/ac.c | 6 +++--- > >> drivers/acpi/acpi_video.c | 6 +++--- > >> drivers/acpi/battery.c | 6 +++--- > >> drivers/acpi/bus.c | 14 ++++++-------- > >> drivers/acpi/hed.c | 6 +++--- > >> drivers/acpi/nfit/core.c | 6 +++--- > >> drivers/acpi/thermal.c | 6 +++--- > >> include/acpi/acpi_bus.h | 9 ++++----- > >> 8 files changed, 28 insertions(+), 31 deletions(-) > >> > >> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c > >> index 225dc6818751..0b245f9f7ec8 100644 > >> --- a/drivers/acpi/ac.c > >> +++ b/drivers/acpi/ac.c > >> @@ -256,8 +256,8 @@ static int acpi_ac_add(struct acpi_device *device) > >> ac->battery_nb.notifier_call = acpi_ac_battery_notify; > >> register_acpi_notifier(&ac->battery_nb); > >> > >> - result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, > >> - acpi_ac_notify); > >> + result = acpi_dev_install_notify_handler(device->handle, ACPI_ALL_NOTIFY, > >> + acpi_ac_notify, device); > >> if (result) > >> goto err_unregister; > >> > >> @@ -306,7 +306,7 @@ static void acpi_ac_remove(struct acpi_device *device) > >> > >> ac = acpi_driver_data(device); > >> > >> - acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY, > >> + acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, > >> acpi_ac_notify); > >> power_supply_unregister(ac->charger); > >> unregister_acpi_notifier(&ac->battery_nb); > >> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c > >> index 948e31f7ce6e..025c17890127 100644 > >> --- a/drivers/acpi/acpi_video.c > >> +++ b/drivers/acpi/acpi_video.c > >> @@ -2059,8 +2059,8 @@ static int acpi_video_bus_add(struct acpi_device *device) > >> > >> acpi_video_bus_add_notify_handler(video); > >> > >> - error = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, > >> - acpi_video_bus_notify); > >> + error = acpi_dev_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, > >> + acpi_video_bus_notify, device); > >> if (error) > >> goto err_remove; > >> > >> @@ -2092,7 +2092,7 @@ static void acpi_video_bus_remove(struct acpi_device *device) > >> > >> video = acpi_driver_data(device); > >> > >> - acpi_dev_remove_notify_handler(device, ACPI_DEVICE_NOTIFY, > >> + acpi_dev_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, > >> acpi_video_bus_notify); > >> > >> mutex_lock(&video_list_lock); > >> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c > >> index 969bf81e8d54..45dae32a8646 100644 > >> --- a/drivers/acpi/battery.c > >> +++ b/drivers/acpi/battery.c > >> @@ -1213,8 +1213,8 @@ static int acpi_battery_add(struct acpi_device *device) > >> > >> device_init_wakeup(&device->dev, 1); > >> > >> - result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, > >> - acpi_battery_notify); > >> + result = acpi_dev_install_notify_handler(device->handle, ACPI_ALL_NOTIFY, > >> + acpi_battery_notify, device); > >> if (result) > >> goto fail_pm; > >> > >> @@ -1241,7 +1241,7 @@ static void acpi_battery_remove(struct acpi_device *device) > >> > >> battery = acpi_driver_data(device); > >> > >> - acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY, > >> + acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, > >> acpi_battery_notify); > >> > >> device_init_wakeup(&device->dev, 0); > >> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c > >> index f41dda2d3493..479fe888d629 100644 > >> --- a/drivers/acpi/bus.c > >> +++ b/drivers/acpi/bus.c > >> @@ -554,14 +554,13 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device, > >> acpi_os_wait_events_complete(); > >> } > >> > >> -int acpi_dev_install_notify_handler(struct acpi_device *adev, > >> - u32 handler_type, > >> - acpi_notify_handler handler) > >> +int acpi_dev_install_notify_handler(acpi_handle handle, u32 handler_type, > >> + acpi_notify_handler handler, void *context) > >> { > >> acpi_status status; > >> > >> - status = acpi_install_notify_handler(adev->handle, handler_type, > >> - handler, adev); > >> + status = acpi_install_notify_handler(handle, handler_type, > >> + handler, context); > > The wrapper now takes exactly the same arguments as the wrapped > > function, so what exactly is the point of having it? The return value > > type? > > I considered removing the wrapper altogether, but decided not to do so. > One trivial advantage of leaving this wrapper is the return value type as > you noticed, another is that the removal wrapper actually does something > extra and removing it would result in duplicate code among the drivers. > So I didn't really want to remove the 'install' wrapper but leave the > 'remove' wrapper, as I think this might be confusing for the future reader. > In my mind if something is removed by the wrapper it should also be > installed by the wrapper. I agree here. > > > >> if (ACPI_FAILURE(status)) > >> return -ENODEV; > >> > >> @@ -569,11 +568,10 @@ int acpi_dev_install_notify_handler(struct acpi_device *adev, > >> } > >> EXPORT_SYMBOL_GPL(acpi_dev_install_notify_handler); > >> > >> -void acpi_dev_remove_notify_handler(struct acpi_device *adev, > >> - u32 handler_type, > >> +void acpi_dev_remove_notify_handler(acpi_handle handle, u32 handler_type, > >> acpi_notify_handler handler) > >> { > >> - acpi_remove_notify_handler(adev->handle, handler_type, handler); > >> + acpi_remove_notify_handler(handle, handler_type, handler); > >> acpi_os_wait_events_complete(); > > Here at least there is the extra workqueues synchronization point. > > > > That said, why exactly is it better to use acpi_handle instead of a > > struct acpi_device pointer? > > I wanted to make the wrapper as close as possible to the wrapped function. > This way it would be easier to remove it in the future i.e if we ever deem > extra synchronization not worth it etc. What the ACPICA function need to > install a wrapper is a handle not a pointer to a device. > So there is no need for a middle man. Taking a struct acpi_device pointer as the first argument is part of duplication reduction, however, because in the most common case it saves the users of it the need to dereference the struct acpi_device they get from ACPI_COMPANION() in order to obtain the handle. Arguably, acpi_handle is an ACPICA concept and it is better to reduce its usage outside ACPICA. > > > > Realistically, in a platform driver you'll need the latter to obtain > > the former anyway. > > I don't want to introduce arbitrary limitations where they are not necessary. I'm not sure what you mean. This patch is changing existing functions. > It is often the case that driver allocates it's own private struct using kmalloc > family of functions, and that structure already contains everything that is > needed to remove the handler, so why force ? There are already examples > in the drivers that do that i.e in acpi_video the function > acpi_video_dev_add_notify_handler() uses raw ACPICA handler to install > a notify handler and it passes private structure there. > So there is value in leaving the choice of an actual type to the user of the > API. No, if the user has a pointer to struct acpi_device already, there is no difference between passing this and passing the acpi_handle from it except for the extra dereference in the latter case. If the user doesn't have a struct acpi_device pointer, let them use the raw ACPICA handler directly and worry about the synchronization themselves. The wrappers are there to cover the most common case, not to cover all cases. > To summarize: > I would say the wrappers are mostly unnecessary, but they actually save > some duplicate code in the drivers, so I decided to leave them, as I don't > want to introduce duplicate code if I can avoid that. What duplicate code do you mean, exactly? IMV you haven't really explained why this particular patch is necessary or even useful. Thanks!
On 10/5/2023 12:57 PM, Rafael J. Wysocki wrote: > On Thu, Oct 5, 2023 at 10:10 AM Wilczynski, Michal > <michal.wilczynski@intel.com> wrote: >> Hi, >> >> Thanks for your review ! >> >> On 10/4/2023 9:09 PM, Rafael J. Wysocki wrote: >>> On Mon, Sep 25, 2023 at 6:31 PM Michal Wilczynski >>> <michal.wilczynski@intel.com> wrote: >>>> acpi_dev_install_notify_handler() and acpi_dev_remove_notify_handler() >>>> are wrappers around ACPICA installers. They are meant to save some >>>> duplicated code from drivers. However as we're moving towards drivers >>>> operating on platform_device they become a bit inconvenient to use as >>>> inside the driver code we mostly want to use driver data of platform >>>> device instead of ACPI device. >>> That's fair enough, but -> >>> >>>> Make notify handlers installer wrappers more generic, while still >>>> saving some code that would be duplicated otherwise. >>>> >>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> >>>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> >>>> --- >>>> >>>> Notes: >>>> So one solution could be to just replace acpi_device with >>>> platform_device as an argument in those functions. However I don't >>>> believe this is a correct solution, as it is very often the case that >>>> drivers declare their own private structures which gets allocated during >>>> the .probe() callback, and become the heart of the driver. When drivers >>>> do that it makes much more sense to just pass the private structure >>>> to the notify handler instead of forcing user to dance with the >>>> platform_device or acpi_device. >>>> >>>> drivers/acpi/ac.c | 6 +++--- >>>> drivers/acpi/acpi_video.c | 6 +++--- >>>> drivers/acpi/battery.c | 6 +++--- >>>> drivers/acpi/bus.c | 14 ++++++-------- >>>> drivers/acpi/hed.c | 6 +++--- >>>> drivers/acpi/nfit/core.c | 6 +++--- >>>> drivers/acpi/thermal.c | 6 +++--- >>>> include/acpi/acpi_bus.h | 9 ++++----- >>>> 8 files changed, 28 insertions(+), 31 deletions(-) >>>> >>>> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c >>>> index 225dc6818751..0b245f9f7ec8 100644 >>>> --- a/drivers/acpi/ac.c >>>> +++ b/drivers/acpi/ac.c >>>> @@ -256,8 +256,8 @@ static int acpi_ac_add(struct acpi_device *device) >>>> ac->battery_nb.notifier_call = acpi_ac_battery_notify; >>>> register_acpi_notifier(&ac->battery_nb); >>>> >>>> - result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, >>>> - acpi_ac_notify); >>>> + result = acpi_dev_install_notify_handler(device->handle, ACPI_ALL_NOTIFY, >>>> + acpi_ac_notify, device); >>>> if (result) >>>> goto err_unregister; >>>> >>>> @@ -306,7 +306,7 @@ static void acpi_ac_remove(struct acpi_device *device) >>>> >>>> ac = acpi_driver_data(device); >>>> >>>> - acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY, >>>> + acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, >>>> acpi_ac_notify); >>>> power_supply_unregister(ac->charger); >>>> unregister_acpi_notifier(&ac->battery_nb); >>>> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c >>>> index 948e31f7ce6e..025c17890127 100644 >>>> --- a/drivers/acpi/acpi_video.c >>>> +++ b/drivers/acpi/acpi_video.c >>>> @@ -2059,8 +2059,8 @@ static int acpi_video_bus_add(struct acpi_device *device) >>>> >>>> acpi_video_bus_add_notify_handler(video); >>>> >>>> - error = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, >>>> - acpi_video_bus_notify); >>>> + error = acpi_dev_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, >>>> + acpi_video_bus_notify, device); >>>> if (error) >>>> goto err_remove; >>>> >>>> @@ -2092,7 +2092,7 @@ static void acpi_video_bus_remove(struct acpi_device *device) >>>> >>>> video = acpi_driver_data(device); >>>> >>>> - acpi_dev_remove_notify_handler(device, ACPI_DEVICE_NOTIFY, >>>> + acpi_dev_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, >>>> acpi_video_bus_notify); >>>> >>>> mutex_lock(&video_list_lock); >>>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c >>>> index 969bf81e8d54..45dae32a8646 100644 >>>> --- a/drivers/acpi/battery.c >>>> +++ b/drivers/acpi/battery.c >>>> @@ -1213,8 +1213,8 @@ static int acpi_battery_add(struct acpi_device *device) >>>> >>>> device_init_wakeup(&device->dev, 1); >>>> >>>> - result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, >>>> - acpi_battery_notify); >>>> + result = acpi_dev_install_notify_handler(device->handle, ACPI_ALL_NOTIFY, >>>> + acpi_battery_notify, device); >>>> if (result) >>>> goto fail_pm; >>>> >>>> @@ -1241,7 +1241,7 @@ static void acpi_battery_remove(struct acpi_device *device) >>>> >>>> battery = acpi_driver_data(device); >>>> >>>> - acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY, >>>> + acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, >>>> acpi_battery_notify); >>>> >>>> device_init_wakeup(&device->dev, 0); >>>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c >>>> index f41dda2d3493..479fe888d629 100644 >>>> --- a/drivers/acpi/bus.c >>>> +++ b/drivers/acpi/bus.c >>>> @@ -554,14 +554,13 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device, >>>> acpi_os_wait_events_complete(); >>>> } >>>> >>>> -int acpi_dev_install_notify_handler(struct acpi_device *adev, >>>> - u32 handler_type, >>>> - acpi_notify_handler handler) >>>> +int acpi_dev_install_notify_handler(acpi_handle handle, u32 handler_type, >>>> + acpi_notify_handler handler, void *context) >>>> { >>>> acpi_status status; >>>> >>>> - status = acpi_install_notify_handler(adev->handle, handler_type, >>>> - handler, adev); >>>> + status = acpi_install_notify_handler(handle, handler_type, >>>> + handler, context); >>> The wrapper now takes exactly the same arguments as the wrapped >>> function, so what exactly is the point of having it? The return value >>> type? >> I considered removing the wrapper altogether, but decided not to do so. >> One trivial advantage of leaving this wrapper is the return value type as >> you noticed, another is that the removal wrapper actually does something >> extra and removing it would result in duplicate code among the drivers. >> So I didn't really want to remove the 'install' wrapper but leave the >> 'remove' wrapper, as I think this might be confusing for the future reader. >> In my mind if something is removed by the wrapper it should also be >> installed by the wrapper. > I agree here. > >>>> if (ACPI_FAILURE(status)) >>>> return -ENODEV; >>>> >>>> @@ -569,11 +568,10 @@ int acpi_dev_install_notify_handler(struct acpi_device *adev, >>>> } >>>> EXPORT_SYMBOL_GPL(acpi_dev_install_notify_handler); >>>> >>>> -void acpi_dev_remove_notify_handler(struct acpi_device *adev, >>>> - u32 handler_type, >>>> +void acpi_dev_remove_notify_handler(acpi_handle handle, u32 handler_type, >>>> acpi_notify_handler handler) >>>> { >>>> - acpi_remove_notify_handler(adev->handle, handler_type, handler); >>>> + acpi_remove_notify_handler(handle, handler_type, handler); >>>> acpi_os_wait_events_complete(); >>> Here at least there is the extra workqueues synchronization point. >>> >>> That said, why exactly is it better to use acpi_handle instead of a >>> struct acpi_device pointer? >> I wanted to make the wrapper as close as possible to the wrapped function. >> This way it would be easier to remove it in the future i.e if we ever deem >> extra synchronization not worth it etc. What the ACPICA function need to >> install a wrapper is a handle not a pointer to a device. >> So there is no need for a middle man. > Taking a struct acpi_device pointer as the first argument is part of > duplication reduction, however, because in the most common case it > saves the users of it the need to dereference the struct acpi_device > they get from ACPI_COMPANION() in order to obtain the handle. User don't even have to use acpi device anywhere, as he can choose to use ACPI_HANDLE() instead on 'struct device*' and never interact with acpi device directly. > > Arguably, acpi_handle is an ACPICA concept and it is better to reduce > its usage outside ACPICA. Use of acpi_handle is deeply entrenched in the kernel. There is even a macro ACPI_HANDLE() that returns acpi_handle. I would say it's way too late to limit it to ACPICA internal code. > >>> Realistically, in a platform driver you'll need the latter to obtain >>> the former anyway. >> I don't want to introduce arbitrary limitations where they are not necessary. > I'm not sure what you mean. This patch is changing existing functions. That's true, but those functions aren't yet deeply entrenched in the kernel yet, so in my view how they should look like should still be a subject for discussion, as for now they're only used locally in drivers/acpi, and my next patchset, that would remove .notify in platform directory would spread them more, and would make them harder to change. For now we can change how they work pretty painlessly. > >> It is often the case that driver allocates it's own private struct using kmalloc >> family of functions, and that structure already contains everything that is >> needed to remove the handler, so why force ? There are already examples >> in the drivers that do that i.e in acpi_video the function >> acpi_video_dev_add_notify_handler() uses raw ACPICA handler to install >> a notify handler and it passes private structure there. >> So there is value in leaving the choice of an actual type to the user of the >> API. > No, if the user has a pointer to struct acpi_device already, there is > no difference between passing this and passing the acpi_handle from it > except for the extra dereference in the latter case. Dereference would happen anyway in the wrapper, and it doesn't cause any harm anyway for readability in my opinion. And of course you don't have to use acpi device at all, you can use ACPI_HANDLE() macro. > > If the user doesn't have a struct acpi_device pointer, let them use > the raw ACPICA handler directly and worry about the synchronization > themselves. As mentioned acpi_device pointer is not really required to use the wrapper. Instead we can use ACPI_HANDLE() macro directly. Look at the usage of the wrapper in the AC driver [1]. -static void acpi_ac_remove(struct acpi_device *device) +static void acpi_ac_remove(struct platform_device *pdev) { - struct acpi_ac *ac = acpi_driver_data(device); + struct acpi_ac *ac = platform_get_drvdata(pdev); - acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, + acpi_dev_remove_notify_handler(ACPI_HANDLE(ac->dev), + ACPI_ALL_NOTIFY, acpi_ac_notify); [1] - https://lore.kernel.org/all/20230925144842.586829-1-michal.wilczynski@intel.com/T/#mff1e8ce1e548b3252d896b56d3be0b1028b7402e > > The wrappers are there to cover the most common case, not to cover all cases. In general all drivers that I'm modifying would benefit from not using direct ACPICA installers/removers by saving that extra synchronization code that would need to be provided otherwise, and not having to deal with acpi_status codes. > >> To summarize: >> I would say the wrappers are mostly unnecessary, but they actually save >> some duplicate code in the drivers, so I decided to leave them, as I don't >> want to introduce duplicate code if I can avoid that. > What duplicate code do you mean, exactly? I would need to declare extra acpi_status variable and use ACPI_FAILURE macro in each usage of the direct ACPICA installer. Also I would need to call acpi_os_wait_events_complete() after calling each direct remove. > > IMV you haven't really explained why this particular patch is > necessary or even useful. Maybe using an example would better illustrate my point. Consider using NFIT driver modification later in this series as an example: 1) With old wrapper it would look: static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data) { struct acpi_device *adev = data; /* Now we need to figure how to get a 'struct device*' from an acpi_device. Mind this we can't just do &adev->dev, as we're not using that device anymore. We need to get a struct device that's embedded in the platform_device that the driver was instantiated with. Not sure how it would look like, but it would require are least one extra line here. */ device_lock(dev); __acpi_nfit_notify(dev, handle, event); device_unlock(dev); } 2) With new wrapper: static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data) { struct device *dev = data; device_lock(dev); __acpi_nfit_notify(dev, handle, event); device_unlock(dev); } So essentially arbitrarily forcing user to use wrapper that takes acpi device as an argument may unnecessarily increase drivers complexity, and if we can help with then we should. That's why this commit exists. > > Thanks! Thank you :-)
On Thu, Oct 5, 2023 at 2:05 PM Wilczynski, Michal <michal.wilczynski@intel.com> wrote: > On 10/5/2023 12:57 PM, Rafael J. Wysocki wrote: > > On Thu, Oct 5, 2023 at 10:10 AM Wilczynski, Michal > > <michal.wilczynski@intel.com> wrote: [cut] > >>> > >>> That said, why exactly is it better to use acpi_handle instead of a > >>> struct acpi_device pointer? > >> I wanted to make the wrapper as close as possible to the wrapped function. > >> This way it would be easier to remove it in the future i.e if we ever deem > >> extra synchronization not worth it etc. What the ACPICA function need to > >> install a wrapper is a handle not a pointer to a device. > >> So there is no need for a middle man. > > Taking a struct acpi_device pointer as the first argument is part of > > duplication reduction, however, because in the most common case it > > saves the users of it the need to dereference the struct acpi_device > > they get from ACPI_COMPANION() in order to obtain the handle. > > User don't even have to use acpi device anywhere, as he can choose > to use ACPI_HANDLE() instead on 'struct device*' and never interact > with acpi device directly. Have you actually looked at this macro? It is a wrapper around ACPI_COMPANION(). So they may think that they don't use struct acpi_device pointers, but in fact they do. > > > > Arguably, acpi_handle is an ACPICA concept and it is better to reduce > > its usage outside ACPICA. > > Use of acpi_handle is deeply entrenched in the kernel. There is even > a macro ACPI_HANDLE() that returns acpi_handle. I would say it's > way too late to limit it to ACPICA internal code. So there is a difference between "limiting to ACPICA" and "reducing". It cannot be limited to ACPICA, because the code outside ACPICA needs to evaluate ACPI objects sometimes and ACPI handles are needed for that. And this observation doesn't invalidate the point. > > > >>> Realistically, in a platform driver you'll need the latter to obtain > >>> the former anyway. > >> I don't want to introduce arbitrary limitations where they are not necessary. > > I'm not sure what you mean. This patch is changing existing functions. > > That's true, but those functions aren't yet deeply entrenched in the > kernel yet, so in my view how they should look like should still be > a subject for discussion, as for now they're only used locally in > drivers/acpi, and my next patchset, that would remove .notify in > platform directory would spread them more, and would > make them harder to change. For now we can change how they > work pretty painlessly. I see no particular reason to do that, though. What specifically is a problem with passing struct acpi_device pointers to the wrappers? I don't see any TBH. > > > >> It is often the case that driver allocates it's own private struct using kmalloc > >> family of functions, and that structure already contains everything that is > >> needed to remove the handler, so why force ? There are already examples > >> in the drivers that do that i.e in acpi_video the function > >> acpi_video_dev_add_notify_handler() uses raw ACPICA handler to install > >> a notify handler and it passes private structure there. > >> So there is value in leaving the choice of an actual type to the user of the > >> API. > > No, if the user has a pointer to struct acpi_device already, there is > > no difference between passing this and passing the acpi_handle from it > > except for the extra dereference in the latter case. > > Dereference would happen anyway in the wrapper, and it doesn't cause > any harm anyway for readability in my opinion. And of course you don't > have to use acpi device at all, you can use ACPI_HANDLE() macro. So one can use ACPI_COMPANION() just as well and it is slightly less overhead. > > > > If the user doesn't have a struct acpi_device pointer, let them use > > the raw ACPICA handler directly and worry about the synchronization > > themselves. > > As mentioned acpi_device pointer is not really required to use the wrapper. > Instead we can use ACPI_HANDLE() macro directly. Look at the usage of > the wrapper in the AC driver [1]. You don't really have to repeat the same argument several times and I know how ACPI_HANDLE() works. Also I don't like some of the things done by this patch. Whoever uses ACPI_HANDLE(), they also use ACPI_COMPANION() which is hidden in the former. If they don't need to store either the acpi_handle or the struct acpi_device pointer, there is no reason at all to use the former instead of the latter. If they get an acpi_handle from somewhere else than ACPI_HANDLE(), then yes, they would need to get the ACPI devices from there (which is possible still), but they may be better off by using the raw ACPICA interface for events in that case. > -static void acpi_ac_remove(struct acpi_device *device) > +static void acpi_ac_remove(struct platform_device *pdev) > { > - struct acpi_ac *ac = acpi_driver_data(device); > + struct acpi_ac *ac = platform_get_drvdata(pdev); > > - acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, > + acpi_dev_remove_notify_handler(ACPI_HANDLE(ac->dev), > + ACPI_ALL_NOTIFY, > acpi_ac_notify); > > > > [1] - https://lore.kernel.org/all/20230925144842.586829-1-michal.wilczynski@intel.com/T/#mff1e8ce1e548b3252d896b56d3be0b1028b7402e > > > > > The wrappers are there to cover the most common case, not to cover all cases. > > In general all drivers that I'm modifying would benefit from not using direct ACPICA > installers/removers by saving that extra synchronization code that would need to be > provided otherwise, and not having to deal with acpi_status codes. Yes, that's the common case. > > > > >> To summarize: > >> I would say the wrappers are mostly unnecessary, but they actually save > >> some duplicate code in the drivers, so I decided to leave them, as I don't > >> want to introduce duplicate code if I can avoid that. > > What duplicate code do you mean, exactly? > > I would need to declare extra acpi_status variable and use ACPI_FAILURE macro > in each usage of the direct ACPICA installer. Also I would need to call > acpi_os_wait_events_complete() after calling each direct remove. I thought you meant some code duplication related to passing struct acpi_device pointers to the wrappers, but we agree that the wrappers are generally useful. > > > > IMV you haven't really explained why this particular patch is > > necessary or even useful. > > Maybe using an example would better illustrate my point. > Consider using NFIT driver modification later in this series as an example: > > 1) With old wrapper it would look: > > static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data) > { > struct acpi_device *adev = data; > /* Now we need to figure how to get a 'struct device*' from an acpi_device. > Mind this we can't just do &adev->dev, as we're not using that device anymore. > We need to get a struct device that's embedded in the platform_device that the > driver was instantiated with. > Not sure how it would look like, but it would require are least one extra line here. > */ > device_lock(dev); > __acpi_nfit_notify(dev, handle, event); > device_unlock(dev); > } > > 2) With new wrapper: > > static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data) > { > struct device *dev = data; > > device_lock(dev); > __acpi_nfit_notify(dev, handle, event); > device_unlock(dev); > } > > > So essentially arbitrarily forcing user to use wrapper that takes acpi device > as an argument may unnecessarily increase drivers complexity, and if we > can help with then we should. That's why this commit exists. Well, I know what's going on now. You really want to add a context argument to acpi_dev_install_notify_handler(), which is quite reasonable, but then you don't have to change the first argument of it. I'll send you my version of this patch later today and we'll see.
On Thursday, October 5, 2023 5:30:59 PM CEST Rafael J. Wysocki wrote: > On Thu, Oct 5, 2023 at 2:05 PM Wilczynski, Michal > <michal.wilczynski@intel.com> wrote: > > On 10/5/2023 12:57 PM, Rafael J. Wysocki wrote: > > > On Thu, Oct 5, 2023 at 10:10 AM Wilczynski, Michal > > > <michal.wilczynski@intel.com> wrote: > > [cut] > > > >>> > > >>> That said, why exactly is it better to use acpi_handle instead of a > > >>> struct acpi_device pointer? > > >> I wanted to make the wrapper as close as possible to the wrapped function. > > >> This way it would be easier to remove it in the future i.e if we ever deem > > >> extra synchronization not worth it etc. What the ACPICA function need to > > >> install a wrapper is a handle not a pointer to a device. > > >> So there is no need for a middle man. > > > Taking a struct acpi_device pointer as the first argument is part of > > > duplication reduction, however, because in the most common case it > > > saves the users of it the need to dereference the struct acpi_device > > > they get from ACPI_COMPANION() in order to obtain the handle. > > > > User don't even have to use acpi device anywhere, as he can choose > > to use ACPI_HANDLE() instead on 'struct device*' and never interact > > with acpi device directly. > > Have you actually looked at this macro? It is a wrapper around > ACPI_COMPANION(). > > So they may think that they don't use struct acpi_device pointers, but > in fact they do. > > > > > > > Arguably, acpi_handle is an ACPICA concept and it is better to reduce > > > its usage outside ACPICA. > > > > Use of acpi_handle is deeply entrenched in the kernel. There is even > > a macro ACPI_HANDLE() that returns acpi_handle. I would say it's > > way too late to limit it to ACPICA internal code. > > So there is a difference between "limiting to ACPICA" and "reducing". > It cannot be limited to ACPICA, because the code outside ACPICA needs > to evaluate ACPI objects sometimes and ACPI handles are needed for > that. > > And this observation doesn't invalidate the point. > > > > > > >>> Realistically, in a platform driver you'll need the latter to obtain > > >>> the former anyway. > > >> I don't want to introduce arbitrary limitations where they are not necessary. > > > I'm not sure what you mean. This patch is changing existing functions. > > > > That's true, but those functions aren't yet deeply entrenched in the > > kernel yet, so in my view how they should look like should still be > > a subject for discussion, as for now they're only used locally in > > drivers/acpi, and my next patchset, that would remove .notify in > > platform directory would spread them more, and would > > make them harder to change. For now we can change how they > > work pretty painlessly. > > I see no particular reason to do that, though. > > What specifically is a problem with passing struct acpi_device > pointers to the wrappers? I don't see any TBH. > > > > > > >> It is often the case that driver allocates it's own private struct using kmalloc > > >> family of functions, and that structure already contains everything that is > > >> needed to remove the handler, so why force ? There are already examples > > >> in the drivers that do that i.e in acpi_video the function > > >> acpi_video_dev_add_notify_handler() uses raw ACPICA handler to install > > >> a notify handler and it passes private structure there. > > >> So there is value in leaving the choice of an actual type to the user of the > > >> API. > > > No, if the user has a pointer to struct acpi_device already, there is > > > no difference between passing this and passing the acpi_handle from it > > > except for the extra dereference in the latter case. > > > > Dereference would happen anyway in the wrapper, and it doesn't cause > > any harm anyway for readability in my opinion. And of course you don't > > have to use acpi device at all, you can use ACPI_HANDLE() macro. > > So one can use ACPI_COMPANION() just as well and it is slightly less overhead. > > > > > > > If the user doesn't have a struct acpi_device pointer, let them use > > > the raw ACPICA handler directly and worry about the synchronization > > > themselves. > > > > As mentioned acpi_device pointer is not really required to use the wrapper. > > Instead we can use ACPI_HANDLE() macro directly. Look at the usage of > > the wrapper in the AC driver [1]. > > You don't really have to repeat the same argument several times and I > know how ACPI_HANDLE() works. Also I don't like some of the things > done by this patch. > > Whoever uses ACPI_HANDLE(), they also use ACPI_COMPANION() which is > hidden in the former. > > If they don't need to store either the acpi_handle or the struct > acpi_device pointer, there is no reason at all to use the former > instead of the latter. > > If they get an acpi_handle from somewhere else than ACPI_HANDLE(), > then yes, they would need to get the ACPI devices from there (which is > possible still), but they may be better off by using the raw ACPICA > interface for events in that case. > > > -static void acpi_ac_remove(struct acpi_device *device) > > +static void acpi_ac_remove(struct platform_device *pdev) > > { > > - struct acpi_ac *ac = acpi_driver_data(device); > > + struct acpi_ac *ac = platform_get_drvdata(pdev); > > > > - acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, > > + acpi_dev_remove_notify_handler(ACPI_HANDLE(ac->dev), > > + ACPI_ALL_NOTIFY, > > acpi_ac_notify); > > > > > > > > [1] - https://lore.kernel.org/all/20230925144842.586829-1-michal.wilczynski@intel.com/T/#mff1e8ce1e548b3252d896b56d3be0b1028b7402e > > > > > > > > The wrappers are there to cover the most common case, not to cover all cases. > > > > In general all drivers that I'm modifying would benefit from not using direct ACPICA > > installers/removers by saving that extra synchronization code that would need to be > > provided otherwise, and not having to deal with acpi_status codes. > > Yes, that's the common case. > > > > > > > > >> To summarize: > > >> I would say the wrappers are mostly unnecessary, but they actually save > > >> some duplicate code in the drivers, so I decided to leave them, as I don't > > >> want to introduce duplicate code if I can avoid that. > > > What duplicate code do you mean, exactly? > > > > I would need to declare extra acpi_status variable and use ACPI_FAILURE macro > > in each usage of the direct ACPICA installer. Also I would need to call > > acpi_os_wait_events_complete() after calling each direct remove. > > I thought you meant some code duplication related to passing struct > acpi_device pointers to the wrappers, but we agree that the wrappers > are generally useful. > > > > > > > IMV you haven't really explained why this particular patch is > > > necessary or even useful. > > > > Maybe using an example would better illustrate my point. > > Consider using NFIT driver modification later in this series as an example: > > > > 1) With old wrapper it would look: > > > > static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data) > > { > > struct acpi_device *adev = data; > > /* Now we need to figure how to get a 'struct device*' from an acpi_device. > > Mind this we can't just do &adev->dev, as we're not using that device anymore. > > We need to get a struct device that's embedded in the platform_device that the > > driver was instantiated with. > > Not sure how it would look like, but it would require are least one extra line here. > > */ > > device_lock(dev); > > __acpi_nfit_notify(dev, handle, event); > > device_unlock(dev); > > } > > > > 2) With new wrapper: > > > > static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data) > > { > > struct device *dev = data; > > > > device_lock(dev); > > __acpi_nfit_notify(dev, handle, event); > > device_unlock(dev); > > } > > > > > > So essentially arbitrarily forcing user to use wrapper that takes acpi device > > as an argument may unnecessarily increase drivers complexity, and if we > > can help with then we should. That's why this commit exists. > > Well, I know what's going on now. > > You really want to add a context argument to > acpi_dev_install_notify_handler(), which is quite reasonable, but then > you don't have to change the first argument of it. > > I'll send you my version of this patch later today and we'll see. See below. It just adds a context argument to acpi_dev_install_notify_handler() without making the other changes made by the original patch that are rather pointless IMO. --- From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Subject: [PATCH v1 1/9] ACPI: bus: Add context argument to acpi_dev_install_notify_handler() Add void *context arrgument to the list of arguments of acpi_dev_install_notify_handler() and modify it to pass that argument as context to acpi_install_notify_handler() instead of its first argument which is problematic in general (for example, if platform drivers used it, they would rather get struct platform_device pointers or pointers to their private data from the context arguments of their notify handlers). Make all of the current callers of acpi_dev_install_notify_handler() take this change into account so as to avoid altering the general functionality. Co-developed-by: Michal Wilczynski <michal.wilczynski@intel.com> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/acpi/ac.c | 2 +- drivers/acpi/acpi_video.c | 2 +- drivers/acpi/battery.c | 2 +- drivers/acpi/bus.c | 4 ++-- drivers/acpi/hed.c | 2 +- drivers/acpi/nfit/core.c | 2 +- drivers/acpi/thermal.c | 2 +- include/acpi/acpi_bus.h | 2 +- 8 files changed, 9 insertions(+), 9 deletions(-) Index: linux-pm/drivers/acpi/ac.c =================================================================== --- linux-pm.orig/drivers/acpi/ac.c +++ linux-pm/drivers/acpi/ac.c @@ -257,7 +257,7 @@ static int acpi_ac_add(struct acpi_devic register_acpi_notifier(&ac->battery_nb); result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, - acpi_ac_notify); + acpi_ac_notify, device); if (result) goto err_unregister; Index: linux-pm/drivers/acpi/acpi_video.c =================================================================== --- linux-pm.orig/drivers/acpi/acpi_video.c +++ linux-pm/drivers/acpi/acpi_video.c @@ -2062,7 +2062,7 @@ static int acpi_video_bus_add(struct acp goto err_del; error = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, - acpi_video_bus_notify); + acpi_video_bus_notify, device); if (error) goto err_remove; Index: linux-pm/drivers/acpi/battery.c =================================================================== --- linux-pm.orig/drivers/acpi/battery.c +++ linux-pm/drivers/acpi/battery.c @@ -1214,7 +1214,7 @@ static int acpi_battery_add(struct acpi_ device_init_wakeup(&device->dev, 1); result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, - acpi_battery_notify); + acpi_battery_notify, device); if (result) goto fail_pm; Index: linux-pm/drivers/acpi/bus.c =================================================================== --- linux-pm.orig/drivers/acpi/bus.c +++ linux-pm/drivers/acpi/bus.c @@ -556,12 +556,12 @@ static void acpi_device_remove_notify_ha int acpi_dev_install_notify_handler(struct acpi_device *adev, u32 handler_type, - acpi_notify_handler handler) + acpi_notify_handler handler, void *context) { acpi_status status; status = acpi_install_notify_handler(adev->handle, handler_type, - handler, adev); + handler, context); if (ACPI_FAILURE(status)) return -ENODEV; Index: linux-pm/drivers/acpi/hed.c =================================================================== --- linux-pm.orig/drivers/acpi/hed.c +++ linux-pm/drivers/acpi/hed.c @@ -57,7 +57,7 @@ static int acpi_hed_add(struct acpi_devi hed_handle = device->handle; err = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, - acpi_hed_notify); + acpi_hed_notify, device); if (err) hed_handle = NULL; Index: linux-pm/drivers/acpi/nfit/core.c =================================================================== --- linux-pm.orig/drivers/acpi/nfit/core.c +++ linux-pm/drivers/acpi/nfit/core.c @@ -3391,7 +3391,7 @@ static int acpi_nfit_add(struct acpi_dev return rc; rc = acpi_dev_install_notify_handler(adev, ACPI_DEVICE_NOTIFY, - acpi_nfit_notify); + acpi_nfit_notify, adev); if (rc) return rc; Index: linux-pm/drivers/acpi/thermal.c =================================================================== --- linux-pm.orig/drivers/acpi/thermal.c +++ linux-pm/drivers/acpi/thermal.c @@ -936,7 +936,7 @@ static int acpi_thermal_add(struct acpi_ acpi_device_bid(device), deci_kelvin_to_celsius(tz->temp_dk)); result = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, - acpi_thermal_notify); + acpi_thermal_notify, device); if (result) goto flush_wq; Index: linux-pm/include/acpi/acpi_bus.h =================================================================== --- linux-pm.orig/include/acpi/acpi_bus.h +++ linux-pm/include/acpi/acpi_bus.h @@ -601,7 +601,7 @@ int acpi_bus_attach_private_data(acpi_ha void acpi_bus_detach_private_data(acpi_handle); int acpi_dev_install_notify_handler(struct acpi_device *adev, u32 handler_type, - acpi_notify_handler handler); + acpi_notify_handler handler, void *context); void acpi_dev_remove_notify_handler(struct acpi_device *adev, u32 handler_type, acpi_notify_handler handler);
On 10/5/2023 7:03 PM, Rafael J. Wysocki wrote: > On Thursday, October 5, 2023 5:30:59 PM CEST Rafael J. Wysocki wrote: >> On Thu, Oct 5, 2023 at 2:05 PM Wilczynski, Michal >> <michal.wilczynski@intel.com> wrote: >>> On 10/5/2023 12:57 PM, Rafael J. Wysocki wrote: >>>> On Thu, Oct 5, 2023 at 10:10 AM Wilczynski, Michal >>>> <michal.wilczynski@intel.com> wrote: >> [cut] >> >>>>>> That said, why exactly is it better to use acpi_handle instead of a >>>>>> struct acpi_device pointer? >>>>> I wanted to make the wrapper as close as possible to the wrapped function. >>>>> This way it would be easier to remove it in the future i.e if we ever deem >>>>> extra synchronization not worth it etc. What the ACPICA function need to >>>>> install a wrapper is a handle not a pointer to a device. >>>>> So there is no need for a middle man. >>>> Taking a struct acpi_device pointer as the first argument is part of >>>> duplication reduction, however, because in the most common case it >>>> saves the users of it the need to dereference the struct acpi_device >>>> they get from ACPI_COMPANION() in order to obtain the handle. >>> User don't even have to use acpi device anywhere, as he can choose >>> to use ACPI_HANDLE() instead on 'struct device*' and never interact >>> with acpi device directly. >> Have you actually looked at this macro? It is a wrapper around >> ACPI_COMPANION(). >> >> So they may think that they don't use struct acpi_device pointers, but >> in fact they do. >> >>>> Arguably, acpi_handle is an ACPICA concept and it is better to reduce >>>> its usage outside ACPICA. >>> Use of acpi_handle is deeply entrenched in the kernel. There is even >>> a macro ACPI_HANDLE() that returns acpi_handle. I would say it's >>> way too late to limit it to ACPICA internal code. >> So there is a difference between "limiting to ACPICA" and "reducing". >> It cannot be limited to ACPICA, because the code outside ACPICA needs >> to evaluate ACPI objects sometimes and ACPI handles are needed for >> that. >> >> And this observation doesn't invalidate the point. >> >>>>>> Realistically, in a platform driver you'll need the latter to obtain >>>>>> the former anyway. >>>>> I don't want to introduce arbitrary limitations where they are not necessary. >>>> I'm not sure what you mean. This patch is changing existing functions. >>> That's true, but those functions aren't yet deeply entrenched in the >>> kernel yet, so in my view how they should look like should still be >>> a subject for discussion, as for now they're only used locally in >>> drivers/acpi, and my next patchset, that would remove .notify in >>> platform directory would spread them more, and would >>> make them harder to change. For now we can change how they >>> work pretty painlessly. >> I see no particular reason to do that, though. >> >> What specifically is a problem with passing struct acpi_device >> pointers to the wrappers? I don't see any TBH. >> >>>>> It is often the case that driver allocates it's own private struct using kmalloc >>>>> family of functions, and that structure already contains everything that is >>>>> needed to remove the handler, so why force ? There are already examples >>>>> in the drivers that do that i.e in acpi_video the function >>>>> acpi_video_dev_add_notify_handler() uses raw ACPICA handler to install >>>>> a notify handler and it passes private structure there. >>>>> So there is value in leaving the choice of an actual type to the user of the >>>>> API. >>>> No, if the user has a pointer to struct acpi_device already, there is >>>> no difference between passing this and passing the acpi_handle from it >>>> except for the extra dereference in the latter case. >>> Dereference would happen anyway in the wrapper, and it doesn't cause >>> any harm anyway for readability in my opinion. And of course you don't >>> have to use acpi device at all, you can use ACPI_HANDLE() macro. >> So one can use ACPI_COMPANION() just as well and it is slightly less overhead. >> >>>> If the user doesn't have a struct acpi_device pointer, let them use >>>> the raw ACPICA handler directly and worry about the synchronization >>>> themselves. >>> As mentioned acpi_device pointer is not really required to use the wrapper. >>> Instead we can use ACPI_HANDLE() macro directly. Look at the usage of >>> the wrapper in the AC driver [1]. >> You don't really have to repeat the same argument several times and I >> know how ACPI_HANDLE() works. Also I don't like some of the things >> done by this patch. >> >> Whoever uses ACPI_HANDLE(), they also use ACPI_COMPANION() which is >> hidden in the former. >> >> If they don't need to store either the acpi_handle or the struct >> acpi_device pointer, there is no reason at all to use the former >> instead of the latter. >> >> If they get an acpi_handle from somewhere else than ACPI_HANDLE(), >> then yes, they would need to get the ACPI devices from there (which is >> possible still), but they may be better off by using the raw ACPICA >> interface for events in that case. >> >>> -static void acpi_ac_remove(struct acpi_device *device) >>> +static void acpi_ac_remove(struct platform_device *pdev) >>> { >>> - struct acpi_ac *ac = acpi_driver_data(device); >>> + struct acpi_ac *ac = platform_get_drvdata(pdev); >>> >>> - acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, >>> + acpi_dev_remove_notify_handler(ACPI_HANDLE(ac->dev), >>> + ACPI_ALL_NOTIFY, >>> acpi_ac_notify); >>> >>> >>> >>> [1] - https://lore.kernel.org/all/20230925144842.586829-1-michal.wilczynski@intel.com/T/#mff1e8ce1e548b3252d896b56d3be0b1028b7402e >>> >>>> The wrappers are there to cover the most common case, not to cover all cases. >>> In general all drivers that I'm modifying would benefit from not using direct ACPICA >>> installers/removers by saving that extra synchronization code that would need to be >>> provided otherwise, and not having to deal with acpi_status codes. >> Yes, that's the common case. >> >>>>> To summarize: >>>>> I would say the wrappers are mostly unnecessary, but they actually save >>>>> some duplicate code in the drivers, so I decided to leave them, as I don't >>>>> want to introduce duplicate code if I can avoid that. >>>> What duplicate code do you mean, exactly? >>> I would need to declare extra acpi_status variable and use ACPI_FAILURE macro >>> in each usage of the direct ACPICA installer. Also I would need to call >>> acpi_os_wait_events_complete() after calling each direct remove. >> I thought you meant some code duplication related to passing struct >> acpi_device pointers to the wrappers, but we agree that the wrappers >> are generally useful. >> >>>> IMV you haven't really explained why this particular patch is >>>> necessary or even useful. >>> Maybe using an example would better illustrate my point. >>> Consider using NFIT driver modification later in this series as an example: >>> >>> 1) With old wrapper it would look: >>> >>> static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data) >>> { >>> struct acpi_device *adev = data; >>> /* Now we need to figure how to get a 'struct device*' from an acpi_device. >>> Mind this we can't just do &adev->dev, as we're not using that device anymore. >>> We need to get a struct device that's embedded in the platform_device that the >>> driver was instantiated with. >>> Not sure how it would look like, but it would require are least one extra line here. >>> */ >>> device_lock(dev); >>> __acpi_nfit_notify(dev, handle, event); >>> device_unlock(dev); >>> } >>> >>> 2) With new wrapper: >>> >>> static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data) >>> { >>> struct device *dev = data; >>> >>> device_lock(dev); >>> __acpi_nfit_notify(dev, handle, event); >>> device_unlock(dev); >>> } >>> >>> >>> So essentially arbitrarily forcing user to use wrapper that takes acpi device >>> as an argument may unnecessarily increase drivers complexity, and if we >>> can help with then we should. That's why this commit exists. >> Well, I know what's going on now. >> >> You really want to add a context argument to >> acpi_dev_install_notify_handler(), which is quite reasonable, but then >> you don't have to change the first argument of it. >> >> I'll send you my version of this patch later today and we'll see. > See below. > > It just adds a context argument to acpi_dev_install_notify_handler() without > making the other changes made by the original patch that are rather pointless > IMO. Thank you ! I think it's fine will include this in next revision. Michał > > --- > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Subject: [PATCH v1 1/9] ACPI: bus: Add context argument to acpi_dev_install_notify_handler() > > Add void *context arrgument to the list of arguments of > acpi_dev_install_notify_handler() and modify it to pass that argument > as context to acpi_install_notify_handler() instead of its first > argument which is problematic in general (for example, if platform > drivers used it, they would rather get struct platform_device pointers > or pointers to their private data from the context arguments of their > notify handlers). > > Make all of the current callers of acpi_dev_install_notify_handler() > take this change into account so as to avoid altering the general > functionality. > > Co-developed-by: Michal Wilczynski <michal.wilczynski@intel.com> > Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/acpi/ac.c | 2 +- > drivers/acpi/acpi_video.c | 2 +- > drivers/acpi/battery.c | 2 +- > drivers/acpi/bus.c | 4 ++-- > drivers/acpi/hed.c | 2 +- > drivers/acpi/nfit/core.c | 2 +- > drivers/acpi/thermal.c | 2 +- > include/acpi/acpi_bus.h | 2 +- > 8 files changed, 9 insertions(+), 9 deletions(-) > > Index: linux-pm/drivers/acpi/ac.c > =================================================================== > --- linux-pm.orig/drivers/acpi/ac.c > +++ linux-pm/drivers/acpi/ac.c > @@ -257,7 +257,7 @@ static int acpi_ac_add(struct acpi_devic > register_acpi_notifier(&ac->battery_nb); > > result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, > - acpi_ac_notify); > + acpi_ac_notify, device); > if (result) > goto err_unregister; > > Index: linux-pm/drivers/acpi/acpi_video.c > =================================================================== > --- linux-pm.orig/drivers/acpi/acpi_video.c > +++ linux-pm/drivers/acpi/acpi_video.c > @@ -2062,7 +2062,7 @@ static int acpi_video_bus_add(struct acp > goto err_del; > > error = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, > - acpi_video_bus_notify); > + acpi_video_bus_notify, device); > if (error) > goto err_remove; > > Index: linux-pm/drivers/acpi/battery.c > =================================================================== > --- linux-pm.orig/drivers/acpi/battery.c > +++ linux-pm/drivers/acpi/battery.c > @@ -1214,7 +1214,7 @@ static int acpi_battery_add(struct acpi_ > device_init_wakeup(&device->dev, 1); > > result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, > - acpi_battery_notify); > + acpi_battery_notify, device); > if (result) > goto fail_pm; > > Index: linux-pm/drivers/acpi/bus.c > =================================================================== > --- linux-pm.orig/drivers/acpi/bus.c > +++ linux-pm/drivers/acpi/bus.c > @@ -556,12 +556,12 @@ static void acpi_device_remove_notify_ha > > int acpi_dev_install_notify_handler(struct acpi_device *adev, > u32 handler_type, > - acpi_notify_handler handler) > + acpi_notify_handler handler, void *context) > { > acpi_status status; > > status = acpi_install_notify_handler(adev->handle, handler_type, > - handler, adev); > + handler, context); > if (ACPI_FAILURE(status)) > return -ENODEV; > > Index: linux-pm/drivers/acpi/hed.c > =================================================================== > --- linux-pm.orig/drivers/acpi/hed.c > +++ linux-pm/drivers/acpi/hed.c > @@ -57,7 +57,7 @@ static int acpi_hed_add(struct acpi_devi > hed_handle = device->handle; > > err = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, > - acpi_hed_notify); > + acpi_hed_notify, device); > if (err) > hed_handle = NULL; > > Index: linux-pm/drivers/acpi/nfit/core.c > =================================================================== > --- linux-pm.orig/drivers/acpi/nfit/core.c > +++ linux-pm/drivers/acpi/nfit/core.c > @@ -3391,7 +3391,7 @@ static int acpi_nfit_add(struct acpi_dev > return rc; > > rc = acpi_dev_install_notify_handler(adev, ACPI_DEVICE_NOTIFY, > - acpi_nfit_notify); > + acpi_nfit_notify, adev); > if (rc) > return rc; > > Index: linux-pm/drivers/acpi/thermal.c > =================================================================== > --- linux-pm.orig/drivers/acpi/thermal.c > +++ linux-pm/drivers/acpi/thermal.c > @@ -936,7 +936,7 @@ static int acpi_thermal_add(struct acpi_ > acpi_device_bid(device), deci_kelvin_to_celsius(tz->temp_dk)); > > result = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, > - acpi_thermal_notify); > + acpi_thermal_notify, device); > if (result) > goto flush_wq; > > Index: linux-pm/include/acpi/acpi_bus.h > =================================================================== > --- linux-pm.orig/include/acpi/acpi_bus.h > +++ linux-pm/include/acpi/acpi_bus.h > @@ -601,7 +601,7 @@ int acpi_bus_attach_private_data(acpi_ha > void acpi_bus_detach_private_data(acpi_handle); > int acpi_dev_install_notify_handler(struct acpi_device *adev, > u32 handler_type, > - acpi_notify_handler handler); > + acpi_notify_handler handler, void *context); > void acpi_dev_remove_notify_handler(struct acpi_device *adev, > u32 handler_type, > acpi_notify_handler handler); > > >
On Thu, Oct 5, 2023 at 8:27 PM Wilczynski, Michal <michal.wilczynski@intel.com> wrote: > > > > On 10/5/2023 7:03 PM, Rafael J. Wysocki wrote: > > On Thursday, October 5, 2023 5:30:59 PM CEST Rafael J. Wysocki wrote: > >> On Thu, Oct 5, 2023 at 2:05 PM Wilczynski, Michal > >> <michal.wilczynski@intel.com> wrote: > >>> On 10/5/2023 12:57 PM, Rafael J. Wysocki wrote: > >>>> On Thu, Oct 5, 2023 at 10:10 AM Wilczynski, Michal > >>>> <michal.wilczynski@intel.com> wrote: > >> [cut] > >> > >>>>>> That said, why exactly is it better to use acpi_handle instead of a > >>>>>> struct acpi_device pointer? > >>>>> I wanted to make the wrapper as close as possible to the wrapped function. > >>>>> This way it would be easier to remove it in the future i.e if we ever deem > >>>>> extra synchronization not worth it etc. What the ACPICA function need to > >>>>> install a wrapper is a handle not a pointer to a device. > >>>>> So there is no need for a middle man. > >>>> Taking a struct acpi_device pointer as the first argument is part of > >>>> duplication reduction, however, because in the most common case it > >>>> saves the users of it the need to dereference the struct acpi_device > >>>> they get from ACPI_COMPANION() in order to obtain the handle. > >>> User don't even have to use acpi device anywhere, as he can choose > >>> to use ACPI_HANDLE() instead on 'struct device*' and never interact > >>> with acpi device directly. > >> Have you actually looked at this macro? It is a wrapper around > >> ACPI_COMPANION(). > >> > >> So they may think that they don't use struct acpi_device pointers, but > >> in fact they do. > >> > >>>> Arguably, acpi_handle is an ACPICA concept and it is better to reduce > >>>> its usage outside ACPICA. > >>> Use of acpi_handle is deeply entrenched in the kernel. There is even > >>> a macro ACPI_HANDLE() that returns acpi_handle. I would say it's > >>> way too late to limit it to ACPICA internal code. > >> So there is a difference between "limiting to ACPICA" and "reducing". > >> It cannot be limited to ACPICA, because the code outside ACPICA needs > >> to evaluate ACPI objects sometimes and ACPI handles are needed for > >> that. > >> > >> And this observation doesn't invalidate the point. > >> > >>>>>> Realistically, in a platform driver you'll need the latter to obtain > >>>>>> the former anyway. > >>>>> I don't want to introduce arbitrary limitations where they are not necessary. > >>>> I'm not sure what you mean. This patch is changing existing functions. > >>> That's true, but those functions aren't yet deeply entrenched in the > >>> kernel yet, so in my view how they should look like should still be > >>> a subject for discussion, as for now they're only used locally in > >>> drivers/acpi, and my next patchset, that would remove .notify in > >>> platform directory would spread them more, and would > >>> make them harder to change. For now we can change how they > >>> work pretty painlessly. > >> I see no particular reason to do that, though. > >> > >> What specifically is a problem with passing struct acpi_device > >> pointers to the wrappers? I don't see any TBH. > >> > >>>>> It is often the case that driver allocates it's own private struct using kmalloc > >>>>> family of functions, and that structure already contains everything that is > >>>>> needed to remove the handler, so why force ? There are already examples > >>>>> in the drivers that do that i.e in acpi_video the function > >>>>> acpi_video_dev_add_notify_handler() uses raw ACPICA handler to install > >>>>> a notify handler and it passes private structure there. > >>>>> So there is value in leaving the choice of an actual type to the user of the > >>>>> API. > >>>> No, if the user has a pointer to struct acpi_device already, there is > >>>> no difference between passing this and passing the acpi_handle from it > >>>> except for the extra dereference in the latter case. > >>> Dereference would happen anyway in the wrapper, and it doesn't cause > >>> any harm anyway for readability in my opinion. And of course you don't > >>> have to use acpi device at all, you can use ACPI_HANDLE() macro. > >> So one can use ACPI_COMPANION() just as well and it is slightly less overhead. > >> > >>>> If the user doesn't have a struct acpi_device pointer, let them use > >>>> the raw ACPICA handler directly and worry about the synchronization > >>>> themselves. > >>> As mentioned acpi_device pointer is not really required to use the wrapper. > >>> Instead we can use ACPI_HANDLE() macro directly. Look at the usage of > >>> the wrapper in the AC driver [1]. > >> You don't really have to repeat the same argument several times and I > >> know how ACPI_HANDLE() works. Also I don't like some of the things > >> done by this patch. > >> > >> Whoever uses ACPI_HANDLE(), they also use ACPI_COMPANION() which is > >> hidden in the former. > >> > >> If they don't need to store either the acpi_handle or the struct > >> acpi_device pointer, there is no reason at all to use the former > >> instead of the latter. > >> > >> If they get an acpi_handle from somewhere else than ACPI_HANDLE(), > >> then yes, they would need to get the ACPI devices from there (which is > >> possible still), but they may be better off by using the raw ACPICA > >> interface for events in that case. > >> > >>> -static void acpi_ac_remove(struct acpi_device *device) > >>> +static void acpi_ac_remove(struct platform_device *pdev) > >>> { > >>> - struct acpi_ac *ac = acpi_driver_data(device); > >>> + struct acpi_ac *ac = platform_get_drvdata(pdev); > >>> > >>> - acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, > >>> + acpi_dev_remove_notify_handler(ACPI_HANDLE(ac->dev), > >>> + ACPI_ALL_NOTIFY, > >>> acpi_ac_notify); > >>> > >>> > >>> > >>> [1] - https://lore.kernel.org/all/20230925144842.586829-1-michal.wilczynski@intel.com/T/#mff1e8ce1e548b3252d896b56d3be0b1028b7402e > >>> > >>>> The wrappers are there to cover the most common case, not to cover all cases. > >>> In general all drivers that I'm modifying would benefit from not using direct ACPICA > >>> installers/removers by saving that extra synchronization code that would need to be > >>> provided otherwise, and not having to deal with acpi_status codes. > >> Yes, that's the common case. > >> > >>>>> To summarize: > >>>>> I would say the wrappers are mostly unnecessary, but they actually save > >>>>> some duplicate code in the drivers, so I decided to leave them, as I don't > >>>>> want to introduce duplicate code if I can avoid that. > >>>> What duplicate code do you mean, exactly? > >>> I would need to declare extra acpi_status variable and use ACPI_FAILURE macro > >>> in each usage of the direct ACPICA installer. Also I would need to call > >>> acpi_os_wait_events_complete() after calling each direct remove. > >> I thought you meant some code duplication related to passing struct > >> acpi_device pointers to the wrappers, but we agree that the wrappers > >> are generally useful. > >> > >>>> IMV you haven't really explained why this particular patch is > >>>> necessary or even useful. > >>> Maybe using an example would better illustrate my point. > >>> Consider using NFIT driver modification later in this series as an example: > >>> > >>> 1) With old wrapper it would look: > >>> > >>> static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data) > >>> { > >>> struct acpi_device *adev = data; > >>> /* Now we need to figure how to get a 'struct device*' from an acpi_device. > >>> Mind this we can't just do &adev->dev, as we're not using that device anymore. > >>> We need to get a struct device that's embedded in the platform_device that the > >>> driver was instantiated with. > >>> Not sure how it would look like, but it would require are least one extra line here. > >>> */ > >>> device_lock(dev); > >>> __acpi_nfit_notify(dev, handle, event); > >>> device_unlock(dev); > >>> } > >>> > >>> 2) With new wrapper: > >>> > >>> static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data) > >>> { > >>> struct device *dev = data; > >>> > >>> device_lock(dev); > >>> __acpi_nfit_notify(dev, handle, event); > >>> device_unlock(dev); > >>> } > >>> > >>> > >>> So essentially arbitrarily forcing user to use wrapper that takes acpi device > >>> as an argument may unnecessarily increase drivers complexity, and if we > >>> can help with then we should. That's why this commit exists. > >> Well, I know what's going on now. > >> > >> You really want to add a context argument to > >> acpi_dev_install_notify_handler(), which is quite reasonable, but then > >> you don't have to change the first argument of it. > >> > >> I'll send you my version of this patch later today and we'll see. > > See below. > > > > It just adds a context argument to acpi_dev_install_notify_handler() without > > making the other changes made by the original patch that are rather pointless > > IMO. > > Thank you ! > I think it's fine will include this in next revision. Sounds good, thanks! > > > > --- > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Subject: [PATCH v1 1/9] ACPI: bus: Add context argument to acpi_dev_install_notify_handler() > > > > Add void *context arrgument to the list of arguments of > > acpi_dev_install_notify_handler() and modify it to pass that argument > > as context to acpi_install_notify_handler() instead of its first > > argument which is problematic in general (for example, if platform > > drivers used it, they would rather get struct platform_device pointers > > or pointers to their private data from the context arguments of their > > notify handlers). > > > > Make all of the current callers of acpi_dev_install_notify_handler() > > take this change into account so as to avoid altering the general > > functionality. > > > > Co-developed-by: Michal Wilczynski <michal.wilczynski@intel.com> > > Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/acpi/ac.c | 2 +- > > drivers/acpi/acpi_video.c | 2 +- > > drivers/acpi/battery.c | 2 +- > > drivers/acpi/bus.c | 4 ++-- > > drivers/acpi/hed.c | 2 +- > > drivers/acpi/nfit/core.c | 2 +- > > drivers/acpi/thermal.c | 2 +- > > include/acpi/acpi_bus.h | 2 +- > > 8 files changed, 9 insertions(+), 9 deletions(-) > > > > Index: linux-pm/drivers/acpi/ac.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/ac.c > > +++ linux-pm/drivers/acpi/ac.c > > @@ -257,7 +257,7 @@ static int acpi_ac_add(struct acpi_devic > > register_acpi_notifier(&ac->battery_nb); > > > > result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, > > - acpi_ac_notify); > > + acpi_ac_notify, device); > > if (result) > > goto err_unregister; > > > > Index: linux-pm/drivers/acpi/acpi_video.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/acpi_video.c > > +++ linux-pm/drivers/acpi/acpi_video.c > > @@ -2062,7 +2062,7 @@ static int acpi_video_bus_add(struct acp > > goto err_del; > > > > error = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, > > - acpi_video_bus_notify); > > + acpi_video_bus_notify, device); > > if (error) > > goto err_remove; > > > > Index: linux-pm/drivers/acpi/battery.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/battery.c > > +++ linux-pm/drivers/acpi/battery.c > > @@ -1214,7 +1214,7 @@ static int acpi_battery_add(struct acpi_ > > device_init_wakeup(&device->dev, 1); > > > > result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, > > - acpi_battery_notify); > > + acpi_battery_notify, device); > > if (result) > > goto fail_pm; > > > > Index: linux-pm/drivers/acpi/bus.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/bus.c > > +++ linux-pm/drivers/acpi/bus.c > > @@ -556,12 +556,12 @@ static void acpi_device_remove_notify_ha > > > > int acpi_dev_install_notify_handler(struct acpi_device *adev, > > u32 handler_type, > > - acpi_notify_handler handler) > > + acpi_notify_handler handler, void *context) > > { > > acpi_status status; > > > > status = acpi_install_notify_handler(adev->handle, handler_type, > > - handler, adev); > > + handler, context); > > if (ACPI_FAILURE(status)) > > return -ENODEV; > > > > Index: linux-pm/drivers/acpi/hed.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/hed.c > > +++ linux-pm/drivers/acpi/hed.c > > @@ -57,7 +57,7 @@ static int acpi_hed_add(struct acpi_devi > > hed_handle = device->handle; > > > > err = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, > > - acpi_hed_notify); > > + acpi_hed_notify, device); > > if (err) > > hed_handle = NULL; > > > > Index: linux-pm/drivers/acpi/nfit/core.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/nfit/core.c > > +++ linux-pm/drivers/acpi/nfit/core.c > > @@ -3391,7 +3391,7 @@ static int acpi_nfit_add(struct acpi_dev > > return rc; > > > > rc = acpi_dev_install_notify_handler(adev, ACPI_DEVICE_NOTIFY, > > - acpi_nfit_notify); > > + acpi_nfit_notify, adev); > > if (rc) > > return rc; > > > > Index: linux-pm/drivers/acpi/thermal.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/thermal.c > > +++ linux-pm/drivers/acpi/thermal.c > > @@ -936,7 +936,7 @@ static int acpi_thermal_add(struct acpi_ > > acpi_device_bid(device), deci_kelvin_to_celsius(tz->temp_dk)); > > > > result = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, > > - acpi_thermal_notify); > > + acpi_thermal_notify, device); > > if (result) > > goto flush_wq; > > > > Index: linux-pm/include/acpi/acpi_bus.h > > =================================================================== > > --- linux-pm.orig/include/acpi/acpi_bus.h > > +++ linux-pm/include/acpi/acpi_bus.h > > @@ -601,7 +601,7 @@ int acpi_bus_attach_private_data(acpi_ha > > void acpi_bus_detach_private_data(acpi_handle); > > int acpi_dev_install_notify_handler(struct acpi_device *adev, > > u32 handler_type, > > - acpi_notify_handler handler); > > + acpi_notify_handler handler, void *context); > > void acpi_dev_remove_notify_handler(struct acpi_device *adev, > > u32 handler_type, > > acpi_notify_handler handler); > > > > > > >
diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c index 225dc6818751..0b245f9f7ec8 100644 --- a/drivers/acpi/ac.c +++ b/drivers/acpi/ac.c @@ -256,8 +256,8 @@ static int acpi_ac_add(struct acpi_device *device) ac->battery_nb.notifier_call = acpi_ac_battery_notify; register_acpi_notifier(&ac->battery_nb); - result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, - acpi_ac_notify); + result = acpi_dev_install_notify_handler(device->handle, ACPI_ALL_NOTIFY, + acpi_ac_notify, device); if (result) goto err_unregister; @@ -306,7 +306,7 @@ static void acpi_ac_remove(struct acpi_device *device) ac = acpi_driver_data(device); - acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY, + acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, acpi_ac_notify); power_supply_unregister(ac->charger); unregister_acpi_notifier(&ac->battery_nb); diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c index 948e31f7ce6e..025c17890127 100644 --- a/drivers/acpi/acpi_video.c +++ b/drivers/acpi/acpi_video.c @@ -2059,8 +2059,8 @@ static int acpi_video_bus_add(struct acpi_device *device) acpi_video_bus_add_notify_handler(video); - error = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, - acpi_video_bus_notify); + error = acpi_dev_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, + acpi_video_bus_notify, device); if (error) goto err_remove; @@ -2092,7 +2092,7 @@ static void acpi_video_bus_remove(struct acpi_device *device) video = acpi_driver_data(device); - acpi_dev_remove_notify_handler(device, ACPI_DEVICE_NOTIFY, + acpi_dev_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, acpi_video_bus_notify); mutex_lock(&video_list_lock); diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c index 969bf81e8d54..45dae32a8646 100644 --- a/drivers/acpi/battery.c +++ b/drivers/acpi/battery.c @@ -1213,8 +1213,8 @@ static int acpi_battery_add(struct acpi_device *device) device_init_wakeup(&device->dev, 1); - result = acpi_dev_install_notify_handler(device, ACPI_ALL_NOTIFY, - acpi_battery_notify); + result = acpi_dev_install_notify_handler(device->handle, ACPI_ALL_NOTIFY, + acpi_battery_notify, device); if (result) goto fail_pm; @@ -1241,7 +1241,7 @@ static void acpi_battery_remove(struct acpi_device *device) battery = acpi_driver_data(device); - acpi_dev_remove_notify_handler(device, ACPI_ALL_NOTIFY, + acpi_dev_remove_notify_handler(device->handle, ACPI_ALL_NOTIFY, acpi_battery_notify); device_init_wakeup(&device->dev, 0); diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index f41dda2d3493..479fe888d629 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -554,14 +554,13 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device, acpi_os_wait_events_complete(); } -int acpi_dev_install_notify_handler(struct acpi_device *adev, - u32 handler_type, - acpi_notify_handler handler) +int acpi_dev_install_notify_handler(acpi_handle handle, u32 handler_type, + acpi_notify_handler handler, void *context) { acpi_status status; - status = acpi_install_notify_handler(adev->handle, handler_type, - handler, adev); + status = acpi_install_notify_handler(handle, handler_type, + handler, context); if (ACPI_FAILURE(status)) return -ENODEV; @@ -569,11 +568,10 @@ int acpi_dev_install_notify_handler(struct acpi_device *adev, } EXPORT_SYMBOL_GPL(acpi_dev_install_notify_handler); -void acpi_dev_remove_notify_handler(struct acpi_device *adev, - u32 handler_type, +void acpi_dev_remove_notify_handler(acpi_handle handle, u32 handler_type, acpi_notify_handler handler) { - acpi_remove_notify_handler(adev->handle, handler_type, handler); + acpi_remove_notify_handler(handle, handler_type, handler); acpi_os_wait_events_complete(); } EXPORT_SYMBOL_GPL(acpi_dev_remove_notify_handler); diff --git a/drivers/acpi/hed.c b/drivers/acpi/hed.c index 46c6f8c35b43..e7e274c5cc9e 100644 --- a/drivers/acpi/hed.c +++ b/drivers/acpi/hed.c @@ -56,8 +56,8 @@ static int acpi_hed_add(struct acpi_device *device) return -EINVAL; hed_handle = device->handle; - err = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, - acpi_hed_notify); + err = acpi_dev_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, + acpi_hed_notify, device); if (err) hed_handle = NULL; @@ -66,7 +66,7 @@ static int acpi_hed_add(struct acpi_device *device) static void acpi_hed_remove(struct acpi_device *device) { - acpi_dev_remove_notify_handler(device, ACPI_DEVICE_NOTIFY, + acpi_dev_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, acpi_hed_notify); hed_handle = NULL; } diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index f0e6738ae3c9..821870f57862 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -3295,7 +3295,7 @@ static void acpi_nfit_remove_notify_handler(void *data) { struct acpi_device *adev = data; - acpi_dev_remove_notify_handler(adev, ACPI_DEVICE_NOTIFY, + acpi_dev_remove_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY, acpi_nfit_notify); } @@ -3390,8 +3390,8 @@ static int acpi_nfit_add(struct acpi_device *adev) if (rc) return rc; - rc = acpi_dev_install_notify_handler(adev, ACPI_DEVICE_NOTIFY, - acpi_nfit_notify); + rc = acpi_dev_install_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY, + acpi_nfit_notify, adev); if (rc) return rc; diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c index 10720a038846..0218c34c16b9 100644 --- a/drivers/acpi/thermal.c +++ b/drivers/acpi/thermal.c @@ -978,8 +978,8 @@ static int acpi_thermal_add(struct acpi_device *device) pr_info("%s [%s] (%ld C)\n", acpi_device_name(device), acpi_device_bid(device), deci_kelvin_to_celsius(tz->temperature)); - result = acpi_dev_install_notify_handler(device, ACPI_DEVICE_NOTIFY, - acpi_thermal_notify); + result = acpi_dev_install_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, + acpi_thermal_notify, device); if (result) goto flush_wq; @@ -1005,7 +1005,7 @@ static void acpi_thermal_remove(struct acpi_device *device) tz = acpi_driver_data(device); - acpi_dev_remove_notify_handler(device, ACPI_DEVICE_NOTIFY, + acpi_dev_remove_notify_handler(device->handle, ACPI_DEVICE_NOTIFY, acpi_thermal_notify); flush_workqueue(acpi_thermal_pm_queue); diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 254685085c82..95d5193f236f 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -515,11 +515,10 @@ void acpi_bus_private_data_handler(acpi_handle, void *); int acpi_bus_get_private_data(acpi_handle, void **); int acpi_bus_attach_private_data(acpi_handle, void *); void acpi_bus_detach_private_data(acpi_handle); -int acpi_dev_install_notify_handler(struct acpi_device *adev, - u32 handler_type, - acpi_notify_handler handler); -void acpi_dev_remove_notify_handler(struct acpi_device *adev, - u32 handler_type, +int acpi_dev_install_notify_handler(acpi_handle handle, u32 handler_type, + acpi_notify_handler handler, + void *context); +void acpi_dev_remove_notify_handler(acpi_handle handle, u32 handler_type, acpi_notify_handler handler); extern int acpi_notifier_call_chain(struct acpi_device *, u32, u32); extern int register_acpi_notifier(struct notifier_block *);