| Message ID | 20241030162754.2110946-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] ACPI: battery: Check for error code from devm_mutex_init() call | expand |
Hi Andy, Oct 30, 2024 10:28:03 Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > Even if it's not critical, the avoidance of checking the error code > from devm_mutex_init() call today diminishes the point of using devm > variant of it. Tomorrow it may even leak something. Add the missed > check. Thanks! Assuming you found this via some sort of tool and you already fixed up all the other places in the tree missing these checks, wouldn't it make sense to mark devm_mutex_init() as __must_check? > Fixes: 0710c1ce5045 ("ACPI: battery: initialize mutexes through devm_ APIs") > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net> > --- > drivers/acpi/battery.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c > index 66662712e288..70f706d7634f 100644 > --- a/drivers/acpi/battery.c > +++ b/drivers/acpi/battery.c > @@ -1226,8 +1226,12 @@ static int acpi_battery_add(struct acpi_device *device) > strscpy(acpi_device_name(device), ACPI_BATTERY_DEVICE_NAME); > strscpy(acpi_device_class(device), ACPI_BATTERY_CLASS); > device->driver_data = battery; > - devm_mutex_init(&device->dev, &battery->lock); > - devm_mutex_init(&device->dev, &battery->sysfs_lock); > + result = devm_mutex_init(&device->dev, &battery->lock); > + if (result) > + return result; > + result = devm_mutex_init(&device->dev, &battery->sysfs_lock); > + if (result) > + return result; > if (acpi_has_method(battery->device->handle, "_BIX")) > set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags); > > -- > 2.43.0.rc1.1336.g36b5255a03ac
On Wed, Oct 30, 2024 at 10:42:18AM -0600, Thomas Weißschuh wrote: > Oct 30, 2024 10:28:03 Andy Shevchenko <andriy.shevchenko@linux.intel.com>: ... > Assuming you found this via some sort of tool and you already fixed up all > the other places in the tree missing these checks, The tool is called `git grep`. > wouldn't it make sense to mark devm_mutex_init() as __must_check? It's macro, any idea how to do that for the macros? ... > Reviewed-by: Thomas Weißschuh <linux@weissschuh.net> Thanks!
Oct 30, 2024 11:31:21 Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > On Wed, Oct 30, 2024 at 10:42:18AM -0600, Thomas Weißschuh wrote: >> Oct 30, 2024 10:28:03 Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > > ... >> wouldn't it make sense to mark devm_mutex_init() as __must_check? > > It's macro, any idea how to do that for the macros? It should work on __devm_mutex_init(). I don't think the expression macro in between should interfere. Unfortunately I can't test it myself right now.
On Wed, Oct 30, 2024 at 12:29:31PM -0600, Thomas Weißschuh wrote: > Oct 30, 2024 11:31:21 Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > > On Wed, Oct 30, 2024 at 10:42:18AM -0600, Thomas Weißschuh wrote: > >> Oct 30, 2024 10:28:03 Andy Shevchenko <andriy.shevchenko@linux.intel.com>: ... > >> wouldn't it make sense to mark devm_mutex_init() as __must_check? > > > > It's macro, any idea how to do that for the macros? > > It should work on __devm_mutex_init(). > I don't think the expression macro in between should interfere. > Unfortunately I can't test it myself right now. Okay, when you have a patch, feel free to Cc me for review.
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c index 66662712e288..70f706d7634f 100644 --- a/drivers/acpi/battery.c +++ b/drivers/acpi/battery.c @@ -1226,8 +1226,12 @@ static int acpi_battery_add(struct acpi_device *device) strscpy(acpi_device_name(device), ACPI_BATTERY_DEVICE_NAME); strscpy(acpi_device_class(device), ACPI_BATTERY_CLASS); device->driver_data = battery; - devm_mutex_init(&device->dev, &battery->lock); - devm_mutex_init(&device->dev, &battery->sysfs_lock); + result = devm_mutex_init(&device->dev, &battery->lock); + if (result) + return result; + result = devm_mutex_init(&device->dev, &battery->sysfs_lock); + if (result) + return result; if (acpi_has_method(battery->device->handle, "_BIX")) set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags);
Even if it's not critical, the avoidance of checking the error code from devm_mutex_init() call today diminishes the point of using devm variant of it. Tomorrow it may even leak something. Add the missed check. Fixes: 0710c1ce5045 ("ACPI: battery: initialize mutexes through devm_ APIs") Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/acpi/battery.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)