Re: [PATCH v2 5/6] ACPI: NFIT: Replace acpi_driver with platform_driver
On 10/17/2023 8:24 PM, Dan Williams wrote: > Michal Wilczynski wrote: >> NFIT driver uses struct acpi_driver incorrectly to register itself. >> This is wrong as the instances of the ACPI devices are not meant >> to be literal devices, they're supposed to describe ACPI entry of a >> particular device. >> >> Use platform_driver instead of acpi_driver. In relevant places call >> platform devices instances pdev to make a distinction with ACPI >> devices instances. >> >> NFIT driver uses devm_*() family of functions extensively. This change >> has no impact on correct functioning of the whole devm_*() family of >> functions, since the lifecycle of the device stays the same. It is still >> being created during the enumeration, and destroyed on platform device >> removal. > I notice this verbiage has the same fundamental misunderstanding of devm > allocation lifetime as the acpi_nfit_init_interleave_set() discussion. > The devm allocation lifetime typically starts in driver->probe() and > ends either with driver->probe() failure, or the driver->remove() call. > Note that the driver->remove() call is invoked not only for > platform-device removal, but also driver "unbind" events. So the > "destroyed on platform device removal" is the least likely way that > these allocations are torn down given ACPI0012 devices are never > removed. > > Outside of that, my main concern about this patch is that I expect it > breaks unit tests. The infrastructure in > tools/testing/nvdimm/test/nfit.c emulates an ACPI0012 device that allows > for deeper regression testing given hardware is difficult to come by, > and because QEMU does not implement some of the tricky corner cases that > the unit tests cover. > > This needs to pass tests, but fair warning, > tools/testing/nvdimm/test/nfit.c does some non-idiomatic + "strange" > things to achieve deeper test coverage. So I expect that if this breaks > tests as I expect the effort needed to fix the emulation could be > significant. > > If you want to give running the tests a try the easiest would be to use > "run_qemu.sh" with --nfit-test option [1], or you can try to setup an > environment manually using the ndctl instructions [2]. > > [1]: https://github.com/pmem/run_qemu > [2]: https://github.com/pmem/ndctl#readme Thanks a lot ! I will run qemu tests and fix the verbiage, Michał
Re: [PATCH v2 5/6] ACPI: NFIT: Replace acpi_driver with platform_driver
On 10/6/2023 7:30 PM, Michal Wilczynski wrote:
> NFIT driver uses struct acpi_driver incorrectly to register itself.
> This is wrong as the instances of the ACPI devices are not meant
> to be literal devices, they're supposed to describe ACPI entry of a
> particular device.
>
> Use platform_driver instead of acpi_driver. In relevant places call
> platform devices instances pdev to make a distinction with ACPI
> devices instances.
>
> NFIT driver uses devm_*() family of functions extensively. This change
> has no impact on correct functioning of the whole devm_*() family of
> functions, since the lifecycle of the device stays the same. It is still
> being created during the enumeration, and destroyed on platform device
> removal.
>
> Suggested-by: Rafael J. Wysocki
> Signed-off-by: Michal Wilczynski
Hi Dan,
Rafael already reviewed this patch series and merged patches
that he likes. We're waiting on your input regarding two NFIT
commits in this series.
Thanks a lot !
Michał
> ---
> drivers/acpi/nfit/core.c | 34 ++
> 1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 942b84d94078..fb0bc16fa186 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include "intel.h"
> @@ -98,7 +99,7 @@ static struct acpi_device *to_acpi_dev(struct
> acpi_nfit_desc *acpi_desc)
> || strcmp(nd_desc->provider_name, "ACPI.NFIT") != 0)
> return NULL;
>
> - return to_acpi_device(acpi_desc->dev);
> + return ACPI_COMPANION(acpi_desc->dev);
> }
>
> static int xlat_bus_status(void *buf, unsigned int cmd, u32 status)
> @@ -3284,11 +3285,11 @@ static void acpi_nfit_put_table(void *table)
>
> static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
> {
> - struct acpi_device *adev = data;
> + struct device *dev = data;
>
> - device_lock(>dev);
> - __acpi_nfit_notify(>dev, handle, event);
> - device_unlock(>dev);
> + device_lock(dev);
> + __acpi_nfit_notify(dev, handle, event);
> + device_unlock(dev);
> }
>
> static void acpi_nfit_remove_notify_handler(void *data)
> @@ -3329,11 +3330,12 @@ void acpi_nfit_shutdown(void *data)
> }
> EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
>
> -static int acpi_nfit_add(struct acpi_device *adev)
> +static int acpi_nfit_probe(struct platform_device *pdev)
> {
> struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> struct acpi_nfit_desc *acpi_desc;
> - struct device *dev = >dev;
> + struct device *dev = >dev;
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> struct acpi_table_header *tbl;
> acpi_status status = AE_OK;
> acpi_size sz;
> @@ -3360,7 +3362,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
> acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL);
> if (!acpi_desc)
> return -ENOMEM;
> - acpi_nfit_desc_init(acpi_desc, >dev);
> + acpi_nfit_desc_init(acpi_desc, dev);
>
> /* Save the acpi header for exporting the revision via sysfs */
> acpi_desc->acpi_header = *tbl;
> @@ -3391,7 +3393,7 @@ static int acpi_nfit_add(struct acpi_device *adev)
> return rc;
>
> rc = acpi_dev_install_notify_handler(adev, ACPI_DEVICE_NOTIFY,
> - acpi_nfit_notify, adev);
> + acpi_nfit_notify, dev);
> if (rc)
> return rc;
>
> @@ -3475,11 +3477,11 @@ static const struct acpi_device_id acpi_nfit_ids[] = {
> };
> MODULE_DEVICE_TABLE(acpi, acpi_nfit_ids);
>
> -static struct acpi_driver acpi_nfit_driver = {
> - .name = KBUILD_MODNAME,
> - .ids = acpi_nfit_ids,
> - .ops = {
> - .add = acpi_nfit_add,
> +static struct platform_driver acpi_nfit_driver = {
> + .probe = acpi_nfit_probe,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .acpi_match_table = acpi_nfit_ids,
> },
> };
>
> @@ -3517,7 +3519,7 @@ static __init int nfit_init(void)
> return -ENOMEM;
>
> nfit_mce_register();
> - ret = acpi_bus_register_driver(_nfit_driver);
> + ret = platform_driver_register(_nfit_driver);
> if (ret) {
> nfit_mce_unregister();
> destroy_workqueue(nfit_wq);
> @@ -3530,7 +3532,7 @@ static __init int nfit_init(void)
> static __exit void nfit_exit(void)
> {
> nfit_mce_unregister();
> - acpi_bus_unregister_driver(_nfit_driver);
> + platform_driver_unregister(_nfit_driver);
> destroy_workqueue(nfit_wq);
> WARN_ON(!list_empty(_descs));
> }
Re: [PATCH v2] ACPI: NFIT: Fix local use of devm_*()
On 10/13/2023 7:05 PM, Dan Williams wrote: > Wilczynski, Michal wrote: >> On 10/13/2023 6:38 PM, Dan Williams wrote: >>> Michal Wilczynski wrote: >>>> devm_*() family of functions purpose is managing memory attached to a >>>> device. So in general it should only be used for allocations that should >>>> last for the whole lifecycle of the device. >>> No, this assertion is not accurate, if it were strictly true then >>> devm_kfree() should be deleted. This patch is only a cleanup to switch >>> the automatic cleanup pattern from devm to the new cleanup.h helpers. >> The memory in question is only used locally in a function, so there is no >> reason >> to use devm_*() family of functions. I think devm_kfree() is more for special >> cases where the memory is meant to be used for the whole lifecycle of device, >> but some special case occurs and it's not and it needs to be freed. >> >> This is an incorrect API usage. Would you propose to change all memory >> allocations currently being done to devm_*() family simply because >> devm_kfree() >> exists ? > Michal, please work with someone else to get these cleanups upstream, I > am done with this thread. I'm really sorry if I offended you, I didn't mean to. Michał
Re: [PATCH v2] ACPI: NFIT: Fix local use of devm_*()
On 10/13/2023 6:38 PM, Dan Williams wrote: > Michal Wilczynski wrote: >> devm_*() family of functions purpose is managing memory attached to a >> device. So in general it should only be used for allocations that should >> last for the whole lifecycle of the device. > No, this assertion is not accurate, if it were strictly true then > devm_kfree() should be deleted. This patch is only a cleanup to switch > the automatic cleanup pattern from devm to the new cleanup.h helpers. The memory in question is only used locally in a function, so there is no reason to use devm_*() family of functions. I think devm_kfree() is more for special cases where the memory is meant to be used for the whole lifecycle of device, but some special case occurs and it's not and it needs to be freed. This is an incorrect API usage. Would you propose to change all memory allocations currently being done to devm_*() family simply because devm_kfree() exists ? Why introduce extra overhead if you don't have to ? > > I am all for modernizing code over time, but patches that make > assertions of "memory leaks" and "incorrect API usage" in code that has > been untouched for almost a decade demand more scrutiny than what > transpired here. I understand that it's not necessarily a big problem, and the code works perfectly, I can change the phrasing if you don't like it, but still the cleanup.h helpers don't really care that much what functions they call to allocate/free. They are meant to care about the scope - like constructor destructor in C++ - you can call anything there. So this commit changes 2 things: - change family of function to allocate from devm_kcalloc() to kcalloc() - use scope based mechanism to call those functions Thanks a lot for your review ! Michał
Re: [PATCH v1 0/2] Fix memory leak and move to modern scope based rollback
On 9/26/2023 8:45 PM, Michal Wilczynski wrote: > In acpi_nfit_init_interleave_set() there is a memory leak + improper use > of devm_*() family of functions for local memory allocations. This patch > series provides two commits - one is meant as a bug fix, and could > potentially be backported, and the other one improves old style rollback > with scope based, similar to C++ RAII [1]. > > Link: https://lwn.net/Articles/934679/ [1] > > Michal Wilczynski (2): > ACPI: NFIT: Fix memory leak, and local use of devm_*() > ACPI: NFIT: Use modern scope based rollback > > drivers/acpi/nfit/core.c | 21 - > 1 file changed, 8 insertions(+), 13 deletions(-) Hi Dan, Do you think this patchset is fine ? Would you like me to change anything ? Regards, Michał >
Re: [PATCH v2 3/6] ACPI: AC: Replace acpi_driver with platform_driver
On 10/9/2023 2:27 PM, Rafael J. Wysocki wrote:
> On Mon, Oct 9, 2023 at 10:40 AM Wilczynski, Michal
> wrote:
>>
>> Hi !
>>
>> Thanks a lot for a review, to both of you ! :-)
>>
>> On 10/7/2023 12:43 PM, Rafael J. Wysocki wrote:
>>> On Sat, Oct 7, 2023 at 12:41 PM Rafael J. Wysocki wrote:
>>>> On Sat, Oct 7, 2023 at 9:56 AM Andy Shevchenko
>>>> wrote:
>>>>> On Fri, Oct 06, 2023 at 09:47:57PM +0200, Rafael J. Wysocki wrote:
>>>>>> On Fri, Oct 6, 2023 at 8:33 PM Michal Wilczynski
>>>>>> wrote:
>>>>> ...
>>>>>
>>>>>>> struct acpi_ac {
>>>>>>> struct power_supply *charger;
>>>>>>> struct power_supply_desc charger_desc;
>>>>>>> - struct acpi_device *device;
>>>>>>> + struct device *dev;
>>>>>> I'm not convinced about this change.
>>>>>>
>>>>>> If I'm not mistaken, you only use the dev pointer above to get the
>>>>>> ACPI_COMPANION() of it, but the latter is already found in _probe(),
>>>>>> so it can be stored in struct acpi_ac for later use and then the dev
>>>>>> pointer in there will not be necessary any more.
>>>>>>
>>>>>> That will save you a bunch of ACPI_HANDLE() evaluations and there's
>>>>>> nothing wrong with using ac->device->handle. The patch will then
>>>>>> become almost trivial AFAICS and if you really need to get from ac to
>>>>>> the underlying platform device, a pointer to it can be added to struct
>>>>>> acpi_ac without removing the ACPI device pointer from it.
>> Yeah we could add platform device without removing acpi device, and
>> yes that would introduce data duplication, like Andy noticed.
> But he hasn't answered my question regarding what data duplication he
> meant, exactly.
>
> So what data duplication do you mean?
So what I meant was that many drivers would find it useful to have
a struct device embedded in their 'private structure', and in that case
there would be a duplication of platform_device and acpi_device as
both pointers would be needed. Mind this if you only have struct device
it's easy to get acpi device, but it's not the case the other way around.
So for this driver it's just a matter of sticking to convention, for the others
like it would be duplication.
In my version of this patch we managed to avoid this duplication, thanks
to the contextual argument introduced before, but look at this patch:
https://github.com/mwilczy/linux-pm/commit/cc8ef52707341e67a12067d6ead991d56ea017ca
Author of this patch had to introduce platform_device and acpi_device to the
struct ac, so
there was some duplication. That is the case for many drivers to come, so I
decided it's better
to change this and have a pattern with keeping only 'struct device'.
>
>> And yes, maybe for this particular driver there is little impact (as struct
>> device is not
>> really used), but in my opinion we should adhere to some common coding
>> pattern among all acpi drivers in order for the code to be easier to maintain
>> and improve readability, also for any newcomer.
> Well, maybe.
>
> But then please minimize the number of code lines changed in this
> particular patch and please avoid changes that are not directly
> related to the purpose of the patch.
Sure, like I've stated before I felt this is part of this patch, we only
narrowly
escaped the duplication by introducing contextual argument before ;-)
>
>>>>> The idea behind is to eliminate data duplication.
>>>> What data duplication exactly do you mean?
>>>>
>>>> struct acpi_device *device is replaced with struct device *dev which
>>>> is the same size. The latter is then used to obtain a struct
>>>> acpi_device pointer. Why is it better to do this than to store the
>>>> struct acpi_device itself?
>>> This should be "store the struct acpi_device pointer itself", sorry.
>> Hi,
>> So let me explain the reasoning here:
>>
>> 1) I've had a pleasure to work with different drivers in ACPI directory. In
>> my
>> opinion the best ones I've seen, were build around the concept of struct
>> device (e.g NFIT). It's a struct that's well understood in the kernel,
>> and
>> it's easier to understand without having to read any ACPI specific code.
>> If I see something like ACPI_HANDLE(dev), I think 'ah-ha - having a
>> struct
>> device I can easily get struct acpi_device - th
Re: [PATCH v2 3/6] ACPI: AC: Replace acpi_driver with platform_driver
Hi !
Thanks a lot for a review, to both of you ! :-)
On 10/7/2023 12:43 PM, Rafael J. Wysocki wrote:
> On Sat, Oct 7, 2023 at 12:41 PM Rafael J. Wysocki wrote:
>> On Sat, Oct 7, 2023 at 9:56 AM Andy Shevchenko
>> wrote:
>>> On Fri, Oct 06, 2023 at 09:47:57PM +0200, Rafael J. Wysocki wrote:
On Fri, Oct 6, 2023 at 8:33 PM Michal Wilczynski
wrote:
>>> ...
>>>
> struct acpi_ac {
> struct power_supply *charger;
> struct power_supply_desc charger_desc;
> - struct acpi_device *device;
> + struct device *dev;
I'm not convinced about this change.
If I'm not mistaken, you only use the dev pointer above to get the
ACPI_COMPANION() of it, but the latter is already found in _probe(),
so it can be stored in struct acpi_ac for later use and then the dev
pointer in there will not be necessary any more.
That will save you a bunch of ACPI_HANDLE() evaluations and there's
nothing wrong with using ac->device->handle. The patch will then
become almost trivial AFAICS and if you really need to get from ac to
the underlying platform device, a pointer to it can be added to struct
acpi_ac without removing the ACPI device pointer from it.
Yeah we could add platform device without removing acpi device, and
yes that would introduce data duplication, like Andy noticed. And yes,
maybe for this particular driver there is little impact (as struct device is not
really used), but in my opinion we should adhere to some common coding
pattern among all acpi drivers in order for the code to be easier to maintain
and improve readability, also for any newcomer.
>>> The idea behind is to eliminate data duplication.
>> What data duplication exactly do you mean?
>>
>> struct acpi_device *device is replaced with struct device *dev which
>> is the same size. The latter is then used to obtain a struct
>> acpi_device pointer. Why is it better to do this than to store the
>> struct acpi_device itself?
> This should be "store the struct acpi_device pointer itself", sorry.
Hi,
So let me explain the reasoning here:
1) I've had a pleasure to work with different drivers in ACPI directory. In my
opinion the best ones I've seen, were build around the concept of struct
device (e.g NFIT). It's a struct that's well understood in the kernel, and
it's easier to understand without having to read any ACPI specific code.
If I see something like ACPI_HANDLE(dev), I think 'ah-ha - having a struct
device I can easily get struct acpi_device - they're connected'. And I think
using a standardized macro, instead of manually dereferencing pointers is
also much easier for ACPI newbs reading the code, as it hides a bit
complexity
of getting acpi device from struct device. And to be honest I don't think
there would
be any measurable performance change, as those code paths are far from
being considered 'hot paths'. So if we can get the code easier to understand
from a newcomer perspective, why not do it.
2) I think it would be good to stick to the convention, and introduce some
coding
pattern, for now some drivers store the struct device pointer, and other
store
acpi device pointer . As I'm doing this change acpi device pointer become
less
relevant, as we're using platform device. So to reflect that loss of
relevance
we can choose to adhere to a pattern where we use it less and less, and the
winning approach would be to use 'struct device' by default everywhere we
can
so maybe eventually we would be able to lose acpi_device altogether at
some point,
as most of the usage is to retrieve acpi handle and execute some AML
method.
So in my understanding acpi device is already obsolete at this point, if
we can
manage to use it less and less, and eventually wipe it out then why not ;-)
Thanks again !
Michał
Re: [PATCH v1 2/9] docs: firmware-guide: ACPI: Clarify ACPI bus concepts
On 10/6/2023 5:36 PM, Rafael J. Wysocki wrote: > On Thu, Oct 5, 2023 at 10:39 PM Wilczynski, Michal > wrote: >> >> >> On 10/5/2023 8:28 PM, Wilczynski, Michal wrote: >>> On 10/5/2023 7:57 PM, Rafael J. Wysocki wrote: >>>> On Monday, September 25, 2023 4:48:35 PM CEST Michal Wilczynski wrote: >>>>> Some devices implement ACPI driver as a way to manage devices >>>>> enumerated by the ACPI. This might be confusing as a preferred way to >>>>> implement a driver for devices not connected to any bus is a platform >>>>> driver, as stated in the documentation. Clarify relationships between >>>>> ACPI device, platform device and ACPI entries. >>>>> >>>>> Suggested-by: Elena Reshetova >>>>> Signed-off-by: Michal Wilczynski >>>>> --- >>>>> Documentation/firmware-guide/acpi/enumeration.rst | 13 + >>>>> 1 file changed, 13 insertions(+) >>>>> >>>>> diff --git a/Documentation/firmware-guide/acpi/enumeration.rst >>>>> b/Documentation/firmware-guide/acpi/enumeration.rst >>>>> index 56d9913a3370..f56cc79a9e83 100644 >>>>> --- a/Documentation/firmware-guide/acpi/enumeration.rst >>>>> +++ b/Documentation/firmware-guide/acpi/enumeration.rst >>>>> @@ -64,6 +64,19 @@ If the driver needs to perform more complex >>>>> initialization like getting and >>>>> configuring GPIOs it can get its ACPI handle and extract this information >>>>> from ACPI tables. >>>>> >>>>> +ACPI bus >>>>> + >>>>> + >>>>> +Historically some devices not connected to any bus were represented as >>>>> ACPI >>>>> +devices, and had to implement ACPI driver. This is not a preferred way >>>>> for new >>>>> +drivers. As explained above devices not connected to any bus should >>>>> implement >>>>> +platform driver. ACPI device would be created during enumeration >>>>> nonetheless, >>>>> +and would be accessible through ACPI_COMPANION() macro, and the ACPI >>>>> handle would >>>>> +be accessible through ACPI_HANDLE() macro. ACPI device is meant to >>>>> describe >>>>> +information related to ACPI entry e.g. handle of the ACPI entry. Think - >>>>> +ACPI device interfaces with the FW, and the platform device with the >>>>> rest of >>>>> +the system. >>>>> + >>>>> DMA support >>>>> === >>>> I rewrote the above entirely, so here's a new patch to replace this one: >>>> >>>> --- >>>> From: Rafael J. Wysocki >>>> Subject: [PATCH v2 2/9] ACPI: docs: enumeration: Clarify ACPI bus concepts >>>> >>>> In some cases, ACPI drivers are implemented as a way to manage devices >>>> enumerated with the help of the platform firmware through ACPI. >>>> >>>> This might be confusing, since the preferred way to implement a driver >>>> for a device that cannot be enumerated natively, is a platform >>>> driver, as stated in the documentation. >>>> >>>> Clarify relationships between ACPI device objects, platform devices and >>>> ACPI Namespace entries. >>>> >>>> Suggested-by: Elena Reshetova >>>> Co-developed-by: Michal Wilczynski >>>> Signed-off-by: Michal Wilczynski >>>> Signed-off-by: Rafael J. Wysocki >>>> --- >>>> Documentation/firmware-guide/acpi/enumeration.rst | 43 >>>> ++ >>>> 1 file changed, 43 insertions(+) >>>> >>>> Index: linux-pm/Documentation/firmware-guide/acpi/enumeration.rst >>>> === >>>> --- linux-pm.orig/Documentation/firmware-guide/acpi/enumeration.rst >>>> +++ linux-pm/Documentation/firmware-guide/acpi/enumeration.rst >>>> @@ -64,6 +64,49 @@ If the driver needs to perform more comp >>>> configuring GPIOs it can get its ACPI handle and extract this information >>>> from ACPI tables. >>>> >>>> +ACPI device objects >>>> +=== >>>> + >>>> +Generally speaking, there are two categories of devices in a system in >>>> which >>>> +ACPI is used as an interf
Re: [PATCH v1 2/9] docs: firmware-guide: ACPI: Clarify ACPI bus concepts
On 10/5/2023 8:28 PM, Wilczynski, Michal wrote: > > On 10/5/2023 7:57 PM, Rafael J. Wysocki wrote: >> On Monday, September 25, 2023 4:48:35 PM CEST Michal Wilczynski wrote: >>> Some devices implement ACPI driver as a way to manage devices >>> enumerated by the ACPI. This might be confusing as a preferred way to >>> implement a driver for devices not connected to any bus is a platform >>> driver, as stated in the documentation. Clarify relationships between >>> ACPI device, platform device and ACPI entries. >>> >>> Suggested-by: Elena Reshetova >>> Signed-off-by: Michal Wilczynski >>> --- >>> Documentation/firmware-guide/acpi/enumeration.rst | 13 + >>> 1 file changed, 13 insertions(+) >>> >>> diff --git a/Documentation/firmware-guide/acpi/enumeration.rst >>> b/Documentation/firmware-guide/acpi/enumeration.rst >>> index 56d9913a3370..f56cc79a9e83 100644 >>> --- a/Documentation/firmware-guide/acpi/enumeration.rst >>> +++ b/Documentation/firmware-guide/acpi/enumeration.rst >>> @@ -64,6 +64,19 @@ If the driver needs to perform more complex >>> initialization like getting and >>> configuring GPIOs it can get its ACPI handle and extract this information >>> from ACPI tables. >>> >>> +ACPI bus >>> + >>> + >>> +Historically some devices not connected to any bus were represented as ACPI >>> +devices, and had to implement ACPI driver. This is not a preferred way for >>> new >>> +drivers. As explained above devices not connected to any bus should >>> implement >>> +platform driver. ACPI device would be created during enumeration >>> nonetheless, >>> +and would be accessible through ACPI_COMPANION() macro, and the ACPI >>> handle would >>> +be accessible through ACPI_HANDLE() macro. ACPI device is meant to describe >>> +information related to ACPI entry e.g. handle of the ACPI entry. Think - >>> +ACPI device interfaces with the FW, and the platform device with the rest >>> of >>> +the system. >>> + >>> DMA support >>> === >> I rewrote the above entirely, so here's a new patch to replace this one: >> >> --- >> From: Rafael J. Wysocki >> Subject: [PATCH v2 2/9] ACPI: docs: enumeration: Clarify ACPI bus concepts >> >> In some cases, ACPI drivers are implemented as a way to manage devices >> enumerated with the help of the platform firmware through ACPI. >> >> This might be confusing, since the preferred way to implement a driver >> for a device that cannot be enumerated natively, is a platform >> driver, as stated in the documentation. >> >> Clarify relationships between ACPI device objects, platform devices and >> ACPI Namespace entries. >> >> Suggested-by: Elena Reshetova >> Co-developed-by: Michal Wilczynski >> Signed-off-by: Michal Wilczynski >> Signed-off-by: Rafael J. Wysocki >> --- >> Documentation/firmware-guide/acpi/enumeration.rst | 43 >> ++ >> 1 file changed, 43 insertions(+) >> >> Index: linux-pm/Documentation/firmware-guide/acpi/enumeration.rst >> === >> --- linux-pm.orig/Documentation/firmware-guide/acpi/enumeration.rst >> +++ linux-pm/Documentation/firmware-guide/acpi/enumeration.rst >> @@ -64,6 +64,49 @@ If the driver needs to perform more comp >> configuring GPIOs it can get its ACPI handle and extract this information >> from ACPI tables. >> >> +ACPI device objects >> +=== >> + >> +Generally speaking, there are two categories of devices in a system in which >> +ACPI is used as an interface between the platform firmware and the OS: >> Devices >> +that can be discovered and enumerated natively, through a protocol defined >> for >> +the specific bus that they are on (for example, configuration space in PCI), >> +without the platform firmware assistance, and devices that need to be >> described >> +by the platform firmware so that they can be discovered. Still, for any >> device >> +known to the platform firmware, regardless of which category it falls into, >> +there can be a corresponding ACPI device object in the ACPI Namespace in >> which >> +case the Linux kernel will create a struct acpi_device object based on it >> for >> +that device. >> + >> +Those struct acpi_device objects are never used for binding drivers to &g
Re: [PATCH v1 2/9] docs: firmware-guide: ACPI: Clarify ACPI bus concepts
On 10/5/2023 7:57 PM, Rafael J. Wysocki wrote: > On Monday, September 25, 2023 4:48:35 PM CEST Michal Wilczynski wrote: >> Some devices implement ACPI driver as a way to manage devices >> enumerated by the ACPI. This might be confusing as a preferred way to >> implement a driver for devices not connected to any bus is a platform >> driver, as stated in the documentation. Clarify relationships between >> ACPI device, platform device and ACPI entries. >> >> Suggested-by: Elena Reshetova >> Signed-off-by: Michal Wilczynski >> --- >> Documentation/firmware-guide/acpi/enumeration.rst | 13 + >> 1 file changed, 13 insertions(+) >> >> diff --git a/Documentation/firmware-guide/acpi/enumeration.rst >> b/Documentation/firmware-guide/acpi/enumeration.rst >> index 56d9913a3370..f56cc79a9e83 100644 >> --- a/Documentation/firmware-guide/acpi/enumeration.rst >> +++ b/Documentation/firmware-guide/acpi/enumeration.rst >> @@ -64,6 +64,19 @@ If the driver needs to perform more complex >> initialization like getting and >> configuring GPIOs it can get its ACPI handle and extract this information >> from ACPI tables. >> >> +ACPI bus >> + >> + >> +Historically some devices not connected to any bus were represented as ACPI >> +devices, and had to implement ACPI driver. This is not a preferred way for >> new >> +drivers. As explained above devices not connected to any bus should >> implement >> +platform driver. ACPI device would be created during enumeration >> nonetheless, >> +and would be accessible through ACPI_COMPANION() macro, and the ACPI handle >> would >> +be accessible through ACPI_HANDLE() macro. ACPI device is meant to describe >> +information related to ACPI entry e.g. handle of the ACPI entry. Think - >> +ACPI device interfaces with the FW, and the platform device with the rest of >> +the system. >> + >> DMA support >> === > I rewrote the above entirely, so here's a new patch to replace this one: > > --- > From: Rafael J. Wysocki > Subject: [PATCH v2 2/9] ACPI: docs: enumeration: Clarify ACPI bus concepts > > In some cases, ACPI drivers are implemented as a way to manage devices > enumerated with the help of the platform firmware through ACPI. > > This might be confusing, since the preferred way to implement a driver > for a device that cannot be enumerated natively, is a platform > driver, as stated in the documentation. > > Clarify relationships between ACPI device objects, platform devices and > ACPI Namespace entries. > > Suggested-by: Elena Reshetova > Co-developed-by: Michal Wilczynski > Signed-off-by: Michal Wilczynski > Signed-off-by: Rafael J. Wysocki > --- > Documentation/firmware-guide/acpi/enumeration.rst | 43 > ++ > 1 file changed, 43 insertions(+) > > Index: linux-pm/Documentation/firmware-guide/acpi/enumeration.rst > === > --- linux-pm.orig/Documentation/firmware-guide/acpi/enumeration.rst > +++ linux-pm/Documentation/firmware-guide/acpi/enumeration.rst > @@ -64,6 +64,49 @@ If the driver needs to perform more comp > configuring GPIOs it can get its ACPI handle and extract this information > from ACPI tables. > > +ACPI device objects > +=== > + > +Generally speaking, there are two categories of devices in a system in which > +ACPI is used as an interface between the platform firmware and the OS: > Devices > +that can be discovered and enumerated natively, through a protocol defined > for > +the specific bus that they are on (for example, configuration space in PCI), > +without the platform firmware assistance, and devices that need to be > described > +by the platform firmware so that they can be discovered. Still, for any > device > +known to the platform firmware, regardless of which category it falls into, > +there can be a corresponding ACPI device object in the ACPI Namespace in > which > +case the Linux kernel will create a struct acpi_device object based on it for > +that device. > + > +Those struct acpi_device objects are never used for binding drivers to > natively > +discoverable devices, because they are represented by other types of device > +objects (for example, struct pci_dev for PCI devices) that are bound to by > +device drivers (the corresponding struct acpi_device object is then used as > +an additional source of information on the configuration of the given > device). > +Moreover, the core ACPI device enumeration code creates struct > platform_device > +objects for the majority of devices that are discovered and enumerated with > the > +help of the platform firmware and those platform device objects can be bound > to > +by platform drivers in direct analogy with the natively enumerable devices > +case. Therefore it is logically inconsistent and so generally invalid to > bind > +drivers to struct acpi_device objects, including drivers for devices that are > +discovered with the help of the platform
Re: [PATCH v1 1/9] ACPI: bus: Make notify wrappers more generic
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 >> wrote: >>> On 10/5/2023 12:57 PM, Rafael J. Wysocki wrote: >>>> On Thu, Oct 5, 2023 at 10:10 AM Wilczynski, Michal >>>> 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 m
Re: [PATCH v1 1/9] ACPI: bus: Make notify wrappers more generic
On 10/5/2023 12:57 PM, Rafael J. Wysocki wrote: > On Thu, Oct 5, 2023 at 10:10 AM Wilczynski, Michal > 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 >>> 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 >>>> Signed-off-by: Michal Wilczynski >>>> --- >>>> >>>> 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(>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(>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 voi
Re: [PATCH v1 1/9] ACPI: bus: Make notify wrappers more generic
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 > 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 >> Signed-off-by: Michal Wilczynski >> --- >> >> 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(>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(>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(_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(>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, >>
Re: [PATCH v1 1/1] ACPI: NFIT: Switch to use acpi_evaluate_dsm_typed()
On 10/2/2023 3:54 PM, Andy Shevchenko wrote:
> The acpi_evaluate_dsm_typed() provides a way to check the type of the
> object evaluated by _DSM call. Use it instead of open coded variant.
>
> Signed-off-by: Andy Shevchenko
> ---
> drivers/acpi/nfit/core.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index f96bf32cd368..280da408c02c 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -1737,9 +1737,8 @@ __weak void nfit_intel_shutdown_status(struct nfit_mem
> *nfit_mem)
> if ((nfit_mem->dsm_mask & (1 << func)) == 0)
> return;
>
> - out_obj = acpi_evaluate_dsm(handle, guid, revid, func, _obj);
> - if (!out_obj || out_obj->type != ACPI_TYPE_BUFFER
> - || out_obj->buffer.length < sizeof(smart)) {
> + out_obj = acpi_evaluate_dsm_typed(handle, guid, revid, func, _obj,
> ACPI_TYPE_BUFFER);
This line is 90 characters long, wouldn't it be better to split it ?
> + if (!out_obj || out_obj->buffer.length < sizeof(smart)) {
> dev_dbg(dev->parent, "%s: failed to retrieve initial health\n",
> dev_name(dev));
While at it maybe fix alignment ? :-)
> ACPI_FREE(out_obj);
Just nitpicks, functionally code seems correct to me.
Reviewed-by: Michal Wilczynski
Re: [PATCH v5 09/10] acpi/nfit: Move handler installing logic to driver
On 6/30/2023 7:19 PM, Dan Williams wrote:
> Wilczynski, Michal wrote:
>>
>> On 6/29/2023 10:54 PM, Dan Williams wrote:
>>> Michal Wilczynski wrote:
>>>> Currently logic for installing notifications from ACPI devices is
>>>> implemented using notify callback in struct acpi_driver. Preparations
>>>> are being made to replace acpi_driver with more generic struct
>>>> platform_driver, which doesn't contain notify callback. Furthermore
>>>> as of now handlers are being called indirectly through
>>>> acpi_notify_device(), which decreases performance.
>>>>
>>>> Call acpi_dev_install_notify_handler() at the end of .add() callback.
>>>> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
>>>> callback. Change arguments passed to the notify function to match with
>>>> what's required by acpi_install_notify_handler(). Remove .notify
>>>> callback initialization in acpi_driver.
>>>>
>>>> Suggested-by: Rafael J. Wysocki
>>>> Signed-off-by: Michal Wilczynski
>>>> ---
>>>> drivers/acpi/nfit/core.c | 24 ++--
>>>> 1 file changed, 18 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>>>> index 95930e9d776c..a281bdfee8a0 100644
>>>> --- a/drivers/acpi/nfit/core.c
>>>> +++ b/drivers/acpi/nfit/core.c
>>>> @@ -3312,11 +3312,13 @@ void acpi_nfit_shutdown(void *data)
>>>> }
>>>> EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
>>>>
>>>> -static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
>>>> +static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
>>>> {
>>>> - device_lock(>dev);
>>>> - __acpi_nfit_notify(>dev, adev->handle, event);
>>>> - device_unlock(>dev);
>>>> + struct acpi_device *device = data;
>>>> +
>>>> + device_lock(>dev);
>>>> + __acpi_nfit_notify(>dev, handle, event);
>>>> + device_unlock(>dev);
>>>> }
>>>>
>>>> static int acpi_nfit_add(struct acpi_device *adev)
>>>> @@ -3375,12 +3377,23 @@ static int acpi_nfit_add(struct acpi_device *adev)
>>>>
>>>>if (rc)
>>>>return rc;
>>>> - return devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
>>>> +
>>>> + rc = devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
>>>> + if (rc)
>>>> + return rc;
>>>> +
>>>> + return acpi_dev_install_notify_handler(adev,
>>>> + ACPI_DEVICE_NOTIFY,
>>>> + acpi_nfit_notify);
>>>> }
>>>>
>>>> static void acpi_nfit_remove(struct acpi_device *adev)
>>>> {
>>>>/* see acpi_nfit_unregister */
>>>> +
>>>> + acpi_dev_remove_notify_handler(adev,
>>>> + ACPI_DEVICE_NOTIFY,
>>>> + acpi_nfit_notify);
>>> Please use devm to trigger this release rather than making
>>> acpi_nfit_remove() contain any logic.
>> I think adding separate devm action to remove event handler is not
>> necessary. I'll put the removal in the beggining of acpi_nfit_shutdown() if
>> you
>> don't object.
> How do you plan to handle an acpi_dev_install_notify_handler() failure?
> acpi_nfit_shutdown() will need to have extra logic to know that it can
> skip acpi_dev_remove_notify_handler() in some cases and not other..
> Maybe it is ok to remove a handler that was never installed, but I would
> rather not go look that up. A devm callback for
> acpi_dev_remove_notify_handler() avoids that.
Sure, I looked at the code and it seems to me that trying to remove a callback
that doesn't
exist shouldn't cause any problems. But maybe it's not very elegant and we
shouldn't rely
on that behavior.
Will add separate devm action for that then.
Re: [PATCH v5 09/10] acpi/nfit: Move handler installing logic to driver
On 6/29/2023 10:54 PM, Dan Williams wrote:
> Michal Wilczynski wrote:
>> Currently logic for installing notifications from ACPI devices is
>> implemented using notify callback in struct acpi_driver. Preparations
>> are being made to replace acpi_driver with more generic struct
>> platform_driver, which doesn't contain notify callback. Furthermore
>> as of now handlers are being called indirectly through
>> acpi_notify_device(), which decreases performance.
>>
>> Call acpi_dev_install_notify_handler() at the end of .add() callback.
>> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
>> callback. Change arguments passed to the notify function to match with
>> what's required by acpi_install_notify_handler(). Remove .notify
>> callback initialization in acpi_driver.
>>
>> Suggested-by: Rafael J. Wysocki
>> Signed-off-by: Michal Wilczynski
>> ---
>> drivers/acpi/nfit/core.c | 24 ++--
>> 1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> index 95930e9d776c..a281bdfee8a0 100644
>> --- a/drivers/acpi/nfit/core.c
>> +++ b/drivers/acpi/nfit/core.c
>> @@ -3312,11 +3312,13 @@ void acpi_nfit_shutdown(void *data)
>> }
>> EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
>>
>> -static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
>> +static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
>> {
>> -device_lock(>dev);
>> -__acpi_nfit_notify(>dev, adev->handle, event);
>> -device_unlock(>dev);
>> +struct acpi_device *device = data;
>> +
>> +device_lock(>dev);
>> +__acpi_nfit_notify(>dev, handle, event);
>> +device_unlock(>dev);
>> }
>>
>> static int acpi_nfit_add(struct acpi_device *adev)
>> @@ -3375,12 +3377,23 @@ static int acpi_nfit_add(struct acpi_device *adev)
>>
>> if (rc)
>> return rc;
>> -return devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
>> +
>> +rc = devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
>> +if (rc)
>> +return rc;
>> +
>> +return acpi_dev_install_notify_handler(adev,
>> + ACPI_DEVICE_NOTIFY,
>> + acpi_nfit_notify);
>> }
>>
>> static void acpi_nfit_remove(struct acpi_device *adev)
>> {
>> /* see acpi_nfit_unregister */
>> +
>> +acpi_dev_remove_notify_handler(adev,
>> + ACPI_DEVICE_NOTIFY,
>> + acpi_nfit_notify);
> Please use devm to trigger this release rather than making
> acpi_nfit_remove() contain any logic.
I think adding separate devm action to remove event handler is not
necessary. I'll put the removal in the beggining of acpi_nfit_shutdown() if you
don't object.
>
> An additional cleanup opportunity with the ->add() path fully devm
> instrumented would be to just delete acpi_nfit_remove() since it is
> optional and serves no purpose.
Will do,
Thanks !
Re: [PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids
On 6/30/2023 1:13 PM, Rafael J. Wysocki wrote: > On Fri, Jun 30, 2023 at 1:04 PM Rafael J. Wysocki wrote: >> On Fri, Jun 30, 2023 at 11:52 AM Wilczynski, Michal >> wrote: >>> >>> >>> On 6/29/2023 6:14 PM, Rafael J. Wysocki wrote: >>>> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski >>>> wrote: >>>>> Currently terminator line contains redunant characters. >>>> Well, they are terminating the list properly AFAICS, so they aren't >>>> redundant and the size of it before and after the change is actually >>>> the same, isn't it? >>> This syntax is correct of course, but we have an internal guidelines >>> specifically >>> saying that terminator line should NOT contain a comma at the end. >>> Justification: >>> >>> "Terminator line is established for the data structure arrays which may >>> have unknown, >>> to the caller, sizes. The purpose of it is to stop iteration over an array >>> and avoid >>> out-of-boundary access. Nevertheless, we may apply a bit more stricter rule >>> to avoid >>> potential, but unlike, event of adding the entry after terminator, already >>> at compile time. >>> This will be achieved by not putting comma at the end of terminator line" >> This certainly applies to any new code. >> >> The existing code, however, is what it is and the question is how much >> of an improvement the given change makes. >> >> So yes, it may not follow the current rules for new code, but then it >> may not be worth changing to follow these rules anyway. > This is a bit like housing in a city. > > Usually, there are strict requirements that must be followed while > constructing a new building, but existing buildings are not > reconstructed to follow them in the majority of cases. It may not > even be a good idea to do that. Thanks, great explanation ! I think it's a shared sentiment among maintainers. I've been watching upstreaming effort of intel new idpf driver, and it got rejected basically because new drivers are held to a higher standard (they didn't modernize their code to use new page pool API). https://lore.kernel.org/netdev/20230621122106.56cb5...@kernel.org/#t
Re: [PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids
On 6/29/2023 10:51 PM, Dan Williams wrote:
> Michal Wilczynski wrote:
>> Currently terminator line contains redunant characters. Remove them and
>> also remove a comma at the end.
>>
>> Signed-off-by: Michal Wilczynski
>> ---
>> drivers/acpi/nfit/core.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> index aff79cbc2190..95930e9d776c 100644
>> --- a/drivers/acpi/nfit/core.c
>> +++ b/drivers/acpi/nfit/core.c
>> @@ -3455,7 +3455,7 @@ EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
>>
>> static const struct acpi_device_id acpi_nfit_ids[] = {
>> { "ACPI0012", 0 },
>> -{ "", 0 },
>> +{}
> Looks like a pointless change to me.
It's not very consequential, but isn't totally pointless in my view:
"Terminator line is established for the data structure arrays which may have
unknown,
to the caller, sizes. The purpose of it is to stop iteration over an array and
avoid
out-of-boundary access. Nevertheless, we may apply a bit more stricter rule to
avoid
potential, but unlike, event of adding the entry after terminator, already at
compile time.
This will be achieved by not putting comma at the end of terminator line"
Anyway I can drop this change, it's just confusing everyone
Re: [PATCH v5 09/10] acpi/nfit: Move handler installing logic to driver
On 6/29/2023 6:18 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> wrote:
>> Currently logic for installing notifications from ACPI devices is
>> implemented using notify callback in struct acpi_driver. Preparations
>> are being made to replace acpi_driver with more generic struct
>> platform_driver, which doesn't contain notify callback. Furthermore
>> as of now handlers are being called indirectly through
>> acpi_notify_device(), which decreases performance.
>>
>> Call acpi_dev_install_notify_handler() at the end of .add() callback.
>> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
>> callback. Change arguments passed to the notify function to match with
>> what's required by acpi_install_notify_handler(). Remove .notify
>> callback initialization in acpi_driver.
>>
>> Suggested-by: Rafael J. Wysocki
>> Signed-off-by: Michal Wilczynski
>> ---
>> drivers/acpi/nfit/core.c | 24 ++--
>> 1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> index 95930e9d776c..a281bdfee8a0 100644
>> --- a/drivers/acpi/nfit/core.c
>> +++ b/drivers/acpi/nfit/core.c
>> @@ -3312,11 +3312,13 @@ void acpi_nfit_shutdown(void *data)
>> }
>> EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
>>
>> -static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
>> +static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
>> {
>> - device_lock(>dev);
>> - __acpi_nfit_notify(>dev, adev->handle, event);
>> - device_unlock(>dev);
> It's totally not necessary to rename the ACPI device variable here.
>
> Just add
>
> struct acpi_device *adev = data;
>
> to this function.
Sure, is adev a preferred name for acpi_device ? I've seen a mix of different
naming
in drivers, some use device, adev, acpi_dev and so on. I suppose it's not a big
deal, but
it would be good to know.
>
>> + struct acpi_device *device = data;
>> +
>> + device_lock(>dev);
>> + __acpi_nfit_notify(>dev, handle, event);
>> + device_unlock(>dev);
>> }
>>
>> static int acpi_nfit_add(struct acpi_device *adev)
>> @@ -3375,12 +3377,23 @@ static int acpi_nfit_add(struct acpi_device *adev)
>>
>> if (rc)
>> return rc;
>> - return devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
>> +
>> + rc = devm_add_action_or_reset(dev, acpi_nfit_shutdown, acpi_desc);
>> + if (rc)
>> + return rc;
>> +
>> + return acpi_dev_install_notify_handler(adev,
>> + ACPI_DEVICE_NOTIFY,
>> + acpi_nfit_notify);
>> }
>>
>> static void acpi_nfit_remove(struct acpi_device *adev)
>> {
>> /* see acpi_nfit_unregister */
>> +
>> + acpi_dev_remove_notify_handler(adev,
>> + ACPI_DEVICE_NOTIFY,
>> + acpi_nfit_notify);
>> }
>>
>> static void acpi_nfit_update_notify(struct device *dev, acpi_handle handle)
>> @@ -3465,7 +3478,6 @@ static struct acpi_driver acpi_nfit_driver = {
>> .ops = {
>> .add = acpi_nfit_add,
>> .remove = acpi_nfit_remove,
>> - .notify = acpi_nfit_notify,
>> },
>> };
>>
>> --
Re: [PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids
On 6/29/2023 6:14 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> wrote:
>> Currently terminator line contains redunant characters.
> Well, they are terminating the list properly AFAICS, so they aren't
> redundant and the size of it before and after the change is actually
> the same, isn't it?
This syntax is correct of course, but we have an internal guidelines
specifically
saying that terminator line should NOT contain a comma at the end.
Justification:
"Terminator line is established for the data structure arrays which may have
unknown,
to the caller, sizes. The purpose of it is to stop iteration over an array and
avoid
out-of-boundary access. Nevertheless, we may apply a bit more stricter rule to
avoid
potential, but unlike, event of adding the entry after terminator, already at
compile time.
This will be achieved by not putting comma at the end of terminator line"
>
>> Remove them and also remove a comma at the end.
> I suppose that this change is made for consistency with the other ACPI
> code, so this would be the motivation to give in the changelog.
>
> In any case, it doesn't seem to be related to the rest of the series.
>
>> Signed-off-by: Michal Wilczynski
>> ---
>> drivers/acpi/nfit/core.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> index aff79cbc2190..95930e9d776c 100644
>> --- a/drivers/acpi/nfit/core.c
>> +++ b/drivers/acpi/nfit/core.c
>> @@ -3455,7 +3455,7 @@ EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
>>
>> static const struct acpi_device_id acpi_nfit_ids[] = {
>> { "ACPI0012", 0 },
>> - { "", 0 },
>> + {}
>> };
>> MODULE_DEVICE_TABLE(acpi, acpi_nfit_ids);
>>
>> --
>> 2.41.0
>>
Re: [PATCH v5 05/10] acpi/battery: Move handler installing logic to driver
On 6/29/2023 6:05 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> wrote:
>> Currently logic for installing notifications from ACPI devices is
>> implemented using notify callback in struct acpi_driver. Preparations
>> are being made to replace acpi_driver with more generic struct
>> platform_driver, which doesn't contain notify callback. Furthermore
>> as of now handlers are being called indirectly through
>> acpi_notify_device(), which decreases performance.
>>
>> Call acpi_dev_install_notify_handler() at the end of .add() callback.
>> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
>> callback. Change arguments passed to the notify function to match with
>> what's required by acpi_install_notify_handler(). Remove .notify
>> callback initialization in acpi_driver.
>>
>> While at it, fix lack of whitespaces in .remove() callback.
>>
>> Suggested-by: Rafael J. Wysocki
>> Signed-off-by: Michal Wilczynski
>> ---
>> drivers/acpi/battery.c | 30 --
>> 1 file changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>> index 9c67ed02d797..6337e7b1f6e1 100644
>> --- a/drivers/acpi/battery.c
>> +++ b/drivers/acpi/battery.c
>> @@ -1034,11 +1034,14 @@ static void acpi_battery_refresh(struct acpi_battery
>> *battery)
>> }
>>
>> /* Driver Interface */
>> -static void acpi_battery_notify(struct acpi_device *device, u32 event)
>> +static void acpi_battery_notify(acpi_handle handle, u32 event, void *data)
>> {
>> - struct acpi_battery *battery = acpi_driver_data(device);
>> + struct acpi_device *device = data;
>> + struct acpi_battery *battery;
>> struct power_supply *old;
>>
>> + battery = acpi_driver_data(device);
>> +
>> if (!battery)
>> return;
>> old = battery->bat;
>> @@ -1212,13 +1215,23 @@ static int acpi_battery_add(struct acpi_device
>> *device)
>>
>> device_init_wakeup(>dev, 1);
>>
>> - return result;
>> + result = acpi_dev_install_notify_handler(device,
>> +ACPI_ALL_NOTIFY,
>> +acpi_battery_notify);
>> + if (result)
>> + goto fail_deinit_wakup_and_unregister;
> You could call the label "fail_pm", for example, which would be more
> concise and so slightly easier to follow, without any loss of clarity
> AFAICS.
Sure
>
>> +
>> + return 0;
>>
>> +fail_deinit_wakup_and_unregister:
>> + device_init_wakeup(>dev, 0);
>> + unregister_pm_notifier(>pm_nb);
>> fail:
>> sysfs_remove_battery(battery);
>> mutex_destroy(>lock);
>> mutex_destroy(>sysfs_lock);
>> kfree(battery);
>> +
>> return result;
>> }
>>
>> @@ -1228,10 +1241,17 @@ static void acpi_battery_remove(struct acpi_device
>> *device)
>>
>> if (!device || !acpi_driver_data(device))
>> return;
>> - device_init_wakeup(>dev, 0);
>> +
>> battery = acpi_driver_data(device);
>> +
>> + acpi_dev_remove_notify_handler(device,
>> + ACPI_ALL_NOTIFY,
>> + acpi_battery_notify);
>> +
>> + device_init_wakeup(>dev, 0);
>> unregister_pm_notifier(>pm_nb);
>> sysfs_remove_battery(battery);
>> +
>> mutex_destroy(>lock);
>> mutex_destroy(>sysfs_lock);
>> kfree(battery);
>> @@ -1264,11 +1284,9 @@ static struct acpi_driver acpi_battery_driver = {
>> .name = "battery",
>> .class = ACPI_BATTERY_CLASS,
>> .ids = battery_device_ids,
>> - .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
>> .ops = {
>> .add = acpi_battery_add,
>> .remove = acpi_battery_remove,
>> - .notify = acpi_battery_notify,
>> },
>> .drv.pm = _battery_pm,
>> };
>> --
>> 2.41.0
>>
Re: [PATCH v5 07/10] acpi/nfit: Move acpi_nfit_notify() before acpi_nfit_add()
On 6/29/2023 6:06 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> wrote:
>> To use new style of installing event handlers acpi_nfit_notify() needs
>> to be known inside acpi_nfit_add(). Move acpi_nfit_notify() upwards in
>> the file, so it can be used inside acpi_nfit_add().
>>
>> Signed-off-by: Michal Wilczynski
>> ---
>> drivers/acpi/nfit/core.c | 14 +++---
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
>> index 07204d482968..aff79cbc2190 100644
>> --- a/drivers/acpi/nfit/core.c
>> +++ b/drivers/acpi/nfit/core.c
>> @@ -3312,6 +3312,13 @@ void acpi_nfit_shutdown(void *data)
>> }
>> EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
>>
>> +static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
>> +{
>> + device_lock(>dev);
>> + __acpi_nfit_notify(>dev, adev->handle, event);
>> + device_unlock(>dev);
>> +}
>> +
>> static int acpi_nfit_add(struct acpi_device *adev)
>> {
>> struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
>> @@ -3446,13 +3453,6 @@ void __acpi_nfit_notify(struct device *dev,
>> acpi_handle handle, u32 event)
>> }
>> EXPORT_SYMBOL_GPL(__acpi_nfit_notify);
>>
>> -static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
>> -{
>> - device_lock(>dev);
>> - __acpi_nfit_notify(>dev, adev->handle, event);
>> - device_unlock(>dev);
>> -}
>> -
>> static const struct acpi_device_id acpi_nfit_ids[] = {
>> { "ACPI0012", 0 },
>> { "", 0 },
>> --
> Please fold this patch into the next one. By itself, it is an
> artificial change IMV.
I agree with you, but I got told specifically to do that.
https://lore.kernel.org/linux-acpi/e0f67199-9feb-432c-f0cb-7bdbdaf9f...@linux.intel.com/
Re: [PATCH v5 04/10] acpi/video: Move handler installing logic to driver
On 6/29/2023 5:58 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> wrote:
>> Currently logic for installing notifications from ACPI devices is
>> implemented using notify callback in struct acpi_driver. Preparations
>> are being made to replace acpi_driver with more generic struct
>> platform_driver, which doesn't contain notify callback. Furthermore
>> as of now handlers are being called indirectly through
>> acpi_notify_device(), which decreases performance.
>>
>> Call acpi_dev_install_notify_handler() at the end of .add() callback.
>> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
>> callback. Change arguments passed to the notify function to match with
>> what's required by acpi_install_notify_handler(). Remove .notify
>> callback initialization in acpi_driver.
>>
>> Suggested-by: Rafael J. Wysocki
>> Signed-off-by: Michal Wilczynski
>> ---
>> drivers/acpi/acpi_video.c | 26 ++
>> 1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
>> index 62f4364e4460..60b7013d0009 100644
>> --- a/drivers/acpi/acpi_video.c
>> +++ b/drivers/acpi/acpi_video.c
>> @@ -77,7 +77,7 @@ static DEFINE_MUTEX(video_list_lock);
>> static LIST_HEAD(video_bus_head);
>> static int acpi_video_bus_add(struct acpi_device *device);
>> static void acpi_video_bus_remove(struct acpi_device *device);
>> -static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
>> +static void acpi_video_bus_notify(acpi_handle handle, u32 event, void
>> *data);
>>
>> /*
>> * Indices in the _BCL method response: the first two items are special,
>> @@ -104,7 +104,6 @@ static struct acpi_driver acpi_video_bus = {
>> .ops = {
>> .add = acpi_video_bus_add,
>> .remove = acpi_video_bus_remove,
>> - .notify = acpi_video_bus_notify,
>> },
>> };
>>
>> @@ -1527,12 +1526,15 @@ static int acpi_video_bus_stop_devices(struct
>> acpi_video_bus *video)
>> acpi_osi_is_win8() ? 0 : 1);
>> }
>>
>> -static void acpi_video_bus_notify(struct acpi_device *device, u32 event)
>> +static void acpi_video_bus_notify(acpi_handle handle, u32 event, void *data)
>> {
>> - struct acpi_video_bus *video = acpi_driver_data(device);
>> + struct acpi_device *device = data;
>> + struct acpi_video_bus *video;
>> struct input_dev *input;
>> int keycode = 0;
>>
>> + video = acpi_driver_data(device);
>> +
>> if (!video || !video->input)
>> return;
>>
>> @@ -2053,8 +2055,20 @@ 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);
>> + if (error)
>> + goto err_remove_and_unregister_video;
> This label name is a bit too long and the second half of it doesn't
> really add any value IMV. err_remove would be sufficient.
I've seen different patterns in the code, sometimes the label describe what
failed,
sometimes it describe what needs to be cleaned up. I don't really have a strong
preference, just thought it might be useful to the reader. Will change as
suggested.
>
>> +
>> return 0;
>>
>> +err_remove_and_unregister_video:
>> + mutex_lock(_list_lock);
>> + list_del(>entry);
>> + mutex_unlock(_list_lock);
>> + acpi_video_bus_remove_notify_handler(video);
>> + acpi_video_bus_unregister_backlight(video);
>> err_put_video:
>> acpi_video_bus_put_devices(video);
>> kfree(video->attached_array);
>> @@ -2075,6 +2089,10 @@ 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_video_bus_notify);
>> +
>> mutex_lock(_list_lock);
>> list_del(>entry);
>> mutex_unlock(_list_lock);
>> --
>> 2.41.0
>>
Re: [PATCH v5 03/10] acpi/ac: Move handler installing logic to driver
On 6/29/2023 5:55 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> wrote:
>> Currently logic for installing notifications from ACPI devices is
>> implemented using notify callback in struct acpi_driver. Preparations
>> are being made to replace acpi_driver with more generic struct
>> platform_driver, which doesn't contain notify callback. Furthermore
>> as of now handlers are being called indirectly through
>> acpi_notify_device(), which decreases performance.
>>
>> Call acpi_dev_install_notify_handler() at the end of .add() callback.
>> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
>> callback. Change arguments passed to the notify function to match with
>> what's required by acpi_install_notify_handler(). Remove .notify
>> callback initialization in acpi_driver.
>>
>> Suggested-by: Rafael J. Wysocki
>> Signed-off-by: Michal Wilczynski
>> ---
>> drivers/acpi/ac.c | 33 -
>> 1 file changed, 24 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
>> index 1ace70b831cd..207ee3c85bad 100644
>> --- a/drivers/acpi/ac.c
>> +++ b/drivers/acpi/ac.c
>> @@ -34,7 +34,7 @@ MODULE_LICENSE("GPL");
>>
>> static int acpi_ac_add(struct acpi_device *device);
>> static void acpi_ac_remove(struct acpi_device *device);
>> -static void acpi_ac_notify(struct acpi_device *device, u32 event);
>> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data);
>>
>> static const struct acpi_device_id ac_device_ids[] = {
>> {"ACPI0003", 0},
>> @@ -54,11 +54,9 @@ static struct acpi_driver acpi_ac_driver = {
>> .name = "ac",
>> .class = ACPI_AC_CLASS,
>> .ids = ac_device_ids,
>> - .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
>> .ops = {
>> .add = acpi_ac_add,
>> .remove = acpi_ac_remove,
>> - .notify = acpi_ac_notify,
>> },
>> .drv.pm = _ac_pm,
>> };
>> @@ -128,9 +126,12 @@ static enum power_supply_property ac_props[] = {
>> };
>>
>> /* Driver Model */
>> -static void acpi_ac_notify(struct acpi_device *device, u32 event)
>> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
>> {
>> - struct acpi_ac *ac = acpi_driver_data(device);
> This line doesn't need to be changed. Just add the device variable
> definition above it.
>
> And the same pattern is present in the other patches in the series.
I like the Reverse Christmas Tree, but sure will change that
>
>> + struct acpi_device *device = data;
>> + struct acpi_ac *ac;
>> +
>> + ac = acpi_driver_data(device);
>>
>> if (!ac)
>> return;
