Date: Wed, 07 Feb 2024 08:05:15 +0100
From: Takashi Iwai <tiwai@...e.de>
To: Wesley Cheng <quic_wcheng@...cinc.com>
Cc: <srinivas.kandagatla@...aro.org>,
	<mathias.nyman@...el.com>,
	<perex@...ex.cz>,
	<conor+dt@...nel.org>,
	<corbet@....net>,
	<lgirdwood@...il.com>,
	<andersson@...nel.org>,
	<krzysztof.kozlowski+dt@...aro.org>,
	<gregkh@...uxfoundation.org>,
	<Thinh.Nguyen@...opsys.com>,
	<broonie@...nel.org>,
	<bgoswami@...cinc.com>,
	<tiwai@...e.com>,
	<robh+dt@...nel.org>,
	<konrad.dybcio@...aro.org>,
	<linux-kernel@...r.kernel.org>,
	<devicetree@...r.kernel.org>,
	<linux-sound@...r.kernel.org>,
	<linux-usb@...r.kernel.org>,
	<linux-arm-msm@...r.kernel.org>,
	<linux-doc@...r.kernel.org>,
	<alsa-devel@...a-project.org>
Subject: Re: [PATCH v13 35/53] ALSA: usb-audio: Prevent starting of audio stream if in use

On Wed, 07 Feb 2024 01:08:00 +0100,
Wesley Cheng wrote:
> 
> Hi Takashi,
> 
> On 2/6/2024 5:07 AM, Takashi Iwai wrote:
> > On Sat, 03 Feb 2024 03:36:27 +0100,
> > Wesley Cheng wrote:
> >> 
> >> With USB audio offloading, an audio session is started from the ASoC
> >> platform sound card and PCM devices.  Likewise, the USB SND path is still
> >> readily available for use, in case the non-offload path is desired.  In
> >> order to prevent the two entities from attempting to use the USB bus,
> >> introduce a flag that determines when either paths are in use.
> >> 
> >> If a PCM device is already in use, the check will return an error to
> >> userspace notifying that the stream is currently busy.  This ensures that
> >> only one path is using the USB substream.
> >> 
> >> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>
> > 
> > Hm, I'm not sure whether it's safe to hold chip->mutex there for the
> > long code path.  It even kicks off the auto-resume, which may call
> > various functions at resuming, and some of them may re-hold
> > chip->mutex.
> > 
> 
> That's a good point.
> 
> > If it's only about the open flag, protect only the flag access with
> > the mutex, not covering the all open function.  At least the re-entry
> > can be avoided by that.
> > 
> 
> Sure, let me re-order the check/assignment and the mutex locking.
> Since this is now checked here in USB PCM and the QC offload driver,
> we want to make sure that if there was some application attempting to
> open both at the same time, we prevent any possible races.
> 
> I think the best way to address this would be something like:
> 
> static int snd_usb_pcm_open(struct snd_pcm_substream *substream)
> {
> ...
> 	mutex_lock(&chip->mutex);
> 	if (subs->opened) {
> 		mutex_unlock(&chip->mutex);
> 		return -EBUSY;
> 	}
> 	subs->opened = 1;
> 	mutex_unlock(&chip->mutex);
> 
> //Execute bulk of PCM open routine
> ...
> 	return 0;
> 
> // If any errors are seen, unwind
> err_resume:
> 	snd_usb_autosuspend(subs->stream->chip);
> err_open:
> 	mutex_lock(&chip->mutex);
> 	subs->opened = 0;
> 	mutex_unlock(&chip->mutex);
> 
> 	return ret;
> }
> 
> Set the opened flag first, so that if QC offload checks it, it can
> exit early and vice versa.  Otherwise, if we set the opened flag at
> the same position as the previous patch, we may be calling the other
> routines in parallel to the QC offload enable stream routine.  The
> only thing with this patch is that we'd need some error handling
> unwinding.

The above is what I had in mind.

But, thinking on this again, you might be able to get the same result
by using the ALSA PCM core substream open_mutex and hw_opened flag.
This is already held and set at snd_pcm_core() (the hw_opened flag is
set after open callback, though).  The offload driver can use those
instead of the own lock and flag, too, although it's not really
well-mannered behavior (hence you need proper comments).


thanks,

Takashi

Powered by blists - more mailing lists