pwm: meson: Remove error print for devm_add_action_or_reset()
Subsystems (3)
PATCH v2 1/1 pwm: meson: Remove error print for devm_add_action_or_reset()
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.
---
Changes in v2:
* Split the patch to one seperate patch for each sub-system.
Link to v1: https://lore.kernel.org/all/[email protected]/
drivers/pwm/pwm-meson.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/pwm/pwm-meson.c +1 -2
@@ -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; }return 0 ;
base-commit: 260f6f4fda93c8485c8037865c941b42b9cba5d2
Signature · 1 line
2.39.5
Uwe Kleine-König
maintainerHello Waqar,
Quoted 22 lines · Waqar Hameed
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 todev_err_probe()whendevm_add_action_or_reset()fails, and just return the value instead.Signed-off-by Waqar Hameed <[email protected]>---
Changes in v2:* Split the patch to one seperate patch for each sub-system.
Link to v1: https://lore.kernel.org/all/[email protected]/
drivers/pwm/pwm-meson.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/pwm/pwm-meson.c +0
-0
drivers/pwm/pwm-meson.c static int meson_pwm_init_channels_s4(struct pwm_chip *chip) · line 523
Quoted 6 lines
drivers/pwm/pwm-meson.c +1
-2@@ -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
include/linux/device/devres.h +3 -0
@@ -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
Quoted 58 lines · On Tue, Aug 05, 2025 at 15:23 +0200 Uwe Kleine-König <[email protected]>…
Hello Waqar,
Quoted 22 lines · Waqar Hameed
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 todev_err_probe()whendevm_add_action_or_reset()fails, and just return the value instead.Signed-off-by Waqar Hameed <[email protected]>---
Changes in v2:* Split the patch to one seperate patch for each sub-system.
Link to v1: https://lore.kernel.org/all/[email protected]/
drivers/pwm/pwm-meson.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/pwm/pwm-meson.c +0
-0drivers/pwm/pwm-meson.c static int meson_pwm_init_channels_s4(struct pwm_chip *chip) · line 523
Quoted 6 lines
drivers/pwm/pwm-meson.c +1
-2@@ -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 thatdevm_add_action_or_reset()can only return success or-ENOMEMand we don't have to watchdevm_add_action_or_reset()to not grow something likeinclude/linux/device/devres.h +3
-0@@ -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
-ENOMEMon failure is to optimize out the small overhead of thedevm_add_action_or_reset()call.
Quoted 6 lines
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...