Message-ID: <s5hlfss862t.wl-tiwai@suse.de>
Date:   Thu, 07 Nov 2019 09:05:14 +0100
From:   Takashi Iwai <tiwai@...e.de>
To:     Andrew Gabbasov <andrew_gabbasov@...tor.com>
Cc:     <alsa-devel@...a-project.org>, <linux-kernel@...r.kernel.org>,
        "Jaroslav Kysela" <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
        Timo Wischer <twischer@...adit-jv.com>
Subject: Re: [PATCH v2 7/8] ALSA: aloop: Support selection of snd_timer instead of jiffies

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

Powered by blists - more mailing lists