From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) (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 EBC9F23816C; Wed, 2 Jul 2025 09:58:34 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.176.79.56 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1751450318; cv=none; b=rvj7JB1YJBp49RF0a2pAoCmSsj/N4LPN8xmXKv9WCgEFzS9XYz5KNrQv8zyXci6qsVpmHUarVfndexxe4XEIY5uK1/fJqvXfivG7cO33xFwj9VkM/PY7ivIBoxEGUJL30ds3wdJQXwLCiCzGsMDwrFJDYAxzSY2RxPKn9185fCg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1751450318; c=relaxed/simple; bh=ac1WHXY9uU6sg9VG9LlQUm/TQKiiff0h5oFc/3+SBzY=; h=Date:From:To:CC:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=D1o5FWMQg66xzNOBz0Px8/T9GIoyqwDXuFCMmW5HS6VoLme9tUfqt3x1dgIbpPhF8tmangdHgyES6Uy87w/C6qLLiPxAGGH+qPbWonAXVjZZHDXxu3EaHHbHBQWvZ9y8345y/6hx5JBHjjecvQAPWEgegBbZn1OrLDzc98PQgxQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=huawei.com; spf=pass smtp.mailfrom=huawei.com; arc=none smtp.client-ip=185.176.79.56 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=huawei.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=huawei.com Received: from mail.maildlp.com (unknown [172.18.186.31]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4bXFdQ6LTjz6L55h; Wed, 2 Jul 2025 17:55:38 +0800 (CST) Received: from frapeml500008.china.huawei.com (unknown [7.182.85.71]) by mail.maildlp.com (Postfix) with ESMTPS id B5DAB1404FD; Wed, 2 Jul 2025 17:58:31 +0800 (CST) Received: from localhost (10.203.177.66) by frapeml500008.china.huawei.com (7.182.85.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Wed, 2 Jul 2025 11:58:28 +0200 Date: Wed, 2 Jul 2025 10:58:26 +0100 From: Jonathan Cameron To: Uwe =?ISO-8859-1?Q?Kleine-K=F6nig?= CC: Jonathan Cameron , Waqar Hameed , Vignesh Raghavendra , "Julien Panis" , William Breathitt Gray , "Linus Walleij" , Bartosz Golaszewski , Peter Rosin , David Lechner , Nuno =?ISO-8859-1?Q?S=E1?= , Andy Shevchenko , Cosmin Tanislav , "Lars-Peter Clausen" , Michael Hennerich , Matthias Brugger , AngeloGioacchino Del Regno , Matteo Martelli , Heiko Stuebner , Francesco Dolcini , =?ISO-8859-1?Q?Jo=E3o?= Paulo =?ISO-8859-1?Q?Gon=E7alves?= , 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 , , , , , , , , , , , , , , , , , , , , , , , , , "Joe Perches" , Andy Whitcroft , "Dwaipayan Ray" , Lukas Bulwahn Subject: Re: [PATCH] Remove error prints for devm_add_action_or_reset() Message-ID: <20250702105826.0000315e@huawei.com> In-Reply-To: References: <20250701185519.1410e831@jic23-huawei> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.42; x86_64-w64-mingw32) 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="ISO-8859-1" Content-Transfer-Encoding: quoted-printable X-ClientProxiedBy: lhrpeml100003.china.huawei.com (7.191.160.210) To frapeml500008.china.huawei.com (7.182.85.71) On Wed, 2 Jul 2025 08:54:48 +0200 Uwe Kleine-K=F6nig wrote: > Hello Jonathan, >=20 > 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=F6nig wrote: > > =20 > > > On Tue, Jul 01, 2025 at 05:03:33PM +0200, Waqar Hameed wrote: =20 > > > > drivers/pwm/pwm-meson.c | 3 +-- =20 > > >=20 > > > Looking at this driver I tried the following: =20 > >=20 > > I'm not sure what we actually want here. > >=20 > > 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 > >=20 > > void *something =3D kmalloc(); > > if (!something) > > return dev_err_probe(dev, -ENOMEM, ..); > >=20 > > 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. =20 >=20 > 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.) >=20 > 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#L64= 42 +CC Joe (who wrote the allocation functions test years ago) and other check= patch folk. >=20 > 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). >=20 > > 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. =20 >=20 > 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. >=20 > I know on which side I am, but I bet we won't find agreement about that > in the kernel community ... >=20 > Best regards > Uwe >=20