Received-SPF: pass (google.com: domain of linux-kernel+bounces-57621-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 08 Feb 2024 09:33:58 +0100
Message-ID: <87zfwb4ao9.wl-tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: Wesley Cheng <quic_wcheng@quicinc.com>
Cc: <srinivas.kandagatla@linaro.org>,
	<mathias.nyman@intel.com>,
	<perex@perex.cz>,
	<conor+dt@kernel.org>,
	<corbet@lwn.net>,
	<lgirdwood@gmail.com>,
	<andersson@kernel.org>,
	<krzysztof.kozlowski+dt@linaro.org>,
	<gregkh@linuxfoundation.org>,
	<Thinh.Nguyen@synopsys.com>,
	<broonie@kernel.org>,
	<bgoswami@quicinc.com>,
	<tiwai@suse.com>,
	<robh+dt@kernel.org>,
	<konrad.dybcio@linaro.org>,
	<linux-kernel@vger.kernel.org>,
	<devicetree@vger.kernel.org>,
	<linux-sound@vger.kernel.org>,
	<linux-usb@vger.kernel.org>,
	<linux-arm-msm@vger.kernel.org>,
	<linux-doc@vger.kernel.org>,
	<alsa-devel@alsa-project.org>
Subject: Re: [PATCH v13 35/53] ALSA: usb-audio: Prevent starting of audio stream if in use
In-Reply-To: <0cb39613-ec01-50aa-807f-b537f201dac0@quicinc.com>
References: <20240203023645.31105-1-quic_wcheng@quicinc.com>
	<20240203023645.31105-36-quic_wcheng@quicinc.com>
	<87y1bxvj0o.wl-tiwai@suse.de>
	<ef83036f-6605-1db3-d962-ac28a10711ac@quicinc.com>
	<877cjg7o0k.wl-tiwai@suse.de>
	<810161b3-4d98-755f-163f-fdfc9fe37063@quicinc.com>
	<0cb39613-ec01-50aa-807f-b537f201dac0@quicinc.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
Precedence: bulk
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 8bit

On Thu, 08 Feb 2024 02:12:00 +0100,
Wesley Cheng wrote:
> 
> Hi Takashi,
> 
> On 2/7/2024 4:02 PM, Wesley Cheng wrote:
> > Hi Takashi,
> > 
> > On 2/6/2024 11:05 PM, Takashi Iwai wrote:
> >> 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@quicinc.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).
> >> 
> > 
> > I think I had looked into this as well previously, and it was
> > difficult to achieve, because from the USB offloading perspective,
> > we don't ever call: snd_usb_pcm_open()
> > 
> > This is actually where we populate the pcm_substream parameter
> > within struct snd_usb_substream based on when userspace opens the
> > USB SND PCM device (which is not the case for offloading).? So the
> > offload driver doesn't have a way to fetch the struct snd_pcm that
> > is allocated to the PCM device created by the USB SND card.
> > 
> 
> Sorry, took a look at it again, and found a way.  Although not pretty,
> we can access it using:
> subs->stream->pcm->streams[direction].substream->hw_opened

Yes, it's not easy to follow it.  So if we want to this path, worth
for a detailed comment.  That said, I don't mind to introduce the new
local mutex and flag as you did if the above became too messy in the
end.


thanks,

Takashi