| Message ID | 20241206094848.1650137-1-joe@pf.is.s.u-tokyo.ac.jp (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: adv_swbutton: disable wakeup in .remove() and the error path of .probe() | expand |
On Fri, 6 Dec 2024, Joe Hattori wrote: > Current code leaves the device's wakeup enabled in the error path of > .probe() and .remove(), which results in a memory leak. Therefore, add > the device_init_wakeup(&device->dev, false) calls. > > Fixes: 3d904005f686 ("platform/x86: add support for Advantech software defined button") > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > --- > drivers/platform/x86/adv_swbutton.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/platform/x86/adv_swbutton.c b/drivers/platform/x86/adv_swbutton.c > index 6fa60f3fc53c..523836be6d4b 100644 > --- a/drivers/platform/x86/adv_swbutton.c > +++ b/drivers/platform/x86/adv_swbutton.c > @@ -84,6 +84,7 @@ static int adv_swbutton_probe(struct platform_device *device) > device); > if (ACPI_FAILURE(status)) { > dev_err(&device->dev, "Error installing notify handler\n"); > + device_init_wakeup(&device->dev, false); > return -EIO; > } > > @@ -96,6 +97,7 @@ static void adv_swbutton_remove(struct platform_device *device) > > acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, > adv_swbutton_notify); > + device_init_wakeup(&device->dev, false); Is the non-symmetric order here intentional? How about using devm_add_action_or_reset() instead?
Hi Ilpo, Thank you for your review. On 12/10/24 22:52, Ilpo Järvinen wrote: > On Fri, 6 Dec 2024, Joe Hattori wrote: > >> Current code leaves the device's wakeup enabled in the error path of >> .probe() and .remove(), which results in a memory leak. Therefore, add >> the device_init_wakeup(&device->dev, false) calls. >> >> Fixes: 3d904005f686 ("platform/x86: add support for Advantech software defined button") >> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> >> --- >> drivers/platform/x86/adv_swbutton.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/platform/x86/adv_swbutton.c b/drivers/platform/x86/adv_swbutton.c >> index 6fa60f3fc53c..523836be6d4b 100644 >> --- a/drivers/platform/x86/adv_swbutton.c >> +++ b/drivers/platform/x86/adv_swbutton.c >> @@ -84,6 +84,7 @@ static int adv_swbutton_probe(struct platform_device *device) >> device); >> if (ACPI_FAILURE(status)) { >> dev_err(&device->dev, "Error installing notify handler\n"); >> + device_init_wakeup(&device->dev, false); >> return -EIO; >> } >> >> @@ -96,6 +97,7 @@ static void adv_swbutton_remove(struct platform_device *device) >> >> acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, >> adv_swbutton_notify); >> + device_init_wakeup(&device->dev, false); > > Is the non-symmetric order here intentional? > > How about using devm_add_action_or_reset() instead? > Agreed, just sent a v2 patch using devm_add_action_or_reset(), so please take a look at it. Best, Joe
diff --git a/drivers/platform/x86/adv_swbutton.c b/drivers/platform/x86/adv_swbutton.c index 6fa60f3fc53c..523836be6d4b 100644 --- a/drivers/platform/x86/adv_swbutton.c +++ b/drivers/platform/x86/adv_swbutton.c @@ -84,6 +84,7 @@ static int adv_swbutton_probe(struct platform_device *device) device); if (ACPI_FAILURE(status)) { dev_err(&device->dev, "Error installing notify handler\n"); + device_init_wakeup(&device->dev, false); return -EIO; } @@ -96,6 +97,7 @@ static void adv_swbutton_remove(struct platform_device *device) acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, adv_swbutton_notify); + device_init_wakeup(&device->dev, false); } static const struct acpi_device_id button_device_ids[] = {
Current code leaves the device's wakeup enabled in the error path of .probe() and .remove(), which results in a memory leak. Therefore, add the device_init_wakeup(&device->dev, false) calls. Fixes: 3d904005f686 ("platform/x86: add support for Advantech software defined button") Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> --- drivers/platform/x86/adv_swbutton.c | 2 ++ 1 file changed, 2 insertions(+)