diff mbox series

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

Message ID 20230601131739.300760-20-michal.wilczynski@intel.com
State New
Headers show
Series [v4,01/35] acpi: Adjust functions installing bus event handlers | expand

Commit Message

Michal Wilczynski June 1, 2023, 1:17 p.m. UTC
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(-)

Comments

Ilpo Järvinen June 2, 2023, 1:20 p.m. UTC | #1
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?

I suspect there's a pre-existing lack of rbtn_acquire(device, false); 
here to begin with.
Michal Wilczynski June 2, 2023, 1:41 p.m. UTC | #2
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.
>
Ilpo Järvinen June 2, 2023, 2:01 p.m. UTC | #3
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.
> >
>
diff mbox series

Patch

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);
 
 }
 
@@ -430,6 +432,8 @@  static void rbtn_remove(struct acpi_device *device)
 {
 	struct rbtn_data *rbtn_data = device->driver_data;
 
+	acpi_device_remove_event_handler(device, ACPI_DEVICE_NOTIFY, rbtn_notify);
+
 	switch (rbtn_data->type) {
 	case RBTN_TOGGLE:
 		rbtn_input_exit(rbtn_data);
@@ -445,9 +449,12 @@  static void rbtn_remove(struct acpi_device *device)
 	device->driver_data = NULL;
 }
 
-static void rbtn_notify(struct acpi_device *device, u32 event)
+static void rbtn_notify(acpi_handle handle, u32 event, void *data)
 {
-	struct rbtn_data *rbtn_data = device->driver_data;
+	struct acpi_device *device = data;
+	struct rbtn_data *rbtn_data;
+
+	rbtn_data = device->driver_data;
 
 	/*
 	 * Some BIOSes send a notification at resume.