[PATCH][RFC] ASoC: soc-core: WARN() is not related to component->driver->probe

[PATCH][RFC] ASoC: soc-core: WARN() is not related to component->driver->probe

[Kuninori Morimoto]

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) {
-- 
2.7.4

Re: [PATCH][RFC] ASoC: soc-core: WARN() is not related to component->driver->probe

[Pierre-Louis Bossart]

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) {
> 

Re: [PATCH][RFC] ASoC: soc-core: WARN() is not related to component->driver->probe

[Kuninori Morimoto]

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

Re: [PATCH][RFC] ASoC: soc-core: WARN() is not related to component->driver->probe

[Pierre-Louis Bossart]

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.

Re: [PATCH][RFC] ASoC: soc-core: WARN() is not related to component->driver->probe

[Kuninori Morimoto]

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

Re: [PATCH][RFC] ASoC: soc-core: WARN() is not related to component->driver->probe

[Pierre-Louis Bossart]

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.

Re: [PATCH][RFC] ASoC: soc-core: WARN() is not related to component->driver->probe

[Mark Brown]

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

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.

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

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

Applied "ASoC: soc-core: WARN() is not related to component->driver->probe" to the asoc tree

[Mark Brown]

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) {
-- 
2.20.1