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

Wesley Cheng
2024-02-08T00:04:42Z

On 2/6/2024 6:53 AM, Takashi Iwai wrote:
> On Tue, 06 Feb 2024 15:50:21 +0100,
> Greg KH wrote:
>>
>> 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?
>
> Heh, it's too new ;)
> That should work gracefully, yes.
>

Thanks Greg/Takashi. That is nifty.

Thanks
Wesley Cheng