Re: [PATCH v4 19/35] platform/x86/dell/dell-rbtn: Move handler installing logic to driver

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

On 6/2/2023 3:20 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/dell/dell-rbtn.c | 17 ++++++++++++-----
>>  1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell/dell-rbtn.c b/drivers/platform/x86/dell/dell-rbtn.c
>> index aa0e6c907494..4dcad59eb035 100644
>> --- a/drivers/platform/x86/dell/dell-rbtn.c
>> +++ b/drivers/platform/x86/dell/dell-rbtn.c
>> @@ -207,7 +207,7 @@ static void rbtn_input_event(struct rbtn_data *rbtn_data)
>>  
>>  static int rbtn_add(struct acpi_device *device);
>>  static void rbtn_remove(struct acpi_device *device);
>> -static void rbtn_notify(struct acpi_device *device, u32 event);
>> +static void rbtn_notify(acpi_handle handle, u32 event, void *data);
>>  
>>  static const struct acpi_device_id rbtn_ids[] = {
>>  	{ "DELRBTN", 0 },
>> @@ -293,7 +293,6 @@ static struct acpi_driver rbtn_driver = {
>>  	.ops = {
>>  		.add = rbtn_add,
>>  		.remove = rbtn_remove,
>> -		.notify = rbtn_notify,
>>  	},
>>  	.owner = THIS_MODULE,
>>  };
>> @@ -422,7 +421,10 @@ static int rbtn_add(struct acpi_device *device)
>>  		ret = -EINVAL;
>>  	}
>>  
>> -	return ret;
>> +	if (ret)
>> +		return ret;
>> +
>> +	return acpi_device_install_event_handler(device, ACPI_DEVICE_NOTIFY, rbtn_notify);
> What about the other things that are done in rbtn_remove(), should you 
> rollback more?

Yeah you're right, but the total lack of rollback in .add() here seems to be an issue on
it's own i.e even before this patchset .add() was leaking resources in case of failure.
I wonder whether to add missing rollback in separate commit ?

>
> I suspect there's a pre-existing lack of rbtn_acquire(device, false); 
> here to begin with.
>