Received-SPF: pass (google.com: domain of linux-kernel+bounces-57310-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <810161b3-4d98-755f-163f-fdfc9fe37063@quicinc.com>
Date: Wed, 7 Feb 2024 16:02:51 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v13 35/53] ALSA: usb-audio: Prevent starting of audio
 stream if in use
Content-Language: en-US
To: Takashi Iwai <tiwai@suse.de>
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>
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>
From: Wesley Cheng <quic_wcheng@quicinc.com>
In-Reply-To: <877cjg7o0k.wl-tiwai@suse.de>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

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.

Thanks
Wesley Cheng