| Message ID | 20230613084310.2775896-1-michal.wilczynski@intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [v2] platform/x86/dell/dell-rbtn: Fix resources leaking on error path | expand |
Hi, On 6/13/23 10:43, Michal Wilczynski wrote: > Currently rbtn_add() in case of failure is leaking resources. Fix this > by adding a proper rollback. Move devm_kzalloc() before rbtn_acquire(), > so it doesn't require rollback in case of failure. While at it, remove > unnecessary assignment of NULL to device->driver_data and unnecessary > whitespace, plus add a break for the default case in a switch. > > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Fixes: 817a5cdb40c8 ("dell-rbtn: Dell Airplane Mode Switch driver") > Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Acked-by: Rafael J. Wysocki <rafael@kernel.org> > --- > v2: > - move devm_kzalloc before rbtn_acquire as suggested Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > drivers/platform/x86/dell/dell-rbtn.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/platform/x86/dell/dell-rbtn.c b/drivers/platform/x86/dell/dell-rbtn.c > index aa0e6c907494..c8fcb537fd65 100644 > --- a/drivers/platform/x86/dell/dell-rbtn.c > +++ b/drivers/platform/x86/dell/dell-rbtn.c > @@ -395,16 +395,16 @@ static int rbtn_add(struct acpi_device *device) > return -EINVAL; > } > > + rbtn_data = devm_kzalloc(&device->dev, sizeof(*rbtn_data), GFP_KERNEL); > + if (!rbtn_data) > + return -ENOMEM; > + > ret = rbtn_acquire(device, true); > if (ret < 0) { > dev_err(&device->dev, "Cannot enable device\n"); > return ret; > } > > - rbtn_data = devm_kzalloc(&device->dev, sizeof(*rbtn_data), GFP_KERNEL); > - if (!rbtn_data) > - return -ENOMEM; > - > rbtn_data->type = type; > device->driver_data = rbtn_data; > > @@ -420,10 +420,12 @@ static int rbtn_add(struct acpi_device *device) > break; > default: > ret = -EINVAL; > + break; > } > + if (ret) > + rbtn_acquire(device, false); > > return ret; > - > } > > static void rbtn_remove(struct acpi_device *device) > @@ -442,7 +444,6 @@ static void rbtn_remove(struct acpi_device *device) > } > > rbtn_acquire(device, false); > - device->driver_data = NULL; > } > > static void rbtn_notify(struct acpi_device *device, u32 event)
On Tue, Jun 13, 2023 at 11:43:10AM +0300, Michal Wilczynski wrote: > Currently rbtn_add() in case of failure is leaking resources. Fix this > by adding a proper rollback. Move devm_kzalloc() before rbtn_acquire(), > so it doesn't require rollback in case of failure. While at it, remove > unnecessary assignment of NULL to device->driver_data and unnecessary > whitespace, plus add a break for the default case in a switch. > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Isn't also suggested by Pali? > Fixes: 817a5cdb40c8 ("dell-rbtn: Dell Airplane Mode Switch driver") > Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Acked-by: Rafael J. Wysocki <rafael@kernel.org> ... Hans, can it (an additional tag) be folded into applied change?
On 6/13/2023 5:12 PM, Andy Shevchenko wrote: > On Tue, Jun 13, 2023 at 11:43:10AM +0300, Michal Wilczynski wrote: >> Currently rbtn_add() in case of failure is leaking resources. Fix this >> by adding a proper rollback. Move devm_kzalloc() before rbtn_acquire(), >> so it doesn't require rollback in case of failure. While at it, remove >> unnecessary assignment of NULL to device->driver_data and unnecessary >> whitespace, plus add a break for the default case in a switch. >> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Isn't also suggested by Pali? Oh yeah, should have added a tag for Pali as well > >> Fixes: 817a5cdb40c8 ("dell-rbtn: Dell Airplane Mode Switch driver") >> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Acked-by: Rafael J. Wysocki <rafael@kernel.org> > ... > > Hans, can it (an additional tag) be folded into applied change? >
Hi, On 6/13/23 17:12, Andy Shevchenko wrote: > On Tue, Jun 13, 2023 at 11:43:10AM +0300, Michal Wilczynski wrote: >> Currently rbtn_add() in case of failure is leaking resources. Fix this >> by adding a proper rollback. Move devm_kzalloc() before rbtn_acquire(), >> so it doesn't require rollback in case of failure. While at it, remove >> unnecessary assignment of NULL to device->driver_data and unnecessary >> whitespace, plus add a break for the default case in a switch. > >> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Isn't also suggested by Pali? > >> Fixes: 817a5cdb40c8 ("dell-rbtn: Dell Airplane Mode Switch driver") >> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Acked-by: Rafael J. Wysocki <rafael@kernel.org> > > ... > > Hans, can it (an additional tag) be folded into applied change? Done and pushed to review-hans. Regards, Hans
On Tuesday 13 June 2023 11:43:10 Michal Wilczynski wrote: > Currently rbtn_add() in case of failure is leaking resources. Fix this > by adding a proper rollback. Move devm_kzalloc() before rbtn_acquire(), > so it doesn't require rollback in case of failure. While at it, remove > unnecessary assignment of NULL to device->driver_data and unnecessary > whitespace, plus add a break for the default case in a switch. > > Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Fixes: 817a5cdb40c8 ("dell-rbtn: Dell Airplane Mode Switch driver") > Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Acked-by: Rafael J. Wysocki <rafael@kernel.org> Looks good, Reviewed-by: Pali Rohár <pali@kernel.org> > --- > v2: > - move devm_kzalloc before rbtn_acquire as suggested > > drivers/platform/x86/dell/dell-rbtn.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/platform/x86/dell/dell-rbtn.c b/drivers/platform/x86/dell/dell-rbtn.c > index aa0e6c907494..c8fcb537fd65 100644 > --- a/drivers/platform/x86/dell/dell-rbtn.c > +++ b/drivers/platform/x86/dell/dell-rbtn.c > @@ -395,16 +395,16 @@ static int rbtn_add(struct acpi_device *device) > return -EINVAL; > } > > + rbtn_data = devm_kzalloc(&device->dev, sizeof(*rbtn_data), GFP_KERNEL); > + if (!rbtn_data) > + return -ENOMEM; > + > ret = rbtn_acquire(device, true); > if (ret < 0) { > dev_err(&device->dev, "Cannot enable device\n"); > return ret; > } > > - rbtn_data = devm_kzalloc(&device->dev, sizeof(*rbtn_data), GFP_KERNEL); > - if (!rbtn_data) > - return -ENOMEM; > - > rbtn_data->type = type; > device->driver_data = rbtn_data; > > @@ -420,10 +420,12 @@ static int rbtn_add(struct acpi_device *device) > break; > default: > ret = -EINVAL; > + break; > } > + if (ret) > + rbtn_acquire(device, false); > > return ret; > - > } > > static void rbtn_remove(struct acpi_device *device) > @@ -442,7 +444,6 @@ static void rbtn_remove(struct acpi_device *device) > } > > rbtn_acquire(device, false); > - device->driver_data = NULL; > } > > static void rbtn_notify(struct acpi_device *device, u32 event) > -- > 2.41.0 >
diff --git a/drivers/platform/x86/dell/dell-rbtn.c b/drivers/platform/x86/dell/dell-rbtn.c index aa0e6c907494..c8fcb537fd65 100644 --- a/drivers/platform/x86/dell/dell-rbtn.c +++ b/drivers/platform/x86/dell/dell-rbtn.c @@ -395,16 +395,16 @@ static int rbtn_add(struct acpi_device *device) return -EINVAL; } + rbtn_data = devm_kzalloc(&device->dev, sizeof(*rbtn_data), GFP_KERNEL); + if (!rbtn_data) + return -ENOMEM; + ret = rbtn_acquire(device, true); if (ret < 0) { dev_err(&device->dev, "Cannot enable device\n"); return ret; } - rbtn_data = devm_kzalloc(&device->dev, sizeof(*rbtn_data), GFP_KERNEL); - if (!rbtn_data) - return -ENOMEM; - rbtn_data->type = type; device->driver_data = rbtn_data; @@ -420,10 +420,12 @@ static int rbtn_add(struct acpi_device *device) break; default: ret = -EINVAL; + break; } + if (ret) + rbtn_acquire(device, false); return ret; - } static void rbtn_remove(struct acpi_device *device) @@ -442,7 +444,6 @@ static void rbtn_remove(struct acpi_device *device) } rbtn_acquire(device, false); - device->driver_data = NULL; } static void rbtn_notify(struct acpi_device *device, u32 event)
Currently rbtn_add() in case of failure is leaking resources. Fix this by adding a proper rollback. Move devm_kzalloc() before rbtn_acquire(), so it doesn't require rollback in case of failure. While at it, remove unnecessary assignment of NULL to device->driver_data and unnecessary whitespace, plus add a break for the default case in a switch. Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Fixes: 817a5cdb40c8 ("dell-rbtn: Dell Airplane Mode Switch driver") Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Acked-by: Rafael J. Wysocki <rafael@kernel.org> --- v2: - move devm_kzalloc before rbtn_acquire as suggested drivers/platform/x86/dell/dell-rbtn.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)