| Message ID | 20200414222713.32660-1-digetx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] iio: magnetometer: ak8974: Silence deferred-probe error | expand |
Hi Dmitry, thanks for your patch! On Wed, Apr 15, 2020 at 12:27 AM Dmitry Osipenko <digetx@gmail.com> wrote: > It's not uncommon that voltage regulator becomes available later during > kernel's boot process, in this case there is no need to print a noisy > error message. This patch moves the message about unavailable regulator > to the debug level in a case of the deferred-probe error and also amends > the message with error code. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/iio/magnetometer/ak8974.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c > index d32996702110..cc3861f97d42 100644 > --- a/drivers/iio/magnetometer/ak8974.c > +++ b/drivers/iio/magnetometer/ak8974.c > @@ -718,6 +718,7 @@ static const struct regmap_config ak8974_regmap_config = { > static int ak8974_probe(struct i2c_client *i2c, > const struct i2c_device_id *id) > { > + const char *level = KERN_ERR; > struct iio_dev *indio_dev; > struct ak8974 *ak8974; > unsigned long irq_trig; > @@ -746,7 +747,11 @@ static int ak8974_probe(struct i2c_client *i2c, > ARRAY_SIZE(ak8974->regs), > ak8974->regs); > if (ret < 0) { > - dev_err(&i2c->dev, "cannot get regulators\n"); > + if (ret == -EPROBE_DEFER) > + level = KERN_DEBUG; > + > + dev_printk(level, &i2c->dev, "cannot get regulators: %d\n", This misses some important aspects of dev_dbg(), notably this: #if defined(CONFIG_DYNAMIC_DEBUG) #define dev_dbg(dev, fmt, ...) \ dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) #elif defined(DEBUG) #define dev_dbg(dev, fmt, ...) \ dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__) #else #define dev_dbg(dev, fmt, ...) \ ({ \ if (0) \ dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \ }) #endif If DEBUG is not defined the entire dev_dbg() message is enclodes in if (0) and compiled out of the kernel, saving space. The above does not fulfil that. Yours, Linus Walleij
16.04.2020 14:33, Linus Walleij пишет: > Hi Dmitry, > > thanks for your patch! > > On Wed, Apr 15, 2020 at 12:27 AM Dmitry Osipenko <digetx@gmail.com> wrote: > >> It's not uncommon that voltage regulator becomes available later during >> kernel's boot process, in this case there is no need to print a noisy >> error message. This patch moves the message about unavailable regulator >> to the debug level in a case of the deferred-probe error and also amends >> the message with error code. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/iio/magnetometer/ak8974.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c >> index d32996702110..cc3861f97d42 100644 >> --- a/drivers/iio/magnetometer/ak8974.c >> +++ b/drivers/iio/magnetometer/ak8974.c >> @@ -718,6 +718,7 @@ static const struct regmap_config ak8974_regmap_config = { >> static int ak8974_probe(struct i2c_client *i2c, >> const struct i2c_device_id *id) >> { >> + const char *level = KERN_ERR; >> struct iio_dev *indio_dev; >> struct ak8974 *ak8974; >> unsigned long irq_trig; >> @@ -746,7 +747,11 @@ static int ak8974_probe(struct i2c_client *i2c, >> ARRAY_SIZE(ak8974->regs), >> ak8974->regs); >> if (ret < 0) { >> - dev_err(&i2c->dev, "cannot get regulators\n"); >> + if (ret == -EPROBE_DEFER) >> + level = KERN_DEBUG; >> + >> + dev_printk(level, &i2c->dev, "cannot get regulators: %d\n", > > This misses some important aspects of dev_dbg(), notably this: > > #if defined(CONFIG_DYNAMIC_DEBUG) > #define dev_dbg(dev, fmt, ...) \ > dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) > #elif defined(DEBUG) > #define dev_dbg(dev, fmt, ...) \ > dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__) > #else > #define dev_dbg(dev, fmt, ...) \ > ({ \ > if (0) \ > dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \ > }) > #endif > > If DEBUG is not defined the entire dev_dbg() message is enclodes in if (0) > and compiled out of the kernel, saving space. The above does not > fulfil that. Hello Linus, After some recent discussions in regards to the EPROBE_DEFER handling, Thierry Reding suggested the form which is used in my patch and we started to use it recently in the Tegra DRM driver [1]. The reason is that we don't want to miss any deferred-probe messages under any circumstances, for example like in a case of a disabled DYNAMIC_DEBUG. The debug messages are usually disabled in a release-build and when not a very experienced person hands you KMSG for diagnosing a problem, the KMSG is pretty much useless if error is hidden silently. By moving the message to a debug level, we reduce the noise in the KMSG because usually people look for a bold-red error messages. Secondly, we don't introduce an additional overhead to the kernel size since the same text is reused for all error conditions. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e32c8c2a5fbe1514e4ae9063a49e76ecf486ffb8 I can change this patch to something like this: if (ret == -EPROBE_DEFER) dev_dbg("deferring probe\n"); else dev_err("cannot..."); but in general this should be a less useful variant, don't you agree?
On Thu, Apr 16, 2020 at 4:45 PM Dmitry Osipenko <digetx@gmail.com> wrote: > 16.04.2020 14:33, Linus Walleij пишет: > > This misses some important aspects of dev_dbg(), notably this: > > > > #if defined(CONFIG_DYNAMIC_DEBUG) > > #define dev_dbg(dev, fmt, ...) \ > > dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) > > #elif defined(DEBUG) > > #define dev_dbg(dev, fmt, ...) \ > > dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__) > > #else > > #define dev_dbg(dev, fmt, ...) \ > > ({ \ > > if (0) \ > > dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \ > > }) > > #endif > > > > If DEBUG is not defined the entire dev_dbg() message is enclodes in if (0) > > and compiled out of the kernel, saving space. The above does not > > fulfil that. > > Hello Linus, > > After some recent discussions in regards to the EPROBE_DEFER handling, > Thierry Reding suggested the form which is used in my patch and we > started to use it recently in the Tegra DRM driver [1]. The reason is > that we don't want to miss any deferred-probe messages under any > circumstances, for example like in a case of a disabled DYNAMIC_DEBUG. I have a hard time to accept this reasoning. Who doesn't feel that way about their subsystem? If you don't want to miss the message under any circumstances then use dev_info(). Don't override the default behaviour of dev_dbg(). > The debug messages are usually disabled in a release-build and when not > a very experienced person hands you KMSG for diagnosing a problem, the > KMSG is pretty much useless if error is hidden silently. So use dev_info(). > By moving the message to a debug level, we reduce the noise in the KMSG > because usually people look for a bold-red error messages. Secondly, we > don't introduce an additional overhead to the kernel size since the same > text is reused for all error conditions. dev_info() is not supposed to be an error message, it is supposed to be information, so use that. Yours, Linus Walleij
16.04.2020 19:51, Linus Walleij пишет: > On Thu, Apr 16, 2020 at 4:45 PM Dmitry Osipenko <digetx@gmail.com> wrote: >> 16.04.2020 14:33, Linus Walleij пишет: > >>> This misses some important aspects of dev_dbg(), notably this: >>> >>> #if defined(CONFIG_DYNAMIC_DEBUG) >>> #define dev_dbg(dev, fmt, ...) \ >>> dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) >>> #elif defined(DEBUG) >>> #define dev_dbg(dev, fmt, ...) \ >>> dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__) >>> #else >>> #define dev_dbg(dev, fmt, ...) \ >>> ({ \ >>> if (0) \ >>> dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \ >>> }) >>> #endif >>> >>> If DEBUG is not defined the entire dev_dbg() message is enclodes in if (0) >>> and compiled out of the kernel, saving space. The above does not >>> fulfil that. >> >> Hello Linus, >> >> After some recent discussions in regards to the EPROBE_DEFER handling, >> Thierry Reding suggested the form which is used in my patch and we >> started to use it recently in the Tegra DRM driver [1]. The reason is >> that we don't want to miss any deferred-probe messages under any >> circumstances, for example like in a case of a disabled DYNAMIC_DEBUG. > > I have a hard time to accept this reasoning. > > Who doesn't feel that way about their subsystem? If you don't want > to miss the message under any circumstances then use dev_info(). > Don't override the default behaviour of dev_dbg(). > >> The debug messages are usually disabled in a release-build and when not >> a very experienced person hands you KMSG for diagnosing a problem, the >> KMSG is pretty much useless if error is hidden silently. > > So use dev_info(). > >> By moving the message to a debug level, we reduce the noise in the KMSG >> because usually people look for a bold-red error messages. Secondly, we >> don't introduce an additional overhead to the kernel size since the same >> text is reused for all error conditions. > > dev_info() is not supposed to be an error message, it is supposed to > be information, so use that. Okay, I'll make a v2. Thank you for the review.
On Thu, 16 Apr 2020 20:35:56 +0300 Dmitry Osipenko <digetx@gmail.com> wrote: > 16.04.2020 19:51, Linus Walleij пишет: > > On Thu, Apr 16, 2020 at 4:45 PM Dmitry Osipenko <digetx@gmail.com> wrote: > >> 16.04.2020 14:33, Linus Walleij пишет: > > > >>> This misses some important aspects of dev_dbg(), notably this: > >>> > >>> #if defined(CONFIG_DYNAMIC_DEBUG) > >>> #define dev_dbg(dev, fmt, ...) \ > >>> dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) > >>> #elif defined(DEBUG) > >>> #define dev_dbg(dev, fmt, ...) \ > >>> dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__) > >>> #else > >>> #define dev_dbg(dev, fmt, ...) \ > >>> ({ \ > >>> if (0) \ > >>> dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \ > >>> }) > >>> #endif > >>> > >>> If DEBUG is not defined the entire dev_dbg() message is enclodes in if (0) > >>> and compiled out of the kernel, saving space. The above does not > >>> fulfil that. > >> > >> Hello Linus, > >> > >> After some recent discussions in regards to the EPROBE_DEFER handling, > >> Thierry Reding suggested the form which is used in my patch and we > >> started to use it recently in the Tegra DRM driver [1]. The reason is > >> that we don't want to miss any deferred-probe messages under any > >> circumstances, for example like in a case of a disabled DYNAMIC_DEBUG. > > > > I have a hard time to accept this reasoning. > > > > Who doesn't feel that way about their subsystem? If you don't want > > to miss the message under any circumstances then use dev_info(). > > Don't override the default behaviour of dev_dbg(). > > > >> The debug messages are usually disabled in a release-build and when not > >> a very experienced person hands you KMSG for diagnosing a problem, the > >> KMSG is pretty much useless if error is hidden silently. > > > > So use dev_info(). > > > >> By moving the message to a debug level, we reduce the noise in the KMSG > >> because usually people look for a bold-red error messages. Secondly, we > >> don't introduce an additional overhead to the kernel size since the same > >> text is reused for all error conditions. > > > > dev_info() is not supposed to be an error message, it is supposed to > > be information, so use that. > > Okay, I'll make a v2. Thank you for the review. Ah I commented on this in v2 - now I see why you did it :) Nope to dev_info. That will often spam normal logs and as Andy pointed out for v2 that can be dozens of entries on a sophisticated board. Much better to stick to dev_dbg but I'd like to see it done explicitly in the form you mention with the if / else Thanks, Jonathan
18.04.2020 17:37, Jonathan Cameron пишет: > On Thu, 16 Apr 2020 20:35:56 +0300 > Dmitry Osipenko <digetx@gmail.com> wrote: > >> 16.04.2020 19:51, Linus Walleij пишет: >>> On Thu, Apr 16, 2020 at 4:45 PM Dmitry Osipenko <digetx@gmail.com> wrote: >>>> 16.04.2020 14:33, Linus Walleij пишет: >>> >>>>> This misses some important aspects of dev_dbg(), notably this: >>>>> >>>>> #if defined(CONFIG_DYNAMIC_DEBUG) >>>>> #define dev_dbg(dev, fmt, ...) \ >>>>> dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__) >>>>> #elif defined(DEBUG) >>>>> #define dev_dbg(dev, fmt, ...) \ >>>>> dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__) >>>>> #else >>>>> #define dev_dbg(dev, fmt, ...) \ >>>>> ({ \ >>>>> if (0) \ >>>>> dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \ >>>>> }) >>>>> #endif >>>>> >>>>> If DEBUG is not defined the entire dev_dbg() message is enclodes in if (0) >>>>> and compiled out of the kernel, saving space. The above does not >>>>> fulfil that. >>>> >>>> Hello Linus, >>>> >>>> After some recent discussions in regards to the EPROBE_DEFER handling, >>>> Thierry Reding suggested the form which is used in my patch and we >>>> started to use it recently in the Tegra DRM driver [1]. The reason is >>>> that we don't want to miss any deferred-probe messages under any >>>> circumstances, for example like in a case of a disabled DYNAMIC_DEBUG. >>> >>> I have a hard time to accept this reasoning. >>> >>> Who doesn't feel that way about their subsystem? If you don't want >>> to miss the message under any circumstances then use dev_info(). >>> Don't override the default behaviour of dev_dbg(). >>> >>>> The debug messages are usually disabled in a release-build and when not >>>> a very experienced person hands you KMSG for diagnosing a problem, the >>>> KMSG is pretty much useless if error is hidden silently. >>> >>> So use dev_info(). >>> >>>> By moving the message to a debug level, we reduce the noise in the KMSG >>>> because usually people look for a bold-red error messages. Secondly, we >>>> don't introduce an additional overhead to the kernel size since the same >>>> text is reused for all error conditions. >>> >>> dev_info() is not supposed to be an error message, it is supposed to >>> be information, so use that. >> >> Okay, I'll make a v2. Thank you for the review. > > Ah I commented on this in v2 - now I see why you did it :) > Nope to dev_info. That will often spam normal logs and as Andy pointed > out for v2 that can be dozens of entries on a sophisticated board. Much > better to stick to dev_dbg but I'd like to see it done explicitly in the > form you mention with the if / else Alright, I'll make a v3.
diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c index d32996702110..cc3861f97d42 100644 --- a/drivers/iio/magnetometer/ak8974.c +++ b/drivers/iio/magnetometer/ak8974.c @@ -718,6 +718,7 @@ static const struct regmap_config ak8974_regmap_config = { static int ak8974_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { + const char *level = KERN_ERR; struct iio_dev *indio_dev; struct ak8974 *ak8974; unsigned long irq_trig; @@ -746,7 +747,11 @@ static int ak8974_probe(struct i2c_client *i2c, ARRAY_SIZE(ak8974->regs), ak8974->regs); if (ret < 0) { - dev_err(&i2c->dev, "cannot get regulators\n"); + if (ret == -EPROBE_DEFER) + level = KERN_DEBUG; + + dev_printk(level, &i2c->dev, "cannot get regulators: %d\n", + ret); return ret; }
It's not uncommon that voltage regulator becomes available later during kernel's boot process, in this case there is no need to print a noisy error message. This patch moves the message about unavailable regulator to the debug level in a case of the deferred-probe error and also amends the message with error code. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/iio/magnetometer/ak8974.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)