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 <[email protected]>
---
Hello,
I had the idea to make dev_err_probe() silent for -ENOMEM for a while now.
Triggered by https://lore.kernel.org/all/[email protected] I
finally implemented this idea.
Best regards
Uwe
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 4d51928c4088..8f1914ca6b6e 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -5054,11 +5054,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);
base-commit: 4118d9533ff3a5d16efb476a0d00afceecd92cf5
prerequisite-patch-id: 950b86247f1ba5dfc0396d9777d83e641ff62c35
prerequisite-patch-id: 59206f0661ac45767ecf89a11ee57a0f9ee26b3c
prerequisite-patch-id: a3ca2b5359cf23a48ca505280d967f02d9ce356c
prerequisite-patch-id: 2b7c70d39b660d374794bac26e7b5e9a7f26da8f
prerequisite-patch-id: 569cb54f9b2390be8d51979045722462e0b63f2d
prerequisite-patch-id: edc1f62b9d5c0e658c7283bd3353d2004a0c1fbf
prerequisite-patch-id: 8201835ed22aa6956d219d84299f9226fb8254c1
prerequisite-patch-id: 07772cf036deb3150560629bc287510901e90f24
prerequisite-patch-id: d84360cad6a23ccbadc1205541ffb218fa589ba7
prerequisite-patch-id: 79e7647450348ef0cca53af2bc5e5a0033dcec57
prerequisite-patch-id: 45568b19e28a0cb3f46699ff945adc654eda07d2
prerequisite-patch-id: 7daf4547e4b3785959e0c024ce95141cd1d936da
prerequisite-patch-id: f3c1f5b72e3b503760d4b70bb661ceb8381c4822
prerequisite-patch-id: 5e0cca8fff7c73448710f42e711eff5a863b8a31
prerequisite-patch-id: 188bb3126861dc5027e5a0af78370ef8bb66fd8a
prerequisite-patch-id: afaee9f242fa90b29a3b8e971d0cbfd00269ad2d
prerequisite-patch-id: 5e815580f4d875d8720236fc016f6848d75b3cc9
prerequisite-patch-id: 0fd48f45bc44d66c9ca01b03d570fea876fc4abb
prerequisite-patch-id: b63f367f8354d56916d33c4236c79cf8e1c7d67a
prerequisite-patch-id: ea68cb08a89ece1fb288f08d8c72ee7bdc378e79
prerequisite-patch-id: 0fa92154ed986e765a41079f5aa441ae0ab4683b
prerequisite-patch-id: 8eca4420a223355531ddfc5331729feb5fff9812
prerequisite-patch-id: d72dbdaa1f3f12e970341277e7e5bbb8da15f228
prerequisite-patch-id: 7699990bf345e9551251211ee798f3b93c257d24
prerequisite-patch-id: 168172f0fec6fcf334cd6cc600749afd57ade8a1
prerequisite-patch-id: 96a01bb6af22da23b4e7acb5897117db959e8a08
prerequisite-patch-id: 82222b0c579003169bcc1f1d84bb34956655cfb0
prerequisite-patch-id: 075db535154891b70ed4e659588e9294e06a5f38
prerequisite-patch-id: 83c646c3aa4ef3578e0fcd86ffc395b51cc47763
prerequisite-patch-id: 4b8d2995e96ae290599d752cf1c1d2537e47bfab
prerequisite-patch-id: 5915e9d3dd78f832ab0017c81df770f443b32169
prerequisite-patch-id: dba378294fcb0aa72ee6bbace049c3ecc2b97bcb
prerequisite-patch-id: 712d0c765bdecbc60d72e62b10b329b525dfd16f
prerequisite-patch-id: 85e037f468c34b7a904f926e0cc555d7863c2cc7
prerequisite-patch-id: 4e475984306a28ba367a171a6450bcfb07d0eb3c
prerequisite-patch-id: 40b6db6c8d4e5bcaab64ce1afd53a79586f3df0b
prerequisite-patch-id: fd4da5ff37eba383f7de7dd219cfcb297155b654
prerequisite-patch-id: b076059966b15d0bf1c8282f48502299987b628a
prerequisite-patch-id: 6b4540ea605fc3feb59128abee429c870b506da5
prerequisite-patch-id: b98d4896c4dbc26adb898815c3e10722796bc9ca
prerequisite-patch-id: f82322e104e3194c77b66a2ab2e465dd2a162d65
prerequisite-patch-id: 3c719132de0cc72d3e548b5f3623cccb28a74ab5
prerequisite-patch-id: d23e9aef7476d7f0976a9d8919c8474a5f91c20c
prerequisite-patch-id: e725b9cf221da371b19d5aef5d1b4a5e813a19ee
prerequisite-patch-id: 932d88887d4028aee27a809eeb89d1dc7835acc8
prerequisite-patch-id: 48554c57f1d06553af3ded861ebf9b88ee6b03c4
prerequisite-patch-id: c26315d8be3a746b04dea921d41c903d054b774f
prerequisite-patch-id: fc598d2febb873efbaf53d4abfba4fe9c27aee52
prerequisite-patch-id: 77b58bda743a1fde9b0b040f7832ec3d5853ea75
prerequisite-patch-id: a64b4850058cd9189ccca081847f8908dbf91ae3
prerequisite-patch-id: 73a6bb8ed4e20eb90127f1c2e6361d52ae17791c
--
2.43.0
On Fri, Apr 12, 2024 at 06:44:05PM +0200, Uwe Kleine-K?nig wrote:
> 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.
..
BUILD_BUG_ON(err == -ENOMEM);
Done!
--
With Best Regards,
Andy Shevchenko
Hello,
On Fri, Apr 12, 2024 at 06:44:05PM +0200, Uwe Kleine-K?nig wrote:
> base-commit: 4118d9533ff3a5d16efb476a0d00afceecd92cf5
That's today's next/master.
> prerequisite-patch-id: 950b86247f1ba5dfc0396d9777d83e641ff62c35
> prerequisite-patch-id: 59206f0661ac45767ecf89a11ee57a0f9ee26b3c
> prerequisite-patch-id: a3ca2b5359cf23a48ca505280d967f02d9ce356c
> prerequisite-patch-id: 2b7c70d39b660d374794bac26e7b5e9a7f26da8f
> prerequisite-patch-id: 569cb54f9b2390be8d51979045722462e0b63f2d
> prerequisite-patch-id: edc1f62b9d5c0e658c7283bd3353d2004a0c1fbf
> prerequisite-patch-id: 8201835ed22aa6956d219d84299f9226fb8254c1
> prerequisite-patch-id: 07772cf036deb3150560629bc287510901e90f24
> prerequisite-patch-id: d84360cad6a23ccbadc1205541ffb218fa589ba7
> prerequisite-patch-id: 79e7647450348ef0cca53af2bc5e5a0033dcec57
> prerequisite-patch-id: 45568b19e28a0cb3f46699ff945adc654eda07d2
> prerequisite-patch-id: 7daf4547e4b3785959e0c024ce95141cd1d936da
> prerequisite-patch-id: f3c1f5b72e3b503760d4b70bb661ceb8381c4822
> prerequisite-patch-id: 5e0cca8fff7c73448710f42e711eff5a863b8a31
> prerequisite-patch-id: 188bb3126861dc5027e5a0af78370ef8bb66fd8a
> prerequisite-patch-id: afaee9f242fa90b29a3b8e971d0cbfd00269ad2d
> prerequisite-patch-id: 5e815580f4d875d8720236fc016f6848d75b3cc9
> prerequisite-patch-id: 0fd48f45bc44d66c9ca01b03d570fea876fc4abb
> prerequisite-patch-id: b63f367f8354d56916d33c4236c79cf8e1c7d67a
> prerequisite-patch-id: ea68cb08a89ece1fb288f08d8c72ee7bdc378e79
> prerequisite-patch-id: 0fa92154ed986e765a41079f5aa441ae0ab4683b
> prerequisite-patch-id: 8eca4420a223355531ddfc5331729feb5fff9812
> prerequisite-patch-id: d72dbdaa1f3f12e970341277e7e5bbb8da15f228
> prerequisite-patch-id: 7699990bf345e9551251211ee798f3b93c257d24
> prerequisite-patch-id: 168172f0fec6fcf334cd6cc600749afd57ade8a1
> prerequisite-patch-id: 96a01bb6af22da23b4e7acb5897117db959e8a08
> prerequisite-patch-id: 82222b0c579003169bcc1f1d84bb34956655cfb0
> prerequisite-patch-id: 075db535154891b70ed4e659588e9294e06a5f38
> prerequisite-patch-id: 83c646c3aa4ef3578e0fcd86ffc395b51cc47763
> prerequisite-patch-id: 4b8d2995e96ae290599d752cf1c1d2537e47bfab
> prerequisite-patch-id: 5915e9d3dd78f832ab0017c81df770f443b32169
> prerequisite-patch-id: dba378294fcb0aa72ee6bbace049c3ecc2b97bcb
> prerequisite-patch-id: 712d0c765bdecbc60d72e62b10b329b525dfd16f
> prerequisite-patch-id: 85e037f468c34b7a904f926e0cc555d7863c2cc7
> prerequisite-patch-id: 4e475984306a28ba367a171a6450bcfb07d0eb3c
> prerequisite-patch-id: 40b6db6c8d4e5bcaab64ce1afd53a79586f3df0b
> prerequisite-patch-id: fd4da5ff37eba383f7de7dd219cfcb297155b654
> prerequisite-patch-id: b076059966b15d0bf1c8282f48502299987b628a
> prerequisite-patch-id: 6b4540ea605fc3feb59128abee429c870b506da5
> prerequisite-patch-id: b98d4896c4dbc26adb898815c3e10722796bc9ca
> prerequisite-patch-id: f82322e104e3194c77b66a2ab2e465dd2a162d65
> prerequisite-patch-id: 3c719132de0cc72d3e548b5f3623cccb28a74ab5
> prerequisite-patch-id: d23e9aef7476d7f0976a9d8919c8474a5f91c20c
> prerequisite-patch-id: e725b9cf221da371b19d5aef5d1b4a5e813a19ee
> prerequisite-patch-id: 932d88887d4028aee27a809eeb89d1dc7835acc8
> prerequisite-patch-id: 48554c57f1d06553af3ded861ebf9b88ee6b03c4
> prerequisite-patch-id: c26315d8be3a746b04dea921d41c903d054b774f
> prerequisite-patch-id: fc598d2febb873efbaf53d4abfba4fe9c27aee52
> prerequisite-patch-id: 77b58bda743a1fde9b0b040f7832ec3d5853ea75
> prerequisite-patch-id: a64b4850058cd9189ccca081847f8908dbf91ae3
> prerequisite-patch-id: 73a6bb8ed4e20eb90127f1c2e6361d52ae17791c
Dang, I applied that patch on one of my working branches based on next.
These other patches are orthogonal to this patch, which applies just
fine to next/master.
Best regards and sorry for the mess,
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Andy,
On Fri, Apr 12, 2024 at 07:51:48PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 12, 2024 at 06:44:05PM +0200, Uwe Kleine-K?nig wrote:
> > 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.
>
> ...
>
> BUILD_BUG_ON(err == -ENOMEM);
>
> Done!
Well no, that doesn't do the trick. Consider for example device_add().
That function can return (at least) -EINVAL and -ENOMEM. To properly
ensure that the error handling is silent with the current
dev_err_probe(), we'd need to do:
ret = device_add(...);
if (ret) {
if (ret != -ENOMEM)
return dev_err_probe(...);
else
return ret;
}
With my suggested patch this can be reduced to:
ret = device_add(...);
if (ret)
return dev_err_probe(...);
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Fri, Apr 12, 2024 at 07:03:01PM +0200, Uwe Kleine-K?nig wrote:
> On Fri, Apr 12, 2024 at 07:51:48PM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 12, 2024 at 06:44:05PM +0200, Uwe Kleine-K?nig wrote:
> > > 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.
> > ...
> >
> > BUILD_BUG_ON(err == -ENOMEM);
> >
> > Done!
>
> Well no, that doesn't do the trick. Consider for example device_add().
> That function can return (at least) -EINVAL and -ENOMEM. To properly
> ensure that the error handling is silent with the current
> dev_err_probe(), we'd need to do:
>
> ret = device_add(...);
> if (ret) {
> if (ret != -ENOMEM)
> return dev_err_probe(...);
> else
> return ret;
> }
>
> With my suggested patch this can be reduced to:
>
> ret = device_add(...);
> if (ret)
> return dev_err_probe(...);
Fair enough, but these two should be combined.
Mine is for the rejecting a dead code on the phase of the submission.
--
With Best Regards,
Andy Shevchenko
On Fri, Apr 12, 2024 at 07:47:43PM +0200, Uwe Kleine-K?nig wrote:
> On Fri, Apr 12, 2024 at 08:07:29PM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 12, 2024 at 07:03:01PM +0200, Uwe Kleine-K?nig wrote:
> > > On Fri, Apr 12, 2024 at 07:51:48PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Apr 12, 2024 at 06:44:05PM +0200, Uwe Kleine-K?nig wrote:
> > > > > 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.
..
> > > > BUILD_BUG_ON(err == -ENOMEM);
> > > >
> > > > Done!
> > >
> > > Well no, that doesn't do the trick. Consider for example device_add().
> > > That function can return (at least) -EINVAL and -ENOMEM. To properly
> > > ensure that the error handling is silent with the current
> > > dev_err_probe(), we'd need to do:
> > >
> > > ret = device_add(...);
> > > if (ret) {
> > > if (ret != -ENOMEM)
> > > return dev_err_probe(...);
> > > else
> > > return ret;
> > > }
> > >
> > > With my suggested patch this can be reduced to:
> > >
> > > ret = device_add(...);
> > > if (ret)
> > > return dev_err_probe(...);
> >
> > Fair enough, but these two should be combined.
> > Mine is for the rejecting a dead code on the phase of the submission.
>
> So something like the following on top of my change (only compile
> tested):
Hmm, but why macro? Shouldn't compiler be clever enough to see it even for
the exported function?
..
> I don't know if the kernel doc for dev_err_probe() should move to
> include/linux/dev_printk.h then?!
Not if you mark it for the __ variant.
But it has other kernel-doc formatting issues...
--
With Best Regards,
Andy Shevchenko
Hello,
On Fri, Apr 12, 2024 at 08:07:29PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 12, 2024 at 07:03:01PM +0200, Uwe Kleine-K?nig wrote:
> > On Fri, Apr 12, 2024 at 07:51:48PM +0300, Andy Shevchenko wrote:
> > > On Fri, Apr 12, 2024 at 06:44:05PM +0200, Uwe Kleine-K?nig wrote:
> > > > 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.
>
> > > ...
> > >
> > > BUILD_BUG_ON(err == -ENOMEM);
> > >
> > > Done!
> >
> > Well no, that doesn't do the trick. Consider for example device_add().
> > That function can return (at least) -EINVAL and -ENOMEM. To properly
> > ensure that the error handling is silent with the current
> > dev_err_probe(), we'd need to do:
> >
> > ret = device_add(...);
> > if (ret) {
> > if (ret != -ENOMEM)
> > return dev_err_probe(...);
> > else
> > return ret;
> > }
> >
> > With my suggested patch this can be reduced to:
> >
> > ret = device_add(...);
> > if (ret)
> > return dev_err_probe(...);
>
> Fair enough, but these two should be combined.
> Mine is for the rejecting a dead code on the phase of the submission.
So something like the following on top of my change (only compile
tested):
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 0bc04dcf31d5..02b6ec9a26c1 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -5033,7 +5033,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;
@@ -5064,7 +5064,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 6bfe70decc9f..a7573355ff90 100644
--- a/include/linux/dev_printk.h
+++ b/include/linux/dev_printk.h
@@ -14,6 +14,7 @@
#include <linux/compiler.h>
#include <linux/types.h>
#include <linux/ratelimit.h>
+#include <linux/build_bug.h>
#ifndef dev_fmt
#define dev_fmt(fmt) fmt
@@ -274,6 +275,7 @@ 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, ...) ( { BUILD_BUG_ON(__builtin_constant_p(err) && err == -ENOMEM); __dev_err_probe((dev), (err), __VA_ARGS__); })
#endif /* _DEVICE_PRINTK_H_ */
I don't know if the kernel doc for dev_err_probe() should move to
include/linux/dev_printk.h then?!
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello,
On Fri, Apr 12, 2024 at 08:53:03PM +0300, Andy Shevchenko wrote:
> Hmm, but why macro? Shouldn't compiler be clever enough to see it even for
> the exported function?
If it's in the .c file only, the compiler doesn't know about it when
compiling the dwc pwm driver ...
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |