| Message ID | 20230601131739.300760-20-michal.wilczynski@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Remove .notify callback in acpi_device_ops | expand |
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.
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. >
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 --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.
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(-)