| Message ID | 20241211085821.3982351-1-joe@pf.is.s.u-tokyo.ac.jp (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2] platform/x86: adv_swbutton: use devm_add_action_or_reset() for cleanup | expand |
On Wed, 11 Dec 2024, Joe Hattori wrote: > Current code leaves the device's wakeup enabled in the error path of > .probe() and .remove(). Also, the registered input device is not > unregistered. Add a devm_add_action_or_reset() call and cleanup these > resources in the callback. > > Fixes: 3d904005f686 ("platform/x86: add support for Advantech software defined button") > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > --- > Changes in V2: > - Use devm_add_action_or_reset(). > - Call input_unregister_device(). > --- > drivers/platform/x86/adv_swbutton.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/platform/x86/adv_swbutton.c b/drivers/platform/x86/adv_swbutton.c > index 6fa60f3fc53c..5b07c42adfad 100644 > --- a/drivers/platform/x86/adv_swbutton.c > +++ b/drivers/platform/x86/adv_swbutton.c > @@ -44,6 +44,14 @@ static void adv_swbutton_notify(acpi_handle handle, u32 event, void *context) > } > } > > +static void adv_swbutton_release(void *__input) > +{ > + struct input_dev *input = __input; > + > + input_unregister_device(input); Better but the input device is managed so the unregister call is unnecessary. devm_input_allocate_device() comment says: * Managed input devices do not need to be explicitly unregistered or * freed as it will be done automatically when owner device unbinds from * its driver (or binding fails). Once managed input device is allocated, * it is ready to be set up and registered in the same fashion as regular * input device. There are no special devm_input_device_[un]register() * variants, regular ones work with both managed and unmanaged devices, * should you need them. In most cases however, managed input device need * not be explicitly unregistered or freed.
Hi Ilpo, Thank you for your review. I am currently submitting a patch to introduce the devm_ version of device_init_wakeup(dev, true) [1], and I would like to send a V3 patch utilizing it after it is merged. [1]: https://lore.kernel.org/linux-pm/20241214021652.3432500-1-joe@pf.is.s.u-tokyo.ac.jp/T/#u On 12/11/24 22:57, Ilpo Järvinen wrote: > On Wed, 11 Dec 2024, Joe Hattori wrote: > >> Current code leaves the device's wakeup enabled in the error path of >> .probe() and .remove(). Also, the registered input device is not >> unregistered. Add a devm_add_action_or_reset() call and cleanup these >> resources in the callback. >> >> Fixes: 3d904005f686 ("platform/x86: add support for Advantech software defined button") >> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> >> --- >> Changes in V2: >> - Use devm_add_action_or_reset(). >> - Call input_unregister_device(). >> --- >> drivers/platform/x86/adv_swbutton.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/platform/x86/adv_swbutton.c b/drivers/platform/x86/adv_swbutton.c >> index 6fa60f3fc53c..5b07c42adfad 100644 >> --- a/drivers/platform/x86/adv_swbutton.c >> +++ b/drivers/platform/x86/adv_swbutton.c >> @@ -44,6 +44,14 @@ static void adv_swbutton_notify(acpi_handle handle, u32 event, void *context) >> } >> } >> >> +static void adv_swbutton_release(void *__input) >> +{ >> + struct input_dev *input = __input; >> + >> + input_unregister_device(input); > > Better but the input device is managed so the unregister call is > unnecessary. > > devm_input_allocate_device() comment says: > > * Managed input devices do not need to be explicitly unregistered or > * freed as it will be done automatically when owner device unbinds from > * its driver (or binding fails). Once managed input device is allocated, > * it is ready to be set up and registered in the same fashion as regular > * input device. There are no special devm_input_device_[un]register() > * variants, regular ones work with both managed and unmanaged devices, > * should you need them. In most cases however, managed input device need > * not be explicitly unregistered or freed. Thank you for pointing out, will be fixed in the V3 patch. > Best, Joe
diff --git a/drivers/platform/x86/adv_swbutton.c b/drivers/platform/x86/adv_swbutton.c index 6fa60f3fc53c..5b07c42adfad 100644 --- a/drivers/platform/x86/adv_swbutton.c +++ b/drivers/platform/x86/adv_swbutton.c @@ -44,6 +44,14 @@ static void adv_swbutton_notify(acpi_handle handle, u32 event, void *context) } } +static void adv_swbutton_release(void *__input) +{ + struct input_dev *input = __input; + + input_unregister_device(input); + device_init_wakeup(input->dev.parent, false); +} + static int adv_swbutton_probe(struct platform_device *device) { struct adv_swbutton *button; @@ -78,6 +86,9 @@ static int adv_swbutton_probe(struct platform_device *device) device_init_wakeup(&device->dev, true); + if (devm_add_action_or_reset(&device->dev, adv_swbutton_release, input)) + return -ENOMEM; + status = acpi_install_notify_handler(handle, ACPI_DEVICE_NOTIFY, adv_swbutton_notify,
Current code leaves the device's wakeup enabled in the error path of .probe() and .remove(). Also, the registered input device is not unregistered. Add a devm_add_action_or_reset() call and cleanup these resources in the callback. Fixes: 3d904005f686 ("platform/x86: add support for Advantech software defined button") Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> --- Changes in V2: - Use devm_add_action_or_reset(). - Call input_unregister_device(). --- drivers/platform/x86/adv_swbutton.c | 11 +++++++++++ 1 file changed, 11 insertions(+)