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