[PATCH] Remove error prints for devm_add_action_or_reset()

[Uwe Kleine-König]

Hello Jonathan,

On Tue, Jul 01, 2025 at 06:55:19PM +0100, Jonathan Cameron wrote:
> On Tue, 1 Jul 2025 19:44:17 +0200
> Uwe Kleine-König <ukleinek at kernel.org> wrote:
> 
> > 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:
> 
> 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 problem I see is that there are two classes of functions: a) Those
that require an error message and b) those that don't. Class b) consists
of the functions that can only return success or -ENOMEM and the
functions that emit an error message themselves. (And another problem I
see is that for the latter the error message is usually non-optimal
because the function doesn't know the all details of the request. See my
reply to Andy for more details about that rant.)

IMHO what takes away the review bandwidth is that the reviewer has to
check which class the failing function is part of. If this effort
results in more driver authors not adding an error message after
devm_add_action_or_reset() that's nice, but in two months I have
forgotten the details of this discussion and I have to recheck if
devm_add_action_or_reset() is part of a) or b) and so the burden is
still on me.

So to give my answer on your question "What do we actually want here?":
Please let us get rid of the need to care for a) or b).

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

To complete implementing my wish all API functions would need to stop to
emit an error message. Unfortunately that isn't without downsides
because the result is that there are more error strings and so the
kernel size is increased. So you have to weight if you prefer individual
error messages and easier review/maintenance at the cost of a bigger
binary size and more dev_err_probe() calls in drivers eating vertical
space in your editor.

I know on which side I am, but I bet we won't find agreement about that
in the kernel community ...

Best regards
Uwe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20250702/9c7ba60b/attachment.sig>