| Message ID | 20230601131739.300760-18-michal.wilczynski@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Remove .notify callback in acpi_device_ops | expand |
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/asus-wireless.c | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/drivers/platform/x86/asus-wireless.c b/drivers/platform/x86/asus-wireless.c > index abf01e00b799..6544e3419ae4 100644 > --- a/drivers/platform/x86/asus-wireless.c > +++ b/drivers/platform/x86/asus-wireless.c > @@ -108,19 +108,22 @@ static void led_state_set(struct led_classdev *led, enum led_brightness value) > queue_work(data->wq, &data->led_work); > } > > -static void asus_wireless_notify(struct acpi_device *adev, u32 event) > +static void asus_wireless_notify(acpi_handle handle, u32 event, void *data) > { > - struct asus_wireless_data *data = acpi_driver_data(adev); > + struct asus_wireless_data *w_data; > + struct acpi_device *adev = data; > + > + w_data = acpi_driver_data(adev); > > dev_dbg(&adev->dev, "event=%#x\n", event); > if (event != 0x88) { > dev_notice(&adev->dev, "Unknown ASHS event: %#x\n", event); > return; > } > - input_report_key(data->idev, KEY_RFKILL, 1); > - input_sync(data->idev); > - input_report_key(data->idev, KEY_RFKILL, 0); > - input_sync(data->idev); > + input_report_key(w_data->idev, KEY_RFKILL, 1); > + input_sync(w_data->idev); > + input_report_key(w_data->idev, KEY_RFKILL, 0); > + input_sync(w_data->idev); > } > > static int asus_wireless_add(struct acpi_device *adev) > @@ -169,16 +172,20 @@ static int asus_wireless_add(struct acpi_device *adev) > data->led.max_brightness = 1; > data->led.default_trigger = "rfkill-none"; > err = devm_led_classdev_register(&adev->dev, &data->led); > - if (err) > + if (err) { > destroy_workqueue(data->wq); > + return err; > + } > > - return err; > + return acpi_device_install_event_handler(adev, ACPI_DEVICE_NOTIFY, asus_wireless_notify); So if acpi_device_install_event_handler() returns error, should you rollback something here like the error branch above? If that's the case, use goto to rollback as you'll have two places calling destroy_workqueue() already.
On 6/2/2023 3:09 PM, Ilpo Järvinen wrote: > 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/asus-wireless.c | 24 +++++++++++++++--------- >> 1 file changed, 15 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/platform/x86/asus-wireless.c b/drivers/platform/x86/asus-wireless.c >> index abf01e00b799..6544e3419ae4 100644 >> --- a/drivers/platform/x86/asus-wireless.c >> +++ b/drivers/platform/x86/asus-wireless.c >> @@ -108,19 +108,22 @@ static void led_state_set(struct led_classdev *led, enum led_brightness value) >> queue_work(data->wq, &data->led_work); >> } >> >> -static void asus_wireless_notify(struct acpi_device *adev, u32 event) >> +static void asus_wireless_notify(acpi_handle handle, u32 event, void *data) >> { >> - struct asus_wireless_data *data = acpi_driver_data(adev); >> + struct asus_wireless_data *w_data; >> + struct acpi_device *adev = data; >> + >> + w_data = acpi_driver_data(adev); >> >> dev_dbg(&adev->dev, "event=%#x\n", event); >> if (event != 0x88) { >> dev_notice(&adev->dev, "Unknown ASHS event: %#x\n", event); >> return; >> } >> - input_report_key(data->idev, KEY_RFKILL, 1); >> - input_sync(data->idev); >> - input_report_key(data->idev, KEY_RFKILL, 0); >> - input_sync(data->idev); >> + input_report_key(w_data->idev, KEY_RFKILL, 1); >> + input_sync(w_data->idev); >> + input_report_key(w_data->idev, KEY_RFKILL, 0); >> + input_sync(w_data->idev); >> } >> >> static int asus_wireless_add(struct acpi_device *adev) >> @@ -169,16 +172,20 @@ static int asus_wireless_add(struct acpi_device *adev) >> data->led.max_brightness = 1; >> data->led.default_trigger = "rfkill-none"; >> err = devm_led_classdev_register(&adev->dev, &data->led); >> - if (err) >> + if (err) { >> destroy_workqueue(data->wq); >> + return err; >> + } >> >> - return err; >> + return acpi_device_install_event_handler(adev, ACPI_DEVICE_NOTIFY, asus_wireless_notify); > So if acpi_device_install_event_handler() returns error, should you > rollback something here like the error branch above? If that's the case, > use goto to rollback as you'll have two places calling destroy_workqueue() > already. Oh yeah, this error path is leaking workqueue now, will fix as suggested, Thanks ! Michał >
diff --git a/drivers/platform/x86/asus-wireless.c b/drivers/platform/x86/asus-wireless.c index abf01e00b799..6544e3419ae4 100644 --- a/drivers/platform/x86/asus-wireless.c +++ b/drivers/platform/x86/asus-wireless.c @@ -108,19 +108,22 @@ static void led_state_set(struct led_classdev *led, enum led_brightness value) queue_work(data->wq, &data->led_work); } -static void asus_wireless_notify(struct acpi_device *adev, u32 event) +static void asus_wireless_notify(acpi_handle handle, u32 event, void *data) { - struct asus_wireless_data *data = acpi_driver_data(adev); + struct asus_wireless_data *w_data; + struct acpi_device *adev = data; + + w_data = acpi_driver_data(adev); dev_dbg(&adev->dev, "event=%#x\n", event); if (event != 0x88) { dev_notice(&adev->dev, "Unknown ASHS event: %#x\n", event); return; } - input_report_key(data->idev, KEY_RFKILL, 1); - input_sync(data->idev); - input_report_key(data->idev, KEY_RFKILL, 0); - input_sync(data->idev); + input_report_key(w_data->idev, KEY_RFKILL, 1); + input_sync(w_data->idev); + input_report_key(w_data->idev, KEY_RFKILL, 0); + input_sync(w_data->idev); } static int asus_wireless_add(struct acpi_device *adev) @@ -169,16 +172,20 @@ static int asus_wireless_add(struct acpi_device *adev) data->led.max_brightness = 1; data->led.default_trigger = "rfkill-none"; err = devm_led_classdev_register(&adev->dev, &data->led); - if (err) + if (err) { destroy_workqueue(data->wq); + return err; + } - return err; + return acpi_device_install_event_handler(adev, ACPI_DEVICE_NOTIFY, asus_wireless_notify); } static void asus_wireless_remove(struct acpi_device *adev) { struct asus_wireless_data *data = acpi_driver_data(adev); + acpi_device_remove_event_handler(adev, ACPI_DEVICE_NOTIFY, asus_wireless_notify); + if (data->wq) { devm_led_classdev_unregister(&adev->dev, &data->led); destroy_workqueue(data->wq); @@ -192,7 +199,6 @@ static struct acpi_driver asus_wireless_driver = { .ops = { .add = asus_wireless_add, .remove = asus_wireless_remove, - .notify = asus_wireless_notify, }, }; module_acpi_driver(asus_wireless_driver);
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/asus-wireless.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)