OOPS: Vmaster not freed on reconfig

OOPS: Vmaster not freed on reconfig

[David Henningsson]

Hi,

I'm playing around with the reconfig functions of snd-hda-codec and got 
an OOPS in slave_put. I tried to research this, and I think I've nailed 
down the problem: the vmaster is not freed on reconfig (presumably, it 
should be fixed in hda_codec.c:snd_hda_ctls_clear).
I'm a little unsure of whether the best thing is to just add it to 
codec.mixers, keep them in a separate array, or something else. What do 
you think?

(It's on 3.0, it's a sigmatel/IDT, and I've got the screen captured by a 
camera if anyone is interested.)

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

Re: OOPS: Vmaster not freed on reconfig

[Takashi Iwai]

At Tue, 08 Nov 2011 17:16:56 +0100,
David Henningsson wrote:
> 
> Hi,
> 
> I'm playing around with the reconfig functions of snd-hda-codec and got 
> an OOPS in slave_put. I tried to research this, and I think I've nailed 
> down the problem: the vmaster is not freed on reconfig (presumably, it 
> should be fixed in hda_codec.c:snd_hda_ctls_clear).

Hmm, I don't figure out how it happens.
The vmaster is added to the list via snd_hda_add_vmaster() by calling
snd_hda_ctl_add().

> I'm a little unsure of whether the best thing is to just add it to 
> codec.mixers, keep them in a separate array, or something else. What do 
> you think?
> 
> (It's on 3.0, it's a sigmatel/IDT, and I've got the screen captured by a 
> camera if anyone is interested.)

Yes, it'd be interesting.  If the bug is easily reproducible, you can
capture the oops text by netconsole, too.


thanks,

Takashi

Re: OOPS: Vmaster not freed on reconfig

[Takashi Iwai]

At Tue, 08 Nov 2011 17:36:02 +0100,
Takashi Iwai wrote:
> 
> At Tue, 08 Nov 2011 17:16:56 +0100,
> David Henningsson wrote:
> > 
> > Hi,
> > 
> > I'm playing around with the reconfig functions of snd-hda-codec and got 
> > an OOPS in slave_put. I tried to research this, and I think I've nailed 
> > down the problem: the vmaster is not freed on reconfig (presumably, it 
> > should be fixed in hda_codec.c:snd_hda_ctls_clear).
> 
> Hmm, I don't figure out how it happens.
> The vmaster is added to the list via snd_hda_add_vmaster() by calling
> snd_hda_ctl_add().
> 
> > I'm a little unsure of whether the best thing is to just add it to 
> > codec.mixers, keep them in a separate array, or something else. What do 
> > you think?
> > 
> > (It's on 3.0, it's a sigmatel/IDT, and I've got the screen captured by a 
> > camera if anyone is interested.)
> 
> Yes, it'd be interesting.  If the bug is easily reproducible, you can
> capture the oops text by netconsole, too.

Also, please give alsa-info.sh output (or equivalent) that triggers
the problem.  I'll try to debug with hda-emu at first.


thanks,

Takashi

Re: OOPS: Vmaster not freed on reconfig

[David Henningsson]

On 11/08/2011 05:36 PM, Takashi Iwai wrote:
> At Tue, 08 Nov 2011 17:16:56 +0100,
> David Henningsson wrote:
>>
>> Hi,
>>
>> I'm playing around with the reconfig functions of snd-hda-codec and got
>> an OOPS in slave_put. I tried to research this, and I think I've nailed
>> down the problem: the vmaster is not freed on reconfig (presumably, it
>> should be fixed in hda_codec.c:snd_hda_ctls_clear).
>
> Hmm, I don't figure out how it happens.
> The vmaster is added to the list via snd_hda_add_vmaster() by calling
> snd_hda_ctl_add().

You're correct. It must be some other error.

>> I'm a little unsure of whether the best thing is to just add it to
>> codec.mixers, keep them in a separate array, or something else. What do
>> you think?
>>
>> (It's on 3.0, it's a sigmatel/IDT, and I've got the screen captured by a
>> camera if anyone is interested.)
>
> Yes, it'd be interesting.  If the bug is easily reproducible, you can
> capture the oops text by netconsole, too.

I haven't had the time to troubleshoot further yet (and thus I don't 
know if it's reproducible), but at least I've uploaded the requested 
information:

https://bugs.launchpad.net/ubuntu/+source/alsa-driver/+bug/888081/+attachment/2590781/+files/alsa-info.txt.ZE05tEjd8T

https://bugs.launchpad.net/ubuntu/+source/alsa-driver/+bug/888081/+attachment/2590787/+files/DSC00007.JPG 


(Sorry about the bad quality of that picture, I think most of it should 
be readable at least)

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

Re: OOPS: Vmaster not freed on reconfig

[Takashi Iwai]

At Wed, 09 Nov 2011 15:30:34 +0100,
David Henningsson wrote:
> 
> On 11/08/2011 05:36 PM, Takashi Iwai wrote:
> > At Tue, 08 Nov 2011 17:16:56 +0100,
> > David Henningsson wrote:
> >>
> >> Hi,
> >>
> >> I'm playing around with the reconfig functions of snd-hda-codec and got
> >> an OOPS in slave_put. I tried to research this, and I think I've nailed
> >> down the problem: the vmaster is not freed on reconfig (presumably, it
> >> should be fixed in hda_codec.c:snd_hda_ctls_clear).
> >
> > Hmm, I don't figure out how it happens.
> > The vmaster is added to the list via snd_hda_add_vmaster() by calling
> > snd_hda_ctl_add().
> 
> You're correct. It must be some other error.
> 
> >> I'm a little unsure of whether the best thing is to just add it to
> >> codec.mixers, keep them in a separate array, or something else. What do
> >> you think?
> >>
> >> (It's on 3.0, it's a sigmatel/IDT, and I've got the screen captured by a
> >> camera if anyone is interested.)
> >
> > Yes, it'd be interesting.  If the bug is easily reproducible, you can
> > capture the oops text by netconsole, too.
> 
> I haven't had the time to troubleshoot further yet (and thus I don't 
> know if it's reproducible), but at least I've uploaded the requested 
> information:
> 
> https://bugs.launchpad.net/ubuntu/+source/alsa-driver/+bug/888081/+attachment/2590781/+files/alsa-info.txt.ZE05tEjd8T
> 
> https://bugs.launchpad.net/ubuntu/+source/alsa-driver/+bug/888081/+attachment/2590787/+files/DSC00007.JPG 
> 
> 
> (Sorry about the bad quality of that picture, I think most of it should 
> be readable at least)

Thanks.  I guess I found the culprit.

It must be IEC958 control of another codec.  It's assigned to the same
bus so the driver takes equally like the primary codec.  At the second
reconfig, the driver tries to make it slave although it doesn't belong
to the codec.  It works at this time, though.  But when you do
reconfig again, this slave won't be cleared properly, thus it'll
become a slave over a dead master.

A fix is the patch like below.


Takashi

---
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 916a186..e913671 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2331,6 +2331,39 @@ int snd_hda_codec_reset(struct hda_codec *codec)
 	return 0;
 }
 
+typedef int (*map_slave_func_t)(void *, struct snd_kcontrol *);
+
+/* apply the function to all matching slave ctls in the mixer list */
+static int map_slaves(struct hda_codec *codec, const char * const *slaves,
+		      map_slave_func_t func, void *data) 
+{
+	struct hda_nid_item *items;
+	const char * const *s;
+	int i, err;
+
+	items = codec->mixers.list;
+	for (i = 0; i < codec->mixers.used; i++) {
+		struct snd_kcontrol *sctl = items[i].kctl;
+		if (!sctl || !sctl->id.name ||
+		    sctl->id.iface != SNDRV_CTL_ELEM_IFACE_MIXER)
+			continue;
+		for (s = slaves; *s; s++) {
+			if (!strcmp(sctl->id.name, *s)) {
+				err = func(data, sctl);
+				if (err)
+					return err;
+				break;
+			}
+		}
+	}
+	return 0;
+}
+
+static int check_slave_present(void *data, struct snd_kcontrol *sctl)
+{
+	return 1;
+}
+
 /**
  * snd_hda_add_vmaster - create a virtual master control and add slaves
  * @codec: HD-audio codec
@@ -2351,12 +2384,10 @@ int snd_hda_add_vmaster(struct hda_codec *codec, char *name,
 			unsigned int *tlv, const char * const *slaves)
 {
 	struct snd_kcontrol *kctl;
-	const char * const *s;
 	int err;
 
-	for (s = slaves; *s && !snd_hda_find_mixer_ctl(codec, *s); s++)
-		;
-	if (!*s) {
+	err = map_slaves(codec, slaves, check_slave_present, NULL);
+	if (err != 1) {
 		snd_printdd("No slave found for %s\n", name);
 		return 0;
 	}
@@ -2367,23 +2398,10 @@ int snd_hda_add_vmaster(struct hda_codec *codec, char *name,
 	if (err < 0)
 		return err;
 
-	for (s = slaves; *s; s++) {
-		struct snd_kcontrol *sctl;
-		int i = 0;
-		for (;;) {
-			sctl = _snd_hda_find_mixer_ctl(codec, *s, i);
-			if (!sctl) {
-				if (!i)
-					snd_printdd("Cannot find slave %s, "
-						    "skipped\n", *s);
-				break;
-			}
-			err = snd_ctl_add_slave(kctl, sctl);
-			if (err < 0)
-				return err;
-			i++;
-		}
-	}
+	err = map_slaves(codec, slaves, (map_slave_func_t)snd_ctl_add_slave,
+			 kctl);
+	if (err < 0)
+		return err;
 	return 0;
 }
 EXPORT_SYMBOL_HDA(snd_hda_add_vmaster);

Re: OOPS: Vmaster not freed on reconfig

[Takashi Iwai]

At Wed, 09 Nov 2011 16:46:14 +0100,
Takashi Iwai wrote:
> 
> At Wed, 09 Nov 2011 15:30:34 +0100,
> David Henningsson wrote:
> > 
> > On 11/08/2011 05:36 PM, Takashi Iwai wrote:
> > > At Tue, 08 Nov 2011 17:16:56 +0100,
> > > David Henningsson wrote:
> > >>
> > >> Hi,
> > >>
> > >> I'm playing around with the reconfig functions of snd-hda-codec and got
> > >> an OOPS in slave_put. I tried to research this, and I think I've nailed
> > >> down the problem: the vmaster is not freed on reconfig (presumably, it
> > >> should be fixed in hda_codec.c:snd_hda_ctls_clear).
> > >
> > > Hmm, I don't figure out how it happens.
> > > The vmaster is added to the list via snd_hda_add_vmaster() by calling
> > > snd_hda_ctl_add().
> > 
> > You're correct. It must be some other error.
> > 
> > >> I'm a little unsure of whether the best thing is to just add it to
> > >> codec.mixers, keep them in a separate array, or something else. What do
> > >> you think?
> > >>
> > >> (It's on 3.0, it's a sigmatel/IDT, and I've got the screen captured by a
> > >> camera if anyone is interested.)
> > >
> > > Yes, it'd be interesting.  If the bug is easily reproducible, you can
> > > capture the oops text by netconsole, too.
> > 
> > I haven't had the time to troubleshoot further yet (and thus I don't 
> > know if it's reproducible), but at least I've uploaded the requested 
> > information:
> > 
> > https://bugs.launchpad.net/ubuntu/+source/alsa-driver/+bug/888081/+attachment/2590781/+files/alsa-info.txt.ZE05tEjd8T
> > 
> > https://bugs.launchpad.net/ubuntu/+source/alsa-driver/+bug/888081/+attachment/2590787/+files/DSC00007.JPG 
> > 
> > 
> > (Sorry about the bad quality of that picture, I think most of it should 
> > be readable at least)
> 
> Thanks.  I guess I found the culprit.
> 
> It must be IEC958 control of another codec.  It's assigned to the same
> bus so the driver takes equally like the primary codec.  At the second
> reconfig, the driver tries to make it slave although it doesn't belong
> to the codec.  It works at this time, though.  But when you do
> reconfig again, this slave won't be cleared properly, thus it'll
> become a slave over a dead master.
> 
> A fix is the patch like below.

The patch below can be used as an alternative fix.
Or better to apply both.

This one changes vmaster code to restore the original slave elements
when the master element is freed.  So, no corrupted link will remain
any longer.


Takashi

---
diff --git a/sound/core/vmaster.c b/sound/core/vmaster.c
index 5dbab38..130cfe6 100644
--- a/sound/core/vmaster.c
+++ b/sound/core/vmaster.c
@@ -52,6 +52,7 @@ struct link_slave {
 	struct link_ctl_info info;
 	int vals[2];		/* current values */
 	unsigned int flags;
+	struct snd_kcontrol *kctl; /* original kcontrol pointer */
 	struct snd_kcontrol slave; /* the copy of original control entry */
 };
 
@@ -252,6 +253,7 @@ int _snd_ctl_add_slave(struct snd_kcontrol *master, struct snd_kcontrol *slave,
 		       slave->count * sizeof(*slave->vd), GFP_KERNEL);
 	if (!srec)
 		return -ENOMEM;
+	srec->kctl = slave;
 	srec->slave = *slave;
 	memcpy(srec->slave.vd, slave->vd, slave->count * sizeof(*slave->vd));
 	srec->master = master_link;
@@ -333,10 +335,18 @@ static int master_put(struct snd_kcontrol *kcontrol,
 static void master_free(struct snd_kcontrol *kcontrol)
 {
 	struct link_master *master = snd_kcontrol_chip(kcontrol);
-	struct link_slave *slave;
+	struct link_slave *slave, *n;
 
-	list_for_each_entry(slave, &master->slaves, list)
-		slave->master = NULL;
+	/* free all slave links and retore the original slave kctls */
+	list_for_each_entry_safe(slave, n, &master->slaves, list) {
+		struct snd_kcontrol *sctl = slave->kctl;
+		struct list_head olist = sctl->list;
+		memcpy(sctl, &slave->slave, sizeof(*sctl));
+		memcpy(sctl->vd, slave->slave.vd,
+		       sctl->count * sizeof(*sctl->vd));
+		sctl->list = olist; /* keep the current linked-list */
+		kfree(slave);
+	}
 	kfree(master);
 }