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

[Waqar Hameed <waqar.hameed@axis.com>]

On Tue, Aug 05, 2025 at 15:23 +0200 Uwe Kleine-König <ukleinek@kernel.org> wrote:

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

I fully understand your point and agree that there should not be a
mental burden of knowing the exact return values from a function. That
should be handled automatically, e.g. by the compiler or other tools.

There was no real consensus in the previous thread (Jonathan Cameron
even CC:ed some checkpatch-people to get some input for automatic
detection from tools, but with no response). I hope that we can have
some good way of solving these in the future, because this currently
doesn't scale well and I'm fairly sure another driver in the future will
hit this exact situation again...

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic