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 <[email protected]>
Signed-off-by: Michal Wilczynski <[email protected]>
---
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(&adev->dev);
- __acpi_nfit_notify(&adev->dev, adev->handle, event);
- device_unlock(&adev->dev);
+ struct acpi_device *device = data;
+
+ device_lock(&device->dev);
+ __acpi_nfit_notify(&device->dev, handle, event);
+ device_unlock(&device->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,
},
};
--
2.41.0
On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
<[email protected]> 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 <[email protected]>
> Signed-off-by: Michal Wilczynski <[email protected]>
> ---
> 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(&adev->dev);
> - __acpi_nfit_notify(&adev->dev, adev->handle, event);
> - device_unlock(&adev->dev);
It's totally not necessary to rename the ACPI device variable here.
Just add
struct acpi_device *adev = data;
to this function.
> + struct acpi_device *device = data;
> +
> + device_lock(&device->dev);
> + __acpi_nfit_notify(&device->dev, handle, event);
> + device_unlock(&device->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,
> },
> };
>
> --
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 <[email protected]>
> Signed-off-by: Michal Wilczynski <[email protected]>
> ---
> 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(&adev->dev);
> - __acpi_nfit_notify(&adev->dev, adev->handle, event);
> - device_unlock(&adev->dev);
> + struct acpi_device *device = data;
> +
> + device_lock(&device->dev);
> + __acpi_nfit_notify(&device->dev, handle, event);
> + device_unlock(&device->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.
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.
On 6/29/2023 6:18 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> <[email protected]> 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 <[email protected]>
>> Signed-off-by: Michal Wilczynski <[email protected]>
>> ---
>> 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(&adev->dev);
>> - __acpi_nfit_notify(&adev->dev, adev->handle, event);
>> - device_unlock(&adev->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(&device->dev);
>> + __acpi_nfit_notify(&device->dev, handle, event);
>> + device_unlock(&device->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,
>> },
>> };
>>
>> --
On Fri, Jun 30, 2023 at 11:55 AM Wilczynski, Michal
<[email protected]> wrote:
>
>
>
> On 6/29/2023 6:18 PM, Rafael J. Wysocki wrote:
> > On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> > <[email protected]> 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 <[email protected]>
> >> Signed-off-by: Michal Wilczynski <[email protected]>
> >> ---
> >> 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(&adev->dev);
> >> - __acpi_nfit_notify(&adev->dev, adev->handle, event);
> >> - device_unlock(&adev->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 ?
In new code, it is.
In the existing code, it depends. If you do a one-line change, it is
better to retain the original naming (for the sake of clarity of the
change itself). If you rearrange it completely, you may as well
change the names while at it. And there is a spectrum in between.
> 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.
Personally, I prefer adev, but this isn't a very strong preference.
Using "device" as a name of a struct acpi_device object (or a pointer
to one of these for that matter) is slightly misleading IMV, because
those things represent AML entities rather than actual hardware.
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 <[email protected]>
>> Signed-off-by: Michal Wilczynski <[email protected]>
>> ---
>> 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(&adev->dev);
>> - __acpi_nfit_notify(&adev->dev, adev->handle, event);
>> - device_unlock(&adev->dev);
>> + struct acpi_device *device = data;
>> +
>> + device_lock(&device->dev);
>> + __acpi_nfit_notify(&device->dev, handle, event);
>> + device_unlock(&device->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 !
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 <[email protected]>
> >> Signed-off-by: Michal Wilczynski <[email protected]>
> >> ---
> >> 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(&adev->dev);
> >> - __acpi_nfit_notify(&adev->dev, adev->handle, event);
> >> - device_unlock(&adev->dev);
> >> + struct acpi_device *device = data;
> >> +
> >> + device_lock(&device->dev);
> >> + __acpi_nfit_notify(&device->dev, handle, event);
> >> + device_unlock(&device->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.
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 <[email protected]>
>>>> Signed-off-by: Michal Wilczynski <[email protected]>
>>>> ---
>>>> 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(&adev->dev);
>>>> - __acpi_nfit_notify(&adev->dev, adev->handle, event);
>>>> - device_unlock(&adev->dev);
>>>> + struct acpi_device *device = data;
>>>> +
>>>> + device_lock(&device->dev);
>>>> + __acpi_nfit_notify(&device->dev, handle, event);
>>>> + device_unlock(&device->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.