[alsa-devel] OOPS: Vmaster not freed on reconfig
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.)
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
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
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)
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.
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);
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); }
participants (2)
-
David Henningsson
-
Takashi Iwai