From: Andrew Gabbasov <andrew_gabbasov@mentor.com> To: 'Takashi Iwai' <tiwai@suse.de> 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 20:56:31 +0300 [thread overview] Message-ID: <000201d59fcb$d895a180$89c0e480$@mentor.com> (raw) In-Reply-To: <s5h36ei8rph.wl-tiwai@suse.de> 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 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 :) > > > > 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
WARNING: multiple messages have this Message-ID (diff)
From: Andrew Gabbasov <andrew_gabbasov@mentor.com> To: 'Takashi Iwai' <tiwai@suse.de> Cc: Timo Wischer <twischer@de.adit-jv.com>, alsa-devel@alsa-project.org, Takashi Iwai <tiwai@suse.com>, linux-kernel@vger.kernel.org Subject: Re: [alsa-devel] [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies Date: Wed, 20 Nov 2019 20:56:31 +0300 [thread overview] Message-ID: <000201d59fcb$d895a180$89c0e480$@mentor.com> (raw) In-Reply-To: <s5h36ei8rph.wl-tiwai@suse.de> 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 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 :) > > > > 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 _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
next prev parent reply other threads:[~2019-11-20 17:57 UTC|newest] Thread overview: 26+ 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 instead of jiffies Andrew Gabbasov 2019-11-20 11:58 ` [alsa-devel] " Andrew Gabbasov 2019-11-20 11:58 ` [PATCH v4 1/7] ALSA: aloop: Describe units of variables Andrew Gabbasov 2019-11-20 11:58 ` [alsa-devel] " 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 ` [alsa-devel] " 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 ` [alsa-devel] " 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 ` [alsa-devel] " 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 ` [alsa-devel] " 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 ` [alsa-devel] " 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 11:58 ` [alsa-devel] " 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:21 ` [alsa-devel] " Andrew Gabbasov 2019-11-20 15:32 ` Takashi Iwai 2019-11-20 15:39 ` Andrew Gabbasov 2019-11-20 15:39 ` [alsa-devel] " Andrew Gabbasov 2019-11-20 15:58 ` Takashi Iwai 2019-11-20 17:56 ` Andrew Gabbasov [this message] 2019-11-20 17:56 ` [alsa-devel] " Andrew Gabbasov 2019-11-22 12:36 ` kbuild test robot
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='000201d59fcb$d895a180$89c0e480$@mentor.com' \ --to=andrew_gabbasov@mentor.com \ --cc=alsa-devel@alsa-project.org \ --cc=linux-kernel@vger.kernel.org \ --cc=perex@perex.cz \ --cc=tiwai@suse.com \ --cc=tiwai@suse.de \ --cc=twischer@de.adit-jv.com \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.