| Message ID | 20230616165034.3630141-6-michal.wilczynski@intel.com |
|---|---|
| State | New |
| 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. > > 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 >
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 >>
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, };
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(-)