[PATCH v2 0/2] Improve handling of -ENOMEM in dev_err_probe()

[PATCH v2 0/2] Improve handling of -ENOMEM in dev_err_probe()

[Uwe Kleine-König]

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

Hello,

the first patch is just a (trivial) forward port of
https://lore.kernel.org/lkml/20240412164405.335657-2-u.kleine-koenig@pengutronix.de
to today's next.

In reply to the above mentioned patch submission Andy Shevchenko
suggested to make passing -ENOMEM unconditionally to dev_err_probe()
(i.e. handling the return value of a function that can only succeed or
return -ENOMEM) a build error. I'm not convinced, but for the purpose to
show good will and get the first patch in, I implemented that in the 2nd
patch. See the comments in that mail for my concerns.

After some discussion about Andy's concern the (implicit) v1 thread
died. To get the discussion going again here comes another patch
submission for these ideas.

Best regards
Uwe

Uwe Kleine-König (2):
  driver core: Make dev_err_probe() silent for -ENOMEM
  driver core: Don't allow passing a -ENOMEM to dev_err_probe()

 drivers/base/core.c        | 21 ++++++++++++++++-----
 include/linux/dev_printk.h |  8 +++++++-
 2 files changed, 23 insertions(+), 6 deletions(-)

base-commit: 6906a84c482f098d31486df8dc98cead21cce2d0
-- 
2.43.0

[PATCH v2 1/2] driver core: Make dev_err_probe() silent for -ENOMEM

[Uwe Kleine-König]

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

For an out-of-memory error there should be no additional output. Adapt
dev_err_probe() to not emit the error message when err is -ENOMEM.
This simplifies handling errors that might among others be -ENOMEM.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/base/core.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2b4c0624b704..730cae66607c 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -5021,11 +5021,22 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
 	vaf.fmt = fmt;
 	vaf.va = &args;
 
-	if (err != -EPROBE_DEFER) {
-		dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
-	} else {
+	switch (err) {
+	case -EPROBE_DEFER:
 		device_set_deferred_probe_reason(dev, &vaf);
 		dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
+		break;
+
+	case -ENOMEM:
+		/*
+		 * We don't print anything on -ENOMEM, there is already enough
+		 * output.
+		 */
+		break;
+
+	default:
+		dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
+		break;
 	}
 
 	va_end(args);
-- 
2.43.0

[PATCH RFC v2 2/2] driver core: Don't allow passing a -ENOMEM to dev_err_probe()

[Uwe Kleine-König]

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

Re: [PATCH v2 1/2] driver core: Make dev_err_probe() silent for -ENOMEM

[Geert Uytterhoeven]

On Thu, Jun 13, 2024 at 11:24 PM Uwe Kleine-König <ukleinek@kernel.org> wrote:
> From: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
>
> For an out-of-memory error there should be no additional output. Adapt
> dev_err_probe() to not emit the error message when err is -ENOMEM.
> This simplifies handling errors that might among others be -ENOMEM.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC v2 2/2] driver core: Don't allow passing a -ENOMEM to dev_err_probe()

[Geert Uytterhoeven]

Hi Uwe,

On Thu, Jun 13, 2024 at 11:24 PM Uwe Kleine-König <ukleinek@kernel.org> wrote:
> 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>

Thanks for your patch!

> 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".

You can find several in public header files:

    git grep -W "return\s*-ENOMEM\>" -- include/

I expect there are more in static code all over the place.

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

IMHO this is the only drawback.
And things may change: a static (inline) function that can only return
zero or -ENOMEM now, can return other error codes tomorrow.
Also, some dummies (e.g. dma_mapping_error()) return -ENOMEM, so it
depends on kernel configuration too.

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

There's still the issue of increased kernel size, mainly due to the
presence of the error message string.

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

My initial reaction was quite positive, until I discovered the dummies...

> --- 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_ */

Looks like dev_err_probe() does not have a dummy for the !CONFIG_PRINTK
case yet, while it could definitely use one.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC v2 2/2] driver core: Don't allow passing a -ENOMEM to dev_err_probe()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4666 bytes --]

Hello Geert,

On Fri, Jun 14, 2024 at 09:26:52AM +0200, Geert Uytterhoeven wrote:
> On Thu, Jun 13, 2024 at 11:24 PM Uwe Kleine-König <ukleinek@kernel.org> wrote:
> > 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>
> 
> Thanks for your patch!
> 
> > 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".
> 
> You can find several in public header files:
> 
>     git grep -W "return\s*-ENOMEM\>" -- include/
> 
> I expect there are more in static code all over the place.
> 
> >  - 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.
> 
> IMHO this is the only drawback.
> And things may change: a static (inline) function that can only return
> zero or -ENOMEM now, can return other error codes tomorrow.
> Also, some dummies (e.g. dma_mapping_error()) return -ENOMEM, so it
> depends on kernel configuration too.

Huh, I didn't spot the dependency on kernel configuration. That makes it
quite bad.

> >  - 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.
> 
> There's still the issue of increased kernel size, mainly due to the
> presence of the error message string.
> 
> > Given these, the more complicated implementation for dev_err_probe()
> > isn't really justified IMHO.
> 
> My initial reaction was quite positive, until I discovered the dummies...
> 
> > --- 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_ */
> 
> Looks like dev_err_probe() does not have a dummy for the !CONFIG_PRINTK
> case yet, while it could definitely use one.

Would you want to drop

	device_set_deferred_probe_reason(dev, &vaf);

from dev_err_probe() for !CONFIG_PRINTK, too? If not, you can throw away
the string only if __builtin_constant_p(__err != -EPROBE_DEFER) && __err
!= -EPROBE_DEFER. I agree such an improvement would be nice, but that's
orthogonal to this series.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC v2 2/2] driver core: Don't allow passing a -ENOMEM to dev_err_probe()

[Geert Uytterhoeven]

Hi Uwe,

On Fri, Jun 14, 2024 at 11:15 AM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
> On Fri, Jun 14, 2024 at 09:26:52AM +0200, Geert Uytterhoeven wrote:
> > Looks like dev_err_probe() does not have a dummy for the !CONFIG_PRINTK
> > case yet, while it could definitely use one.
>
> Would you want to drop
>
>         device_set_deferred_probe_reason(dev, &vaf);
>
> from dev_err_probe() for !CONFIG_PRINTK, too? If not, you can throw away
> the string only if __builtin_constant_p(__err != -EPROBE_DEFER) && __err
> != -EPROBE_DEFER. I agree such an improvement would be nice, but that's
> orthogonal to this series.

I would drop it. CONFIG_PRINTK=n is only intended for production
systems where no console is available, and the full behavior of the system
is understood well.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds