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

[Andy Shevchenko]

On Wed, Jul 02, 2025 at 08:10:28AM +0200, Uwe Kleine-König wrote:
> On Tue, Jul 01, 2025 at 08:57:02PM +0300, Andy Shevchenko wrote:
> > On Tue, Jul 1, 2025 at 8:44 PM Uwe Kleine-König <ukleinek@xxxxxxxxxx> wrote:
> > > On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote:

...

> > > 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.
> > 
> > Why do we need a dev_err_probe() after devm_add_action*()? I would
> > expect that the original call (if needed) can spit out a message.
> 
> I'm not a big fan of API functions that emit an error message.

We do have that in devm_ioremap*() family. Just saying...

> In general the caller knows better what went wrong (here:
> devm_add_action_or_reset() doesn't know this to be about the clk_put
> action), so the error message can be more expressive.

I'm not sure I was clear about my suggestion. What I argued is something like
this

devm_foo_alloc()
{
	ret = foo_alloc();
	if (ret)
		return dev_err_probe();

	return devm_add_action_or_reset();
}

foo_alloc() in my example is left untouched.

> Also in general an API function doesn't know if a failure is fatal or if
> the consumer handles the failure just well and if the call is part of a
> driver's .probe() so it's unclear if dev_err_probe() can/should be used.
> (I admit that the last two probably don't apply to
> devm_add_action_or_reset() but that's not a good enough reason to
> make this function special. Every special case is a maintanance burden.)

devm_*() are only supposed to be called in the probe phase. So using
dev_err_probe() there (implementations) is natural thing to do, if required.
And see above, we have such cases already.

-- 
With Best Regards,
Andy Shevchenko