linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Takashi Iwai <tiwai@suse.de>
To: Andrew Gabbasov <andrew_gabbasov@mentor.com>
Cc: <alsa-devel@alsa-project.org>, <linux-kernel@vger.kernel.org>,
	Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
	Timo Wischer <twischer@de.adit-jv.com>
Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies
Date: Wed, 20 Nov 2019 16:32:22 +0100	[thread overview]
Message-ID: <s5h5zje8sxl.wl-tiwai@suse.de> (raw)
In-Reply-To: <000001d59fb6$4ca36aa0$e5ea3fe0$@mentor.com>

On Wed, 20 Nov 2019 16:21:36 +0100,
Andrew Gabbasov wrote:
> 
> Hello Takashi,
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, November 20, 2019 5:34 PM
> > To: Gabbasov, Andrew
> > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Jaroslav
> > Kysela; Takashi Iwai; Timo Wischer
> > Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer
> > instead of jiffies
> > 
> > On Wed, 20 Nov 2019 12:58:55 +0100,
> > Andrew Gabbasov wrote:
> > > +/* call in loopback->cable_lock */
> > > +static int loopback_snd_timer_open(struct loopback_pcm *dpcm)
> > > +{
> > > +	int err = 0;
> > > +	struct snd_timer_id tid = {
> > > +		.dev_class = SNDRV_TIMER_CLASS_PCM,
> > > +		.dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION,
> > > +	};
> > > +	struct snd_timer_instance *timeri;
> > > +	struct loopback_cable *cable = dpcm->cable;
> > > +
> > > +	spin_lock_irq(&cable->lock);
> > > +
> > > +	/* check if timer was already opened. It is only opened once
> > > +	 * per playback and capture subdevice (aka cable).
> > > +	 */
> > > +	if (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;
> > > +	}
> > > +
> > > +	cable->snd_timer.stream = dpcm->substream->stream;
> > > +	cable->snd_timer.id = tid;
> > > +
> > > +	timeri = snd_timer_instance_new(dpcm->loopback->card->id);
> > > +	if (!timeri) {
> > > +		err = -ENOMEM;
> > > +		goto unlock;
> > > +	}
> > > +	/* 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.
> > > +	 */
> > > +	timeri->flags |= SNDRV_TIMER_IFLG_AUTO;
> > > +	timeri->callback = loopback_snd_timer_function;
> > > +	timeri->callback_data = (void *)cable;
> > > +	timeri->ccallback = loopback_snd_timer_event;
> > > +
> > > +	/* 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_irq(&cable->lock);
> > > +	err = snd_timer_open(timeri, &cable->snd_timer.id, current->pid);
> > > +	if (err < 0) {
> > > +		pcm_err(dpcm->substream->pcm,
> > > +			"snd_timer_open (%d,%d,%d) failed with %d",
> > > +			cable->snd_timer.id.card,
> > > +			cable->snd_timer.id.device,
> > > +			cable->snd_timer.id.subdevice,
> > > +			err);
> > > +		snd_timer_instance_free(timeri);
> > > +		return err;
> > > +	}
> > > +	spin_lock_irq(&cable->lock);
> > > +
> > > +	cable->snd_timer.instance = timeri;
> > > +
> > > +	/* initialise a tasklet used for draining */
> > > +	tasklet_init(&cable->snd_timer.event_tasklet,
> > > +		     loopback_snd_timer_tasklet, (unsigned long)timeri);
> > 
> > This has to be set before snd_timer_open().  The callback might be
> > called immediately after snd_timer_open().
> 
> This tasklet is used/scheduled only in ccallback (not regular tick
> callback),
> and only for SNDRV_TIMER_EVENT_MSTOP event. Can this event really happen
> immediately after snd_timer_open()?

Why not?  The master timer can be stopped at any time, even between
these two lines.

Beware that there are fuzzer programs that can trigger such racy
things, and you're adding the code to the target that is actively
slapped by them :)


thanks,

Takashi

  reply	other threads:[~2019-11-20 15:32 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-20 11:58 [PATCH v4 0/7] ALSA: aloop: Support sound timer as clock source " Andrew Gabbasov
2019-11-20 11:58 ` [PATCH v4 1/7] ALSA: aloop: Describe units of variables Andrew Gabbasov
2019-11-20 11:58   ` [PATCH v4 2/7] ALSA: aloop: Support return of error code for timer start and stop Andrew Gabbasov
2019-11-20 11:58     ` [PATCH v4 3/7] ALSA: aloop: Use callback functions for timer specific implementations Andrew Gabbasov
2019-11-20 11:58       ` [PATCH v4 4/7] ALSA: aloop: Rename all jiffies timer specific functions Andrew Gabbasov
2019-11-20 11:58         ` [PATCH v4 5/7] ALSA: aloop: Move CABLE_VALID_BOTH to the top of file Andrew Gabbasov
2019-11-20 11:58           ` [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies Andrew Gabbasov
2019-11-20 11:58             ` [PATCH v4 7/7] ALSA: aloop: Support runtime change of snd_timer via info interface Andrew Gabbasov
2019-11-20 14:33             ` [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies Takashi Iwai
2019-11-20 15:21               ` Andrew Gabbasov
2019-11-20 15:32                 ` Takashi Iwai [this message]
2019-11-20 15:39                   ` Andrew Gabbasov
2019-11-20 15:58                     ` Takashi Iwai
2019-11-20 17:56                       ` Andrew Gabbasov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=s5h5zje8sxl.wl-tiwai@suse.de \
    --to=tiwai@suse.de \
    --cc=alsa-devel@alsa-project.org \
    --cc=andrew_gabbasov@mentor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=perex@perex.cz \
    --cc=tiwai@suse.com \
    --cc=twischer@de.adit-jv.com \
    --subject='Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).