Re: [PATCH] Remove error prints for devm_add_action_or_reset()

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 1 Jul 2025 19:44:17 +0200
Uwe Kleine-König <ukleinek@kernel.org> wrote:

> Hello,
> 
> On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote:
> >  drivers/pwm/pwm-meson.c                          | 3 +--  
> 
> Looking at this driver I tried the following:
> 

Hi Uqe,

I'm not sure what we actually want here.

My thought when suggesting removing instances of this
particular combination wasn't saving on code size, but rather just
general removal of pointless code that was getting cut and
paste into new drivers and wasting a tiny bit of review bandwidth.
I'd consider it bad practice to have patterns like

void *something = kmalloc();
if  (!something)
	return dev_err_probe(dev, -ENOMEM, ..);

and my assumption was people would take a similar view with
devm_add_action_or_reset().

It is a bit nuanced to have some cases where we think prints
are reasonable and others where they aren't so I get your
point about consistency.

The code size reduction is nice so I'd not be against it as an extra
if the reduction across a kernel builds is significant and enough
people want to keep these non printing prints.

Jonathan


> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 3809baed42f3..58a2ab74f14c 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -5062,7 +5062,7 @@ static void __dev_probe_failed(const struct device *dev, int err, bool fatal,
>   *
>   * Returns @err.
>   */
> -int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
> +int _dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
>  {
>  	va_list vargs;
>  
> @@ -5075,7 +5075,7 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
>  
>  	return err;
>  }
> -EXPORT_SYMBOL_GPL(dev_err_probe);
> +EXPORT_SYMBOL_GPL(_dev_err_probe);
>  
>  /**
>   * dev_warn_probe - probe error check and log helper
> diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
> index eb2094e43050..23ef250727f1 100644
> --- a/include/linux/dev_printk.h
> +++ b/include/linux/dev_printk.h
> @@ -275,7 +275,13 @@ do {									\
>  	WARN_ONCE(condition, "%s %s: " format, \
>  			dev_driver_string(dev), dev_name(dev), ## arg)
>  
> -__printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
> +__printf(3, 4) int _dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
> +
> +#define dev_err_probe(dev, err, ...) (								\
> +	(__builtin_constant_p(err) && err == -ENOMEM) ? err : _dev_err_probe(dev, err, __VA_ARGS__)	\
> +)
> +
> +
>  __printf(3, 4) int dev_warn_probe(const struct device *dev, int err, const char *fmt, ...);
>  
>  /* Simple helper for dev_err_probe() when ERR_PTR() is to be returned. */
> diff --git a/include/linux/device/devres.h b/include/linux/device/devres.h
> index ae696d10faff..abfa5152b5a7 100644
> --- a/include/linux/device/devres.h
> +++ b/include/linux/device/devres.h
> @@ -157,8 +157,11 @@ static inline int __devm_add_action_or_reset(struct device *dev, void (*action)(
>  	int ret;
>  
>  	ret = __devm_add_action(dev, action, data, name);
> -	if (ret)
> +	if (ret) {
> +		if (ret != -ENOMEM)
> +			__builtin_unreachable();
>  		action(data);
> +	}
>  
>  	return ret;
>  }
> 
> With that
> 
>         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");
> 
> from drivers/pwm/pwm-meson.c is optimized to
> 
>         ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
>                                        meson->channels[i].clk);
>         if (ret)
>                 return ret;
> 
> .
> 
> I would prefer this approach, because a) there is no need to drop all
> dev_err_probe()s after devm_add_action_or_reset() and b) the
> dev_err_probe()s could stay for consistency in the error paths of a
> driver.
> 
> Best regards
> Uwe