- Alsa-devel - mailman.alsa-project.org

[alsa-devel] [PATCH][RFC] pcm: add exclusion between hw_params and prepare callbacks
by Dave Dillow 28 Nov '07

28 Nov '07

While working on the locking in the sis7019 driver, and as also noted by Trent Piepho, there is a race between threads calling into the driver's hw_params callback and its prepare callback. This can lead to incorrect/inconsistent data being programmed into the audio device, and lead to unexpected results, if not a possible system crash. While it is arguably a bug in the user space program, we should protect the system and provide some assurance to the driver code about concurrency between these two callbacks. The attached patch adds a mutex to the two paths to provide the locking without giving up the ability for those calls to schedule, as documented in the API. It just to get a discussion going, as the code to be restructured a bit to clean up the error exit paths. It is also likely there are other callbacks that should be excluded when either of these are executing as well. This applies to the base kernel tree. Dave --- include/sound/pcm.h | 2 ++ sound/core/pcm.c | 1 + sound/core/pcm_native.c | 20 +++++++++++++++++--- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 73334e0..989e84f 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -388,6 +388,8 @@ struct snd_pcm_substream { struct snd_info_entry *proc_prealloc_entry; struct snd_info_entry *proc_prealloc_max_entry; #endif + /* -- hw_param/prepare exclusion -- */ + struct mutex hw_param_mutex; /* misc flags */ unsigned int hw_opened: 1; }; diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 2743414..d728594 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -651,6 +651,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count) list_add_tail(&substream->link_list, &substream->self_group.substreams); spin_lock_init(&substream->timer_lock); atomic_set(&substream->mmap_count, 0); + mutex_init(&substream->hw_param_mutex); prev = substream; } return 0; diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 59b29cd..638d78a 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -373,6 +373,7 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, snd_assert(substream != NULL, return -ENXIO); runtime = substream->runtime; snd_assert(runtime != NULL, return -ENXIO); + mutex_lock(&substream->hw_param_mutex); snd_pcm_stream_lock_irq(substream); switch (runtime->status->state) { case SNDRV_PCM_STATE_OPEN: @@ -380,6 +381,7 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, case SNDRV_PCM_STATE_PREPARED: break; default: + mutex_unlock(&substream->hw_param_mutex); snd_pcm_stream_unlock_irq(substream); return -EBADFD; } @@ -387,8 +389,10 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, #if defined(CONFIG_SND_PCM_OSS) || defined(CONFIG_SND_PCM_OSS_MODULE) if (!substream->oss.oss) #endif - if (atomic_read(&substream->mmap_count)) + if (atomic_read(&substream->mmap_count)) { + mutex_unlock(&substream->hw_param_mutex); return -EBADFD; + } params->rmask = ~0U; err = snd_pcm_hw_refine(substream, params); @@ -450,6 +454,7 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, remove_acceptable_latency(substream->latency_id); if ((usecs = period_to_usecs(runtime)) >= 0) set_acceptable_latency(substream->latency_id, usecs); + mutex_unlock(&substream->hw_param_mutex); return 0; _error: /* hardware might be unuseable from this time, @@ -458,6 +463,7 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, runtime->status->state = SNDRV_PCM_STATE_OPEN; if (substream->ops->hw_free != NULL) substream->ops->hw_free(substream); + mutex_unlock(&substream->hw_param_mutex); return err; } @@ -1345,9 +1351,10 @@ static struct action_ops snd_pcm_action_prepare = { static int snd_pcm_prepare(struct snd_pcm_substream *substream, struct file *file) { - int res; struct snd_card *card = substream->pcm->card; + struct snd_pcm_substream *s; int f_flags; + int res; if (file) f_flags = file->f_flags; @@ -1355,9 +1362,16 @@ static int snd_pcm_prepare(struct snd_pcm_substream *substream, f_flags = substream->f_flags; snd_power_lock(card); - if ((res = snd_power_wait(card, SNDRV_CTL_POWER_D0)) >= 0) + if ((res = snd_power_wait(card, SNDRV_CTL_POWER_D0)) >= 0) { + down_read(&snd_pcm_link_rwsem); + snd_pcm_group_for_each_entry(s, substream) + mutex_lock(&s->hw_param_mutex); res = snd_pcm_action_nonatomic(&snd_pcm_action_prepare, substream, f_flags); + snd_pcm_group_for_each_entry(s, substream) + mutex_unlock(&s->hw_param_mutex); + up_read(&snd_pcm_link_rwsem); + } snd_power_unlock(card); return res; }

2 1

Re: [alsa-devel] mbox II
by rmgls＠free.fr 28 Nov '07

28 Nov '07

Hi Clemens and list, ... > try plughw ... i tried: aplay plughw:0 (plughw:0,0) (plughw:0,1) (plughw:0,2) coq.wav this time no coredump, aplay try to play, but no sound on the monitoring of the mbox II, a complete silence. - perhaps the mixer is muted! i cannot adjust the mixer: amiser no elements found. the technical support contacted tell me: they have no tech doc for the box. i will try on the duc forum on digidisign.com. i am unsure to have something there! otherwise, is a wrapper to the win driver suitable, or even possible? thanks. best regards Raoul rmgls(a)free.fr

2 1

[alsa-devel] No sound on Intel DQ965GF motherboard -- SOLVED
by Eric S. Raymond 28 Nov '07

28 Nov '07

Thanks to all who tried to help. The ALSA 1.0.15 backport modules solved my problem, you can consider the DQ965GF fully supported now. -- >>esr>>

1 0

Re: [alsa-devel] [PATCH 2/3] PNP cleanups - Unify the pnp macros to access resources in the pnp resource table
by Rene Herman 27 Nov '07

27 Nov '07

On 20-11-07 15:19, Thomas Renninger wrote: > At the end is some example code how things could get even more cleaned > up. It shows how I think pnp layer and one example driver would get > adjusted. There are not that much drivers making use of > pnp_resource_change... The ALSA ISA-PnP drivers do in what I personally consider a bad layering violation. Brought up not doing that a while ago on alsa-devel, but no discussion ensued. If the PnP maintainer(s) agree with me though, I'd be more than happy to rip all of that out of ALSA. (added alsa-devel) Rene.

3 4

[alsa-devel] 1.0.15: volume levels drift on HDA with STAC92XX codec
by Chuck Ebbert 26 Nov '07

26 Nov '07

We have two reports now of unstable volume levels: https://bugzilla.redhat.com/show_bug.cgi?id=361051 https://bugzilla.redhat.com/show_bug.cgi?id=354981 This is with kernel 2.6.23 plus the two ALSA merge patches from 2.6.23-rc1.

6 25

[alsa-devel] [PATCH] add number of periods constraint to snd-aoa [try 3]
by Heikki O Lindholm 26 Nov '07

26 Nov '07

From: Heikki Lindholm <holindho(a)cs.helsinki.fi> The aoa driver is not specifying constraints on number of periods, and, it seems, it might end with a non-integer number, which it cannot deal with. Fix by adding a proper constraint. Signed-off-by: Heikki Lindholm <holindho(a)cs.helsinki.fi> --- try 3: fixed coding style diff -r 1b54a8725ded aoa/soundbus/i2sbus/i2sbus-pcm.c --- a/aoa/soundbus/i2sbus/i2sbus-pcm.c Wed Nov 14 17:07:17 2007 +0100 +++ b/aoa/soundbus/i2sbus/i2sbus-pcm.c Fri Nov 23 14:16:07 2007 +0200 @@ -194,6 +194,12 @@ static int i2sbus_pcm_open(struct i2sbus hw->period_bytes_max = 16384; hw->periods_min = 3; hw->periods_max = MAX_DBDMA_COMMANDS; + err = snd_pcm_hw_constraint_integer(pi->substream->runtime, + SNDRV_PCM_HW_PARAM_PERIODS); + if (err < 0) { + result = err; + goto out_unlock; + } list_for_each_entry(cii, &sdev->codec_list, list) { if (cii->codec->open) { err = cii->codec->open(cii, pi->substream);

5 10

[alsa-devel] ALSA Leaking memory
by Lennart Poettering 26 Nov '07

26 Nov '07

Hi! Apparently alsa-libs .15 is leaking memory when using snd_pcm_open() on some devices that cannot be opened. On my USB wecam which has only a single mono audio stream, opening it as "front:3" (which of course fails) leaks a bit of memory. Consider this example: <snip> #include <asoundlib.h> #include <assert.h> int main(int argc, char *argv[]) { snd_pcm_t *pcm; if (snd_pcm_open(&pcm, "front:3", SND_PCM_STREAM_CAPTURE, SND_PCM_NONBLOCK) == 0) { int r = snd_pcm_close(pcm); assert(r == 0); } snd_config_update_free_global(); return 0; } </snip> And there they are gone, the 10 bytes of memory. Here's a snippet from valgrinding this program: ==22198== 10 bytes in 1 blocks are definitely lost in loss record 1 of 3 ==22198== at 0x4023765: malloc (vg_replace_malloc.c:149) ==22198== by 0x417C44F: strdup (strdup.c:43) ==22198== by 0x4071954: snd_determine_driver (in /usr/lib/libasound.so.2.0.0) ==22198== by 0x406E80C: snd_config_hook_load_for_all_cards (in /usr/lib/libasound.so.2.0.0) ==22198== by 0x406EC7C: (within /usr/lib/libasound.so.2.0.0) ==22198== by 0x406F896: snd_config_searcha_hooks (in /usr/lib/libasound.so.2.0.0) ==22198== by 0x406FA74: snd_config_search_alias_hooks (in /usr/lib/libasound.so.2.0.0) ==22198== by 0x406FCD9: snd_config_search_definition (in /usr/lib/libasound.so.2.0.0) ==22198== by 0x40952FE: (within /usr/lib/libasound.so.2.0.0) ==22198== by 0x80484D7: main (in /home/lennart/alsa-test) And there seem to be more. When PulseAudio starts up it probes for available audio devices. On my machine there are 5 sound cards, which are all probed with device strings like "surround51:0" and so on, and in the end some memory with a caller history like the one shown above is shown, and another one: ==22229== 235 (180 direct, 55 indirect) bytes in 5 blocks are definitely lost in loss record 6 of 6 ==22229== at 0x4023AA4: calloc (vg_replace_malloc.c:279) ==22229== by 0x46967B8: (within /usr/lib/libasound.so.2.0.0) ==22229== by 0x4696C48: snd_config_make (in /usr/lib/libasound.so.2.0.0) ==22229== by 0x4696CD0: snd_config_imake_pointer (in /usr/lib/libasound.so.2.0.0) ==22229== by 0x46EA573: _snd_pcm_hook_ctl_elems_install (in /usr/lib/libasound.so.2.0.0) ==22229== by 0x46EAC10: (within /usr/lib/libasound.so.2.0.0) ==22229== by 0x46EB2BC: _snd_pcm_hooks_open (in /usr/lib/libasound.so.2.0.0) ==22229== by 0x46C0CD4: (within /usr/lib/libasound.so.2.0.0) ==22229== by 0x46C1467: snd_pcm_open_slave (in /usr/lib/libasound.so.2.0.0) ==22229== by 0x46D232B: _snd_pcm_route_open (in /usr/lib/libasound.so.2.0.0) ==22229== by 0x46C0CD4: (within /usr/lib/libasound.so.2.0.0) ==22229== by 0x46C1467: snd_pcm_open_slave (in /usr/lib/libasound.so.2.0.0) ==22229== by 0x46E47CC: _snd_pcm_empty_open (in /usr/lib/libasound.so.2.0.0) ==22229== by 0x46C0CD4: (within /usr/lib/libasound.so.2.0.0) ==22229== by 0x46C13D2: (within /usr/lib/libasound.so.2.0.0) ==22229== by 0x412E007: pa_alsa_open_by_device_id (alsa-util.c:529) ==22229== by 0x4135904: module_alsa_sink_LTX_pa__init (module-alsa-sink.c:781) ==22229== by 0x419CD2B: pa_module_load (module.c:115) ==22229== by 0x4164D33: hal_device_load_alsa (module-hal-detect.c:204) ==22229== by 0x4165313: hal_device_add (module-hal-detect.c:323) Lennart -- Lennart Poettering Red Hat, Inc. lennart [at] poettering [dot] net ICQ# 11060553 http://0pointer.net/lennart/ GnuPG 0x1A015CC4

3 6

[alsa-devel] snd_pcm_name() returns NULL is used on a surround51: device
by Lennart Poettering 26 Nov '07

26 Nov '07

Hi! snd_pcm_name() returns NULL if used shortly after opening a "surround51" device. ("surround51" is just an example, it returns NULL for a lot of other devices, too.) Also reported as bug in the BTS: https://bugtrack.alsa-project.org/alsa-bug/view.php?id=3563 Lennart -- Lennart Poettering Red Hat, Inc. lennart [at] poettering [dot] net ICQ# 11060553 http://0pointer.net/lennart/ GnuPG 0x1A015CC4

2 1

Re: [alsa-devel] How to cont the sound cards?
by Takashi Iwai 26 Nov '07

26 Nov '07

At Mon, 26 Nov 2007 12:53:52 +0200, Cosmin Humeniuc wrote: > > Thank you for the reply, Takashi. > > The problem is, I'm not sure what should I loop through. I saw this > list on the wiki ( > http://www.alsa-project.org/main/index.php/DeviceNames ) but I don't > know how it was obtained. The code would be like: int card = -1; while (!snd_card_next(&card) && card >= 0) { ... } Takashi > > Cosmin > > On 11/23/07, Takashi Iwai <tiwai(a)suse.de> wrote: > > At Mon, 19 Nov 2007 09:12:51 +0200, > > Cosmin Humeniuc wrote: > > > > > > Hi, > > > > > > I am trying to develop an application using the ALSA library. I read > > > in a book about the snd_cards function, but it doesn't seem to be > > > defined anywhere. (The book is rather old, though, it talks about the > > > 0.5.x series). > > > > > > The compiler doesn't complain, but the linker gives me an "undefined > > > reference to `snd_cards'" error. I assume this function is deprecated, > > > but what can I use instead? > > > > Oh that myth. I myself already forgot about that function :) > > > > On ALSA 1.x, there is no corresponding function. You'll need to > > loop and check the presence of each card. > > > > > > Takashi > > >

1 0

[alsa-devel] [PATCH] Use different buffer metrics in the PulseAudio plugin
by Lennart Poettering 26 Nov '07

26 Nov '07

Hi! Please commit this patch to alsa-plugins we ship in Fedora: http://cvs.fedoraproject.org/viewcvs/*checkout*/devel/alsa-plugins/1.0.14-b… It increases the "pre-buffering level" (i.e. start threshold) to the full buffer size minus one period. This makes PA work a little bit more like normal audio devices, and makes a few drop outs go away for software which uses very small period sizes. It also increases the initial maximum buffer size, which allows a small overcommit. That's not really an issue, but cleaner nonetheless so I smuggled it into this patch. Also reported in the ALSA BTS: https://bugtrack.alsa-project.org/alsa-bug/view.php?id=3578 Lennart -- Lennart Poettering Red Hat, Inc. lennart [at] poettering [dot] net ICQ# 11060553 http://0pointer.net/lennart/ GnuPG 0x1A015CC4

2 1