[alsa-devel] 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);