[PATCH] ASoC: Fix dapm_is_shared_kcontrol so everything isn't shared

[PATCH] ASoC: Fix dapm_is_shared_kcontrol so everything isn't shared

[Stephen Warren]

Commit af46800 ("ASoC: Implement mux control sharing") introduced
function dapm_is_shared_kcontrol.

When this function returns true, the naming of DAPM controls is derived
from the kcontrol_new. Otherwise, the name comes from the widget (and
possibly a widget's naming prefix).

A bug in the implementation of dapm_is_shared_kcontrol made it return 1
in all cases. Hence, that commit caused a change in control naming for
all controls instead of just shared controls.

Specifically, a control is always considered shared because it is always
compared against itself. Solve this by never comparing against the widget
containing the control being created.

I tested that with the Tegra WM8903 driver:
* Shared is now mostly 0 as expected, and sometimes 1.
* The expected controls are still generated after this change.

Howwever, I don't have any systems that have a widget/control naming
prefix, so I can't test that aspect.

Reported-by: Liam Girdwood <lrg@ti.com>
Root-caused-by: Jarkko Nikula <jhnikula@gmail.com>
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 sound/soc/soc-dapm.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 456617e..5397699 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -325,6 +325,7 @@ static int dapm_connect_mixer(struct snd_soc_dapm_context *dapm,
 }
 
 static int dapm_is_shared_kcontrol(struct snd_soc_dapm_context *dapm,
+	struct snd_soc_dapm_widget *kcontrolw,
 	const struct snd_kcontrol_new *kcontrol_new,
 	struct snd_kcontrol **kcontrol)
 {
@@ -334,6 +335,8 @@ static int dapm_is_shared_kcontrol(struct snd_soc_dapm_context *dapm,
 	*kcontrol = NULL;
 
 	list_for_each_entry(w, &dapm->card->widgets, list) {
+		if (w == kcontrolw)
+			continue;
 		for (i = 0; i < w->num_kcontrols; i++) {
 			if (&w->kcontrol_news[i] == kcontrol_new) {
 				if (w->kcontrols)
@@ -468,7 +471,7 @@ static int dapm_new_mux(struct snd_soc_dapm_context *dapm,
 		return -EINVAL;
 	}
 
-	shared = dapm_is_shared_kcontrol(dapm, &w->kcontrol_news[0],
+	shared = dapm_is_shared_kcontrol(dapm, w, &w->kcontrol_news[0],
 					 &kcontrol);
 	if (kcontrol) {
 		wlist = kcontrol->private_data;
-- 
1.7.0.4
nvpublic

Re: [PATCH] ASoC: Fix dapm_is_shared_kcontrol so everything isn't shared

[Jarkko Nikula]

On Tue, 24 May 2011 17:12:01 -0600
Stephen Warren <swarren@nvidia.com> wrote:

> Commit af46800 ("ASoC: Implement mux control sharing") introduced
> function dapm_is_shared_kcontrol.
> 
> When this function returns true, the naming of DAPM controls is derived
> from the kcontrol_new. Otherwise, the name comes from the widget (and
> possibly a widget's naming prefix).
> 
> A bug in the implementation of dapm_is_shared_kcontrol made it return 1
> in all cases. Hence, that commit caused a change in control naming for
> all controls instead of just shared controls.
> 
> Specifically, a control is always considered shared because it is always
> compared against itself. Solve this by never comparing against the widget
> containing the control being created.
> 
> I tested that with the Tegra WM8903 driver:
> * Shared is now mostly 0 as expected, and sometimes 1.
> * The expected controls are still generated after this change.
> 
> Howwever, I don't have any systems that have a widget/control naming
> prefix, so I can't test that aspect.
> 
> Reported-by: Liam Girdwood <lrg@ti.com>
> Root-caused-by: Jarkko Nikula <jhnikula@gmail.com>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  sound/soc/soc-dapm.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
> index 456617e..5397699 100644
> --- a/sound/soc/soc-dapm.c
> +++ b/sound/soc/soc-dapm.c
> @@ -325,6 +325,7 @@ static int dapm_connect_mixer(struct snd_soc_dapm_context *dapm,
>  }
>  
>  static int dapm_is_shared_kcontrol(struct snd_soc_dapm_context *dapm,
> +	struct snd_soc_dapm_widget *kcontrolw,
>  	const struct snd_kcontrol_new *kcontrol_new,
>  	struct snd_kcontrol **kcontrol)
>  {
> @@ -334,6 +335,8 @@ static int dapm_is_shared_kcontrol(struct snd_soc_dapm_context *dapm,
>  	*kcontrol = NULL;
>  
>  	list_for_each_entry(w, &dapm->card->widgets, list) {
> +		if (w == kcontrolw)
> +			continue;

Hmm.. it seems this test is not enough with tlv320aic3x.c. It still
classifies a lot of kcontrols to be shared. Will keep hunting.

-- 
Jarkko

Re: [PATCH] ASoC: Fix dapm_is_shared_kcontrol so everything isn't shared

[Jarkko Nikula]

Hi Stephen

On Wed, 25 May 2011 10:01:55 +0300
Jarkko Nikula <jhnikula@gmail.com> wrote:

> On Tue, 24 May 2011 17:12:01 -0600
> Stephen Warren <swarren@nvidia.com> wrote:
> 
> > Commit af46800 ("ASoC: Implement mux control sharing") introduced
> > function dapm_is_shared_kcontrol.
> > 
> > When this function returns true, the naming of DAPM controls is derived
> > from the kcontrol_new. Otherwise, the name comes from the widget (and
> > possibly a widget's naming prefix).
> > 
> > A bug in the implementation of dapm_is_shared_kcontrol made it return 1
> > in all cases. Hence, that commit caused a change in control naming for
> > all controls instead of just shared controls.
> > 
> > Specifically, a control is always considered shared because it is always
> > compared against itself. Solve this by never comparing against the widget
> > containing the control being created.
> > 
> > I tested that with the Tegra WM8903 driver:
> > * Shared is now mostly 0 as expected, and sometimes 1.
> > * The expected controls are still generated after this change.
> > 
> > Howwever, I don't have any systems that have a widget/control naming
> > prefix, so I can't test that aspect.
> > 
> > Reported-by: Liam Girdwood <lrg@ti.com>
> > Root-caused-by: Jarkko Nikula <jhnikula@gmail.com>

I would rather see here Reported-by/Tested-by as I don't feel that I
broke here something :-)

> Hmm.. it seems this test is not enough with tlv320aic3x.c. It still
> classifies a lot of kcontrols to be shared. Will keep hunting.
> 
I managed to find why this didn't work with tlv320aic3x.c on rx51.c.

This triggered out that "[Left | Right] Line1[L | R]" Muxes in
tlv320aic3x were pointing to same mux controls which is wrong since
there are separate registers for them. I prepare a fix for this
hopefully tomorrow.

Then we must skip test for widgets that are in different dapm context,
i.e. we don't want to share mux controls if they are in different device
instance. Does it work for you if you add the dapm test below to your
patch?

@@ -334,6 +335,8 @@ static int dapm_is_shared_kcontrol(struct
snd_soc_dapm_context *dapm, *kcontrol = NULL;
 
 	list_for_each_entry(w, &dapm->card->widgets, list) {
+		if (w == kcontrolw || w->dapm != kcontrolw->dapm)
+			continue;

-- 
Jarkko

Re: [PATCH] ASoC: Fix dapm_is_shared_kcontrol so everything isn't shared

[Stephen Warren]

Jarkko Nikula wrote Wednesday, May 25, 2011 8:52 AM:
> On Wed, 25 May 2011 10:01:55 +0300 Jarkko Nikula <jhnikula@gmail.com> wrote:
> 
> > On Tue, 24 May 2011 17:12:01 -0600
> > Stephen Warren <swarren@nvidia.com> wrote:
> >
> > > Commit af46800 ("ASoC: Implement mux control sharing") introduced
> > > function dapm_is_shared_kcontrol.
> > >
> > > When this function returns true, the naming of DAPM controls is derived
> > > from the kcontrol_new. Otherwise, the name comes from the widget (and
> > > possibly a widget's naming prefix).
> > >
> > > A bug in the implementation of dapm_is_shared_kcontrol made it return 1
> > > in all cases. Hence, that commit caused a change in control naming for
> > > all controls instead of just shared controls.
> > >
> > > Specifically, a control is always considered shared because it is always
> > > compared against itself. Solve this by never comparing against the widget
> > > containing the control being created.
> > >
> > > I tested that with the Tegra WM8903 driver:
> > > * Shared is now mostly 0 as expected, and sometimes 1.
> > > * The expected controls are still generated after this change.
> > >
> > > Howwever, I don't have any systems that have a widget/control naming
> > > prefix, so I can't test that aspect.
> > >
> > > Reported-by: Liam Girdwood <lrg@ti.com>
> > > Root-caused-by: Jarkko Nikula <jhnikula@gmail.com>
> 
> I would rather see here Reported-by/Tested-by as I don't feel that I
> broke here something :-)

OK, I can change it to Tested-by, but what I meant by that (admittedly
non-standard) tag was simply that you'd pointed out what the basic problem
was, i.e. done the work to determine the root-cause, not that you were
the root cause!

Thanks for testing.

> > Hmm.. it seems this test is not enough with tlv320aic3x.c. It still
> > classifies a lot of kcontrols to be shared. Will keep hunting.
>
> I managed to find why this didn't work with tlv320aic3x.c on rx51.c.
> 
> This triggered out that "[Left | Right] Line1[L | R]" Muxes in
> tlv320aic3x were pointing to same mux controls which is wrong since
> there are separate registers for them. I prepare a fix for this
> hopefully tomorrow.
> 
> Then we must skip test for widgets that are in different dapm context,
> i.e. we don't want to share mux controls if they are in different device
> instance. Does it work for you if you add the dapm test below to your
> patch?

I suppose that's technically correct, and I'm fine adding this to the
patch. However, I'm surprised that change is needed; do we actually have
a situation where two widgets in different DAPM contexts point at the
same control definition? It seems like that'd be a bug; why would there
be two controls pointing at the same register field, yet in different
DAPM contexts?

> @@ -334,6 +335,8 @@ static int dapm_is_shared_kcontrol(struct
> snd_soc_dapm_context *dapm, *kcontrol = NULL;
> 
>  	list_for_each_entry(w, &dapm->card->widgets, list) {
> +		if (w == kcontrolw || w->dapm != kcontrolw->dapm)
> +			continue;

-- 
nvpublic

Re: [PATCH] ASoC: Fix dapm_is_shared_kcontrol so everything isn't shared

[Jarkko Nikula]

On Wed, 25 May 2011 08:24:59 -0700
Stephen Warren <swarren@nvidia.com> wrote:

> > Then we must skip test for widgets that are in different dapm context,
> > i.e. we don't want to share mux controls if they are in different device
> > instance. Does it work for you if you add the dapm test below to your
> > patch?
> 
> I suppose that's technically correct, and I'm fine adding this to the
> patch. However, I'm surprised that change is needed; do we actually have
> a situation where two widgets in different DAPM contexts point at the
> same control definition? It seems like that'd be a bug; why would there
> be two controls pointing at the same register field, yet in different
> DAPM contexts?
> 
Yes, I noticed this can happen when you have multiple same codecs in a
system. Let's assume we have multiple wm8903 codecs and we have these
in the driver:

static const struct snd_kcontrol_new adcinput_mux =
	SOC_DAPM_ENUM("ADC Input", adcinput_enum);

static const struct snd_soc_dapm_widget wm8903_dapm_widgets[] = {
  SND_SOC_DAPM_MUX("Left ADC Input", SND_SOC_NOPM, 0, 0, &adcinput_mux),
};

Even the core duplicates widgets for each codec instance in this
example, they still point to same adcinput_mux and thus
dapm_is_shared_kcontrol() miss detects them as shared if we don't skip
different dapm context.

-- 
Jarkko

Re: [PATCH] ASoC: Fix dapm_is_shared_kcontrol so everything isn't shared

[Stephen Warren]

Jarkko Nikula wrote at Thursday, May 26, 2011 12:58 AM:
> On Wed, 25 May 2011 08:24:59 -0700 Stephen Warren <swarren@nvidia.com> wrote:
> > > Then we must skip test for widgets that are in different dapm context,
> > > i.e. we don't want to share mux controls if they are in different device
> > > instance. Does it work for you if you add the dapm test below to your
> > > patch?
> >
> > I suppose that's technically correct, and I'm fine adding this to the
> > patch. However, I'm surprised that change is needed; do we actually have
> > a situation where two widgets in different DAPM contexts point at the
> > same control definition? It seems like that'd be a bug; why would there
> > be two controls pointing at the same register field, yet in different
> > DAPM contexts?
>
> Yes, I noticed this can happen when you have multiple same codecs in a
> system. Let's assume we have multiple wm8903 codecs and we have these
> in the driver:

Yes, of course, that makes perfect sense. I'll rev the patch as you suggested.

> static const struct snd_kcontrol_new adcinput_mux =
> 	SOC_DAPM_ENUM("ADC Input", adcinput_enum);
> 
> static const struct snd_soc_dapm_widget wm8903_dapm_widgets[] = {
>   SND_SOC_DAPM_MUX("Left ADC Input", SND_SOC_NOPM, 0, 0, &adcinput_mux),
> };
> 
> Even the core duplicates widgets for each codec instance in this
> example, they still point to same adcinput_mux and thus
> dapm_is_shared_kcontrol() miss detects them as shared if we don't skip
> different dapm context.

-- 
nvpublic

[PATCH] ASoC: Fix dapm_is_shared_kcontrol so everything isn't shared

[Stephen Warren]

Commit af46800 ("ASoC: Implement mux control sharing") introduced
function dapm_is_shared_kcontrol.

When this function returns true, the naming of DAPM controls is derived
from the kcontrol_new. Otherwise, the name comes from the widget (and
possibly a widget's naming prefix).

A bug in the implementation of dapm_is_shared_kcontrol made it return 1
in all cases. Hence, that commit caused a change in control naming for
all controls instead of just shared controls.

Specifically, a control is always considered shared because it is always
compared against itself. Solve this by never comparing against the widget
containing the control being created.

Equally, controls should never be shared between DAPM contexts; when the
same codec is instantiated multiple times, the same kcontrol_new will be
used. However, the control should no be shared between the multiple
instances.

I tested that with the Tegra WM8903 driver:
* Shared is now mostly 0 as expected, and sometimes 1.
* The expected controls are still generated after this change.

However, I don't have any systems that have a widget/control naming
prefix, so I can't test that aspect.

Thanks for Jarkko Nikula for pointing out how to fix this.

v2:
* Don't share across DAPM contexts.
* Changed to Tested-by tag below for Jarkko.

Reported-by: Liam Girdwood <lrg@ti.com>
Tested-by: Jarkko Nikula <jhnikula@gmail.com>
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 sound/soc/soc-dapm.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 456617e..10bb49b 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -325,6 +325,7 @@ static int dapm_connect_mixer(struct snd_soc_dapm_context *dapm,
 }
 
 static int dapm_is_shared_kcontrol(struct snd_soc_dapm_context *dapm,
+	struct snd_soc_dapm_widget *kcontrolw,
 	const struct snd_kcontrol_new *kcontrol_new,
 	struct snd_kcontrol **kcontrol)
 {
@@ -334,6 +335,8 @@ static int dapm_is_shared_kcontrol(struct snd_soc_dapm_context *dapm,
 	*kcontrol = NULL;
 
 	list_for_each_entry(w, &dapm->card->widgets, list) {
+		if (w == kcontrolw || w->dapm != kcontrolw->dapm)
+			continue;
 		for (i = 0; i < w->num_kcontrols; i++) {
 			if (&w->kcontrol_news[i] == kcontrol_new) {
 				if (w->kcontrols)
@@ -468,7 +471,7 @@ static int dapm_new_mux(struct snd_soc_dapm_context *dapm,
 		return -EINVAL;
 	}
 
-	shared = dapm_is_shared_kcontrol(dapm, &w->kcontrol_news[0],
+	shared = dapm_is_shared_kcontrol(dapm, w, &w->kcontrol_news[0],
 					 &kcontrol);
 	if (kcontrol) {
 		wlist = kcontrol->private_data;
-- 
1.7.0.4

Re: [PATCH] ASoC: Fix dapm_is_shared_kcontrol so everything isn't shared

[Jarkko Nikula]

On Thu, 26 May 2011 09:57:33 -0600
Stephen Warren <swarren@nvidia.com> wrote:

> Commit af46800 ("ASoC: Implement mux control sharing") introduced
> function dapm_is_shared_kcontrol.
> 
> When this function returns true, the naming of DAPM controls is derived
> from the kcontrol_new. Otherwise, the name comes from the widget (and
> possibly a widget's naming prefix).
> 
> A bug in the implementation of dapm_is_shared_kcontrol made it return 1
> in all cases. Hence, that commit caused a change in control naming for
> all controls instead of just shared controls.
> 
> Specifically, a control is always considered shared because it is always
> compared against itself. Solve this by never comparing against the widget
> containing the control being created.
> 
> Equally, controls should never be shared between DAPM contexts; when the
> same codec is instantiated multiple times, the same kcontrol_new will be
> used. However, the control should no be shared between the multiple
> instances.
> 
> I tested that with the Tegra WM8903 driver:
> * Shared is now mostly 0 as expected, and sometimes 1.
> * The expected controls are still generated after this change.
> 
> However, I don't have any systems that have a widget/control naming
> prefix, so I can't test that aspect.
> 
> Thanks for Jarkko Nikula for pointing out how to fix this.
> 
> v2:
> * Don't share across DAPM contexts.
> * Changed to Tested-by tag below for Jarkko.
> 
> Reported-by: Liam Girdwood <lrg@ti.com>
> Tested-by: Jarkko Nikula <jhnikula@gmail.com>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  sound/soc/soc-dapm.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
Tested-by confirmation :-)

-- 
Jarkko

Re: [PATCH] ASoC: Fix dapm_is_shared_kcontrol so everything isn't shared

[Liam Girdwood]

On 26/05/11 16:57, Stephen Warren wrote:
> Commit af46800 ("ASoC: Implement mux control sharing") introduced
> function dapm_is_shared_kcontrol.
> 
> When this function returns true, the naming of DAPM controls is derived
> from the kcontrol_new. Otherwise, the name comes from the widget (and
> possibly a widget's naming prefix).
> 
> A bug in the implementation of dapm_is_shared_kcontrol made it return 1
> in all cases. Hence, that commit caused a change in control naming for
> all controls instead of just shared controls.
> 
> Specifically, a control is always considered shared because it is always
> compared against itself. Solve this by never comparing against the widget
> containing the control being created.
> 
> Equally, controls should never be shared between DAPM contexts; when the
> same codec is instantiated multiple times, the same kcontrol_new will be
> used. However, the control should no be shared between the multiple
> instances.
> 
> I tested that with the Tegra WM8903 driver:
> * Shared is now mostly 0 as expected, and sometimes 1.
> * The expected controls are still generated after this change.
> 
> However, I don't have any systems that have a widget/control naming
> prefix, so I can't test that aspect.
> 
> Thanks for Jarkko Nikula for pointing out how to fix this.
> 
> v2:
> * Don't share across DAPM contexts.
> * Changed to Tested-by tag below for Jarkko.
> 
> Reported-by: Liam Girdwood <lrg@ti.com>
> Tested-by: Jarkko Nikula <jhnikula@gmail.com>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---

Acked-by: Liam Girdwood <lrg@ti.com>

Re: [PATCH] ASoC: Fix dapm_is_shared_kcontrol so everything isn't shared

[Mark Brown]

On Thu, May 26, 2011 at 09:57:33AM -0600, Stephen Warren wrote:
> Commit af46800 ("ASoC: Implement mux control sharing") introduced
> function dapm_is_shared_kcontrol.
> 
> When this function returns true, the naming of DAPM controls is derived
> from the kcontrol_new. Otherwise, the name comes from the widget (and
> possibly a widget's naming prefix).

Applied, thanks.