| Message ID | 1463578889-13709-1-git-send-email-alexander.stein@systec-electronic.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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.
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
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.
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
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.
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
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
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
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.
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; }
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(-)