[alsa-devel] Questions on locking, snd-usb-caiaq

[Mark Hills]

I fixed some race issues in snd-usb-caiaq, but have some questions on the 
interface between the driver and ALSA.

First fix is a lock in pointer(), which caused snd_pcm_update_hw_ptr_pos() 
to report an out of range buffer position (even though it was sucessfully 
corrected to zero).

Second is a lock as part of trigger(). On testing, this appears to fix an 
issue where white noise could sometimes be transferred by the driver.

According to the documentation, trigger() and pointer() are called with 
local interrupts disabled. This means they should really be non-irqsave 
locks?

(http://www.alsa-project.org/~tiwai/writing-an-alsa-driver/ch05s08.html)

Even for callbacks documented as atomic, if read_completed() and 
write_completed() can be called at any time interrupts are enabled (eg. on 
another CPU) it seems that additional locking my be required. Candidates 
are:

   snd_usb_caiaq_substream_open
   snd_usb_caiaq_substream_close
   snd_usb_caiaq_pcm_hw_free
   snd_usb_caiaq_pcm_prepare

Are there any guarantees on these functions which mean the lock is not 
required?

To lock in prepare() would (I think) require demoting a 
spin_lock_irqsave() to a spin_lock() to send initialisation commands. If 
so, what is the best design for this?

Thanks,

-- 
Mark

diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
index 121af06..c659f81 100644
--- a/sound/usb/caiaq/audio.c
+++ b/sound/usb/caiaq/audio.c
@@ -62,10 +62,15 @@ static void
  activate_substream(struct snd_usb_caiaqdev *dev,
  	           struct snd_pcm_substream *sub)
  {
+	unsigned long flags;
+	spin_lock_irqsave(&dev->spinlock, flags);
+
  	if (sub->stream == SNDRV_PCM_STREAM_PLAYBACK)
  		dev->sub_playback[sub->number] = sub;
  	else
  		dev->sub_capture[sub->number] = sub;
+
+	spin_unlock_irqrestore(&dev->spinlock, flags);
  }

  static void
@@ -269,16 +274,23 @@ snd_usb_caiaq_pcm_pointer(struct snd_pcm_substream *sub)
  {
  	int index = sub->number;
  	struct snd_usb_caiaqdev *dev = snd_pcm_substream_chip(sub);
+	unsigned long flags;
+	snd_pcm_uframes_t ptr;
+
+	spin_lock_irqsave(&dev->spinlock, flags);

  	if (dev->input_panic || dev->output_panic)
-		return SNDRV_PCM_POS_XRUN;
+		ptr = SNDRV_PCM_POS_XRUN;

  	if (sub->stream == SNDRV_PCM_STREAM_PLAYBACK)
-		return bytes_to_frames(sub->runtime,
+		ptr = bytes_to_frames(sub->runtime,
  					dev->audio_out_buf_pos[index]);
  	else
-		return bytes_to_frames(sub->runtime,
+		ptr = bytes_to_frames(sub->runtime,
  					dev->audio_in_buf_pos[index]);
+
+	spin_unlock_irqrestore(&dev->spinlock, flags);
+	return ptr;
  }

  /* operators for both playback and capture */