linux-kernel - Re: [PATCH v2] pwm: meson: Remove error print for devm_add_action_or

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <sveurgnigarzdjreweoibcxkkl7rekcpufuwqr7bxcrdx7zdrd@kz4ohstmfyjh>
Date: Tue, 5 Aug 2025 15:23:12 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Waqar Hameed <waqar.hameed@...s.com>
Cc: Neil Armstrong <neil.armstrong@...aro.org>, 
	Kevin Hilman <khilman@...libre.com>, Jerome Brunet <jbrunet@...libre.com>, 
	Martin Blumenstingl <martin.blumenstingl@...glemail.com>, kernel@...s.com, linux-pwm@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] pwm: meson: Remove error print for
 devm_add_action_or_reset()

Hello Waqar,

On Tue, Aug 05, 2025 at 11:33:36AM +0200, Waqar Hameed wrote:
> When `devm_add_action_or_reset()` fails, it is due to a failed memory
> allocation and will thus return `-ENOMEM`. `dev_err_probe()` doesn't do
> anything when error is `-ENOMEM`. Therefore, remove the useless call to
> `dev_err_probe()` when `devm_add_action_or_reset()` fails, and just
> return the value instead.
> 
> Signed-off-by: Waqar Hameed <waqar.hameed@...s.com>
> ---
> Changes in v2:
> 
> * Split the patch to one seperate patch for each sub-system.
> 
> Link to v1: https://lore.kernel.org/all/pnd7c0s6ji2.fsf@axis.com/
> 
>  drivers/pwm/pwm-meson.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 8c6bf3d49753..e90d37d4f956 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -520,8 +520,7 @@ static int meson_pwm_init_channels_s4(struct pwm_chip *chip)
>  		ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
>  					       meson->channels[i].clk);
>  		if (ret)
> -			return dev_err_probe(dev, ret,
> -					     "Failed to add clk_put action\n");
> +			return ret;

On the other hand the call to dev_err_probe() also doesn't hurt, right?
And when we keep it, it is clear that this error path is correctly
handled without having to know that devm_add_action_or_reset() can only
return success or -ENOMEM and we don't have to watch
devm_add_action_or_reset() to not grow something like

diff --git a/include/linux/device/devres.h b/include/linux/device/devres.h
index ae696d10faff..0876cce68776 100644
--- a/include/linux/device/devres.h
+++ b/include/linux/device/devres.h
@@ -156,6 +156,9 @@ static inline int __devm_add_action_or_reset(struct device *dev, void (*action)(
 {
 	int ret;
 
+	if (IS_ERR_OR_NULL(dev))
+		return -EINVAL;
+
 	ret = __devm_add_action(dev, action, data, name);
 	if (ret)
 		action(data);

From a subsystem maintainer's POV it would be great if it was easy to
notice if a given function needs an error message or not. One excellent
way to cover functions that can only return -ENOMEM on failure is to
optimize out the small overhead of the devm_add_action_or_reset() call.

See
https://lore.kernel.org/all/ylr7cuxldwb24ccenen4khtyddzq3owgzzfblbohkdxb7p7eeo@qpuddn6wrz3x/
for a prototype of what I imagine. Oh, you were the addressee of that
mail, so you already know.

To make my position here explicit: This is a nack.

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)