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@xxxxxxxxx> >> Signed-off-by: Michal Wilczynski <michal.wilczynski@xxxxxxxxx> >> --- >> 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 !