From: Timo Wischer twischer@de.adit-jv.com

This is required for additional timer implementations which could detect errors and want to throw them.

Signed-off-by: Timo Wischer twischer@de.adit-jv.com Signed-off-by: Andrew Gabbasov andrew_gabbasov@mentor.com --- sound/drivers/aloop.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c index b9ee5b72a996..dc9154662d5b 100644 --- a/sound/drivers/aloop.c +++ b/sound/drivers/aloop.c @@ -176,15 +176,19 @@ static int loopback_timer_start(struct loopback_pcm *dpcm) }

/* call in cable->lock */ -static inline void loopback_timer_stop(struct loopback_pcm *dpcm) +static inline int loopback_timer_stop(struct loopback_pcm *dpcm) { del_timer(&dpcm->timer); dpcm->timer.expires = 0; + + return 0; }

-static inline void loopback_timer_stop_sync(struct loopback_pcm *dpcm) +static inline int loopback_timer_stop_sync(struct loopback_pcm *dpcm) { del_timer_sync(&dpcm->timer); + + return 0; }

#define CABLE_VALID_PLAYBACK (1 << SNDRV_PCM_STREAM_PLAYBACK) @@ -275,7 +279,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd) spin_lock(&cable->lock); cable->running &= ~stream; cable->pause &= ~stream; - loopback_timer_stop(dpcm); + err = loopback_timer_stop(dpcm); spin_unlock(&cable->lock); if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) loopback_active_notify(dpcm); @@ -284,7 +288,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd) case SNDRV_PCM_TRIGGER_SUSPEND: spin_lock(&cable->lock); cable->pause |= stream; - loopback_timer_stop(dpcm); + err = loopback_timer_stop(dpcm); spin_unlock(&cable->lock); if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) loopback_active_notify(dpcm); @@ -323,9 +327,11 @@ static int loopback_prepare(struct snd_pcm_substream *substream) struct snd_pcm_runtime *runtime = substream->runtime; struct loopback_pcm *dpcm = runtime->private_data; struct loopback_cable *cable = dpcm->cable; - int bps, salign; + int err, bps, salign;

- loopback_timer_stop_sync(dpcm); + err = loopback_timer_stop_sync(dpcm); + if (err < 0) + return err;

salign = (snd_pcm_format_physical_width(runtime->format) * runtime->channels) / 8;

From: Timo Wischer twischer@de.adit-jv.com

This commit does not change the behaviour. It only separates the jiffies timer specific implementation from the generic part.

Signed-off-by: Timo Wischer twischer@de.adit-jv.com Signed-off-by: Andrew Gabbasov andrew_gabbasov@mentor.com --- sound/drivers/aloop.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c index 92134e9c6ea7..178f7260a650 100644 --- a/sound/drivers/aloop.c +++ b/sound/drivers/aloop.c @@ -188,7 +188,7 @@ static inline unsigned int get_rate_shift(struct loopback_pcm *dpcm) }

/* call in cable->lock */ -static int loopback_timer_start(struct loopback_pcm *dpcm) +static int loopback_jiffies_timer_start(struct loopback_pcm *dpcm) { unsigned long tick; unsigned int rate_shift = get_rate_shift(dpcm); @@ -209,7 +209,7 @@ static int loopback_timer_start(struct loopback_pcm *dpcm) }

/* call in cable->lock */ -static inline int loopback_timer_stop(struct loopback_pcm *dpcm) +static inline int loopback_jiffies_timer_stop(struct loopback_pcm *dpcm) { del_timer(&dpcm->timer); dpcm->timer.expires = 0; @@ -217,7 +217,7 @@ static inline int loopback_timer_stop(struct loopback_pcm *dpcm) return 0; }

-static inline int loopback_timer_stop_sync(struct loopback_pcm *dpcm) +static inline int loopback_jiffies_timer_stop_sync(struct loopback_pcm *dpcm) { del_timer_sync(&dpcm->timer);

@@ -502,7 +502,8 @@ static inline void bytepos_finish(struct loopback_pcm *dpcm, }

/* call in cable->lock */ -static unsigned int loopback_pos_update(struct loopback_cable *cable) +static unsigned int loopback_jiffies_timer_pos_update + (struct loopback_cable *cable) { struct loopback_pcm *dpcm_play = cable->streams[SNDRV_PCM_STREAM_PLAYBACK]; @@ -555,14 +556,15 @@ static unsigned int loopback_pos_update(struct loopback_cable *cable) return running; }

-static void loopback_timer_function(struct timer_list *t) +static void loopback_jiffies_timer_function(struct timer_list *t) { struct loopback_pcm *dpcm = from_timer(dpcm, t, timer); unsigned long flags;

spin_lock_irqsave(&dpcm->cable->lock, flags); - if (loopback_pos_update(dpcm->cable) & (1 << dpcm->substream->stream)) { - loopback_timer_start(dpcm); + if (loopback_jiffies_timer_pos_update(dpcm->cable) & + (1 << dpcm->substream->stream)) { + loopback_jiffies_timer_start(dpcm); if (dpcm->period_update_pending) { dpcm->period_update_pending = 0; spin_unlock_irqrestore(&dpcm->cable->lock, flags); @@ -732,18 +734,18 @@ static void free_cable(struct snd_pcm_substream *substream)

static int loopback_jiffies_timer_open(struct loopback_pcm *dpcm) { - timer_setup(&dpcm->timer, loopback_timer_function, 0); + timer_setup(&dpcm->timer, loopback_jiffies_timer_function, 0);

return 0; }

static struct loopback_ops loopback_jiffies_timer_ops = { .open = loopback_jiffies_timer_open, - .start = loopback_timer_start, - .stop = loopback_timer_stop, - .stop_sync = loopback_timer_stop_sync, - .close_substream = loopback_timer_stop_sync, - .pos_update = loopback_pos_update, + .start = loopback_jiffies_timer_start, + .stop = loopback_jiffies_timer_stop, + .stop_sync = loopback_jiffies_timer_stop_sync, + .close_substream = loopback_jiffies_timer_stop_sync, + .pos_update = loopback_jiffies_timer_pos_update, .dpcm_info = loopback_jiffies_timer_dpcm_info, };

From: Timo Wischer twischer@de.adit-jv.com

to do synchronous audio forwarding between hardware sound card and aloop devices. Such an audio route could look like the following: Sound card -> Loopback application -> ALSA loop device -> arecord

In this case the loopback device should use the sound timer of the sound card. Without this patch the loopback application has to implement an adaptive sample rate converter to align the different clocks of the different ALSA devices.

The used timer can be selected by referring to a sound card, its device and subdevice, when loading the module: $ modprobe snd_aloop enable=1 timer_source=[<card>[.<dev>[.<subdev>]]] <card> is the name (id) of the sound card or a card number. <dev> and <subdev> are device and subdevice numbers (defaults are 0). Empty string as a value of timer_source= parameter enables previous functionality (using jiffies timer).

Signed-off-by: Timo Wischer twischer@de.adit-jv.com Signed-off-by: Andrew Gabbasov andrew_gabbasov@mentor.com --- sound/drivers/aloop.c | 466 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 465 insertions(+), 1 deletion(-)

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c index 313d7ffe6c91..6db70ebd46f6 100644 --- a/sound/drivers/aloop.c +++ b/sound/drivers/aloop.c @@ -28,6 +28,7 @@ #include <sound/pcm_params.h> #include <sound/info.h> #include <sound/initval.h> +#include <sound/timer.h>

MODULE_AUTHOR("Jaroslav Kysela perex@perex.cz"); MODULE_DESCRIPTION("A loopback soundcard"); @@ -41,6 +42,7 @@ static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR; /* ID for this card */ static bool enable[SNDRV_CARDS] = {1, [1 ... (SNDRV_CARDS - 1)] = 0}; static int pcm_substreams[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS - 1)] = 8}; static int pcm_notify[SNDRV_CARDS]; +static char *timer_source[SNDRV_CARDS];

module_param_array(index, int, NULL, 0444); MODULE_PARM_DESC(index, "Index value for loopback soundcard."); @@ -52,6 +54,8 @@ module_param_array(pcm_substreams, int, NULL, 0444); MODULE_PARM_DESC(pcm_substreams, "PCM substreams # (1-8) for loopback driver."); module_param_array(pcm_notify, int, NULL, 0444); MODULE_PARM_DESC(pcm_notify, "Break capture when PCM format/rate/channels changes."); +module_param_array(timer_source, charp, NULL, 0444); +MODULE_PARM_DESC(timer_source, "Sound card name or number and device/subdevice number of timer to be used. Empty string for jiffies timer [default].");

#define NO_PITCH 100000

@@ -102,6 +106,13 @@ struct loopback_cable { unsigned int pause; /* timer specific */ struct loopback_ops *ops; + /* If sound timer is used */ + struct { + int owner; + struct snd_timer_id id; + struct tasklet_struct event_tasklet; + struct snd_timer_instance *instance; + } snd_timer; };

struct loopback_setup { @@ -122,6 +133,7 @@ struct loopback { struct loopback_cable *cables[MAX_PCM_SUBSTREAMS][2]; struct snd_pcm *pcm[2]; struct loopback_setup setup[MAX_PCM_SUBSTREAMS][2]; + char *timer_source; };

struct loopback_pcm { @@ -145,6 +157,7 @@ struct loopback_pcm { unsigned int period_size_frac; /* period size in jiffies ticks */ unsigned int last_drift; unsigned long last_jiffies; + /* If jiffies timer is used */ struct timer_list timer; };

@@ -212,6 +225,34 @@ static int loopback_jiffies_timer_start(struct loopback_pcm *dpcm) return 0; }

+/* call in cable->lock */ +static int loopback_snd_timer_start(struct loopback_pcm *dpcm) +{ + int err; + + /* Loopback device has to use same period as timer card. Therefore + * wake up for each snd_pcm_period_elapsed() call of timer card. + */ + err = snd_timer_start(dpcm->cable->snd_timer.instance, 1); + if (err < 0) { + /* do not report error if trying to start but already + * running. For example called by opposite substream + * of the same cable + */ + if (err == -EBUSY) + return 0; + + pcm_err(dpcm->substream->pcm, + "snd_timer_start(%d,%d,%d) failed with %d", + dpcm->cable->snd_timer.id.card, + dpcm->cable->snd_timer.id.device, + dpcm->cable->snd_timer.id.subdevice, + err); + } + + return err; +} + /* call in cable->lock */ static inline int loopback_jiffies_timer_stop(struct loopback_pcm *dpcm) { @@ -221,6 +262,28 @@ static inline int loopback_jiffies_timer_stop(struct loopback_pcm *dpcm) return 0; }

+/* call in cable->lock */ +static int loopback_snd_timer_stop(struct loopback_pcm *dpcm) +{ + int err; + + /* only stop if both devices (playback and capture) are not running */ + if (dpcm->cable->running) + return 0; + + err = snd_timer_stop(dpcm->cable->snd_timer.instance); + if (err < 0) { + pcm_err(dpcm->substream->pcm, + "snd_timer_stop(%d,%d,%d) failed with %d", + dpcm->cable->snd_timer.id.card, + dpcm->cable->snd_timer.id.device, + dpcm->cable->snd_timer.id.subdevice, + err); + } + + return err; +} + static inline int loopback_jiffies_timer_stop_sync(struct loopback_pcm *dpcm) { del_timer_sync(&dpcm->timer); @@ -228,6 +291,37 @@ static inline int loopback_jiffies_timer_stop_sync(struct loopback_pcm *dpcm) return 0; }

+/* call in loopback->cable_lock */ +static int loopback_snd_timer_close_cable(struct loopback_pcm *dpcm) +{ + int err; + + /* snd_timer was not opened */ + if (!dpcm->cable->snd_timer.instance) + return 0; + + /* wait till drain tasklet has finished if requested */ + tasklet_kill(&dpcm->cable->snd_timer.event_tasklet); + + /* will only be called from free_cable() when other stream was + * already closed. Other stream cannot be reopened as long as + * loopback->cable_lock is locked. Therefore no need to lock + * cable->lock; + */ + err = snd_timer_close(dpcm->cable->snd_timer.instance); + if (err < 0) { + pcm_err(dpcm->substream->pcm, + "snd_timer_close(%d,%d,%d) failed with %d", + dpcm->cable->snd_timer.id.card, + dpcm->cable->snd_timer.id.device, + dpcm->cable->snd_timer.id.subdevice, + err); + } + memset(&dpcm->cable->snd_timer, 0, sizeof(dpcm->cable->snd_timer)); + + return err; +} + static int loopback_check_format(struct loopback_cable *cable, int stream) { struct snd_pcm_runtime *runtime, *cruntime; @@ -353,6 +447,13 @@ static void params_change(struct snd_pcm_substream *substream) cable->hw.rate_max = runtime->rate; cable->hw.channels_min = runtime->channels; cable->hw.channels_max = runtime->channels; + + if (cable->snd_timer.instance) { + cable->hw.period_bytes_min = + frames_to_bytes(runtime, runtime->period_size); + cable->hw.period_bytes_max = cable->hw.period_bytes_min; + } + }

static int loopback_prepare(struct snd_pcm_substream *substream) @@ -576,6 +677,164 @@ static void loopback_jiffies_timer_function(struct timer_list *t) spin_unlock_irqrestore(&dpcm->cable->lock, flags); }

+/* call in cable->lock */ +static int loopback_snd_timer_check_resolution(struct snd_pcm_runtime *runtime, + unsigned long resolution) +{ + if (resolution != runtime->timer_resolution) { + struct loopback_pcm *dpcm = runtime->private_data; + struct loopback_cable *cable = dpcm->cable; + /* Worst case estimation of possible values for resolution + * resolution <= (512 * 1024) frames / 8kHz in nsec + * resolution <= 65.536.000.000 nsec + * + * period_size <= 65.536.000.000 nsec / 1000nsec/usec * 192kHz + + * 500.000 + * period_size <= 12.582.912.000.000 <64bit + * / 1.000.000 usec/sec + */ + const snd_pcm_uframes_t period_size_usec = resolution / 1000 * + runtime->rate; + /* round to nearest sample rate */ + const snd_pcm_uframes_t period_size = (period_size_usec + 500 * + 1000) / (1000 * 1000); + + pcm_err(dpcm->substream->pcm, + "Period size (%lu frames) of loopback device is not corresponding to timer resolution (%lu nsec = %lu frames) of card timer %d,%d,%d. Use period size of %lu frames for loopback device.", + runtime->period_size, resolution, period_size, + cable->snd_timer.id.card, + cable->snd_timer.id.device, + cable->snd_timer.id.subdevice, + period_size); + return -EINVAL; + } + return 0; +} + +static void loopback_snd_timer_period_elapsed( + struct loopback_cable * const cable, + const int event, const unsigned long resolution) +{ + struct loopback_pcm *dpcm_play = + cable->streams[SNDRV_PCM_STREAM_PLAYBACK]; + struct loopback_pcm *dpcm_capt = + cable->streams[SNDRV_PCM_STREAM_CAPTURE]; + struct snd_pcm_runtime *valid_runtime; + unsigned int running, elapsed_bytes; + unsigned long flags; + + spin_lock_irqsave(&cable->lock, flags); + running = cable->running ^ cable->pause; + /* no need to do anything if no stream is running */ + if (!running) { + spin_unlock_irqrestore(&cable->lock, flags); + return; + } + + if (event == SNDRV_TIMER_EVENT_MSTOP) { + if (!dpcm_play || !dpcm_play->substream || + !dpcm_play->substream->runtime || + !dpcm_play->substream->runtime->status || + dpcm_play->substream->runtime->status->state != + SNDRV_PCM_STATE_DRAINING) { + spin_unlock_irqrestore(&cable->lock, flags); + return; + } + } + + valid_runtime = (running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) ? + dpcm_play->substream->runtime : + dpcm_capt->substream->runtime; + + /* resolution is only valid for SNDRV_TIMER_EVENT_TICK events */ + if (event == SNDRV_TIMER_EVENT_TICK) { + /* The hardware rules guarantee that playback and capture period + * are the same. Therefore only one device has to be checked + * here. + */ + if (loopback_snd_timer_check_resolution(valid_runtime, + resolution) < 0) { + spin_unlock_irqrestore(&cable->lock, flags); + if (cable->running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) + snd_pcm_stop_xrun(dpcm_play->substream); + if (cable->running & (1 << SNDRV_PCM_STREAM_CAPTURE)) + snd_pcm_stop_xrun(dpcm_capt->substream); + return; + } + } + + elapsed_bytes = frames_to_bytes(valid_runtime, + valid_runtime->period_size); + /* The same timer interrupt is used for playback and capture device */ + if ((running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) && + (running & (1 << SNDRV_PCM_STREAM_CAPTURE))) { + copy_play_buf(dpcm_play, dpcm_capt, elapsed_bytes); + bytepos_finish(dpcm_play, elapsed_bytes); + bytepos_finish(dpcm_capt, elapsed_bytes); + } else if (running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) { + bytepos_finish(dpcm_play, elapsed_bytes); + } else if (running & (1 << SNDRV_PCM_STREAM_CAPTURE)) { + clear_capture_buf(dpcm_capt, elapsed_bytes); + bytepos_finish(dpcm_capt, elapsed_bytes); + } + spin_unlock_irqrestore(&cable->lock, flags); + + if (running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) + snd_pcm_period_elapsed(dpcm_play->substream); + if (running & (1 << SNDRV_PCM_STREAM_CAPTURE)) + snd_pcm_period_elapsed(dpcm_capt->substream); +} + +static void loopback_snd_timer_function(struct snd_timer_instance *timeri, + unsigned long resolution, + unsigned long ticks) +{ + struct loopback_cable *cable = timeri->callback_data; + + loopback_snd_timer_period_elapsed(cable, SNDRV_TIMER_EVENT_TICK, + resolution); +} + +static void loopback_snd_timer_tasklet(unsigned long arg) +{ + struct snd_timer_instance *timeri = (struct snd_timer_instance *)arg; + struct loopback_cable *cable = timeri->callback_data; + + loopback_snd_timer_period_elapsed(cable, SNDRV_TIMER_EVENT_MSTOP, 0); +} + +static void loopback_snd_timer_event(struct snd_timer_instance * const timeri, + const int event, + struct timespec * const tstamp, + const unsigned long resolution) +{ + /* Do not lock cable->lock here because timer->lock is already hold. + * There are other functions which first lock cable->lock and than + * timer->lock e.g. + * loopback_trigger() + * spin_lock(&cable->lock) + * loopback_snd_timer_start() + * snd_timer_start() + * spin_lock(&timer->lock) + * Therefore when using the oposit order of locks here it could result + * in a deadlock. + */ + + if (event == SNDRV_TIMER_EVENT_MSTOP) { + struct loopback_cable *cable = timeri->callback_data; + + /* sound card of the timer was stopped. Therefore there will not + * be any further timer callbacks. Due to this forward audio + * data from here if in draining state. When still in running + * state the streaming will be aborted by the usual timeout. It + * should not be aborted here because may be the timer sound + * card does only a recovery and the timer is back soon. + * This tasklet triggers loopback_snd_timer_tasklet() + */ + tasklet_schedule(&cable->snd_timer.event_tasklet); + } +} + static void loopback_jiffies_timer_dpcm_info(struct loopback_pcm *dpcm, struct snd_info_buffer *buffer) { @@ -588,6 +847,18 @@ static void loopback_jiffies_timer_dpcm_info(struct loopback_pcm *dpcm, snd_iprintf(buffer, " timer_expires:\t%lu\n", dpcm->timer.expires); }

+static void loopback_snd_timer_dpcm_info(struct loopback_pcm *dpcm, + struct snd_info_buffer *buffer) +{ + snd_iprintf(buffer, " sound timer:\thw:%d,%d,%d\n", + dpcm->cable->snd_timer.id.card, + dpcm->cable->snd_timer.id.device, + dpcm->cable->snd_timer.id.subdevice); + snd_iprintf(buffer, " owner:\t\t%s\n", + (dpcm->cable->snd_timer.owner == SNDRV_PCM_STREAM_CAPTURE) ? + "capture" : "playback"); +} + static snd_pcm_uframes_t loopback_pointer(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; @@ -707,6 +978,23 @@ static int rule_channels(struct snd_pcm_hw_params *params, return snd_interval_refine(hw_param_interval(params, rule->var), &t); }

+static int rule_period_bytes(struct snd_pcm_hw_params *params, + struct snd_pcm_hw_rule *rule) +{ + struct loopback_pcm *dpcm = rule->private; + struct loopback_cable *cable = dpcm->cable; + struct snd_interval t; + + mutex_lock(&dpcm->loopback->cable_lock); + t.min = cable->hw.period_bytes_min; + t.max = cable->hw.period_bytes_max; + mutex_unlock(&dpcm->loopback->cable_lock); + t.openmin = 0; + t.openmax = 0; + t.integer = 0; + return snd_interval_refine(hw_param_interval(params, rule->var), &t); +} + static void free_cable(struct snd_pcm_substream *substream) { struct loopback *loopback = substream->private_data; @@ -749,6 +1037,152 @@ static struct loopback_ops loopback_jiffies_timer_ops = { .dpcm_info = loopback_jiffies_timer_dpcm_info, };

+static int loopback_parse_timer_id(const char * const str, + struct snd_timer_id *tid) +{ + /* [<pref>:](<card name>|<card idx>)[{.,}<dev idx>[{.,}<subdev idx>]] */ + const char * const sep_dev = ".,"; + const char * const sep_pref = ":"; + const char *name = str; + char save, *sep; + int card = 0, device = 0, subdevice = 0; + int err; + + sep = strpbrk(str, sep_pref); + if (sep) + name = sep + 1; + sep = strpbrk(name, sep_dev); + if (sep) { + save = *sep; + *sep = '\0'; + } + err = kstrtoint(name, 0, &card); + if (err == -EINVAL) { + /* Must be the name, not number */ + for (card = 0; card < snd_ecards_limit; card++) { + if (snd_cards[card] && + !strcmp(snd_cards[card]->id, name)) { + err = 0; + break; + } + } + } + if (!err && sep) { + char save2, *sep2; + sep2 = strpbrk(sep + 1, sep_dev); + if (sep2) { + save2 = *sep2; + *sep2 = '\0'; + } + err = kstrtoint(sep + 1, 0, &device); + if (!err && sep2) { + err = kstrtoint(sep2 + 1, 0, &subdevice); + } + if (sep2) + *sep2 = save2; + } + if (!err && tid) { + tid->card = card; + tid->device = device; + tid->subdevice = subdevice; + } + if (sep) + *sep = save; + return err; +} + +/* call in loopback->cable_lock */ +static int loopback_snd_timer_open(struct loopback_pcm *dpcm) +{ + int err = 0; + unsigned long flags; + struct snd_timer_id tid = { + .dev_class = SNDRV_TIMER_CLASS_PCM, + .dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION, + }; + struct snd_timer_instance *timer = NULL; + + spin_lock_irqsave(&dpcm->cable->lock, flags); + + /* check if timer was already opened. It is only opened once + * per playback and capture subdevice (aka cable). + */ + if (dpcm->cable->snd_timer.instance) + goto unlock; + + err = loopback_parse_timer_id(dpcm->loopback->timer_source, &tid); + if (err < 0) { + pcm_err(dpcm->substream->pcm, + "Parsing timer source '%s' failed with %d", + dpcm->loopback->timer_source, err); + goto unlock; + } + + dpcm->cable->snd_timer.owner = dpcm->substream->stream; + dpcm->cable->snd_timer.id = tid; + + /* snd_timer_close() and snd_timer_open() should not be called with + * locked spinlock because both functions can block on a mutex. The + * mutex loopback->cable_lock is kept locked. Therefore snd_timer_open() + * cannot be called a second time by the other device of the same cable. + * Therefore the following issue cannot happen: + * [proc1] Call loopback_timer_open() -> + * Unlock cable->lock for snd_timer_close/open() call + * [proc2] Call loopback_timer_open() -> snd_timer_open(), + * snd_timer_start() + * [proc1] Call snd_timer_open() and overwrite running timer + * instance + */ + spin_unlock_irqrestore(&dpcm->cable->lock, flags); + err = snd_timer_open(&timer, dpcm->loopback->card->id, + &dpcm->cable->snd_timer.id, + current->pid); + if (err < 0) { + pcm_err(dpcm->substream->pcm, + "snd_timer_open (%d,%d,%d) failed with %d", + dpcm->cable->snd_timer.id.card, + dpcm->cable->snd_timer.id.device, + dpcm->cable->snd_timer.id.subdevice, + err); + return err; + } + spin_lock_irqsave(&dpcm->cable->lock, flags); + + /* The callback has to be called from another tasklet. If + * SNDRV_TIMER_IFLG_FAST is specified it will be called from the + * snd_pcm_period_elapsed() call of the selected sound card. + * snd_pcm_period_elapsed() helds snd_pcm_stream_lock_irqsave(). + * Due to our callback loopback_snd_timer_function() also calls + * snd_pcm_period_elapsed() which calls snd_pcm_stream_lock_irqsave(). + * This would end up in a dead lock. + */ + timer->flags |= SNDRV_TIMER_IFLG_AUTO; + timer->callback = loopback_snd_timer_function; + timer->callback_data = (void *)dpcm->cable; + timer->ccallback = loopback_snd_timer_event; + dpcm->cable->snd_timer.instance = timer; + + /* initialise a tasklet used for draining */ + tasklet_init(&dpcm->cable->snd_timer.event_tasklet, + loopback_snd_timer_tasklet, (unsigned long)timer); + +unlock: + spin_unlock_irqrestore(&dpcm->cable->lock, flags); + + return err; +} + +/* stop_sync() is not required for sound timer because it does not need to be + * restarted in loopback_prepare() on Xrun recovery + */ +static struct loopback_ops loopback_snd_timer_ops = { + .open = loopback_snd_timer_open, + .start = loopback_snd_timer_start, + .stop = loopback_snd_timer_stop, + .close_cable = loopback_snd_timer_close_cable, + .dpcm_info = loopback_snd_timer_dpcm_info, +}; + static int loopback_open(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; @@ -776,7 +1210,10 @@ static int loopback_open(struct snd_pcm_substream *substream) } spin_lock_init(&cable->lock); cable->hw = loopback_pcm_hardware; - cable->ops = &loopback_jiffies_timer_ops; + if (loopback->timer_source) + cable->ops = &loopback_snd_timer_ops; + else + cable->ops = &loopback_jiffies_timer_ops; loopback->cables[substream->number][dev] = cable; } dpcm->cable = cable; @@ -812,6 +1249,19 @@ static int loopback_open(struct snd_pcm_substream *substream) if (err < 0) goto unlock;

+ /* In case of sound timer the period time of both devices of the same + * loop has to be the same. + * This rule only takes effect if a sound timer was chosen + */ + if (cable->snd_timer.instance) { + err = snd_pcm_hw_rule_add(runtime, 0, + SNDRV_PCM_HW_PARAM_PERIOD_BYTES, + rule_period_bytes, dpcm, + SNDRV_PCM_HW_PARAM_PERIOD_BYTES, -1); + if (err < 0) + goto unlock; + } + /* loopback_runtime_free() has not to be called if kfree(dpcm) was * already called here. Otherwise it will end up with a double free. */ @@ -1214,6 +1664,18 @@ static int loopback_proc_new(struct loopback *loopback, int cidx) print_cable_info); }

+static void loopback_set_timer_source(struct loopback *loopback, + const char *value) +{ + if (loopback->timer_source) { + devm_kfree(loopback->card->dev, loopback->timer_source); + loopback->timer_source = NULL; + } + if (value && *value) + loopback->timer_source = devm_kstrdup(loopback->card->dev, + value, GFP_KERNEL); +} + static int loopback_probe(struct platform_device *devptr) { struct snd_card *card; @@ -1233,6 +1695,8 @@ static int loopback_probe(struct platform_device *devptr) pcm_substreams[dev] = MAX_PCM_SUBSTREAMS; loopback->card = card; + loopback_set_timer_source(loopback, timer_source[dev]); + mutex_init(&loopback->cable_lock);

err = loopback_pcm_new(loopback, 0, pcm_substreams[dev]);

On Thu, 07 Nov 2019 11:40:18 +0100, Gabbasov, Andrew wrote:

Hello Takashi,

Thank you very much for your feedback!

...

-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, November 07, 2019 11:06 AM To: Gabbasov, Andrew Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Jaroslav Kysela; Takashi Iwai; Timo Wischer Subject: Re: [PATCH v2 8/8] ALSA: aloop: Support runtime change of snd_timer via info interface

On Tue, 05 Nov 2019 15:32:18 +0100, Andrew Gabbasov wrote:

...

Show and change sound card timer source with read-write info file in proc filesystem. Initial string can still be set as module parameter.

The timer source string value can be changed at any time, but it is latched by PCM substream open callback (the first one for a particular cable). At this point it is actually used, that is the string is parsed, and the timer is looked up and opened.

The timer source is set for a loopback card (the same as initial setting by module parameter), but every cable uses the value, current at the moment of open.

Setting the value to empty string switches the timer to jiffies.

Signed-off-by: Andrew Gabbasov andrew_gabbasov@mentor.com

Unfortunately the whole code here are racy. It may lead to a crash or use-after-free easily. Some locking is needed definitely.

You are right, using and changing of loopback->timer_source should be protected. I'll add locking with loopback->cable_lock to the bodies of print_timer_source_info() and change_timer_source_info() (like in the example diff below), similarly to other /proc files and mixer controls. All other uses of loopback->timer_source are already covered by loopback->cable_lock, except for loopback_set_timer_source() call from loopback_probe(), that is done at the very early stage and doesn't conflict with other uses. I think, in order to avoid racing problems, this will be enough, won't it?

Yes, I guess so. loopback_set_timer_source() replaces only the timer_source and it's referred only at opening a stream.

thanks,

Takashi

Thanks.

Best regards, Andrew

diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c index 415128a97774..ca9307dd780e 100644 --- a/sound/drivers/aloop.c +++ b/sound/drivers/aloop.c @@ -1684,8 +1684,10 @@ static void print_timer_source_info(struct snd_info_entry *entry, { struct loopback *loopback = entry->private_data;

•   mutex_lock(&loopback->cable_lock);
    snd_iprintf(buffer, "%s\n",
                loopback->timer_source ? loopback->timer_source : "");
  

•   mutex_unlock(&loopback->cable_lock);
  

}

static void change_timer_source_info(struct snd_info_entry *entry, @@ -1694,8 +1696,10 @@ static void change_timer_source_info(struct snd_info_entry *entry, struct loopback *loopback = entry->private_data; char line[64];

•   mutex_lock(&loopback->cable_lock);
    if (!snd_info_get_line(buffer, line, sizeof(line)))
            loopback_set_timer_source(loopback, strim(line));
  

•   mutex_unlock(&loopback->cable_lock);
  

}

static int loopback_timer_source_proc_new(struct loopback *loopback)

Hello Takashi,

Thank you very much for your feedback.

-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, November 07, 2019 11:05 AM

On Tue, 05 Nov 2019 15:32:17 +0100, Andrew Gabbasov wrote:

...

@@ -102,6 +106,13 @@ struct loopback_cable { unsigned int pause; /* timer specific */ struct loopback_ops *ops;

• /* If sound timer is used */
• struct {

• int owner;
  

The term "owner" is a bit confusing here. It seems holding the PCM direction, but most people expect it being a process-id or something like that from the word.

Will be fixed in next update.

...

• struct snd_timer_id id;
  

• struct tasklet_struct event_tasklet;
  

You don't need to make own tasklet. The timer core calls it via tasklet in anyway unless you pass SNDRV_TIMER_IFLG_FAST -- see below.

And the tasklet is no longer recommended infrastructure in the recent kernel, we should avoid it as much as possible.

See below about the tasklet.

...

struct loopback_setup { @@ -122,6 +133,7 @@ struct loopback { struct loopback_cable *cables[MAX_PCM_SUBSTREAMS][2]; struct snd_pcm *pcm[2]; struct loopback_setup setup[MAX_PCM_SUBSTREAMS][2];

• char *timer_source;

This can be const char *, I suppose.

Yes, this will be fixed.

...

+static void loopback_snd_timer_period_elapsed(

• struct loopback_cable * const cable,
  

• const int event, const unsigned long resolution)
  

+{

• struct loopback_pcm *dpcm_play =

• 	cable->streams[SNDRV_PCM_STREAM_PLAYBACK];
  

• struct loopback_pcm *dpcm_capt =

• 	cable->streams[SNDRV_PCM_STREAM_CAPTURE];
  

You shouldn't assign them outside the cable->lock.

I'll move these assigns to after obtaining the lock.

...

• struct snd_pcm_runtime *valid_runtime;
• unsigned int running, elapsed_bytes;
• unsigned long flags;
• spin_lock_irqsave(&cable->lock, flags);
• running = cable->running ^ cable->pause;
• /* no need to do anything if no stream is running */
• if (!running) {

• spin_unlock_irqrestore(&cable->lock, flags);
  

• return;
  

• }
• if (event == SNDRV_TIMER_EVENT_MSTOP) {

• if (!dpcm_play || !dpcm_play->substream ||
  

•     !dpcm_play->substream->runtime ||
  

•     !dpcm_play->substream->runtime->status ||
  

Would these conditions really happen?

This seems to be kind of over cautiousness ;-) I remove all intermediate members checks.

...

•     dpcm_play->substream->runtime->status->state !=
  

•     SNDRV_PCM_STATE_DRAINING) {
  

What's special with DRAINING state? The code doesn't show or explain it. And for other conditions, we keep going even if the event is MSTOP?

There are some comments below in loopback_snd_timer_event() function. When the sound timer stops and the stream is in the draining state, we still need to flush the data remaining in the buffer, and the regular "elapsed" functions are called for that.

...

• 	spin_unlock_irqrestore(&cable->lock, flags);
  

• 	return;
  

• }
  

• }
• valid_runtime = (running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) ?

• 		dpcm_play->substream->runtime :
  

• 		dpcm_capt->substream->runtime;
  

• /* resolution is only valid for SNDRV_TIMER_EVENT_TICK events */
• if (event == SNDRV_TIMER_EVENT_TICK) {

• /* The hardware rules guarantee that playback and capture
  

period

...

•  * are the same. Therefore only one device has to be checked
  

•  * here.
  

•  */
  

• if (loopback_snd_timer_check_resolution(valid_runtime,
  

• 					resolution) < 0) {
  

• 	spin_unlock_irqrestore(&cable->lock, flags);
  

• 	if (cable->running & (1 <<
  

SNDRV_PCM_STREAM_PLAYBACK))

...

• 		snd_pcm_stop_xrun(dpcm_play->substream);
  

• 	if (cable->running & (1 <<
  

SNDRV_PCM_STREAM_CAPTURE))

...

• 		snd_pcm_stop_xrun(dpcm_capt->substream);
  

Referencing outside the lock isn't really safe. In the case of jiffies timer code, it's a kind of OK because it's done in the timer callback function that is assigned for each stream open -- that is, the stream runtime is guaranteed to be present in the timer callback. But I'm not sure about your case...

I re-structure this code a little to make all de-referencing inside the lock.

...

@@ -749,6 +1037,152 @@ static struct loopback_ops

loopback_jiffies_timer_ops = {

...

.dpcm_info = loopback_jiffies_timer_dpcm_info, };

+static int loopback_parse_timer_id(const char * const str,

• 		   struct snd_timer_id *tid)
  

+{

• /* [<pref>:](<card name>|<card idx>)[{.,}<dev idx>[{.,}<subdev

idx>]] */

...

• const char * const sep_dev = ".,";
• const char * const sep_pref = ":";
• const char *name = str;
• char save, *sep;
• int card = 0, device = 0, subdevice = 0;
• int err;
• sep = strpbrk(str, sep_pref);
• if (sep)

• name = sep + 1;
  

• sep = strpbrk(name, sep_dev);
• if (sep) {

• save = *sep;
  

• *sep = '\0';
  

• }
• err = kstrtoint(name, 0, &card);
• if (err == -EINVAL) {

• /* Must be the name, not number */
  

• for (card = 0; card < snd_ecards_limit; card++) {
  

• 	if (snd_cards[card] &&
  

• 	    !strcmp(snd_cards[card]->id, name)) {
  

• 		err = 0;
  

• 		break;
  

• 	}
  

• }
  

• }

As kbuildbot reported, this is obviously broken with the recent kernel. snd_cards[] is no longer exported! And I don't want to export again.

IOW, if we need this kind of thing, it's better to modify the existing code in sound/core/init.c and export that function.

Yes, I realized it a little late ;-) So far I changed the loop to something like

for (card_idx = 0; card_idx < snd_ecards_limit; card_idx++) { struct snd_card *card = snd_card_ref(card_idx); if (card) { if (!strcmp(card->id, name)) err = 0; snd_card_unref(card); } if (!err) break; }

Of course, adding a separate lookup helper function to sound/core/init.c, like, for example, struct snd_card *snd_card_find_id(const char *id) that makes similar loop in a more "atomic" way, with proper locking and reference incrementing for the result, would be more efficient, so if you decide to create such a helper, that would help to eliminate this loop from here.

...

+/* call in loopback->cable_lock */ +static int loopback_snd_timer_open(struct loopback_pcm *dpcm) +{

• int err = 0;
• unsigned long flags;
• struct snd_timer_id tid = {

• .dev_class = SNDRV_TIMER_CLASS_PCM,
  

• .dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION,
  

• };
• struct snd_timer_instance *timer = NULL;

Why initialing to NULL here?

Will be fixed too.

...

• spin_lock_irqsave(&dpcm->cable->lock, flags);

You need no irqsave version but spin_lock_irq() for the context like open/close that is guaranteed to be sleepable.

And this will be fixed.

...

• spin_lock_irqsave(&dpcm->cable->lock, flags);
• /* The callback has to be called from another tasklet. If

• * SNDRV_TIMER_IFLG_FAST is specified it will be called from the
  

• * snd_pcm_period_elapsed() call of the selected sound card.
  

Well, you're the one who specifies SNDRV_TIMER_IFLG_XXX, so you know that the flag isn't set. So tasklet makes little sense.

Indeed, that flag is not set, and the regular callback is already called from the tasklet. That's why the callback function, registered here (loopback_snd_timer_function) does not use the tasklet. However, the ccallback event handler is called by sound core directly, without a tasklet, and within "spin_lock_irqsave(&timer->lock, flags)". So, it's not possible to call snd_pcm_period_elapsed() directly from ccallback function (loopback_snd_timer_event). In order to be able to make this call, the local tasklet is used.

...

• * snd_pcm_period_elapsed() helds snd_pcm_stream_lock_irqsave().
  

• * Due to our callback loopback_snd_timer_function() also calls
  

• * snd_pcm_period_elapsed() which calls
  

snd_pcm_stream_lock_irqsave().

...

• * This would end up in a dead lock.
  

• */
  

• timer->flags |= SNDRV_TIMER_IFLG_AUTO;
• timer->callback = loopback_snd_timer_function;
• timer->callback_data = (void *)dpcm->cable;
• timer->ccallback = loopback_snd_timer_event;

This reminds me that we need a safer way to assign these stuff in general... But it's above this patch set, in anyway.

I'm preparing the next version of this patch set with the changes, described above, and some more code cleanup. It will be submitted soon.

Thanks!

Best regards, Andrew

On Tue, 05 Nov 2019 15:32:12 +0100, Andrew Gabbasov wrote:

@@ -264,7 +266,7 @@ static int loopback_trigger(struct snd_pcm_substream *substream, int cmd) spin_lock(&cable->lock); cable->running |= stream; cable->pause &= ~stream;

• loopback_timer_start(dpcm);
  

• err = loopback_timer_start(dpcm);
  

  spin_unlock(&cable->lock); if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) loopback_active_notify(dpcm);

Will we notify even if the start/stop fails?

thanks,

Takashi