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
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
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
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
© 2016 - 2026 Red Hat, Inc.