| Message ID | 20230616165034.3630141-4-michal.wilczynski@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remove .notify callback in acpi_device_ops | expand |
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;
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;
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.
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 --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);
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(-)