At Wed, 17 Oct 2012 19:05:46 +0530, Omair Mohammed Abdullah wrote:
If the timer is deleted while the timer handler is running, it may get rescheduled and an unnecessary period elapsed will be sent.
Add a flag to reschedule the timer only if it has not been stopped.
In which situation is it rescheduled exactly?
Actually there is a small race in the check of running bit and the timer lock. Do you mean this?
If so, wouldn't it be cleaner to get rid of timer_lock and protect the whole start/stop with cable->lock, too, like below?
thanks,
Takashi
--- diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c index 0fe6d64..1904046 100644 --- a/sound/drivers/aloop.c +++ b/sound/drivers/aloop.c @@ -120,7 +120,6 @@ struct loopback_pcm { unsigned int last_drift; unsigned long last_jiffies; struct timer_list timer; - spinlock_t timer_lock; };
static struct platform_device *devices[SNDRV_CARDS]; @@ -166,12 +165,12 @@ static inline unsigned int get_rate_shift(struct loopback_pcm *dpcm) return get_setup(dpcm)->rate_shift; }
+/* call in cable->lock */ static void loopback_timer_start(struct loopback_pcm *dpcm) { unsigned long tick; unsigned int rate_shift = get_rate_shift(dpcm);
- spin_lock(&dpcm->timer_lock); if (rate_shift != dpcm->pcm_rate_shift) { dpcm->pcm_rate_shift = rate_shift; dpcm->period_size_frac = frac_pos(dpcm, dpcm->pcm_period_size); @@ -184,15 +183,13 @@ static void loopback_timer_start(struct loopback_pcm *dpcm) tick = (tick + dpcm->pcm_bps - 1) / dpcm->pcm_bps; dpcm->timer.expires = jiffies + tick; add_timer(&dpcm->timer); - spin_unlock(&dpcm->timer_lock); }
+/* call in cable->lock */ static inline void loopback_timer_stop(struct loopback_pcm *dpcm) { - spin_lock(&dpcm->timer_lock); del_timer(&dpcm->timer); dpcm->timer.expires = 0; - spin_unlock(&dpcm->timer_lock); }
#define CABLE_VALID_PLAYBACK (1 << SNDRV_PCM_STREAM_PLAYBACK) @@ -274,8 +271,8 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd) spin_lock(&cable->lock); cable->running |= stream; cable->pause &= ~stream; - spin_unlock(&cable->lock); loopback_timer_start(dpcm); + spin_unlock(&cable->lock); if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) loopback_active_notify(dpcm); break; @@ -283,23 +280,23 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd) spin_lock(&cable->lock); cable->running &= ~stream; cable->pause &= ~stream; - spin_unlock(&cable->lock); loopback_timer_stop(dpcm); + spin_unlock(&cable->lock); if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) loopback_active_notify(dpcm); break; case SNDRV_PCM_TRIGGER_PAUSE_PUSH: spin_lock(&cable->lock); cable->pause |= stream; - spin_unlock(&cable->lock); loopback_timer_stop(dpcm); + spin_unlock(&cable->lock); break; case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: spin_lock(&cable->lock); dpcm->last_jiffies = jiffies; cable->pause &= ~stream; - spin_unlock(&cable->lock); loopback_timer_start(dpcm); + spin_unlock(&cable->lock); break; default: return -EINVAL; @@ -477,6 +474,7 @@ static inline void bytepos_finish(struct loopback_pcm *dpcm, dpcm->buf_pos %= dpcm->pcm_buffer_size; }
+/* call in cable->lock */ static unsigned int loopback_pos_update(struct loopback_cable *cable) { struct loopback_pcm *dpcm_play = @@ -485,9 +483,7 @@ static unsigned int loopback_pos_update(struct loopback_cable *cable) cable->streams[SNDRV_PCM_STREAM_CAPTURE]; unsigned long delta_play = 0, delta_capt = 0; unsigned int running, count1, count2; - unsigned long flags;
- spin_lock_irqsave(&cable->lock, flags); running = cable->running ^ cable->pause; if (running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) { delta_play = jiffies - dpcm_play->last_jiffies; @@ -529,32 +525,39 @@ static unsigned int loopback_pos_update(struct loopback_cable *cable) bytepos_finish(dpcm_play, count1); bytepos_finish(dpcm_capt, count1); unlock: - spin_unlock_irqrestore(&cable->lock, flags); return running; }
static void loopback_timer_function(unsigned long data) { struct loopback_pcm *dpcm = (struct loopback_pcm *)data; - unsigned int running; + unsigned long flags;
- running = loopback_pos_update(dpcm->cable); - if (running & (1 << dpcm->substream->stream)) { + spin_lock_irqsave(&dpcm->cable->lock, flags); + if (loopback_pos_update(dpcm->cable) & (1 << dpcm->substream->stream)) { loopback_timer_start(dpcm); if (dpcm->period_update_pending) { dpcm->period_update_pending = 0; + spin_unlock_irqrestore(&dpcm->cable->lock, flags); + /* need to unlock before calling below */ snd_pcm_period_elapsed(dpcm->substream); + return; } } + spin_unlock_irqrestore(&dpcm->cable->lock, flags); }
static snd_pcm_uframes_t loopback_pointer(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; struct loopback_pcm *dpcm = runtime->private_data; + snd_pcm_uframes_t pos;
+ spin_lock(&dpcm->cable->lock); loopback_pos_update(dpcm->cable); - return bytes_to_frames(runtime, dpcm->buf_pos); + pos = dpcm->buf_pos; + spin_unlock(&dpcm->cable->lock); + return bytes_to_frames(runtime, pos); }
static struct snd_pcm_hardware loopback_pcm_hardware = @@ -672,7 +675,6 @@ static int loopback_open(struct snd_pcm_substream *substream) dpcm->substream = substream; setup_timer(&dpcm->timer, loopback_timer_function, (unsigned long)dpcm); - spin_lock_init(&dpcm->timer_lock);
cable = loopback->cables[substream->number][dev]; if (!cable) {