On Wed, 2 Jul 2025 08:54:48 +0200 Uwe Kleine-König <ukleinek@xxxxxxxxxx> 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@xxxxxxxxxx> 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 >
