Skip to content

Commit

Permalink
ALSA: timer: Call notifier in the same spinlock
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
Takashi Iwai committed Feb 12, 2016
1 parent 9984d1b commit f65e0d2
Showing 1 changed file with 102 additions and 118 deletions.
220 changes: 102 additions & 118 deletions sound/core/timer.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,8 +305,6 @@ int snd_timer_open(struct snd_timer_instance **ti,
return 0;
}

static int _snd_timer_stop(struct snd_timer_instance *timeri, int event);

/*
* close a timer instance
*/
Expand Down Expand Up @@ -389,7 +387,6 @@ unsigned long snd_timer_resolution(struct snd_timer_instance *timeri)
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;
Expand All @@ -413,34 +410,66 @@ static void snd_timer_notify1(struct snd_timer_instance *ti, int event)
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;

Expand All @@ -454,88 +483,37 @@ static int snd_timer_start_slave(struct snd_timer_instance *timeri)
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)) {
Expand All @@ -550,76 +528,82 @@ static int _snd_timer_stop(struct snd_timer_instance *timeri, int event)
}
}
timeri->flags &= ~(SNDRV_TIMER_IFLG_RUNNING | SNDRV_TIMER_IFLG_START);
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);
}

/*
* start again.. the tick is kept.
*/
int snd_timer_continue(struct snd_timer_instance *timeri)
{
struct snd_timer *timer;
int result = -EINVAL;
unsigned long flags;

if (timeri == NULL)
return result;
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);
}

/*
* pause.. remember the ticks left
*/
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);
}

/*
Expand Down

0 comments on commit f65e0d2

Please sign in to comment.