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/+attachmen...
https://bugs.launchpad.net/ubuntu/+source/alsa-driver/+bug/888081/+attachmen...
(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); }