Re: [alsa-devel] [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies
Hello Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, November 20, 2019 6:59 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 16:39:00 +0100, Andrew Gabbasov wrote:
Hello Takashi,
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, November 20, 2019 6:32 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 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 fromthe
* snd_pcm_period_elapsed() call of the selected sound card.* snd_pcm_period_elapsed() heldssnd_pcm_stream_lock_irqsave().
* Due to our callback loopback_snd_timer_function() alsocalls
* snd_pcm_period_elapsed() which callssnd_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 amutex. The
* mutex loopback->cable_lock is kept locked. Thereforesnd_timer_open()
* cannot be called a second time by the other device of thesame
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, (unsignedlong)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 :)
OK, got it. I'll move this initialization to before snd_timer_open() in the next update together with the fixes for the other issues you will find in this version.
I have no other issues, so you can just resubmit only that patch, too.
I'm not sure how to correctly format resubmitting of a single patch from a patch set, so I'm submitting the next version v5 of the whole patch set: https://mailman.alsa-project.org/pipermail/alsa-devel/2019-November/158939.h tml
Thanks!
Best regards, Andrew
participants (1)
-
Andrew Gabbasov