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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Wed, 2 Jul 2025 08:54:48 +0200
Uwe Kleine-König <ukleinek@kernel.org> wrote:

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

Maybe this is a job for checkpatch, at least for the common cases.

There is already a check for kmalloc etc.
https://elixir.bootlin.com/linux/v6.16-rc4/source/scripts/checkpatch.pl#L6442

+CC Joe (who wrote the allocation functions test years ago) and other checkpatch
folk.


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