Re: [PATCH v13 32/53] ALSA: usb-audio: Check for support for requested audio format

Takashi Iwai
2024-02-06T13:12:51Z

On Sat, 03 Feb 2024 03:36:24 +0100,
Wesley Cheng wrote:
>
> Allow for checks on a specific USB audio device to see if a requested PCM
> format is supported. This is needed for support when playback is
> initiated by the ASoC USB backend path.
>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>

Just cosmetic:

> +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx,
> + struct snd_pcm_hw_params *params, int direction)
> +{
> + struct snd_usb_audio *chip;
> + struct snd_usb_substream *subs;
> + struct snd_usb_stream *as;
> + const struct audioformat *fmt;
> +
> + /*
> + * Register mutex is held when populating and clearing usb_chip
> + * array.
> + */
> + mutex_lock(&register_mutex);
> + chip = usb_chip[card_idx];
> + if (!chip) {
> + mutex_unlock(&register_mutex);
> + return NULL;
> + }
> +
> + if (enable[card_idx]) {
> + list_for_each_entry(as, &chip->pcm_list, list) {
> + subs = &as->substream[direction];
> + fmt = snd_usb_find_substream_format(subs, params);
> + if (fmt) {
> + mutex_unlock(&register_mutex);
> + return as;
> + }
> + }
> + }
> + mutex_unlock(&register_mutex);

I prefer having the single lock/unlock call pair, e.g.

struct snd_usb_stream *as, *ret;

ret = NULL;
mutex_lock(&register_mutex);
chip = usb_chip[card_idx];
if (chip && enable[card_idx]) {
list_for_each_entry(as, &chip->pcm_list, list) {
subs = &as->substream[direction];
if (snd_usb_find_substream_format(subs, params)) {
ret = as;
break;
}
}
}
mutex_unlock(&register_mutex);
return ret;
}

In this case, we shouldn't reuse "as" for the return value since it
can be non-NULL after the loop end.

thanks,

Takashi