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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2764 bytes --]

On Fri, 2 Jun 2023, Wilczynski, Michal wrote:
> 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 ?

Yes, make separate patch out of it and mark it with Fixes tag. You can 
send it separately.

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

-- 
 i.