Re: [alsa-devel] [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