[alsa-devel] [PATCH][RFC] ASoC: soc-core: WARN() is not related to component->driver->probe
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_probe_component() has WARN() under if (component->driver->probe), but, this WARN() check is not related to .probe callback. So, it should be called at (B) instead of (A). This patch moves it out of if().
if (component->driver->probe) { ret = component->driver->probe(component); ... (A) WARN(...) } (B) WARN(...)
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- Mark, Pierre-Louis, Vinod, Liam
I think this patch is correct, but I'm not sure. I'm happy someone can confirm it.
sound/soc/soc-core.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index cab30ae..7157d67 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1420,12 +1420,11 @@ static int soc_probe_component(struct snd_soc_card *card, "ASoC: failed to probe component %d\n", ret); goto err_probe; } - - WARN(dapm->idle_bias_off && - dapm->bias_level != SND_SOC_BIAS_OFF, - "codec %s can not start from non-off bias with idle_bias_off==1\n", - component->name); } + WARN(dapm->idle_bias_off && + dapm->bias_level != SND_SOC_BIAS_OFF, + "codec %s can not start from non-off bias with idle_bias_off==1\n", + component->name);
/* machine specific init */ if (component->init) {
On 5/17/19 1:06 AM, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_probe_component() has WARN() under if (component->driver->probe), but, this WARN() check is not related to .probe callback. So, it should be called at (B) instead of (A). This patch moves it out of if().
if (component->driver->probe) { ret = component->driver->probe(component); ... (A) WARN(...) } (B) WARN(...)
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Mark, Pierre-Louis, Vinod, Liam
I think this patch is correct, but I'm not sure. I'm happy someone can confirm it.
This WARN() was added in 2012 by ff541f4b2a75 ('ASoC: core: giving WARN when device starting from non-off bias with idle_bias_off')
The commit message hints at an intentional check
" Just found some cases that some codec drivers set the bias to _STANDBY and set idle_bias_off to 1 during probing. It will cause unpaired runtime_get_sync/put() issue. Also as Mark suggested, there is no reason to start from _STANDBY bias with idle_bias_off == 1.
So here giving one warning when detected (dapm.idle_bias_off == 1) and (dapm.bias_level != SND_SOC_BIAS_OFF) just after driver->probe(). "
My take is that unless we can prove this is incorrect we leave it as is.
sound/soc/soc-core.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index cab30ae..7157d67 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1420,12 +1420,11 @@ static int soc_probe_component(struct snd_soc_card *card, "ASoC: failed to probe component %d\n", ret); goto err_probe; }
WARN(dapm->idle_bias_off &&dapm->bias_level != SND_SOC_BIAS_OFF,"codec %s can not start from non-off bias with idle_bias_off==1\n",component->name);
WARN(dapm->idle_bias_off &&
dapm->bias_level != SND_SOC_BIAS_OFF,"codec %s can not start from non-off bias with idle_bias_off==1\n",component->name);/* machine specific init */ if (component->init) {
Hi Pierre-Louis
Thank you for your feedback
if (component->driver->probe) { ret = component->driver->probe(component); ... (A) WARN(...) } (B) WARN(...)
(snip)
This WARN() was added in 2012 by ff541f4b2a75 ('ASoC: core: giving WARN when device starting from non-off bias with idle_bias_off')
The commit message hints at an intentional check
" Just found some cases that some codec drivers set the bias to _STANDBY and set idle_bias_off to 1 during probing. It will cause unpaired runtime_get_sync/put() issue. Also as Mark suggested, there is no reason to start from _STANDBY bias with idle_bias_off == 1.
So here giving one warning when detected (dapm.idle_bias_off == 1) and (dapm.bias_level != SND_SOC_BIAS_OFF) just after driver->probe(). "
My take is that unless we can prove this is incorrect we leave it as is.
I think this commit is correct, thanks. But, then, it sounds we need to check it even though without .prove ?
Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Morimoto-san,
if (component->driver->probe) { ret = component->driver->probe(component); ... (A) WARN(...) } (B) WARN(...)
(snip)
This WARN() was added in 2012 by ff541f4b2a75 ('ASoC: core: giving WARN when device starting from non-off bias with idle_bias_off')
The commit message hints at an intentional check
" Just found some cases that some codec drivers set the bias to _STANDBY and set idle_bias_off to 1 during probing. It will cause unpaired runtime_get_sync/put() issue. Also as Mark suggested, there is no reason to start from _STANDBY bias with idle_bias_off == 1.
So here giving one warning when detected (dapm.idle_bias_off == 1) and (dapm.bias_level != SND_SOC_BIAS_OFF) just after driver->probe(). "
My take is that unless we can prove this is incorrect we leave it as is.
I think this commit is correct, thanks. But, then, it sounds we need to check it even though without .prove ?
Sorry, I am not getting your question. I don't have a trace of which codecs need this check, and I don't know either if this check needs to be done in other cases than the .probe(). Given all this, why would we try to move this WARN statement outside of the .probe case? It seems like asking for trouble.
Hi Pierre-Louis
if (component->driver->probe) { ret = component->driver->probe(component); ... (A) WARN(...) } (B) WARN(...)
(snip)
My take is that unless we can prove this is incorrect we leave it as is.
I think this commit is correct, thanks. But, then, it sounds we need to check it even though without .prove ?
Sorry, I am not getting your question. I don't have a trace of which codecs need this check, and I don't know either if this check needs to be done in other cases than the .probe(). Given all this, why would we try to move this WARN statement outside of the .probe case? It seems like asking for trouble.
Sorry for bother you. Actually, I'm planning to post soc-pcm.c / soc-core.c cleanup patch because, it is not readable/understandable/balanceable, thus, it is difficult to support multi CPU / multi Platform, etc, etc, etc...
Then, I can see below cade too many places.
if (component->driver->xxx) ret = component->driver->xxx()
this kind of code is very verbose for me. As one of this cleanup, I want to create very simple new_func like below. Then, this WARN() here is not good match for this purpose. In my understanding, this WARN() is effective even though it doesn't have .probe If so, code will be more simple.
int xxx_probe(component) { if (component->driver->probe) return component->driver->probe() return 0; }
... ret = xxx_probe(component); => WARN(); ...
Thank you for your help !! Best regards --- Kuninori Morimoto
On 5/20/19 7:42 PM, Kuninori Morimoto wrote:
Hi Pierre-Louis
if (component->driver->probe) { ret = component->driver->probe(component); ... (A) WARN(...) } (B) WARN(...)
(snip)
My take is that unless we can prove this is incorrect we leave it as is.
I think this commit is correct, thanks. But, then, it sounds we need to check it even though without .prove ?
Sorry, I am not getting your question. I don't have a trace of which codecs need this check, and I don't know either if this check needs to be done in other cases than the .probe(). Given all this, why would we try to move this WARN statement outside of the .probe case? It seems like asking for trouble.
Sorry for bother you. Actually, I'm planning to post soc-pcm.c / soc-core.c cleanup patch because, it is not readable/understandable/balanceable, thus, it is difficult to support multi CPU / multi Platform, etc, etc, etc...
Then, I can see below cade too many places.
if (component->driver->xxx) ret = component->driver->xxx()this kind of code is very verbose for me. As one of this cleanup, I want to create very simple new_func like below. Then, this WARN() here is not good match for this purpose. In my understanding, this WARN() is effective even though it doesn't have .probe If so, code will be more simple.
int xxx_probe(component) { if (component->driver->probe) return component->driver->probe() return 0; }
... ret = xxx_probe(component); => WARN(); ...
I don't know how to help further. Doing this change will result in the warning being thrown in cases where it wasn't before. and since no one knows what this warning means in the first place, either we leave it or we remove it, changing its location would be very odd.
On Tue, May 21, 2019 at 09:22:47AM -0500, Pierre-Louis Bossart wrote:
I don't know how to help further. Doing this change will result in the warning being thrown in cases where it wasn't before. and since no one knows what this warning means in the first place, either we leave it or we remove it, changing its location would be very odd.
It's fairly clear from the commit message what the warning means - it's trying to catch drivers that had buggy probe() functions that left the device powered up when it was expected to be idled.
The patch
ASoC: soc-core: WARN() is not related to component->driver->probe
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.3
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 2ffb0f580bded5f16ec4d619f8abb4745425e864 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Fri, 17 May 2019 15:06:37 +0900 Subject: [PATCH] ASoC: soc-core: WARN() is not related to component->driver->probe
soc_probe_component() has WARN() under if (component->driver->probe), but, this WARN() check is not related to .probe callback. So, it should be called at (B) instead of (A). This patch moves it out of if().
if (component->driver->probe) { ret = component->driver->probe(component); ... (A) WARN(...) } (B) WARN(...)
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-core.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index e83edbe27041..ce8c057bcd5b 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1420,12 +1420,11 @@ static int soc_probe_component(struct snd_soc_card *card, "ASoC: failed to probe component %d\n", ret); goto err_probe; } - - WARN(dapm->idle_bias_off && - dapm->bias_level != SND_SOC_BIAS_OFF, - "codec %s can not start from non-off bias with idle_bias_off==1\n", - component->name); } + WARN(dapm->idle_bias_off && + dapm->bias_level != SND_SOC_BIAS_OFF, + "codec %s can not start from non-off bias with idle_bias_off==1\n", + component->name);
/* machine specific init */ if (component->init) {
participants (3)
-
Kuninori Morimoto -
Mark Brown -
Pierre-Louis Bossart