Message-ID: <20250702105826.0000315e@huawei.com>
Date: Wed, 2 Jul 2025 10:58:26 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Uwe Kleine-König <ukleinek@...nel.org>
CC: Jonathan Cameron <jic23@...nel.org>, 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>, "Joe Perches" <joe@...ches.com>, Andy
 Whitcroft <apw@...onical.com>, "Dwaipayan Ray" <dwaipayanray1@...il.com>,
	Lukas Bulwahn <lukas.bulwahn@...il.com>
Subject: Re: [PATCH] Remove error prints for devm_add_action_or_reset()

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

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
> 

Powered by blists - more mailing lists