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 */ _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
