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@xxxxxxxxxxx>
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.
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.
thanks,
Takashi
> ---
> sound/usb/card.h | 1 +
> sound/usb/pcm.c | 19 +++++++++++++++++--
> 2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/sound/usb/card.h b/sound/usb/card.h
> index ed4a664e24e5..6d59995440c3 100644
> --- a/sound/usb/card.h
> +++ b/sound/usb/card.h
> @@ -165,6 +165,7 @@ struct snd_usb_substream {
> unsigned int pkt_offset_adj; /* Bytes to drop from beginning of packets (for non-compliant devices) */
> unsigned int stream_offset_adj; /* Bytes to drop from beginning of stream (for non-compliant devices) */
>
> + unsigned int opened:1; /* pcm device opened */
> unsigned int running: 1; /* running status */
> unsigned int period_elapsed_pending; /* delay period handling */
>
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index 3adb09ce1702..c2cb52cd5d23 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -1241,8 +1241,15 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream)
> struct snd_usb_stream *as = snd_pcm_substream_chip(substream);
> struct snd_pcm_runtime *runtime = substream->runtime;
> struct snd_usb_substream *subs = &as->substream[direction];
> + struct snd_usb_audio *chip = subs->stream->chip;
> int ret;
>
> + mutex_lock(&chip->mutex);
> + if (subs->opened) {
> + mutex_unlock(&chip->mutex);
> + return -EBUSY;
> + }
> +
> runtime->hw = snd_usb_hardware;
> /* need an explicit sync to catch applptr update in low-latency mode */
> if (direction == SNDRV_PCM_STREAM_PLAYBACK &&
> @@ -1259,13 +1266,17 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream)
>
> ret = setup_hw_info(runtime, subs);
> if (ret < 0)
> - return ret;
> + goto out;
> ret = snd_usb_autoresume(subs->stream->chip);
> if (ret < 0)
> - return ret;
> + goto out;
> ret = snd_media_stream_init(subs, as->pcm, direction);
> if (ret < 0)
> snd_usb_autosuspend(subs->stream->chip);
> + subs->opened = 1;
> +out:
> + mutex_unlock(&chip->mutex);
> +
> return ret;
> }
>
> @@ -1274,6 +1285,7 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream)
> int direction = substream->stream;
> struct snd_usb_stream *as = snd_pcm_substream_chip(substream);
> struct snd_usb_substream *subs = &as->substream[direction];
> + struct snd_usb_audio *chip = subs->stream->chip;
> int ret;
>
> snd_media_stop_pipeline(subs);
> @@ -1287,6 +1299,9 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream)
>
> subs->pcm_substream = NULL;
> snd_usb_autosuspend(subs->stream->chip);
> + mutex_lock(&chip->mutex);
> + subs->opened = 0;
> + mutex_unlock(&chip->mutex);
>
> return 0;
> }
