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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx/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@xxxxxxxxx>
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@xxxxxxxxx>
Signed-off-by: Michal Wilczynski <michal.wilczynski@xxxxxxxxx>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
---
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);
