| Message ID | 20230601131739.300760-3-michal.wilczynski@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,01/35] acpi: Adjust functions installing bus event handlers | expand |
On Thu, Jun 01, 2023 at 03:17:21PM +0200, 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_device_install_event_handler() at the end of .add() callback. > Call acpi_device_remove_event_handler() at the beginning of .remove() > callback. Change arguments passed to the notify callback to match with > what's required by acpi_device_install_event_handler(). > > Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> > --- > drivers/platform/x86/classmate-laptop.c | 53 +++++++++++++++++++------ > 1 file changed, 41 insertions(+), 12 deletions(-) > > diff --git a/drivers/platform/x86/classmate-laptop.c b/drivers/platform/x86/classmate-laptop.c > index 2edaea2492df..2d36abf5ecfe 100644 > --- a/drivers/platform/x86/classmate-laptop.c > +++ b/drivers/platform/x86/classmate-laptop.c > @@ -180,8 +180,9 @@ static acpi_status cmpc_get_accel_v4(acpi_handle handle, > return status; > } > > -static void cmpc_accel_handler_v4(struct acpi_device *dev, u32 event) > +static void cmpc_accel_handler_v4(acpi_handle handle, u32 event, void *data) > { > + struct acpi_device *dev = data; > if (event == 0x81) { > int16_t x, y, z; > acpi_status status; > @@ -407,6 +408,11 @@ static int cmpc_accel_add_v4(struct acpi_device *acpi) > inputdev = dev_get_drvdata(&acpi->dev); > dev_set_drvdata(&inputdev->dev, accel); > > + error = acpi_device_install_event_handler(acpi, ACPI_DEVICE_NOTIFY, > + cmpc_accel_handler_v4); > + if (error) > + goto failed_input; > + In all cases, acpi_device_install_event_handler is being called after cmpc_add_acpi_notify_device, which allocates and registers an input device. You should cleanup in case acpi_device_install_event_handler fails and call cmpc_remove_acpi_notify_device. Cascardo. > return 0; > > failed_input: > @@ -420,6 +426,7 @@ static int cmpc_accel_add_v4(struct acpi_device *acpi) > > static void cmpc_accel_remove_v4(struct acpi_device *acpi) > { > + acpi_device_remove_event_handler(acpi, ACPI_DEVICE_NOTIFY, cmpc_accel_handler_v4); > device_remove_file(&acpi->dev, &cmpc_accel_sensitivity_attr_v4); > device_remove_file(&acpi->dev, &cmpc_accel_g_select_attr_v4); > cmpc_remove_acpi_notify_device(acpi); > @@ -441,7 +448,6 @@ static struct acpi_driver cmpc_accel_acpi_driver_v4 = { > .ops = { > .add = cmpc_accel_add_v4, > .remove = cmpc_accel_remove_v4, > - .notify = cmpc_accel_handler_v4, > }, > .drv.pm = &cmpc_accel_pm, > }; > @@ -523,8 +529,10 @@ static acpi_status cmpc_get_accel(acpi_handle handle, > return status; > } > > -static void cmpc_accel_handler(struct acpi_device *dev, u32 event) > +static void cmpc_accel_handler(acpi_handle handle, u32 event, void *data) > { > + struct acpi_device *dev = data; > + > if (event == 0x81) { > unsigned char x, y, z; > acpi_status status; > @@ -639,6 +647,11 @@ static int cmpc_accel_add(struct acpi_device *acpi) > inputdev = dev_get_drvdata(&acpi->dev); > dev_set_drvdata(&inputdev->dev, accel); > > + error = acpi_device_install_event_handler(acpi, ACPI_DEVICE_NOTIFY, > + cmpc_accel_handler); > + if (error) > + goto failed_input; > + > return 0; > > failed_input: > @@ -650,6 +663,7 @@ static int cmpc_accel_add(struct acpi_device *acpi) > > static void cmpc_accel_remove(struct acpi_device *acpi) > { > + acpi_device_remove_event_handler(acpi, ACPI_DEVICE_NOTIFY, cmpc_accel_handler); > device_remove_file(&acpi->dev, &cmpc_accel_sensitivity_attr); > cmpc_remove_acpi_notify_device(acpi); > } > @@ -667,7 +681,6 @@ static struct acpi_driver cmpc_accel_acpi_driver = { > .ops = { > .add = cmpc_accel_add, > .remove = cmpc_accel_remove, > - .notify = cmpc_accel_handler, > } > }; > > @@ -693,8 +706,9 @@ static acpi_status cmpc_get_tablet(acpi_handle handle, > return status; > } > > -static void cmpc_tablet_handler(struct acpi_device *dev, u32 event) > +static void cmpc_tablet_handler(acpi_handle handle, u32 event, void *data) > { > + struct acpi_device *dev = data; > unsigned long long val = 0; > struct input_dev *inputdev = dev_get_drvdata(&dev->dev); > > @@ -723,12 +737,20 @@ static void cmpc_tablet_idev_init(struct input_dev *inputdev) > > static int cmpc_tablet_add(struct acpi_device *acpi) > { > - return cmpc_add_acpi_notify_device(acpi, "cmpc_tablet", > - cmpc_tablet_idev_init); > + int ret; > + > + ret = cmpc_add_acpi_notify_device(acpi, "cmpc_tablet", > + cmpc_tablet_idev_init); > + if (ret) > + return ret; > + > + return acpi_device_install_event_handler(acpi, ACPI_DEVICE_NOTIFY, > + cmpc_tablet_handler); > } > > static void cmpc_tablet_remove(struct acpi_device *acpi) > { > + acpi_device_remove_event_handler(acpi, ACPI_DEVICE_NOTIFY, cmpc_tablet_handler); > cmpc_remove_acpi_notify_device(acpi); > } > > @@ -761,7 +783,6 @@ static struct acpi_driver cmpc_tablet_acpi_driver = { > .ops = { > .add = cmpc_tablet_add, > .remove = cmpc_tablet_remove, > - .notify = cmpc_tablet_handler, > }, > .drv.pm = &cmpc_tablet_pm, > }; > @@ -1026,8 +1047,9 @@ static int cmpc_keys_codes[] = { > KEY_MAX > }; > > -static void cmpc_keys_handler(struct acpi_device *dev, u32 event) > +static void cmpc_keys_handler(acpi_handle handle, u32 event, void *data) > { > + struct acpi_device *dev = data; > struct input_dev *inputdev; > int code = KEY_MAX; > > @@ -1049,12 +1071,20 @@ static void cmpc_keys_idev_init(struct input_dev *inputdev) > > static int cmpc_keys_add(struct acpi_device *acpi) > { > - return cmpc_add_acpi_notify_device(acpi, "cmpc_keys", > - cmpc_keys_idev_init); > + int error; > + > + error = cmpc_add_acpi_notify_device(acpi, "cmpc_keys", > + cmpc_keys_idev_init); > + if (error) > + return error; > + > + return acpi_device_install_event_handler(acpi, ACPI_DEVICE_NOTIFY, > + cmpc_keys_handler); > } > > static void cmpc_keys_remove(struct acpi_device *acpi) > { > + acpi_device_remove_event_handler(acpi, ACPI_DEVICE_NOTIFY, cmpc_keys_handler); > cmpc_remove_acpi_notify_device(acpi); > } > > @@ -1071,7 +1101,6 @@ static struct acpi_driver cmpc_keys_acpi_driver = { > .ops = { > .add = cmpc_keys_add, > .remove = cmpc_keys_remove, > - .notify = cmpc_keys_handler, > } > }; > > -- > 2.40.1 >
On Thu, 1 Jun 2023, 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_device_install_event_handler() at the end of .add() callback. > Call acpi_device_remove_event_handler() at the beginning of .remove() > callback. Change arguments passed to the notify callback to match with > what's required by acpi_device_install_event_handler(). > > Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> > --- > drivers/platform/x86/eeepc-laptop.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c > index 62b71e8e3567..bd6ada963d88 100644 > --- a/drivers/platform/x86/eeepc-laptop.c > +++ b/drivers/platform/x86/eeepc-laptop.c > @@ -1204,12 +1204,15 @@ static void eeepc_input_notify(struct eeepc_laptop *eeepc, int event) > pr_info("Unknown key %x pressed\n", event); > } > > -static void eeepc_acpi_notify(struct acpi_device *device, u32 event) > +static void eeepc_acpi_notify(acpi_handle handle, u32 event, void *data) > { > - struct eeepc_laptop *eeepc = acpi_driver_data(device); > int old_brightness, new_brightness; > + struct acpi_device *device = data; > + struct eeepc_laptop *eeepc; > u16 count; > > + eeepc = acpi_driver_data(device); > + > if (event > ACPI_MAX_SYS_NOTIFY) > return; > count = eeepc->event_count[event % 128]++; > @@ -1423,6 +1426,11 @@ static int eeepc_acpi_add(struct acpi_device *device) > goto fail_rfkill; > > eeepc_device_present = true; > + result = acpi_device_install_event_handler(device, ACPI_ALL_NOTIFY, > + eeepc_acpi_notify); > + if (result) > + goto fail_rfkill; This too is incorrectly rolled back. You shouldn't just copy the previous goto label like this but think what _more_ should be rolled back if that preceeding call succeeded. Usually, the remove function gives you good hint on what additional things should be rolled back. In here it looks like eeepc_rfkill_exit() would be needed too. > + > return 0; > > fail_rfkill: > @@ -1444,6 +1452,8 @@ static void eeepc_acpi_remove(struct acpi_device *device) > { > struct eeepc_laptop *eeepc = acpi_driver_data(device); > > + acpi_device_remove_event_handler(device, ACPI_ALL_NOTIFY, > + eeepc_acpi_notify); > eeepc_backlight_exit(eeepc); > eeepc_rfkill_exit(eeepc); > eeepc_input_exit(eeepc);
On 6/2/2023 12:29 PM, Thadeu Lima de Souza Cascardo wrote: > On Thu, Jun 01, 2023 at 03:17:21PM +0200, 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_device_install_event_handler() at the end of .add() callback. >> Call acpi_device_remove_event_handler() at the beginning of .remove() >> callback. Change arguments passed to the notify callback to match with >> what's required by acpi_device_install_event_handler(). >> >> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> >> --- >> drivers/platform/x86/classmate-laptop.c | 53 +++++++++++++++++++------ >> 1 file changed, 41 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/platform/x86/classmate-laptop.c b/drivers/platform/x86/classmate-laptop.c >> index 2edaea2492df..2d36abf5ecfe 100644 >> --- a/drivers/platform/x86/classmate-laptop.c >> +++ b/drivers/platform/x86/classmate-laptop.c >> @@ -180,8 +180,9 @@ static acpi_status cmpc_get_accel_v4(acpi_handle handle, >> return status; >> } >> >> -static void cmpc_accel_handler_v4(struct acpi_device *dev, u32 event) >> +static void cmpc_accel_handler_v4(acpi_handle handle, u32 event, void *data) >> { >> + struct acpi_device *dev = data; >> if (event == 0x81) { >> int16_t x, y, z; >> acpi_status status; >> @@ -407,6 +408,11 @@ static int cmpc_accel_add_v4(struct acpi_device *acpi) >> inputdev = dev_get_drvdata(&acpi->dev); >> dev_set_drvdata(&inputdev->dev, accel); >> >> + error = acpi_device_install_event_handler(acpi, ACPI_DEVICE_NOTIFY, >> + cmpc_accel_handler_v4); >> + if (error) >> + goto failed_input; >> + > In all cases, acpi_device_install_event_handler is being called after > cmpc_add_acpi_notify_device, which allocates and registers an input > device. > > You should cleanup in case acpi_device_install_event_handler fails and > call cmpc_remove_acpi_notify_device. > > Cascardo. Hi Cascardo Yeah I see this now, I'm not freeing the resource in the error path properly, will add another label for example 'failed_notify_install' that would call cmpc_remove_acpi_notify_device, Thanks ! Michał > >> return 0; >> >> failed_input: >> @@ -420,6 +426,7 @@ static int cmpc_accel_add_v4(struct acpi_device *acpi) >> >> static void cmpc_accel_remove_v4(struct acpi_device *acpi) >> { >> + acpi_device_remove_event_handler(acpi, ACPI_DEVICE_NOTIFY, cmpc_accel_handler_v4); >> device_remove_file(&acpi->dev, &cmpc_accel_sensitivity_attr_v4); >> device_remove_file(&acpi->dev, &cmpc_accel_g_select_attr_v4); >> cmpc_remove_acpi_notify_device(acpi); >> @@ -441,7 +448,6 @@ static struct acpi_driver cmpc_accel_acpi_driver_v4 = { >> .ops = { >> .add = cmpc_accel_add_v4, >> .remove = cmpc_accel_remove_v4, >> - .notify = cmpc_accel_handler_v4, >> }, >> .drv.pm = &cmpc_accel_pm, >> }; >> @@ -523,8 +529,10 @@ static acpi_status cmpc_get_accel(acpi_handle handle, >> return status; >> } >> >> -static void cmpc_accel_handler(struct acpi_device *dev, u32 event) >> +static void cmpc_accel_handler(acpi_handle handle, u32 event, void *data) >> { >> + struct acpi_device *dev = data; >> + >> if (event == 0x81) { >> unsigned char x, y, z; >> acpi_status status; >> @@ -639,6 +647,11 @@ static int cmpc_accel_add(struct acpi_device *acpi) >> inputdev = dev_get_drvdata(&acpi->dev); >> dev_set_drvdata(&inputdev->dev, accel); >> >> + error = acpi_device_install_event_handler(acpi, ACPI_DEVICE_NOTIFY, >> + cmpc_accel_handler); >> + if (error) >> + goto failed_input; >> + >> return 0; >> >> failed_input: >> @@ -650,6 +663,7 @@ static int cmpc_accel_add(struct acpi_device *acpi) >> >> static void cmpc_accel_remove(struct acpi_device *acpi) >> { >> + acpi_device_remove_event_handler(acpi, ACPI_DEVICE_NOTIFY, cmpc_accel_handler); >> device_remove_file(&acpi->dev, &cmpc_accel_sensitivity_attr); >> cmpc_remove_acpi_notify_device(acpi); >> } >> @@ -667,7 +681,6 @@ static struct acpi_driver cmpc_accel_acpi_driver = { >> .ops = { >> .add = cmpc_accel_add, >> .remove = cmpc_accel_remove, >> - .notify = cmpc_accel_handler, >> } >> }; >> >> @@ -693,8 +706,9 @@ static acpi_status cmpc_get_tablet(acpi_handle handle, >> return status; >> } >> >> -static void cmpc_tablet_handler(struct acpi_device *dev, u32 event) >> +static void cmpc_tablet_handler(acpi_handle handle, u32 event, void *data) >> { >> + struct acpi_device *dev = data; >> unsigned long long val = 0; >> struct input_dev *inputdev = dev_get_drvdata(&dev->dev); >> >> @@ -723,12 +737,20 @@ static void cmpc_tablet_idev_init(struct input_dev *inputdev) >> >> static int cmpc_tablet_add(struct acpi_device *acpi) >> { >> - return cmpc_add_acpi_notify_device(acpi, "cmpc_tablet", >> - cmpc_tablet_idev_init); >> + int ret; >> + >> + ret = cmpc_add_acpi_notify_device(acpi, "cmpc_tablet", >> + cmpc_tablet_idev_init); >> + if (ret) >> + return ret; >> + >> + return acpi_device_install_event_handler(acpi, ACPI_DEVICE_NOTIFY, >> + cmpc_tablet_handler); >> } >> >> static void cmpc_tablet_remove(struct acpi_device *acpi) >> { >> + acpi_device_remove_event_handler(acpi, ACPI_DEVICE_NOTIFY, cmpc_tablet_handler); >> cmpc_remove_acpi_notify_device(acpi); >> } >> >> @@ -761,7 +783,6 @@ static struct acpi_driver cmpc_tablet_acpi_driver = { >> .ops = { >> .add = cmpc_tablet_add, >> .remove = cmpc_tablet_remove, >> - .notify = cmpc_tablet_handler, >> }, >> .drv.pm = &cmpc_tablet_pm, >> }; >> @@ -1026,8 +1047,9 @@ static int cmpc_keys_codes[] = { >> KEY_MAX >> }; >> >> -static void cmpc_keys_handler(struct acpi_device *dev, u32 event) >> +static void cmpc_keys_handler(acpi_handle handle, u32 event, void *data) >> { >> + struct acpi_device *dev = data; >> struct input_dev *inputdev; >> int code = KEY_MAX; >> >> @@ -1049,12 +1071,20 @@ static void cmpc_keys_idev_init(struct input_dev *inputdev) >> >> static int cmpc_keys_add(struct acpi_device *acpi) >> { >> - return cmpc_add_acpi_notify_device(acpi, "cmpc_keys", >> - cmpc_keys_idev_init); >> + int error; >> + >> + error = cmpc_add_acpi_notify_device(acpi, "cmpc_keys", >> + cmpc_keys_idev_init); >> + if (error) >> + return error; >> + >> + return acpi_device_install_event_handler(acpi, ACPI_DEVICE_NOTIFY, >> + cmpc_keys_handler); >> } >> >> static void cmpc_keys_remove(struct acpi_device *acpi) >> { >> + acpi_device_remove_event_handler(acpi, ACPI_DEVICE_NOTIFY, cmpc_keys_handler); >> cmpc_remove_acpi_notify_device(acpi); >> } >> >> @@ -1071,7 +1101,6 @@ static struct acpi_driver cmpc_keys_acpi_driver = { >> .ops = { >> .add = cmpc_keys_add, >> .remove = cmpc_keys_remove, >> - .notify = cmpc_keys_handler, >> } >> }; >> >> -- >> 2.40.1 >>
diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c index 1ace70b831cd..208af5a3106e 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; @@ -256,6 +257,8 @@ static int acpi_ac_add(struct acpi_device *device) ac->battery_nb.notifier_call = acpi_ac_battery_notify; register_acpi_notifier(&ac->battery_nb); + + result = acpi_device_install_event_handler(device, ACPI_ALL_NOTIFY, acpi_ac_notify); end: if (result) kfree(ac); @@ -297,6 +300,7 @@ static void acpi_ac_remove(struct acpi_device *device) ac = acpi_driver_data(device); + acpi_device_remove_event_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_device_install_event_handler() at the end of .add callback. Call acpi_device_remove_event_handler() at the beginning of .remove callback. Change arguments passed to the notify callback to match with what's required by acpi_install_event_handler(). Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> --- drivers/acpi/ac.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)