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

On Tue, Feb 06, 2024 at 02:12:44PM +0100, Takashi Iwai wrote:

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.

Why not just use guard(mutex) for this, making it all not an issue?

thanks,

greg k-h