| Message ID | 20190808123655.31520-1-stefan@agner.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: soc-core: remove error due to probe deferral | expand |
On Thu, Aug 08, 2019 at 02:36:55PM +0200, Stefan Agner wrote: > From: Stefan Agner <stefan.agner@toradex.com> > > Deferred probes shouldn't cause error messages in the boot log. Avoid > printing with dev_err() in case EPROBE_DEFER is the return value. No, they absolutely should tell the user why they are deferring so the user has some information to go on when they're trying to figure out why their device isn't instantiating.
On 2019-08-08 14:44, Mark Brown wrote: > On Thu, Aug 08, 2019 at 02:36:55PM +0200, Stefan Agner wrote: >> From: Stefan Agner <stefan.agner@toradex.com> >> >> Deferred probes shouldn't cause error messages in the boot log. Avoid >> printing with dev_err() in case EPROBE_DEFER is the return value. > > No, they absolutely should tell the user why they are deferring so the > user has some information to go on when they're trying to figure out why > their device isn't instantiating. Hm, I see, if the driver defers and does not manage in the end, then the messages are indeed helpful. But can we lower severity, e.g. to dev_info? In my case it succeeds in the end, just defers about 6 times. I have 3 links which then leads to 18 error messages which confuse users... From what I can see soc_init_dai_link() would print dev_err in case there is an actual error. -- Stefan
On Thu, 08 Aug 2019 14:44:37 +0200, Mark Brown wrote: > > On Thu, Aug 08, 2019 at 02:36:55PM +0200, Stefan Agner wrote: > > From: Stefan Agner <stefan.agner@toradex.com> > > > > Deferred probes shouldn't cause error messages in the boot log. Avoid > > printing with dev_err() in case EPROBE_DEFER is the return value. > > No, they absolutely should tell the user why they are deferring so the > user has some information to go on when they're trying to figure out why > their device isn't instantiating. But it's no real error that *must* be printed on the console, either. Maybe downgrading the printk level? thanks, Takashi
On Thu, Aug 08, 2019 at 03:00:06PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > No, they absolutely should tell the user why they are deferring so the > > user has some information to go on when they're trying to figure out why > > their device isn't instantiating. > But it's no real error that *must* be printed on the console, either. > Maybe downgrading the printk level? Yes, downgrading can be OK though it does bloat the code.
On Thu, 08 Aug 2019 15:02:17 +0200, Mark Brown wrote: > > On Thu, Aug 08, 2019 at 03:00:06PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > No, they absolutely should tell the user why they are deferring so the > > > user has some information to go on when they're trying to figure out why > > > their device isn't instantiating. > > > But it's no real error that *must* be printed on the console, either. > > Maybe downgrading the printk level? > > Yes, downgrading can be OK though it does bloat the code. I guess we can use dev_printk() with the conditional level choice. Takashi
On 2019-08-08 15:14, Takashi Iwai wrote: > On Thu, 08 Aug 2019 15:02:17 +0200, > Mark Brown wrote: >> >> On Thu, Aug 08, 2019 at 03:00:06PM +0200, Takashi Iwai wrote: >> > Mark Brown wrote: >> >> > > No, they absolutely should tell the user why they are deferring so the >> > > user has some information to go on when they're trying to figure out why >> > > their device isn't instantiating. >> >> > But it's no real error that *must* be printed on the console, either. >> > Maybe downgrading the printk level? >> >> Yes, downgrading can be OK though it does bloat the code. > > I guess we can use dev_printk() with the conditional level choice. > How about use dev_info always? We get a dev_err message from soc_init_dai_link in error cases... ret = soc_init_dai_link(card, dai_link); if (ret && ret != -EPROBE_DEFER) { dev_info(card->dev, "ASoC: failed to init link %s: %d\n", dai_link->name, ret); } if (ret) { soc_cleanup_platform(card); mutex_unlock(&client_mutex); return ret; } -- Stefan
On Thu, Aug 08, 2019 at 03:16:53PM +0200, Stefan Agner wrote: > On 2019-08-08 15:14, Takashi Iwai wrote: > > Mark Brown wrote: > > I guess we can use dev_printk() with the conditional level choice. > How about use dev_info always? We get a dev_err message from > soc_init_dai_link in error cases... > ret = soc_init_dai_link(card, dai_link); > if (ret && ret != -EPROBE_DEFER) { > dev_info(card->dev, "ASoC: failed to init link %s: %d\n", > dai_link->name, ret); > } Well, if there's adequate error reporting in init_dai_link() it's a bit different - we can just remove the print entirely regardless of what the return code is. The point is to ensure that we don't just silently fail. Unfortunately there's no prints in the probe deferral case there so they need adding, that'll actually improve things though since we can make it print the name of the thing it's mising which will be useful to people trying to figure out what's going on (we used to do that but it got lost in reshufflings).
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index fd6eaae6c0ed..98e1e80b5493 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1985,9 +1985,11 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) mutex_lock(&client_mutex); for_each_card_prelinks(card, i, dai_link) { ret = soc_init_dai_link(card, dai_link); - if (ret) { + if (ret && ret != -EPROBE_DEFER) { dev_err(card->dev, "ASoC: failed to init link %s: %d\n", dai_link->name, ret); + } + if (ret) { mutex_unlock(&client_mutex); return ret; }