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] [day] [month] [year] [list]
Message-ID: <pndcy99lvfz.a.out@axis.com>
Date: Tue, 5 Aug 2025 16:00:48 +0200
From: Waqar Hameed <waqar.hameed@...s.com>
To: Uwe Kleine-König <ukleinek@...nel.org>
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()

On Tue, Aug 05, 2025 at 15:23 +0200 Uwe Kleine-König <ukleinek@...nel.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@...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.

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ