[alsa-devel] [PATCH 0/10] snd_pcm_stop() lock fixes

[Takashi Iwai]

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
  *