| From: Takashi Iwai <tiwai@suse.de> |
| Date: Wed, 10 Feb 2016 12:47:03 +0100 |
| Subject: ALSA: timer: Call notifier in the same spinlock |
| |
| commit f65e0d299807d8a11812845c972493c3f9a18e10 upstream. |
| |
| snd_timer_notify1() is called outside the spinlock and it retakes the |
| lock after the unlock. This is rather racy, and it's safer to move |
| snd_timer_notify() call inside the main spinlock. |
| |
| The patch also contains a slight refactoring / cleanup of the code. |
| Now all start/stop/continue/pause look more symmetric and a bit better |
| readable. |
| |
| Signed-off-by: Takashi Iwai <tiwai@suse.de> |
| [bwh: Backported to 3.16: |
| - Fix up another use of "event" in _snd_timer_stop() |
| - Adjust context] |
| Signed-off-by: Ben Hutchings <ben@decadent.org.uk> |
| --- |
| --- a/sound/core/timer.c |
| +++ b/sound/core/timer.c |
| @@ -347,8 +347,6 @@ int snd_timer_open(struct snd_timer_inst |
| return err; |
| } |
| |
| -static int _snd_timer_stop(struct snd_timer_instance *timeri, int event); |
| - |
| /* |
| * close a timer instance |
| * call this with register_mutex down. |
| @@ -445,7 +443,6 @@ unsigned long snd_timer_resolution(struc |
| static void snd_timer_notify1(struct snd_timer_instance *ti, int event) |
| { |
| struct snd_timer *timer; |
| - unsigned long flags; |
| unsigned long resolution = 0; |
| struct snd_timer_instance *ts; |
| struct timespec tstamp; |
| @@ -469,34 +466,66 @@ static void snd_timer_notify1(struct snd |
| return; |
| if (timer->hw.flags & SNDRV_TIMER_HW_SLAVE) |
| return; |
| - spin_lock_irqsave(&timer->lock, flags); |
| list_for_each_entry(ts, &ti->slave_active_head, active_list) |
| if (ts->ccallback) |
| ts->ccallback(ts, event + 100, &tstamp, resolution); |
| - spin_unlock_irqrestore(&timer->lock, flags); |
| } |
| |
| -static int snd_timer_start1(struct snd_timer *timer, struct snd_timer_instance *timeri, |
| - unsigned long sticks) |
| +/* start/continue a master timer */ |
| +static int snd_timer_start1(struct snd_timer_instance *timeri, |
| + bool start, unsigned long ticks) |
| { |
| + struct snd_timer *timer; |
| + int result; |
| + unsigned long flags; |
| + |
| + timer = timeri->timer; |
| + if (!timer) |
| + return -EINVAL; |
| + |
| + spin_lock_irqsave(&timer->lock, flags); |
| + if (timer->card && timer->card->shutdown) { |
| + result = -ENODEV; |
| + goto unlock; |
| + } |
| + if (timeri->flags & (SNDRV_TIMER_IFLG_RUNNING | |
| + SNDRV_TIMER_IFLG_START)) { |
| + result = -EBUSY; |
| + goto unlock; |
| + } |
| + |
| + if (start) |
| + timeri->ticks = timeri->cticks = ticks; |
| + else if (!timeri->cticks) |
| + timeri->cticks = 1; |
| + timeri->pticks = 0; |
| + |
| list_move_tail(&timeri->active_list, &timer->active_list_head); |
| if (timer->running) { |
| if (timer->hw.flags & SNDRV_TIMER_HW_SLAVE) |
| goto __start_now; |
| timer->flags |= SNDRV_TIMER_FLG_RESCHED; |
| timeri->flags |= SNDRV_TIMER_IFLG_START; |
| - return 1; /* delayed start */ |
| + result = 1; /* delayed start */ |
| } else { |
| - timer->sticks = sticks; |
| + if (start) |
| + timer->sticks = ticks; |
| timer->hw.start(timer); |
| __start_now: |
| timer->running++; |
| timeri->flags |= SNDRV_TIMER_IFLG_RUNNING; |
| - return 0; |
| + result = 0; |
| } |
| + snd_timer_notify1(timeri, start ? SNDRV_TIMER_EVENT_START : |
| + SNDRV_TIMER_EVENT_CONTINUE); |
| + unlock: |
| + spin_unlock_irqrestore(&timer->lock, flags); |
| + return result; |
| } |
| |
| -static int snd_timer_start_slave(struct snd_timer_instance *timeri) |
| +/* start/continue a slave timer */ |
| +static int snd_timer_start_slave(struct snd_timer_instance *timeri, |
| + bool start) |
| { |
| unsigned long flags; |
| |
| @@ -510,88 +539,37 @@ static int snd_timer_start_slave(struct |
| spin_lock(&timeri->timer->lock); |
| list_add_tail(&timeri->active_list, |
| &timeri->master->slave_active_head); |
| + snd_timer_notify1(timeri, start ? SNDRV_TIMER_EVENT_START : |
| + SNDRV_TIMER_EVENT_CONTINUE); |
| spin_unlock(&timeri->timer->lock); |
| } |
| spin_unlock_irqrestore(&slave_active_lock, flags); |
| return 1; /* delayed start */ |
| } |
| |
| -/* |
| - * start the timer instance |
| - */ |
| -int snd_timer_start(struct snd_timer_instance *timeri, unsigned int ticks) |
| -{ |
| - struct snd_timer *timer; |
| - int result = -EINVAL; |
| - unsigned long flags; |
| - |
| - if (timeri == NULL || ticks < 1) |
| - return -EINVAL; |
| - if (timeri->flags & SNDRV_TIMER_IFLG_SLAVE) { |
| - result = snd_timer_start_slave(timeri); |
| - if (result >= 0) |
| - snd_timer_notify1(timeri, SNDRV_TIMER_EVENT_START); |
| - return result; |
| - } |
| - timer = timeri->timer; |
| - if (timer == NULL) |
| - return -EINVAL; |
| - if (timer->card && timer->card->shutdown) |
| - return -ENODEV; |
| - spin_lock_irqsave(&timer->lock, flags); |
| - if (timeri->flags & (SNDRV_TIMER_IFLG_RUNNING | |
| - SNDRV_TIMER_IFLG_START)) { |
| - result = -EBUSY; |
| - goto unlock; |
| - } |
| - timeri->ticks = timeri->cticks = ticks; |
| - timeri->pticks = 0; |
| - result = snd_timer_start1(timer, timeri, ticks); |
| - unlock: |
| - spin_unlock_irqrestore(&timer->lock, flags); |
| - if (result >= 0) |
| - snd_timer_notify1(timeri, SNDRV_TIMER_EVENT_START); |
| - return result; |
| -} |
| - |
| -static int _snd_timer_stop(struct snd_timer_instance *timeri, int event) |
| +/* stop/pause a master timer */ |
| +static int snd_timer_stop1(struct snd_timer_instance *timeri, bool stop) |
| { |
| struct snd_timer *timer; |
| + int result = 0; |
| unsigned long flags; |
| |
| - if (snd_BUG_ON(!timeri)) |
| - return -ENXIO; |
| - |
| - if (timeri->flags & SNDRV_TIMER_IFLG_SLAVE) { |
| - spin_lock_irqsave(&slave_active_lock, flags); |
| - if (!(timeri->flags & SNDRV_TIMER_IFLG_RUNNING)) { |
| - spin_unlock_irqrestore(&slave_active_lock, flags); |
| - return -EBUSY; |
| - } |
| - if (timeri->timer) |
| - spin_lock(&timeri->timer->lock); |
| - timeri->flags &= ~SNDRV_TIMER_IFLG_RUNNING; |
| - list_del_init(&timeri->ack_list); |
| - list_del_init(&timeri->active_list); |
| - if (timeri->timer) |
| - spin_unlock(&timeri->timer->lock); |
| - spin_unlock_irqrestore(&slave_active_lock, flags); |
| - goto __end; |
| - } |
| timer = timeri->timer; |
| if (!timer) |
| return -EINVAL; |
| spin_lock_irqsave(&timer->lock, flags); |
| if (!(timeri->flags & (SNDRV_TIMER_IFLG_RUNNING | |
| SNDRV_TIMER_IFLG_START))) { |
| - spin_unlock_irqrestore(&timer->lock, flags); |
| - return -EBUSY; |
| + result = -EBUSY; |
| + goto unlock; |
| } |
| list_del_init(&timeri->ack_list); |
| list_del_init(&timeri->active_list); |
| - if (timer->card && timer->card->shutdown) { |
| - spin_unlock_irqrestore(&timer->lock, flags); |
| - return 0; |
| + if (timer->card && timer->card->shutdown) |
| + goto unlock; |
| + if (stop) { |
| + timeri->cticks = timeri->ticks; |
| + timeri->pticks = 0; |
| } |
| if ((timeri->flags & SNDRV_TIMER_IFLG_RUNNING) && |
| !(--timer->running)) { |
| @@ -606,39 +584,64 @@ static int _snd_timer_stop(struct snd_ti |
| } |
| } |
| timeri->flags &= ~(SNDRV_TIMER_IFLG_RUNNING | SNDRV_TIMER_IFLG_START); |
| - if (event == SNDRV_TIMER_EVENT_STOP) |
| + if (stop) |
| timeri->flags &= ~SNDRV_TIMER_IFLG_PAUSED; |
| else |
| timeri->flags |= SNDRV_TIMER_IFLG_PAUSED; |
| + snd_timer_notify1(timeri, stop ? SNDRV_TIMER_EVENT_STOP : |
| + SNDRV_TIMER_EVENT_CONTINUE); |
| + unlock: |
| spin_unlock_irqrestore(&timer->lock, flags); |
| - __end: |
| - if (event != SNDRV_TIMER_EVENT_RESOLUTION) |
| - snd_timer_notify1(timeri, event); |
| + return result; |
| +} |
| + |
| +/* stop/pause a slave timer */ |
| +static int snd_timer_stop_slave(struct snd_timer_instance *timeri, bool stop) |
| +{ |
| + unsigned long flags; |
| + |
| + spin_lock_irqsave(&slave_active_lock, flags); |
| + if (!(timeri->flags & SNDRV_TIMER_IFLG_RUNNING)) { |
| + spin_unlock_irqrestore(&slave_active_lock, flags); |
| + return -EBUSY; |
| + } |
| + timeri->flags &= ~SNDRV_TIMER_IFLG_RUNNING; |
| + if (timeri->timer) { |
| + spin_lock(&timeri->timer->lock); |
| + list_del_init(&timeri->ack_list); |
| + list_del_init(&timeri->active_list); |
| + snd_timer_notify1(timeri, stop ? SNDRV_TIMER_EVENT_STOP : |
| + SNDRV_TIMER_EVENT_CONTINUE); |
| + spin_unlock(&timeri->timer->lock); |
| + } |
| + spin_unlock_irqrestore(&slave_active_lock, flags); |
| return 0; |
| } |
| |
| /* |
| + * start the timer instance |
| + */ |
| +int snd_timer_start(struct snd_timer_instance *timeri, unsigned int ticks) |
| +{ |
| + if (timeri == NULL || ticks < 1) |
| + return -EINVAL; |
| + if (timeri->flags & SNDRV_TIMER_IFLG_SLAVE) |
| + return snd_timer_start_slave(timeri, true); |
| + else |
| + return snd_timer_start1(timeri, true, ticks); |
| +} |
| + |
| +/* |
| * stop the timer instance. |
| * |
| * do not call this from the timer callback! |
| */ |
| int snd_timer_stop(struct snd_timer_instance *timeri) |
| { |
| - struct snd_timer *timer; |
| - unsigned long flags; |
| - int err; |
| - |
| - err = _snd_timer_stop(timeri, SNDRV_TIMER_EVENT_STOP); |
| - if (err < 0) |
| - return err; |
| - timer = timeri->timer; |
| - if (!timer) |
| - return -EINVAL; |
| - spin_lock_irqsave(&timer->lock, flags); |
| - timeri->cticks = timeri->ticks; |
| - timeri->pticks = 0; |
| - spin_unlock_irqrestore(&timer->lock, flags); |
| - return 0; |
| + if (timeri->flags & SNDRV_TIMER_IFLG_SLAVE) |
| + return snd_timer_stop_slave(timeri, true); |
| + else |
| + return snd_timer_stop1(timeri, true); |
| } |
| |
| /* |
| @@ -646,36 +649,14 @@ int snd_timer_stop(struct snd_timer_inst |
| */ |
| int snd_timer_continue(struct snd_timer_instance *timeri) |
| { |
| - struct snd_timer *timer; |
| - int result = -EINVAL; |
| - unsigned long flags; |
| - |
| - if (timeri == NULL) |
| - return result; |
| /* timer can continue only after pause */ |
| if (!(timeri->flags & SNDRV_TIMER_IFLG_PAUSED)) |
| return -EINVAL; |
| |
| if (timeri->flags & SNDRV_TIMER_IFLG_SLAVE) |
| - return snd_timer_start_slave(timeri); |
| - timer = timeri->timer; |
| - if (! timer) |
| - return -EINVAL; |
| - if (timer->card && timer->card->shutdown) |
| - return -ENODEV; |
| - spin_lock_irqsave(&timer->lock, flags); |
| - if (timeri->flags & SNDRV_TIMER_IFLG_RUNNING) { |
| - result = -EBUSY; |
| - goto unlock; |
| - } |
| - if (!timeri->cticks) |
| - timeri->cticks = 1; |
| - timeri->pticks = 0; |
| - result = snd_timer_start1(timer, timeri, timer->sticks); |
| - unlock: |
| - spin_unlock_irqrestore(&timer->lock, flags); |
| - snd_timer_notify1(timeri, SNDRV_TIMER_EVENT_CONTINUE); |
| - return result; |
| + return snd_timer_start_slave(timeri, false); |
| + else |
| + return snd_timer_start1(timeri, false, 0); |
| } |
| |
| /* |
| @@ -683,7 +664,10 @@ int snd_timer_continue(struct snd_timer_ |
| */ |
| int snd_timer_pause(struct snd_timer_instance * timeri) |
| { |
| - return _snd_timer_stop(timeri, SNDRV_TIMER_EVENT_PAUSE); |
| + if (timeri->flags & SNDRV_TIMER_IFLG_SLAVE) |
| + return snd_timer_stop_slave(timeri, false); |
| + else |
| + return snd_timer_stop1(timeri, false); |
| } |
| |
| /* |