[PATCH 1/1] ASoC: core: Do not print an actual error when deferring probe

[PATCH 1/1] ASoC: core: Do not print an actual error when deferring probe

[Alexander Stein]

In case there is no DAI (yet), do not print an error, this might happen
a lot of times. Print a notice instead.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 sound/soc/soc-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index d2e62b15..352f7c6 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1001,7 +1001,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
 	cpu_dai_component.dai_name = dai_link->cpu_dai_name;
 	rtd->cpu_dai = snd_soc_find_dai(&cpu_dai_component);
 	if (!rtd->cpu_dai) {
-		dev_err(card->dev, "ASoC: CPU DAI %s not registered\n",
+		dev_notice(card->dev, "ASoC: CPU DAI %s not registered\n",
 			dai_link->cpu_dai_name);
 		goto _err_defer;
 	}
@@ -1013,7 +1013,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
 	for (i = 0; i < rtd->num_codecs; i++) {
 		codec_dais[i] = snd_soc_find_dai(&codecs[i]);
 		if (!codec_dais[i]) {
-			dev_err(card->dev, "ASoC: CODEC DAI %s not registered\n",
+			dev_notice(card->dev, "ASoC: CODEC DAI %s not registered\n",
 				codecs[i].dai_name);
 			goto _err_defer;
 		}
@@ -1042,7 +1042,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
 		rtd->platform = platform;
 	}
 	if (!rtd->platform) {
-		dev_err(card->dev, "ASoC: platform %s not registered\n",
+		dev_notice(card->dev, "ASoC: platform %s not registered\n",
 			dai_link->platform_name);
 		return -EPROBE_DEFER;
 	}
-- 
2.7.3

Re: [PATCH 1/1] ASoC: core: Do not print an actual error when deferring probe

[Ricard Wanderlof]

On Wed, 18 May 2016, Alexander Stein wrote:

> In case there is no DAI (yet), do not print an error, this might happen
> a lot of times. Print a notice instead.
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
>  sound/soc/soc-core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index d2e62b15..352f7c6 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -1001,7 +1001,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
>  	cpu_dai_component.dai_name = dai_link->cpu_dai_name;
>  	rtd->cpu_dai = snd_soc_find_dai(&cpu_dai_component);
>  	if (!rtd->cpu_dai) {
> -		dev_err(card->dev, "ASoC: CPU DAI %s not registered\n",
> +		dev_notice(card->dev, "ASoC: CPU DAI %s not registered\n",
>  			dai_link->cpu_dai_name);
>  		goto _err_defer;
>  	}
> @@ -1013,7 +1013,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
>  	for (i = 0; i < rtd->num_codecs; i++) {
>  		codec_dais[i] = snd_soc_find_dai(&codecs[i]);
>  		if (!codec_dais[i]) {
> -			dev_err(card->dev, "ASoC: CODEC DAI %s not registered\n",
> +			dev_notice(card->dev, "ASoC: CODEC DAI %s not registered\n",
>  				codecs[i].dai_name);
>  			goto _err_defer;
>  		}
> @@ -1042,7 +1042,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
>  		rtd->platform = platform;
>  	}
>  	if (!rtd->platform) {
> -		dev_err(card->dev, "ASoC: platform %s not registered\n",
> +		dev_notice(card->dev, "ASoC: platform %s not registered\n",
>  			dai_link->platform_name);
>  		return -EPROBE_DEFER;
>  	}
> -- 
> 2.7.3

Good change, but there is one more case in soc-core.c: when 
soc_bind_aux_dev() returns -EPROBE_DEFER.

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [PATCH 1/1] ASoC: core: Do not print an actual error when deferring probe

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 235 bytes --]

On Wed, May 18, 2016 at 03:41:29PM +0200, Alexander Stein wrote:
> In case there is no DAI (yet), do not print an error, this might happen
> a lot of times. Print a notice instead.

This is changing *all* error reports which isn't OK.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/1] ASoC: core: Do not print an actual error when deferring probe

[Alexander Stein]

On Wednesday 18 May 2016 16:08:12, Mark Brown wrote:
> On Wed, May 18, 2016 at 03:41:29PM +0200, Alexander Stein wrote:
> > In case there is no DAI (yet), do not print an error, this might happen
> > a lot of times. Print a notice instead.
> 
> This is changing *all* error reports which isn't OK.

Well, it's debatable if using dev_err() and then returning EPROBE_DEFER is 
correct either. IMHO if there is an actual error, an error code has to be 
returned and not "well, can't continue now, probe me again later".
At first glance those EPROBE_DEFER are correct as other dependent drivers may 
be probed later, so printing an error in that case seems wrong to me.

Best regards,
Alexander

Re: [PATCH 1/1] ASoC: core: Do not print an actual error when deferring probe

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 839 bytes --]

On Wed, May 18, 2016 at 05:18:41PM +0200, Alexander Stein wrote:

> Well, it's debatable if using dev_err() and then returning EPROBE_DEFER is 
> correct either. IMHO if there is an actual error, an error code has to be 
> returned and not "well, can't continue now, probe me again later".
> At first glance those EPROBE_DEFER are correct as other dependent drivers may 
> be probed later, so printing an error in that case seems wrong to me.

Half the point with probe deferral is that it's transparent to anything
that isn't actually resolving a resource - if we have to add custom code
in to everywhere were we need to print an error in a probe path that's
not great either.  We also don't want to silently ignore probe deferral
all the time since then it becomes very hard to diagnose why something
is not instantiating.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/1] ASoC: core: Do not print an actual error when deferring probe

[Alexander Stein]

On Wednesday 18 May 2016 16:28:20, Mark Brown wrote:
> On Wed, May 18, 2016 at 05:18:41PM +0200, Alexander Stein wrote:
> > Well, it's debatable if using dev_err() and then returning EPROBE_DEFER is
> > correct either. IMHO if there is an actual error, an error code has to be
> > returned and not "well, can't continue now, probe me again later".
> > At first glance those EPROBE_DEFER are correct as other dependent drivers
> > may be probed later, so printing an error in that case seems wrong to me.
> Half the point with probe deferral is that it's transparent to anything
> that isn't actually resolving a resource - if we have to add custom code
> in to everywhere were we need to print an error in a probe path that's
> not great either.  We also don't want to silently ignore probe deferral
> all the time since then it becomes very hard to diagnose why something
> is not instantiating.

I came up with this because on one of our boards there are lots of probe 
deferals, mainly caused by i2c gpio expanders and audio. Anyway, in order to 
reduce boot time I silence the boot console using 'quiet' on command line. 
Nevertheless It's quite useless to have several error (!) messages during boot 
just caused by probe deferral. Here is one bootup. It's from an old vendor 
kernel, but you get the idea.

> Starting kernel ...
>
> [    5.490931] i2c i2c-0: of_i2c: modalias failure on
> /soc/i2c@2180000/cyusb3314@60 init started: BusyBox v1.22.0 (2015-10-13
> 14:03:10 CEST)
> starting pid 122, tty '/dev/console': '/etc/init.d/rcS'
> [    6.533391] vf610-sgtl5000 sound.9: ASoC: CPU DAI (null) not registered
> [    6.571234] vf610-sgtl5000 sound.9: ASoC: CPU DAI (null) not registered
> [    6.586243] vf610-sgtl5000 sound.9: ASoC: CPU DAI (null) not registered
> [    6.598094] vf610-sgtl5000 sound.9: ASoC: CPU DAI (null) not registered
> starting pid 238, tty '/dev/console': '/sbin/getty -L 115200 ttyS0 vt102'
> 
> ecucore login:

My first idea was to use dev_dbg() but that actualy remove that message at 
all, unless dynamic debug is used, so I went with dev_notice. It's still 
there, but does not clobber quiet boot log, nor some sophisticated kernel log 
parser as journalctl which prints errors in red color.

Best regards,
Alexander

Re: [PATCH 1/1] ASoC: core: Do not print an actual error when deferring probe

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 562 bytes --]

On Wed, May 18, 2016 at 05:50:50PM +0200, Alexander Stein wrote:

> My first idea was to use dev_dbg() but that actualy remove that message at 
> all, unless dynamic debug is used, so I went with dev_notice. It's still 
> there, but does not clobber quiet boot log, nor some sophisticated kernel log 
> parser as journalctl which prints errors in red color.

It's just shifting the problem around...  it sounds like for your use
case suppressing the messages until we finish kernel boot would deal
with most of the issue in a far more general fashion.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/1] ASoC: core: Do not print an actual error when deferring probe

[Takashi Iwai]

On Wed, 18 May 2016 18:21:54 +0200,
Mark Brown wrote:
> 
> On Wed, May 18, 2016 at 05:50:50PM +0200, Alexander Stein wrote:
> 
> > My first idea was to use dev_dbg() but that actualy remove that message at 
> > all, unless dynamic debug is used, so I went with dev_notice. It's still 
> > there, but does not clobber quiet boot log, nor some sophisticated kernel log 
> > parser as journalctl which prints errors in red color.
> 
> It's just shifting the problem around...  it sounds like for your use
> case suppressing the messages until we finish kernel boot would deal
> with most of the issue in a far more general fashion.

It comes to the question whether this message must be shown verbosely
as an error at all.  EPROBE_DEFER is usually a mechanism for the
delayed probe, and it doesn't indicate an error per se.  dev_err() is,
OTOH, for real errors that have to be notified to user inevitably.
That's why "quiet" boot option still shows it.


Takashi

Re: [PATCH 1/1] ASoC: core: Do not print an actual error when deferring probe

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1028 bytes --]

On Wed, May 18, 2016 at 06:57:50PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > It's just shifting the problem around...  it sounds like for your use
> > case suppressing the messages until we finish kernel boot would deal
> > with most of the issue in a far more general fashion.

> It comes to the question whether this message must be shown verbosely
> as an error at all.  EPROBE_DEFER is usually a mechanism for the
> delayed probe, and it doesn't indicate an error per se.  dev_err() is,
> OTOH, for real errors that have to be notified to user inevitably.
> That's why "quiet" boot option still shows it.

We can't tell the difference between something that's delayed and will
come up later and something that's just never going to work - missing
or misidentified components has always been a common source of errors
in ASoC device bringup.  There's also just the fact that the noise from
deferred probe is not an ASoC specific thing, we need to tackle this at
a system level rather than hacking individual cases.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/1] ASoC: core: Do not print an actual error when deferring probe

[Takashi Iwai]

On Wed, 18 May 2016 19:20:19 +0200,
Mark Brown wrote:
> 
> On Wed, May 18, 2016 at 06:57:50PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > It's just shifting the problem around...  it sounds like for your use
> > > case suppressing the messages until we finish kernel boot would deal
> > > with most of the issue in a far more general fashion.
> 
> > It comes to the question whether this message must be shown verbosely
> > as an error at all.  EPROBE_DEFER is usually a mechanism for the
> > delayed probe, and it doesn't indicate an error per se.  dev_err() is,
> > OTOH, for real errors that have to be notified to user inevitably.
> > That's why "quiet" boot option still shows it.
> 
> We can't tell the difference between something that's delayed and will
> come up later and something that's just never going to work - missing
> or misidentified components has always been a common source of errors
> in ASoC device bringup.

Yes, but it still means that most of cases are false-positive to show
as "error" message.

> There's also just the fact that the noise from
> deferred probe is not an ASoC specific thing, we need to tackle this at
> a system level rather than hacking individual cases.

Such other noises are usually now shown as errors with dev_err() but
with dev_*() with a lower level.

I guess the patch subject and description are misleading.  This
doesn't suppress the messages to be printed.  In usual boots without
quiet boot option, there will be no visible change with this patch,
the messages are still shown.  It's not shown, however, when booted
with quiet boot option.  That's the whole point.

dev_notice() lowers the log level to somewhat between info and
warning, and it looks like a sensible choice to me.


thanks,

Takashi

Re: [PATCH 1/1] ASoC: core: Do not print an actual error when deferring probe

[Alexander Stein]

On Wednesday 18 May 2016 21:33:01, Takashi Iwai wrote:
> On Wed, 18 May 2016 19:20:19 +0200,
> 
> Mark Brown wrote:
> > On Wed, May 18, 2016 at 06:57:50PM +0200, Takashi Iwai wrote:
> > > Mark Brown wrote:
> > > > It's just shifting the problem around...  it sounds like for your use
> > > > case suppressing the messages until we finish kernel boot would deal
> > > > with most of the issue in a far more general fashion.
> > > 
> > > It comes to the question whether this message must be shown verbosely
> > > as an error at all.  EPROBE_DEFER is usually a mechanism for the
> > > delayed probe, and it doesn't indicate an error per se.  dev_err() is,
> > > OTOH, for real errors that have to be notified to user inevitably.
> > > That's why "quiet" boot option still shows it.
> > 
> > We can't tell the difference between something that's delayed and will
> > come up later and something that's just never going to work - missing
> > or misidentified components has always been a common source of errors
> > in ASoC device bringup.
> 
> Yes, but it still means that most of cases are false-positive to show
> as "error" message.

I think the same way. Of course it is an error if probing never finishes due 
to some missing configuration/dt entry, but that needs to be checked in dmesg 
anyway. dev_notice still allows that.

> > There's also just the fact that the noise from
> > deferred probe is not an ASoC specific thing, we need to tackle this at
> > a system level rather than hacking individual cases.
> 
> Such other noises are usually now shown as errors with dev_err() but
> with dev_*() with a lower level.
> 
> I guess the patch subject and description are misleading.  This
> doesn't suppress the messages to be printed.  In usual boots without
> quiet boot option, there will be no visible change with this patch,
> the messages are still shown.  It's not shown, however, when booted
> with quiet boot option.  That's the whole point.
> 
> dev_notice() lowers the log level to somewhat between info and
> warning, and it looks like a sensible choice to me.

You want me to change the subject to be more clear?

Regards,
Alexander

Re: [PATCH 1/1] ASoC: core: Do not print an actual error when deferring probe

[Takashi Iwai]

On Thu, 19 May 2016 08:28:02 +0200,
Alexander Stein wrote:
> 
> On Wednesday 18 May 2016 21:33:01, Takashi Iwai wrote:
> > On Wed, 18 May 2016 19:20:19 +0200,
> > 
> > Mark Brown wrote:
> > > On Wed, May 18, 2016 at 06:57:50PM +0200, Takashi Iwai wrote:
> > > > Mark Brown wrote:
> > > > > It's just shifting the problem around...  it sounds like for your use
> > > > > case suppressing the messages until we finish kernel boot would deal
> > > > > with most of the issue in a far more general fashion.
> > > > 
> > > > It comes to the question whether this message must be shown verbosely
> > > > as an error at all.  EPROBE_DEFER is usually a mechanism for the
> > > > delayed probe, and it doesn't indicate an error per se.  dev_err() is,
> > > > OTOH, for real errors that have to be notified to user inevitably.
> > > > That's why "quiet" boot option still shows it.
> > > 
> > > We can't tell the difference between something that's delayed and will
> > > come up later and something that's just never going to work - missing
> > > or misidentified components has always been a common source of errors
> > > in ASoC device bringup.
> > 
> > Yes, but it still means that most of cases are false-positive to show
> > as "error" message.
> 
> I think the same way. Of course it is an error if probing never finishes due 
> to some missing configuration/dt entry, but that needs to be checked in dmesg 
> anyway. dev_notice still allows that.
> 
> > > There's also just the fact that the noise from
> > > deferred probe is not an ASoC specific thing, we need to tackle this at
> > > a system level rather than hacking individual cases.
> > 
> > Such other noises are usually now shown as errors with dev_err() but
> > with dev_*() with a lower level.
> > 
> > I guess the patch subject and description are misleading.  This
> > doesn't suppress the messages to be printed.  In usual boots without
> > quiet boot option, there will be no visible change with this patch,
> > the messages are still shown.  It's not shown, however, when booted
> > with quiet boot option.  That's the whole point.
> > 
> > dev_notice() lowers the log level to somewhat between info and
> > warning, and it looks like a sensible choice to me.
> 
> You want me to change the subject to be more clear?

If the patch can go in, yes.  The changelog text should be also
clearer why it's needed and what it gives, too.


Takashi

Re: [PATCH 1/1] ASoC: core: Do not print an actual error when deferring probe

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 937 bytes --]

On Thu, May 19, 2016 at 08:53:31AM +0200, Takashi Iwai wrote:
> Alexander Stein wrote:
> > On Wednesday 18 May 2016 21:33:01, Takashi Iwai wrote:

> > > dev_notice() lowers the log level to somewhat between info and
> > > warning, and it looks like a sensible choice to me.

> > You want me to change the subject to be more clear?

> If the patch can go in, yes.  The changelog text should be also
> clearer why it's needed and what it gives, too.

There's also the problem with it just squashing all errors, not just
probe deferral.  I really would prefer to see at least some attempt to
push this into a central helper rather than just open coding hacks in
every single user though, that's the thing here.  We want a simple and
consistent way of handling this, going round and doing something custom
at every single caller is going to result in inconsistent behaviour and
means that it's harder to deploy and new and better solutions.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]