Message-ID: <jeajjewfbg5qo736imozpghnpxln2pux74aegtqsi57qsbpug2@opndel6zc3m3>
Date: Wed, 2 Jul 2025 08:54:48 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Waqar Hameed <waqar.hameed@...s.com>, 
	Vignesh Raghavendra <vigneshr@...com>, Julien Panis <jpanis@...libre.com>, 
	William Breathitt Gray <wbg@...nel.org>, Linus Walleij <linus.walleij@...aro.org>, 
	Bartosz Golaszewski <brgl@...ev.pl>, Peter Rosin <peda@...ntia.se>, 
	David Lechner <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, 
	Andy Shevchenko <andy@...nel.org>, Cosmin Tanislav <cosmin.tanislav@...log.com>, 
	Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, 
	Matthias Brugger <matthias.bgg@...il.com>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Matteo Martelli <matteomartelli3@...il.com>, 
	Heiko Stuebner <heiko@...ech.de>, Francesco Dolcini <francesco@...cini.it>, 
	João Paulo Gonçalves <jpaulo.silvagoncalves@...il.com>, Hugo Villeneuve <hvilleneuve@...onoff.com>, 
	Subhajit Ghosh <subhajit.ghosh@...aklogic.com>, Mudit Sharma <muditsharma.info@...il.com>, 
	Gerald Loacker <gerald.loacker@...fvision.net>, Song Qiang <songqiang1304521@...il.com>, 
	Crt Mori <cmo@...exis.com>, Dmitry Torokhov <dmitry.torokhov@...il.com>, 
	Ulf Hansson <ulf.hansson@...aro.org>, Karol Gugala <kgugala@...micro.com>, 
	Mateusz Holenko <mholenko@...micro.com>, Gabriel Somlo <gsomlo@...il.com>, Joel Stanley <joel@....id.au>, 
	Claudiu Manoil <claudiu.manoil@....com>, Vladimir Oltean <vladimir.oltean@....com>, 
	Wei Fang <wei.fang@....com>, Clark Wang <xiaoning.wang@....com>, 
	Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Vinod Koul <vkoul@...nel.org>, 
	Kishon Vijay Abraham I <kishon@...nel.org>, Krzysztof Kozlowski <krzk@...nel.org>, 
	Alim Akhtar <alim.akhtar@...sung.com>, Sebastian Reichel <sre@...nel.org>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Kevin Hilman <khilman@...libre.com>, 
	Jerome Brunet <jbrunet@...libre.com>, Martin Blumenstingl <martin.blumenstingl@...glemail.com>, 
	Han Xu <han.xu@....com>, Haibo Chen <haibo.chen@....com>, 
	Yogesh Gaur <yogeshgaur.83@...il.com>, Mark Brown <broonie@...nel.org>, 
	Avri Altman <avri.altman@....com>, Bart Van Assche <bvanassche@....org>, 
	"James E.J. Bottomley" <James.Bottomley@...senpartnership.com>, "Martin K. Petersen" <martin.petersen@...cle.com>, 
	Souradeep Chowdhury <quic_schowdhu@...cinc.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Liam Girdwood <lgirdwood@...il.com>, Peter Ujfalusi <peter.ujfalusi@...ux.intel.com>, 
	Bard Liao <yung-chuan.liao@...ux.intel.com>, Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>, 
	Daniel Baluta <daniel.baluta@....com>, Kai Vehmanen <kai.vehmanen@...ux.intel.com>, 
	Pierre-Louis Bossart <pierre-louis.bossart@...ux.dev>, Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>, 
	Shawn Guo <shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>, 
	Pengutronix Kernel Team <kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>, kernel@...s.com, 
	linux-iio@...r.kernel.org, linux-omap@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-mediatek@...ts.infradead.org, linux-rockchip@...ts.infradead.org, linux-input@...r.kernel.org, 
	linux-mmc@...r.kernel.org, imx@...ts.linux.dev, netdev@...r.kernel.org, 
	linux-phy@...ts.infradead.org, linux-samsung-soc@...r.kernel.org, linux-pm@...r.kernel.org, 
	linux-pwm@...r.kernel.org, linux-amlogic@...ts.infradead.org, linux-spi@...r.kernel.org, 
	linux-scsi@...r.kernel.org, linux-arm-msm@...r.kernel.org, linux-usb@...r.kernel.org, 
	sound-open-firmware@...a-project.org, linux-sound@...r.kernel.org
Subject: Re: [PATCH] Remove error prints for devm_add_action_or_reset()

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists