Messages in this thread | | | Date | Wed, 7 Feb 2024 17:12:00 -0800 | Subject | Re: [PATCH v13 35/53] ALSA: usb-audio: Prevent starting of audio stream if in use | From | Wesley Cheng <> |
| |
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
Thanks Wesley Cheng
| |