[PATCH v5 00/10] Remove .notify callback in acpi_device_ops

[PATCH v5 00/10] Remove .notify callback in acpi_device_ops

[Michal Wilczynski]

*** IMPORTANT ***
This is part 1 - only drivers in acpi directory to ease up review
process. Rest of the drivers will be handled in separate patchsets.

Currently drivers support ACPI event handlers by defining .notify
callback in acpi_device_ops. This solution is suboptimal as event
handler installer installs intermediary function acpi_notify_device as a
handler in every driver. Also this approach requires extra variable
'flags' for specifying event types that the driver want to subscribe to.
Additionally this is a pre-work required to align acpi_driver with
platform_driver and eventually replace acpi_driver with platform_driver.

Remove .notify callback from the acpi_device_ops. Replace it with each
driver installing and removing it's event handlers.

v5:
 - rebased on top of Rafael changes [1], they're not merged yet
 - fixed rollback in multiple drivers so they don't leak resources on
   failure
 - made this part 1, meaning only drivers in acpi directory, rest of
   the drivers will be handled in separate patchsets to ease up review

v4:
 - added one commit for previously missed driver sony-laptop,
   refactored return statements, added NULL check for event installer
v3:
 - lkp still reported some failures for eeepc, fujitsu and
   toshiba_bluetooth, fix those
v2:
 - fix compilation errors for drivers

[1]: https://lore.kernel.org/linux-acpi/1847933.atdPhlSkOF@kreacher/

Michal Wilczynski (10):
  acpi/bus: Introduce wrappers for ACPICA event handler install/remove
  acpi/bus: Set driver_data to NULL every time .add() fails
  acpi/ac: Move handler installing logic to driver
  acpi/video: Move handler installing logic to driver
  acpi/battery: Move handler installing logic to driver
  acpi/hed: Move handler installing logic to driver
  acpi/nfit: Move acpi_nfit_notify() before acpi_nfit_add()
  acpi/nfit: Improve terminator line in acpi_nfit_ids
  acpi/nfit: Move handler installing logic to driver
  acpi/thermal: Move handler installing logic to driver

 drivers/acpi/ac.c         | 33 ++++++++++++++++++++++++---------
 drivers/acpi/acpi_video.c | 26 ++++++++++++++++++++++----
 drivers/acpi/battery.c    | 30 ++++++++++++++++++++++++------
 drivers/acpi/bus.c        | 30 +++++++++++++++++++++++++++++-
 drivers/acpi/hed.c        | 17 ++++++++++++++---
 drivers/acpi/nfit/core.c  | 32 ++++++++++++++++++++++----------
 drivers/acpi/thermal.c    | 28 ++++++++++++++++++++++------
 include/acpi/acpi_bus.h   |  6 ++++++
 8 files changed, 163 insertions(+), 39 deletions(-)

-- 
2.41.0

[PATCH v5 01/10] acpi/bus: Introduce wrappers for ACPICA event handler install/remove

[Michal Wilczynski]

Introduce new acpi_dev_install_notify_handler() and
acpi_dev_remove_notify_handler(). Those functions are replacing old
installers, and after all drivers switch to the new model, old installers
will be removed.

Make acpi_dev_install_notify_handler() and acpi_dev_remove_notify_handler()
non-static, and export symbols. This will allow the drivers to call them
directly, instead of relying on .notify callback.

Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
---
 drivers/acpi/bus.c      | 26 ++++++++++++++++++++++++++
 include/acpi/acpi_bus.h |  6 ++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index b6ab3608d782..22468589c551 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -557,6 +557,32 @@ static void acpi_device_remove_notify_handler(struct acpi_device *device,
 	acpi_os_wait_events_complete();
 }
 
+int acpi_dev_install_notify_handler(struct acpi_device *adev,
+				    u32 handler_type,
+				    acpi_notify_handler handler)
+{
+	acpi_status status;
+
+	status = acpi_install_notify_handler(adev->handle,
+					     handler_type,
+					     handler,
+					     adev);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	return 0;
+}
+EXPORT_SYMBOL(acpi_dev_install_notify_handler);
+
+void acpi_dev_remove_notify_handler(struct acpi_device *adev,
+				    u32 handler_type,
+				    acpi_notify_handler handler)
+{
+	acpi_remove_notify_handler(adev->handle, handler_type, handler);
+	acpi_os_wait_events_complete();
+}
+EXPORT_SYMBOL(acpi_dev_remove_notify_handler);
+
 /* Handle events targeting \_SB device (at present only graceful shutdown) */
 
 #define ACPI_SB_NOTIFY_SHUTDOWN_REQUEST 0x81
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index c941d99162c0..23fbe4a16972 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -515,6 +515,12 @@ void acpi_bus_private_data_handler(acpi_handle, void *);
 int acpi_bus_get_private_data(acpi_handle, void **);
 int acpi_bus_attach_private_data(acpi_handle, void *);
 void acpi_bus_detach_private_data(acpi_handle);
+int acpi_dev_install_notify_handler(struct acpi_device *adev,
+				    u32 handler_type,
+				    acpi_notify_handler handler);
+void acpi_dev_remove_notify_handler(struct acpi_device *adev,
+				    u32 handler_type,
+				    acpi_notify_handler handler);
 extern int acpi_notifier_call_chain(struct acpi_device *, u32, u32);
 extern int register_acpi_notifier(struct notifier_block *);
 extern int unregister_acpi_notifier(struct notifier_block *);
-- 
2.41.0

[PATCH v5 02/10] acpi/bus: Set driver_data to NULL every time .add() fails

[Michal Wilczynski]

Most drivers set driver_data during .add() callback, but usually
they don't set it back to NULL in case of a failure. Set driver_data to
NULL in acpi_device_probe() to avoid code duplication.

Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
---
 drivers/acpi/bus.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 22468589c551..c1cb570c8d8c 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -1017,8 +1017,10 @@ static int acpi_device_probe(struct device *dev)
 		return -ENOSYS;
 
 	ret = acpi_drv->ops.add(acpi_dev);
-	if (ret)
+	if (ret) {
+		acpi_dev->driver_data = NULL;
 		return ret;
+	}
 
 	pr_debug("Driver [%s] successfully bound to device [%s]\n",
 		 acpi_drv->name, acpi_dev->pnp.bus_id);
-- 
2.41.0

[PATCH v5 03/10] acpi/ac: Move handler installing logic to driver

[Michal Wilczynski]

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/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 = &acpi_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);
+	struct acpi_device *device = data;
+	struct acpi_ac *ac;
+
+	ac = acpi_driver_data(device);
 
 	if (!ac)
 		return;
@@ -235,7 +236,7 @@ static int acpi_ac_add(struct acpi_device *device)
 
 	result = acpi_ac_get_state(ac);
 	if (result)
-		goto end;
+		goto err_release_ac;
 
 	psy_cfg.drv_data = ac;
 
@@ -248,7 +249,7 @@ static int acpi_ac_add(struct acpi_device *device)
 					    &ac->charger_desc, &psy_cfg);
 	if (IS_ERR(ac->charger)) {
 		result = PTR_ERR(ac->charger);
-		goto end;
+		goto err_release_ac;
 	}
 
 	pr_info("%s [%s] (%s)\n", acpi_device_name(device),
@@ -256,9 +257,20 @@ static int acpi_ac_add(struct acpi_device *device)
 
 	ac->battery_nb.notifier_call = acpi_ac_battery_notify;
 	register_acpi_notifier(&ac->battery_nb);
-end:
+
+	result = acpi_dev_install_notify_handler(device,
+						 ACPI_ALL_NOTIFY,
+						 acpi_ac_notify);
 	if (result)
-		kfree(ac);
+		goto err_unregister;
+
+	return 0;
+
+err_unregister:
+	power_supply_unregister(ac->charger);
+	unregister_acpi_notifier(&ac->battery_nb);
+err_release_ac:
+	kfree(ac);
 
 	return result;
 }
@@ -297,6 +309,9 @@ static void acpi_ac_remove(struct acpi_device *device)
 
 	ac = acpi_driver_data(device);
 
+	acpi_dev_remove_notify_handler(device,
+				       ACPI_ALL_NOTIFY,
+				       acpi_ac_notify);
 	power_supply_unregister(ac->charger);
 	unregister_acpi_notifier(&ac->battery_nb);
 
-- 
2.41.0

Re: [PATCH v5 03/10] acpi/ac: Move handler installing logic to driver

[Rafael J. Wysocki]

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/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 = &acpi_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.

> +       struct acpi_device *device = data;
> +       struct acpi_ac *ac;
> +
> +       ac = acpi_driver_data(device);
>
>         if (!ac)
>                 return;

Re: [PATCH v5 03/10] acpi/ac: Move handler installing logic to driver

[Wilczynski, Michal]

On 6/29/2023 5:55 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/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 = &acpi_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;

Re: [PATCH v5 03/10] acpi/ac: Move handler installing logic to driver

[Rafael J. Wysocki]

On Fri, Jun 30, 2023 at 11:39 AM Wilczynski, Michal
<michal.wilczynski@intel.com> wrote:
>
>
>
> On 6/29/2023 5:55 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/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 = &acpi_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

I do too, but in the cases when it costs 3 extra lines of code I'd
rather have something simpler.

Re: [PATCH v5 03/10] acpi/ac: Move handler installing logic to driver

[Rafael J. Wysocki]

On Fri, Jun 30, 2023 at 11:41 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Jun 30, 2023 at 11:39 AM Wilczynski, Michal
> <michal.wilczynski@intel.com> wrote:
> >
> >
> >
> > On 6/29/2023 5:55 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/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 = &acpi_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
>
> I do too, but in the cases when it costs 3 extra lines of code I'd
> rather have something simpler.

Besides, moving code around is not strictly related to the functional
changes made by the patch and it kind of hides those changes.  It is
better to move code around in a separate patch if you really want to
do that.

[PATCH v5 04/10] acpi/video: Move handler installing logic to driver

[Michal Wilczynski]

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/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;
+
 	return 0;
 
+err_remove_and_unregister_video:
+	mutex_lock(&video_list_lock);
+	list_del(&video->entry);
+	mutex_unlock(&video_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(&video_list_lock);
 	list_del(&video->entry);
 	mutex_unlock(&video_list_lock);
-- 
2.41.0

Re: [PATCH v5 04/10] acpi/video: Move handler installing logic to driver

[Rafael J. Wysocki]

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

> +
>         return 0;
>
> +err_remove_and_unregister_video:
> +       mutex_lock(&video_list_lock);
> +       list_del(&video->entry);
> +       mutex_unlock(&video_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(&video_list_lock);
>         list_del(&video->entry);
>         mutex_unlock(&video_list_lock);
> --
> 2.41.0
>

Re: [PATCH v5 04/10] acpi/video: Move handler installing logic to driver

[Wilczynski, Michal]

On 6/29/2023 5:58 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/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(&video_list_lock);
>> +       list_del(&video->entry);
>> +       mutex_unlock(&video_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(&video_list_lock);
>>         list_del(&video->entry);
>>         mutex_unlock(&video_list_lock);
>> --
>> 2.41.0
>>

[PATCH v5 05/10] acpi/battery: Move handler installing logic to driver

[Michal Wilczynski]

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 <rafael.j.wysocki@intel.com>
Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
---
 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(&device->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;
+
+	return 0;
 
+fail_deinit_wakup_and_unregister:
+	device_init_wakeup(&device->dev, 0);
+	unregister_pm_notifier(&battery->pm_nb);
 fail:
 	sysfs_remove_battery(battery);
 	mutex_destroy(&battery->lock);
 	mutex_destroy(&battery->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(&device->dev, 0);
+
 	battery = acpi_driver_data(device);
+
+	acpi_dev_remove_notify_handler(device,
+				       ACPI_ALL_NOTIFY,
+				       acpi_battery_notify);
+
+	device_init_wakeup(&device->dev, 0);
 	unregister_pm_notifier(&battery->pm_nb);
 	sysfs_remove_battery(battery);
+
 	mutex_destroy(&battery->lock);
 	mutex_destroy(&battery->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 = &acpi_battery_pm,
 };
-- 
2.41.0

Re: [PATCH v5 05/10] acpi/battery: Move handler installing logic to driver

[Rafael J. Wysocki]

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.
>
> While at it, fix lack of whitespaces in .remove() callback.
>
> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
> ---
>  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(&device->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.

> +
> +       return 0;
>
> +fail_deinit_wakup_and_unregister:
> +       device_init_wakeup(&device->dev, 0);
> +       unregister_pm_notifier(&battery->pm_nb);
>  fail:
>         sysfs_remove_battery(battery);
>         mutex_destroy(&battery->lock);
>         mutex_destroy(&battery->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(&device->dev, 0);
> +
>         battery = acpi_driver_data(device);
> +
> +       acpi_dev_remove_notify_handler(device,
> +                                      ACPI_ALL_NOTIFY,
> +                                      acpi_battery_notify);
> +
> +       device_init_wakeup(&device->dev, 0);
>         unregister_pm_notifier(&battery->pm_nb);
>         sysfs_remove_battery(battery);
> +
>         mutex_destroy(&battery->lock);
>         mutex_destroy(&battery->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 = &acpi_battery_pm,
>  };
> --
> 2.41.0
>

Re: [PATCH v5 05/10] acpi/battery: Move handler installing logic to driver

[Wilczynski, Michal]

On 6/29/2023 6:05 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.
>>
>> While at it, fix lack of whitespaces in .remove() callback.
>>
>> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
>> ---
>>  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(&device->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(&device->dev, 0);
>> +       unregister_pm_notifier(&battery->pm_nb);
>>  fail:
>>         sysfs_remove_battery(battery);
>>         mutex_destroy(&battery->lock);
>>         mutex_destroy(&battery->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(&device->dev, 0);
>> +
>>         battery = acpi_driver_data(device);
>> +
>> +       acpi_dev_remove_notify_handler(device,
>> +                                      ACPI_ALL_NOTIFY,
>> +                                      acpi_battery_notify);
>> +
>> +       device_init_wakeup(&device->dev, 0);
>>         unregister_pm_notifier(&battery->pm_nb);
>>         sysfs_remove_battery(battery);
>> +
>>         mutex_destroy(&battery->lock);
>>         mutex_destroy(&battery->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 = &acpi_battery_pm,
>>  };
>> --
>> 2.41.0
>>

[PATCH v5 06/10] acpi/hed: Move handler installing logic to driver

[Michal Wilczynski]

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/hed.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/hed.c b/drivers/acpi/hed.c
index 78d44e3fe129..8f54560c6d1c 100644
--- a/drivers/acpi/hed.c
+++ b/drivers/acpi/hed.c
@@ -42,22 +42,34 @@ EXPORT_SYMBOL_GPL(unregister_acpi_hed_notifier);
  * it is used by HEST Generic Hardware Error Source with notify type
  * SCI.
  */
-static void acpi_hed_notify(struct acpi_device *device, u32 event)
+static void acpi_hed_notify(acpi_handle handle, u32 event, void *data)
 {
 	blocking_notifier_call_chain(&acpi_hed_notify_list, 0, NULL);
 }
 
 static int acpi_hed_add(struct acpi_device *device)
 {
+	int err;
+
 	/* Only one hardware error device */
 	if (hed_handle)
 		return -EINVAL;
 	hed_handle = device->handle;
-	return 0;
+
+	err = acpi_dev_install_notify_handler(device,
+					      ACPI_DEVICE_NOTIFY,
+					      acpi_hed_notify);
+	if (err)
+		hed_handle = NULL;
+
+	return err;
 }
 
 static void acpi_hed_remove(struct acpi_device *device)
 {
+	acpi_dev_remove_notify_handler(device,
+				       ACPI_DEVICE_NOTIFY,
+				       acpi_hed_notify);
 	hed_handle = NULL;
 }
 
@@ -68,7 +80,6 @@ static struct acpi_driver acpi_hed_driver = {
 	.ops = {
 		.add = acpi_hed_add,
 		.remove = acpi_hed_remove,
-		.notify = acpi_hed_notify,
 	},
 };
 module_acpi_driver(acpi_hed_driver);
-- 
2.41.0

[PATCH v5 07/10] acpi/nfit: Move acpi_nfit_notify() before acpi_nfit_add()

[Michal Wilczynski]

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 <michal.wilczynski@intel.com>
---
 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(&adev->dev);
+	__acpi_nfit_notify(&adev->dev, adev->handle, event);
+	device_unlock(&adev->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(&adev->dev);
-	__acpi_nfit_notify(&adev->dev, adev->handle, event);
-	device_unlock(&adev->dev);
-}
-
 static const struct acpi_device_id acpi_nfit_ids[] = {
 	{ "ACPI0012", 0 },
 	{ "", 0 },
-- 
2.41.0

Re: [PATCH v5 07/10] acpi/nfit: Move acpi_nfit_notify() before acpi_nfit_add()

[Rafael J. Wysocki]

On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
<michal.wilczynski@intel.com> 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 <michal.wilczynski@intel.com>
> ---
>  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(&adev->dev);
> +       __acpi_nfit_notify(&adev->dev, adev->handle, event);
> +       device_unlock(&adev->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(&adev->dev);
> -       __acpi_nfit_notify(&adev->dev, adev->handle, event);
> -       device_unlock(&adev->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.

Re: [PATCH v5 07/10] acpi/nfit: Move acpi_nfit_notify() before acpi_nfit_add()

[Wilczynski, Michal]

On 6/29/2023 6:06 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> <michal.wilczynski@intel.com> 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 <michal.wilczynski@intel.com>
>> ---
>>  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(&adev->dev);
>> +       __acpi_nfit_notify(&adev->dev, adev->handle, event);
>> +       device_unlock(&adev->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(&adev->dev);
>> -       __acpi_nfit_notify(&adev->dev, adev->handle, event);
>> -       device_unlock(&adev->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-7bdbdaf9ff63@linux.intel.com/

Re: [PATCH v5 07/10] acpi/nfit: Move acpi_nfit_notify() before acpi_nfit_add()

[Rafael J. Wysocki]

On Fri, Jun 30, 2023 at 11:48 AM Wilczynski, Michal
<michal.wilczynski@intel.com> wrote:
>
>
>
> On 6/29/2023 6:06 PM, Rafael J. Wysocki wrote:
> > On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> > <michal.wilczynski@intel.com> 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 <michal.wilczynski@intel.com>
> >> ---
> >>  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(&adev->dev);
> >> +       __acpi_nfit_notify(&adev->dev, adev->handle, event);
> >> +       device_unlock(&adev->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(&adev->dev);
> >> -       __acpi_nfit_notify(&adev->dev, adev->handle, event);
> >> -       device_unlock(&adev->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-7bdbdaf9ff63@linux.intel.com/

Whether or not this is easier to review is kind of subjective.

If there were more code to move, I would agree, but in this particular
case having to review two patches instead of just one is a bit of a
hassle IMV.

[PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids

[Michal Wilczynski]

Currently terminator line contains redunant characters. Remove them and
also remove a comma at the end.

Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
---
 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 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids

[Rafael J. Wysocki]

On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
<michal.wilczynski@intel.com> 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?

> 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 <michal.wilczynski@intel.com>
> ---
>  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 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids

[Wilczynski, Michal]

On 6/29/2023 6:14 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> <michal.wilczynski@intel.com> 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 <michal.wilczynski@intel.com>
>> ---
>>  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 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids

[Rafael J. Wysocki]

On Fri, Jun 30, 2023 at 11:52 AM Wilczynski, Michal
<michal.wilczynski@intel.com> wrote:
>
>
>
> On 6/29/2023 6:14 PM, Rafael J. Wysocki wrote:
> > On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> > <michal.wilczynski@intel.com> 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.

Re: [PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids

[Rafael J. Wysocki]

On Fri, Jun 30, 2023 at 1:04 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Jun 30, 2023 at 11:52 AM Wilczynski, Michal
> <michal.wilczynski@intel.com> wrote:
> >
> >
> >
> > On 6/29/2023 6:14 PM, Rafael J. Wysocki wrote:
> > > On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> > > <michal.wilczynski@intel.com> 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.

Re: [PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids

[Wilczynski, Michal]

On 6/30/2023 1:13 PM, Rafael J. Wysocki wrote:
> On Fri, Jun 30, 2023 at 1:04 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Fri, Jun 30, 2023 at 11:52 AM Wilczynski, Michal
>> <michal.wilczynski@intel.com> wrote:
>>>
>>>
>>> On 6/29/2023 6:14 PM, Rafael J. Wysocki wrote:
>>>> On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
>>>> <michal.wilczynski@intel.com> 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.56cb5bf1@kernel.org/#t

RE: [PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids

[Dan Williams]

Michal Wilczynski wrote:
> Currently terminator line contains redunant characters. Remove them and
> also remove a comma at the end.
> 
> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
> ---
>  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.

Re: [PATCH v5 08/10] acpi/nfit: Improve terminator line in acpi_nfit_ids

[Wilczynski, Michal]

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 <michal.wilczynski@intel.com>
>> ---
>>  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

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

[Michal Wilczynski]

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

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

[Rafael J. Wysocki]

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

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

[Wilczynski, Michal]

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

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

[Rafael J. Wysocki]

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.

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

[Dan Williams]

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.

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

[Wilczynski, Michal]

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 !

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

[Dan Williams]

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.

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

[Wilczynski, Michal]

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.

[PATCH v5 10/10] acpi/thermal: Move handler installing logic to driver

[Michal Wilczynski]

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 whitespaces in .remove() callback and move tz
assignment upwards.

Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
---
 drivers/acpi/thermal.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index f9f6ebb08fdb..84716e4b967c 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -825,9 +825,12 @@ static void acpi_queue_thermal_check(struct acpi_thermal *tz)
 		queue_work(acpi_thermal_pm_queue, &tz->thermal_check_work);
 }
 
-static void acpi_thermal_notify(struct acpi_device *device, u32 event)
+static void acpi_thermal_notify(acpi_handle handle, u32 event, void *data)
 {
-	struct acpi_thermal *tz = acpi_driver_data(device);
+	struct acpi_device *device = data;
+	struct acpi_thermal *tz;
+
+	tz = acpi_driver_data(device);
 
 	if (!tz)
 		return;
@@ -997,11 +1000,20 @@ static int acpi_thermal_add(struct acpi_device *device)
 
 	pr_info("%s [%s] (%ld C)\n", acpi_device_name(device),
 		acpi_device_bid(device), deci_kelvin_to_celsius(tz->temperature));
-	goto end;
 
+	result = acpi_dev_install_notify_handler(device,
+						 ACPI_DEVICE_NOTIFY,
+						 acpi_thermal_notify);
+	if (result)
+		goto flush_wq_and_unregister;
+
+	return 0;
+
+flush_wq_and_unregister:
+	flush_workqueue(acpi_thermal_pm_queue);
+	acpi_thermal_unregister_thermal_zone(tz);
 free_memory:
 	kfree(tz);
-end:
 	return result;
 }
 
@@ -1012,10 +1024,15 @@ static void acpi_thermal_remove(struct acpi_device *device)
 	if (!device || !acpi_driver_data(device))
 		return;
 
-	flush_workqueue(acpi_thermal_pm_queue);
 	tz = acpi_driver_data(device);
 
+	acpi_dev_remove_notify_handler(device,
+				       ACPI_DEVICE_NOTIFY,
+				       acpi_thermal_notify);
+
+	flush_workqueue(acpi_thermal_pm_queue);
 	acpi_thermal_unregister_thermal_zone(tz);
+
 	kfree(tz);
 }
 
@@ -1078,7 +1095,6 @@ static struct acpi_driver acpi_thermal_driver = {
 	.ops = {
 		.add = acpi_thermal_add,
 		.remove = acpi_thermal_remove,
-		.notify = acpi_thermal_notify,
 		},
 	.drv.pm = &acpi_thermal_pm,
 };
-- 
2.41.0