At Mon, 15 Jul 2013 15:17:58 +0200, Takashi Iwai wrote:
At Mon, 15 Jul 2013 15:12:07 +0200, Jaroslav Kysela wrote:
Date 15.7.2013 15:06, Takashi Iwai wrote:
Hi,
while reviewing another patch series, I stumbled on the snd_pcm_lock() call in an unlocked context. Then I started looking through the whole tree, and found a bunch of drivers doing it wrong, too. So here is a patch series to fix them.
[PATCH 01/10] ALSA: asihpi: Fix unlocked snd_pcm_stop() call [PATCH 02/10] ALSA: atiixp: Fix unlocked snd_pcm_stop() call [PATCH 03/10] ALSA: 6fire: Fix unlocked snd_pcm_stop() call [PATCH 04/10] ALSA: ua101: Fix unlocked snd_pcm_stop() call [PATCH 05/10] ALSA: usx2y: Fix unlocked snd_pcm_stop() call [PATCH 06/10] ALSA: pxa2xx: Fix unlocked snd_pcm_stop() call [PATCH 07/10] ASoC: atmel: Fix unlocked snd_pcm_stop() call [PATCH 08/10] ASoC: s6000: Fix unlocked snd_pcm_stop() call [PATCH 09/10] [media] saa7134: Fix unlocked snd_pcm_stop() call [PATCH 10/10] staging: line6: Fix unlocked snd_pcm_stop() call
Probably, it may be better to introduce 'snd_pcm_stop_dolock()' helper (inline?) in include/sound/pcm.h .
Yes, it'll be the next step. These patches are done in open-coded way so that they can be applied easily to each stable kernel.
BTW the conversion I thought of is a patch like below.
Takashi
--- diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 84b10f9..96eda75 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -502,6 +502,7 @@ int snd_pcm_status(struct snd_pcm_substream *substream, struct snd_pcm_status *status); int snd_pcm_start(struct snd_pcm_substream *substream); int snd_pcm_stop(struct snd_pcm_substream *substream, snd_pcm_state_t status); +int snd_pcm_lock_and_stop(struct snd_pcm_substream *substream, snd_pcm_state_t status); int snd_pcm_drain_done(struct snd_pcm_substream *substream); #ifdef CONFIG_PM int snd_pcm_suspend(struct snd_pcm_substream *substream); diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index f928181..56582ac 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -809,9 +809,10 @@ static int snd_pcm_action_lock_irq(struct action_ops *ops, struct snd_pcm_substream *substream, int state) { + unsigned long flags; int res;
- read_lock_irq(&snd_pcm_link_rwlock); + read_lock_irqsave(&snd_pcm_link_rwlock, flags); if (snd_pcm_stream_linked(substream)) { spin_lock(&substream->group->lock); spin_lock(&substream->self_group.lock); @@ -823,7 +824,7 @@ static int snd_pcm_action_lock_irq(struct action_ops *ops, res = snd_pcm_action_single(ops, substream, state); spin_unlock(&substream->self_group.lock); } - read_unlock_irq(&snd_pcm_link_rwlock); + read_unlock_irqrestore(&snd_pcm_link_rwlock, flags); return res; }
@@ -900,6 +901,8 @@ static struct action_ops snd_pcm_action_start = { * @substream: the PCM substream instance * * Return: Zero if successful, or a negative error code. + * + * Note that this function must be called in substream lock context. */ int snd_pcm_start(struct snd_pcm_substream *substream) { @@ -955,6 +958,8 @@ static struct action_ops snd_pcm_action_stop = { * The state of each stream is then changed to the given state unconditionally. * * Return: Zero if succesful, or a negative error code. + * + * Note that this function must be called in substream lock context. */ int snd_pcm_stop(struct snd_pcm_substream *substream, snd_pcm_state_t state) { @@ -964,6 +969,20 @@ int snd_pcm_stop(struct snd_pcm_substream *substream, snd_pcm_state_t state) EXPORT_SYMBOL(snd_pcm_stop);
/** + * snd_pcm_lock_and_stop - lock substream and call snd_pcm_stop() + * @substream: the PCM substream instance + * @state: PCM state after stopping the stream + * + * Call this function instead of snd_pcm_stop() when need to stop a + * PCM stream outside the stream lock context. + */ +int snd_pcm_lock_and_stop(struct snd_pcm_substream *substream, snd_pcm_state_t state) +{ + return snd_pcm_action_lock_irq(&snd_pcm_action_stop, substream, state); +} +EXPORT_SYMBOL_GPL(snd_pcm_lock_and_stop); + +/** * snd_pcm_drain_done - stop the DMA only when the given stream is playback * @substream: the PCM substream *