Re: [PATCH v1 1/9] ACPI: bus: Make notify wrappers more generic

[Wilczynski, Michal]

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@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.

Thank you !
I think it's fine will include this in next revision.

Michał

>
> ---
> 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);
>
>
>