diff mbox series

[v5,09/10] acpi/nfit: Move handler installing logic to driver

Message ID 20230616165034.3630141-10-michal.wilczynski@intel.com (mailing list archive)
State Superseded
Headers show
Series Remove .notify callback in acpi_device_ops | expand

Commit Message

Wilczynski, Michal June 16, 2023, 4:50 p.m. UTC
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 <rafael.j.wysocki@intel.com>
Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
---
 drivers/acpi/nfit/core.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

Comments

Rafael J. Wysocki June 29, 2023, 4:18 p.m. UTC | #1
On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
<michal.wilczynski@intel.com> 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 <rafael.j.wysocki@intel.com>
> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
> ---
>  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,
>         },
>  };
>
> --
Dan Williams June 29, 2023, 8:54 p.m. UTC | #2
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 <rafael.j.wysocki@intel.com>
> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
> ---
>  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.
Wilczynski, Michal June 30, 2023, 9:55 a.m. UTC | #3
On 6/29/2023 6:18 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> <michal.wilczynski@intel.com> 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 <rafael.j.wysocki@intel.com>
>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
>> ---
>>  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,
>>         },
>>  };
>>
>> --
Rafael J. Wysocki June 30, 2023, 11 a.m. UTC | #4
On Fri, Jun 30, 2023 at 11:55 AM Wilczynski, Michal
<michal.wilczynski@intel.com> wrote:
>
>
>
> On 6/29/2023 6:18 PM, Rafael J. Wysocki wrote:
> > On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> > <michal.wilczynski@intel.com> 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 <rafael.j.wysocki@intel.com>
> >> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
> >> ---
> >>  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.
Wilczynski, Michal June 30, 2023, 4:56 p.m. UTC | #5
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 <rafael.j.wysocki@intel.com>
>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
>> ---
>>  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 !
Dan Williams June 30, 2023, 5:19 p.m. UTC | #6
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 <rafael.j.wysocki@intel.com>
> >> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
> >> ---
> >>  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.
Wilczynski, Michal June 30, 2023, 5:26 p.m. UTC | #7
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 <rafael.j.wysocki@intel.com>
>>>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
>>>> ---
>>>>  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.
diff mbox series

Patch

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,
 	},
 };