Re: [PATCH v26 26/33] ALSA: usb-audio: Prevent starting of audio stream if in use

[Wesley Cheng <quic_wcheng@xxxxxxxxxxx>]

Hi Pierre,

On 8/30/2024 2:45 AM, Pierre-Louis Bossart wrote:
>
> On 8/29/24 21:40, 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>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
>
> I would also move this patch earlier in the series since it has no
> dependency on USB offload really, and if somehow it breaks USB audio
> regular paths we'd want to know early for bisection.

Sure I'll re-order this earlier since I'm going to send out another rev.

Thanks

Wesley Cheng

>
>> ---
>>  sound/usb/card.h |  1 +
>>  sound/usb/pcm.c  | 29 ++++++++++++++++++++++++++---
>>  2 files changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/sound/usb/card.h b/sound/usb/card.h
>> index 15cda1730076..d8b8522e1613 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 18467da6fd9e..b24ee38fad72 100644
>> --- a/sound/usb/pcm.c
>> +++ b/sound/usb/pcm.c
>> @@ -1241,8 +1241,17 @@ 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;
>> +	}
>> +	subs->opened = 1;
>> +	mutex_unlock(&chip->mutex);
>> +
>>  	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 +1268,23 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream)
>>  
>>  	ret = setup_hw_info(runtime, subs);
>>  	if (ret < 0)
>> -		return ret;
>> +		goto err_open;
>>  	ret = snd_usb_autoresume(subs->stream->chip);
>>  	if (ret < 0)
>> -		return ret;
>> +		goto err_open;
>>  	ret = snd_media_stream_init(subs, as->pcm, direction);
>>  	if (ret < 0)
>> -		snd_usb_autosuspend(subs->stream->chip);
>> +		goto err_resume;
>> +
>> +	return 0;
>> +
>> +err_resume:
>> +	snd_usb_autosuspend(subs->stream->chip);
>> +err_open:
>> +	mutex_lock(&chip->mutex);
>> +	subs->opened = 0;
>> +	mutex_unlock(&chip->mutex);
>> +
>>  	return ret;
>>  }
>>  
>> @@ -1274,6 +1293,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 +1307,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;
>>  }