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@xxxxxxxxx>
>> ---
>> 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ł
>
