Received-SPF: pass (google.com: domain of linux-kernel+bounces-214025-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <ukleinek@kernel.org>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@baylibre.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Petr Mladek <pmladek@suse.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Xiubo Li <xiubli@redhat.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	linux-kernel@vger.kernel.org
Subject: [PATCH RFC v2 2/2] driver core: Don't allow passing a -ENOMEM to dev_err_probe()
Date: Thu, 13 Jun 2024 23:23:53 +0200
Message-ID:  <85a3d444f2943ebe9d64e722b1717a5f7d06ed48.1718311756.git.u.kleine-koenig@baylibre.com>
In-Reply-To: <cover.1718311756.git.u.kleine-koenig@baylibre.com>
References: <cover.1718311756.git.u.kleine-koenig@baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

From: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

If a function returns the error code -ENOMEM, there should be no error
output, because a failing allocation is already quite talkative and
adding another indication only makes it harder to determine the actual
problem.

So the construct:

	ret = some_function(...);
	if (ret)
		return dev_err_probe(dev, ret, ...);

is questionable if some_function() can only succeed or return -ENODEV.

Catch some of these failures during compile time.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
Hello,

I have some concerns about this patch, I only implemented it because in
reply to the first submission of patch #1 Andy wrote that he thinks this
should be done, too. So the idea of this patch is only to keep the
discussion about handling a constant -ENOMEM to dev_err_probe() away
from patch 1, in the hope to make application of patch 1 more likely :-)

So, I think this patch 2/2 is a bad idea, because:

 - Let's assume there are functions, that return either success or
   -ENOMEM. (I'm not aware of such a function, but I didn't search for
   one and probably something like that exists.) Probably the compiler
   won't be able to know that, and so doesn't catch that "problem".
 - Using dev_err_probe() to handle the return code of some_function() is
   convenient. First to make error handling in the calling function
   uniform, and second, to not create a patch opportunity for all
   callers when some_function() might return another error code in the
   future. So dev_err_probe() can just be used without caring for the
   details of the handled error.
 - In the presence of patch #1, there is no real problem with calling
   dev_err_probe(dev, -ENOMEM, ...), because this is an error path and
   so not performance critical, and no error message is emitted.

Given these, the more complicated implementation for dev_err_probe()
isn't really justified IMHO.

Best regards
Uwe

 drivers/base/core.c        | 4 ++--
 include/linux/dev_printk.h | 8 +++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 730cae66607c..87b9eda95178 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -5012,7 +5012,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
  *
  * 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, ...)
 {
 	struct va_format vaf;
 	va_list args;
@@ -5043,7 +5043,7 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
 
 	return err;
 }
-EXPORT_SYMBOL_GPL(dev_err_probe);
+EXPORT_SYMBOL_GPL(__dev_err_probe);
 
 static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
 {
diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
index ae80a303c216..84cbf67d92c8 100644
--- a/include/linux/dev_printk.h
+++ b/include/linux/dev_printk.h
@@ -275,6 +275,12 @@ do {									\
 	WARN_ONCE(condition, "%s %s: " format, \
 			dev_driver_string(dev), dev_name(dev), ## arg)
 
-__printf(3, 4) int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
+__printf(3, 4) int __dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
+#define dev_err_probe(dev, err, ...)						\
+	({									\
+		int __err = (err);						\
+		BUILD_BUG_ON(__builtin_constant_p(__err) && __err == -ENOMEM);	\
+		__dev_err_probe((dev), __err, __VA_ARGS__);			\
+	 })
 
 #endif /* _DEVICE_PRINTK_H_ */
-- 
2.43.0