[PATCH v2] pwm: meson: Remove error print for devm_add_action_or_reset()

[Uwe Kleine-König]

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 at axis.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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20250805/8f4b652b/attachment.sig>