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@xxxxxxxxx>
> >> ---
> >> 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.
