diff mbox series

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

Message ID 20230616165034.3630141-4-michal.wilczynski@intel.com
State Superseded
Headers show
Series Remove .notify callback in acpi_device_ops | expand

Commit Message

Michal Wilczynski June 16, 2023, 4:50 p.m. UTC
Currently logic for installing notifications from ACPI devices is
implemented using notify callback in struct acpi_driver. Preparations
are being made to replace acpi_driver with more generic struct
platform_driver, which doesn't contain notify callback. Furthermore
as of now handlers are being called indirectly through
acpi_notify_device(), which decreases performance.

Call acpi_dev_install_notify_handler() at the end of .add() callback.
Call acpi_dev_remove_notify_handler() at the beginning of .remove()
callback. Change arguments passed to the notify function to match with
what's required by acpi_install_notify_handler(). Remove .notify
callback initialization in acpi_driver.

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

Comments

Rafael J. Wysocki June 29, 2023, 3:55 p.m. UTC | #1
On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
<michal.wilczynski@intel.com> wrote:
>
> Currently logic for installing notifications from ACPI devices is
> implemented using notify callback in struct acpi_driver. Preparations
> are being made to replace acpi_driver with more generic struct
> platform_driver, which doesn't contain notify callback. Furthermore
> as of now handlers are being called indirectly through
> acpi_notify_device(), which decreases performance.
>
> Call acpi_dev_install_notify_handler() at the end of .add() callback.
> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
> callback. Change arguments passed to the notify function to match with
> what's required by acpi_install_notify_handler(). Remove .notify
> callback initialization in acpi_driver.
>
> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
> ---
>  drivers/acpi/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;
Michal Wilczynski June 30, 2023, 9:39 a.m. UTC | #2
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;
Rafael J. Wysocki June 30, 2023, 9:41 a.m. UTC | #3
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.
Rafael J. Wysocki June 30, 2023, 9:45 a.m. UTC | #4
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.
diff mbox series

Patch

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