On Wed, 2 Jul 2025 08:54:48 +0200
Uwe Kleine-König [off-list ref] wrote:

Hello Jonathan,

On Tue, Jul 01, 2025 at 06:55:19PM +0100, Jonathan Cameron wrote:

quoted

On Tue, 1 Jul 2025 19:44:17 +0200
Uwe Kleine-König [off-list ref] wrote:
  

quoted

On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote:  

quoted

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

quoted

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