[PATCH v1 1/1] ACPI: battery: Check for error code from devm_mutex_init() call

[PATCH v1 1/1] ACPI: battery: Check for error code from devm_mutex_init() call

[Andy Shevchenko]

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(-)

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

Re: [PATCH v1 1/1] ACPI: battery: Check for error code from devm_mutex_init() call

[Thomas Weißschuh]

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

Re: [PATCH v1 1/1] ACPI: battery: Check for error code from devm_mutex_init() call

[Andy Shevchenko]

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!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] ACPI: battery: Check for error code from devm_mutex_init() call

[Thomas Weißschuh]

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.

Re: [PATCH v1 1/1] ACPI: battery: Check for error code from devm_mutex_init() call

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] ACPI: battery: Check for error code from devm_mutex_init() call

[Rafael J. Wysocki]

On Wed, Oct 30, 2024 at 5:42 PM Thomas Weißschuh <thomas@weissschuh.net> wrote:
>
> 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>

Applied, thanks!

> > ---
> > 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
>