Re: [PATCH v4 17/35] platform/x86/asus-wireless: Move handler installing logic to driver

["Wilczynski, Michal" <michal.wilczynski@intel.com>]

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ł

>