snd_usb_caiaq: fix potential lockups locking
Subsystems (2)
PATCH 1/1 snd_usb_caiaq: fix potential lockups locking
Hi,
attached is a patch for snd_usb_caiaq which fixes system potential lockups when stoping and restarting the stream very quickly multiple times in a row which is what some players do when the user drags the song position slider.
Also, it seems that snd_pcm_period_elapsed() must not be called when holding a spinlock? The documentation[*], however, doesn't state that.
As this fixes an annoying bug, I would appreciate seeing that one being pushed into 2.6.25. Still possible?
Thanks and best regards,
Daniel
----
Subject: [PATCH] snd_usb_caiaq: fix potential lockups
This patch fixes potential lockups in snd_usb_caiaq by refining the locking mechanims and by using usb_kill_urb() in favor to usb_unlink_urb().
sound/usb/caiaq/caiaq-audio.c +29 -42
@@ -1,5 +1,5 @@/* -* Copyright (c) 2006,2007 Daniel Mack, Karsten Wiese +* Copyright (c) 2006-2008 Daniel Mack, Karsten Wiese * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by
@@ -77,10 +77,15 @@ static void deactivate_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] =NULL ;else dev->sub_capture[sub->number] =NULL ; + +spin_unlock_irqrestore (&dev->spinlock, flags); } static int
@@ -97,13 +102,13 @@ static int stream_start(struct snd_usb_caiaqdev *dev) {int i, ret; -debug ("stream_start(%p)\n" , dev); -spin_lock_irq (&dev->spinlock); -if (dev->streaming) { -spin_unlock_irq (&dev->spinlock); +debug ("%s(%p)\n" , __func__, dev); + +if (dev->streaming)return -EINVAL ; - } +memset (dev->sub_playback,0 ,sizeof (dev->sub_playback)); +memset (dev->sub_capture,0 ,sizeof (dev->sub_capture)); dev->input_panic =0 ; dev->output_panic =0 ; dev->first_packet =1 ;
@@ -112,37 +117,35 @@ static int stream_start(struct snd_usb_caiaqdev *dev)for (i =0 ; i <N_URBS ; i++) { ret =usb_submit_urb (dev->data_urbs_in[i],GFP_ATOMIC );if (ret) { -log ("unable to trigger initial read #%d! (ret = %d)\n" , - i, ret); +log ("unable to trigger read #%d! (ret %d)\n" , i, ret); dev->streaming =0 ; -spin_unlock_irq (&dev->spinlock);return -EPIPE ; } } -spin_unlock_irq (&dev->spinlock);return 0 ; }static void stream_stop (struct snd_usb_caiaqdev *dev) {int i; - -debug ("stream_stop(%p)\n" , dev); + +debug ("%s(%p)\n" , __func__, dev);if (!dev->streaming)return ; dev->streaming =0 ; +for (i =0 ; i <N_URBS ; i++) { -usb_unlink_urb (dev->data_urbs_in[i]); -usb_unlink_urb (dev->data_urbs_out[i]); +usb_kill_urb (dev->data_urbs_in[i]); +usb_kill_urb (dev->data_urbs_out[i]); } }static int snd_usb_caiaq_substream_open (struct snd_pcm_substream *substream) {struct snd_usb_caiaqdev *dev =snd_pcm_substream_chip (substream); -debug ("snd_usb_caiaq_substream_open(%p)\n" , substream); +debug ("%s(%p)\n" , __func__, substream); substream->runtime->hw = dev->pcm_info;snd_pcm_limit_hw_rates (substream->runtime);return 0 ;
@@ -152,7 +155,7 @@ static int snd_usb_caiaq_substream_close(struct snd_pcm_substream *substream) {struct snd_usb_caiaqdev *dev =snd_pcm_substream_chip (substream); -debug ("snd_usb_caiaq_substream_close(%p)\n" , substream); +debug ("%s(%p)\n" , __func__, substream);if (all_substreams_zero (dev->sub_playback) &&all_substreams_zero (dev->sub_capture)) {/* when the last client has stopped streaming,
@@ -160,24 +163,22 @@ static int snd_usb_caiaq_substream_close(struct snd_pcm_substream *substream)stream_stop (dev); dev->pcm_info.rates = dev->samplerates; } - +return 0 ; }static int snd_usb_caiaq_pcm_hw_params (struct snd_pcm_substream *sub,struct snd_pcm_hw_params *hw_params) { -debug ("snd_usb_caiaq_pcm_hw_params(%p)\n" , sub); +debug ("%s(%p)\n" , __func__, sub);return snd_pcm_lib_malloc_pages (sub,params_buffer_bytes (hw_params)); }static int snd_usb_caiaq_pcm_hw_free (struct snd_pcm_substream *sub) {struct snd_usb_caiaqdev *dev =snd_pcm_substream_chip (sub); -debug ("snd_usb_caiaq_pcm_hw_free(%p)\n" , sub); -spin_lock_irq (&dev->spinlock); +debug ("%s(%p)\n" , __func__, sub);deactivate_substream (dev, sub); -spin_unlock_irq (&dev->spinlock);return snd_pcm_lib_free_pages (sub); }
@@ -196,7 +197,7 @@ static int snd_usb_caiaq_pcm_prepare(struct snd_pcm_substream *substream)struct snd_usb_caiaqdev *dev =snd_pcm_substream_chip (substream);struct snd_pcm_runtime *runtime = substream->runtime; -debug ("snd_usb_caiaq_pcm_prepare(%p)\n" , substream); +debug ("%s(%p)\n" , __func__, substream);if (substream->stream ==SNDRV_PCM_STREAM_PLAYBACK ) dev->audio_out_buf_pos[index] =BYTES_PER_SAMPLE +1 ;
@@ -247,15 +248,11 @@ static int snd_usb_caiaq_pcm_trigger(struct snd_pcm_substream *sub, int cmd)switch (cmd) {case SNDRV_PCM_TRIGGER_START :case SNDRV_PCM_TRIGGER_PAUSE_RELEASE : -spin_lock (&dev->spinlock);activate_substream (dev, sub); -spin_unlock (&dev->spinlock);break ;case SNDRV_PCM_TRIGGER_STOP :case SNDRV_PCM_TRIGGER_PAUSE_PUSH : -spin_lock (&dev->spinlock);deactivate_substream (dev, sub); -spin_unlock (&dev->spinlock);break ;default :return -EINVAL ;
@@ -328,8 +325,6 @@ static void read_in_urb_mode0(struct snd_usb_caiaqdev *dev,if (all_substreams_zero (dev->sub_capture))return ; -spin_lock (&dev->spinlock); -for (i =0 ; i < iso->actual_length;) {for (stream =0 ; stream < dev->n_streams; stream++, i++) { sub = dev->sub_capture[stream];
@@ -345,8 +340,6 @@ static void read_in_urb_mode0(struct snd_usb_caiaqdev *dev, } } } - -spin_unlock (&dev->spinlock); } static void read_in_urb_mode2(struct snd_usb_caiaqdev *dev,
@@ -358,8 +351,6 @@ static void read_in_urb_mode2(struct snd_usb_caiaqdev *dev,struct snd_pcm_substream *sub;int stream, i; -spin_lock (&dev->spinlock); -for (i =0 ; i < iso->actual_length;) {if (i % (dev->n_streams *BYTES_PER_SAMPLE_USB ) ==0 ) { for (stream = 0;
@@ -393,8 +384,6 @@ static void read_in_urb_mode2(struct snd_usb_caiaqdev *dev, } } } - -spin_unlock (&dev->spinlock); } static void read_in_urb(struct snd_usb_caiaqdev *dev,
@@ -418,8 +407,6 @@ static void read_in_urb(struct snd_usb_caiaqdev *dev, dev->input_panic ?"(input)" :"" , dev->output_panic ? "(output)" : ""); } - -check_for_elapsed_periods (dev, dev->sub_capture); } static void fill_out_urb(struct snd_usb_caiaqdev *dev,
@@ -429,8 +416,6 @@ static void fill_out_urb(struct snd_usb_caiaqdev *dev,unsigned char *usb_buf = urb->transfer_buffer + iso->offset;struct snd_pcm_substream *sub;int stream, i; - -spin_lock (&dev->spinlock);for (i =0 ; i < iso->length;) {for (stream =0 ; stream < dev->n_streams; stream++, i++) {
@@ -456,9 +441,6 @@ static void fill_out_urb(struct snd_usb_caiaqdev *dev,for (stream =0 ; stream < dev->n_streams; stream++, i++) usb_buf[i] =MAKE_CHECKBYTE (dev, stream, i); } - -spin_unlock (&dev->spinlock); -check_for_elapsed_periods (dev, dev->sub_playback); }static void read_completed (struct urb *urb)
@@ -472,6 +454,7 @@ static void read_completed(struct urb *urb)return ; dev = info->dev; +if (!dev->streaming)return ;
@@ -489,8 +472,12 @@ static void read_completed(struct urb *urb) out->iso_frame_desc[outframe].offset =BYTES_PER_FRAME * frame;if (len >0 ) { +spin_lock (&dev->spinlock);fill_out_urb (dev, out, &out->iso_frame_desc[outframe]);read_in_urb (dev, urb, &urb->iso_frame_desc[frame]); +spin_unlock (&dev->spinlock); +check_for_elapsed_periods (dev, dev->sub_playback); +check_for_elapsed_periods (dev, dev->sub_capture); send_it =1 ; }
@@ -696,7 +683,7 @@ int snd_usb_caiaq_audio_init(struct snd_usb_caiaqdev *dev)void snd_usb_caiaq_audio_free (struct snd_usb_caiaqdev *dev) { -debug ("snd_usb_caiaq_audio_free (%p)\n" , dev); +debug ("%s(%p)\n" , __func__, dev);stream_stop (dev);free_urbs (dev->data_urbs_in);free_urbs (dev->data_urbs_out);
sound/usb/caiaq/caiaq-device.c +2 -2
@@ -42,7 +42,7 @@#endif MODULE_AUTHOR ("Daniel Mack <[email protected]>" ); -MODULE_DESCRIPTION ("caiaq USB audio, version 1.3.2" ); +MODULE_DESCRIPTION ("caiaq USB audio, version 1.3.4" );MODULE_LICENSE ("GPL" );MODULE_SUPPORTED_DEVICE ("{{Native Instruments, RigKontrol2}," "{Native Instruments, RigKontrol3},"
@@ -456,7 +456,7 @@ static void snd_disconnect(struct usb_interface *intf)struct snd_usb_caiaqdev *dev;struct snd_card *card =dev_get_drvdata (&intf->dev); -debug ("snd_disconnect(%p)\n" , intf); +debug ("%s(%p)\n" , __func__, intf);if (!card)return ;
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel