From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5479A28031D; Tue, 1 Jul 2025 17:55:50 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1751392551; cv=none; b=e3Pj48VFaSrQnd8NXOY5gVo1l7w1h0wPfaoHaRwJ/CtrQ2EhqHy3Yow5Ixqc2lUlPtQbmScuglJ4zHeu7kHk4Snsvq05wQobF2iX3K2D0xG/BpiUzgBPoANDVqw5NG1J6riiqD3jpL+H/FOnuCbrQTaRT+2LiF8ja9PAZ734cpI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1751392551; c=relaxed/simple; bh=wNjXDKjq0I6/9yhbTcfL1kL2qUBF2TKpXGL7HuWci6Y=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=H4+2EjQRHt6lphTGGXoLHlt8iiiJWUFncRfErh2A1VjTMj/rCZODSBYjLMXqZgvYlBY2MRWByVJE9JTC1U5yAP6z7l0RpfG3Sh7G8liatF/nCZKy4wrvm84Ewd5AJepuZ1aAWRGojGvGTP2mgoIJRk0Fqew4oT32pnJbrawwiPA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=HriBBdwv; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="HriBBdwv" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 59A38C4CEEB; Tue, 1 Jul 2025 17:55:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1751392550; bh=wNjXDKjq0I6/9yhbTcfL1kL2qUBF2TKpXGL7HuWci6Y=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=HriBBdwvDpiR3MqtXKwPsYZ1oyhJc4LuNhlhGwLR9DueVpFhqgxh0iifGRF8evW5b JkSKN6s2pXENJuY4L7jw9EN0o02oGOqxbejPJl98m6f7SgO53gilTl2QYWbDazLHJG GMk+QoES7HtD/8pJ/jE4gIULQYzcfilklDsIvNLqEXk8jjIvhTDISALb6F1T5xkyuV 2ceg8TIweRtzrpnnwxZfvr4CfQb7JsO9KTeLrZbTN2IdbDo0841Ct19ICweXdr48Es os2K+GpqIeRuS8b0lEvHCyatYjQShJnNz4T08zfYD8j5YgIEEztxJptyVRDDkHG/JJ xDnujlxhZfnhA== Date: Tue, 1 Jul 2025 18:55:19 +0100 From: Jonathan Cameron To: Uwe =?UTF-8?B?S2xlaW5lLUvDtm5pZw==?= Cc: Waqar Hameed , Vignesh Raghavendra , Julien Panis , William Breathitt Gray , Linus Walleij , Bartosz Golaszewski , Peter Rosin , David Lechner , Nuno =?UTF-8?B?U8Oh?= , Andy Shevchenko , Cosmin Tanislav , Lars-Peter Clausen , Michael Hennerich , Matthias Brugger , AngeloGioacchino Del Regno , Matteo Martelli , Heiko Stuebner , Francesco Dolcini , =?UTF-8?B?Sm/Do28=?= Paulo =?UTF-8?B?R29uw6dhbHZlcw==?= , Hugo Villeneuve , Subhajit Ghosh , Mudit Sharma , Gerald Loacker , Song Qiang , Crt Mori , Dmitry Torokhov , Ulf Hansson , Karol Gugala , Mateusz Holenko , Gabriel Somlo , Joel Stanley , Claudiu Manoil , Vladimir Oltean , Wei Fang , Clark Wang , Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Vinod Koul , Kishon Vijay Abraham I , Krzysztof Kozlowski , Alim Akhtar , Sebastian Reichel , Neil Armstrong , Kevin Hilman , Jerome Brunet , Martin Blumenstingl , Han Xu , Haibo Chen , Yogesh Gaur , Mark Brown , Avri Altman , Bart Van Assche , "James E.J. Bottomley" , "Martin K. Petersen" , Souradeep Chowdhury , Greg Kroah-Hartman , Liam Girdwood , Peter Ujfalusi , Bard Liao , Ranjani Sridharan , Daniel Baluta , Kai Vehmanen , Pierre-Louis Bossart , Jaroslav Kysela , Takashi Iwai , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , kernel@axis.com, linux-iio@vger.kernel.org, linux-omap@vger.kernel.org, linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-rockchip@lists.infradead.org, linux-input@vger.kernel.org, linux-mmc@vger.kernel.org, imx@lists.linux.dev, netdev@vger.kernel.org, linux-phy@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-pm@vger.kernel.org, linux-pwm@vger.kernel.org, linux-amlogic@lists.infradead.org, linux-spi@vger.kernel.org, linux-scsi@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org, sound-open-firmware@alsa-project.org, linux-sound@vger.kernel.org Subject: Re: [PATCH] Remove error prints for devm_add_action_or_reset() Message-ID: <20250701185519.1410e831@jic23-huawei> In-Reply-To: References: X-Mailer: Claws Mail 4.3.1 (GTK 3.24.49; x86_64-pc-linux-gnu) Precedence: bulk X-Mailing-List: linux-usb@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Tue, 1 Jul 2025 19:44:17 +0200 Uwe Kleine-K=C3=B6nig wrote: > Hello, >=20 > On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote: > > drivers/pwm/pwm-meson.c | 3 +-- =20 >=20 > Looking at this driver I tried the following: >=20 Hi Uqe, 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 =3D 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 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. Jonathan > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 3809baed42f3..58a2ab74f14c 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -5062,7 +5062,7 @@ static void __dev_probe_failed(const struct device = *dev, int err, bool fatal, > * > * Returns @err. > */ > -int dev_err_probe(const struct device *dev, int err, const char *fmt, ..= .) > +int _dev_err_probe(const struct device *dev, int err, const char *fmt, .= ..) > { > va_list vargs; > =20 > @@ -5075,7 +5075,7 @@ int dev_err_probe(const struct device *dev, int err= , const char *fmt, ...) > =20 > return err; > } > -EXPORT_SYMBOL_GPL(dev_err_probe); > +EXPORT_SYMBOL_GPL(_dev_err_probe); > =20 > /** > * dev_warn_probe - probe error check and log helper > diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h > index eb2094e43050..23ef250727f1 100644 > --- a/include/linux/dev_printk.h > +++ b/include/linux/dev_printk.h > @@ -275,7 +275,13 @@ do { \ > WARN_ONCE(condition, "%s %s: " format, \ > dev_driver_string(dev), dev_name(dev), ## arg) > =20 > -__printf(3, 4) int dev_err_probe(const struct device *dev, int err, cons= t char *fmt, ...); > +__printf(3, 4) int _dev_err_probe(const struct device *dev, int err, con= st char *fmt, ...); > + > +#define dev_err_probe(dev, err, ...) ( \ > + (__builtin_constant_p(err) && err =3D=3D -ENOMEM) ? err : _dev_err_prob= e(dev, err, __VA_ARGS__) \ > +) > + > + > __printf(3, 4) int dev_warn_probe(const struct device *dev, int err, con= st char *fmt, ...); > =20 > /* Simple helper for dev_err_probe() when ERR_PTR() is to be returned. */ > diff --git a/include/linux/device/devres.h b/include/linux/device/devres.h > index ae696d10faff..abfa5152b5a7 100644 > --- a/include/linux/device/devres.h > +++ b/include/linux/device/devres.h > @@ -157,8 +157,11 @@ static inline int __devm_add_action_or_reset(struct = device *dev, void (*action)( > int ret; > =20 > ret =3D __devm_add_action(dev, action, data, name); > - if (ret) > + if (ret) { > + if (ret !=3D -ENOMEM) > + __builtin_unreachable(); > action(data); > + } > =20 > return ret; > } >=20 > With that >=20 > ret =3D devm_add_action_or_reset(dev, meson_pwm_s4_put_clk, > meson->channels[i].clk); > if (ret) > return dev_err_probe(dev, ret, > "Failed to add clk_put action\n"); >=20 > from drivers/pwm/pwm-meson.c is optimized to >=20 > ret =3D devm_add_action_or_reset(dev, meson_pwm_s4_put_clk, > meson->channels[i].clk); > if (ret) > return ret; >=20 > . >=20 > I would prefer this approach, because a) there is no need to drop all > dev_err_probe()s after devm_add_action_or_reset() and b) the > dev_err_probe()s could stay for consistency in the error paths of a > driver. >=20 > Best regards > Uwe