Message-ID: <cfcedbc9-e176-754a-9a34-cfca8d6a3984@quicinc.com>
Date: Thu, 8 Feb 2024 12:19:11 -0800
From: Wesley Cheng <quic_wcheng@...cinc.com>
To: Takashi Iwai <tiwai@...e.de>
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

Hi Takashi,

On 2/8/2024 12:33 AM, Takashi Iwai wrote:
> 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@...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).
>>>>
>>>
>>> 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.
> 

If you don't mind, I prefer to keep it the way it was with the local 
mutex and flag.  It makes it a lot easier to follow, and for other users 
to adopt as well compared to the long equation above :).

Thanks
Wesley Cheng

Powered by blists - more mailing lists