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.
> + 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.
> 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.
> +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.
> + 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?
> + 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?
> + 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...
> @@ -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.
> +/* 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?
> + 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.
> + 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.
> + * 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.
thanks,
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
