| Message ID | 20240203023645.31105-33-quic_wcheng@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce QC USB SND audio offloading support | expand |
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(®ister_mutex); > + chip = usb_chip[card_idx]; > + if (!chip) { > + mutex_unlock(®ister_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(®ister_mutex); > + return as; > + } > + } > + } > + mutex_unlock(®ister_mutex); I prefer having the single lock/unlock call pair, e.g. struct snd_usb_stream *as, *ret; ret = NULL; mutex_lock(®ister_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(®ister_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
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(®ister_mutex); > > + chip = usb_chip[card_idx]; > > + if (!chip) { > > + mutex_unlock(®ister_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(®ister_mutex); > > + return as; > > + } > > + } > > + } > > + mutex_unlock(®ister_mutex); > > I prefer having the single lock/unlock call pair, e.g. > > struct snd_usb_stream *as, *ret; > > ret = NULL; > mutex_lock(®ister_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(®ister_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
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(®ister_mutex); > > > + chip = usb_chip[card_idx]; > > > + if (!chip) { > > > + mutex_unlock(®ister_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(®ister_mutex); > > > + return as; > > > + } > > > + } > > > + } > > > + mutex_unlock(®ister_mutex); > > > > I prefer having the single lock/unlock call pair, e.g. > > > > struct snd_usb_stream *as, *ret; > > > > ret = NULL; > > mutex_lock(®ister_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(®ister_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. Takashi
Hi Takashi, On 2/6/2024 5:12 AM, 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(®ister_mutex); >> + chip = usb_chip[card_idx]; >> + if (!chip) { >> + mutex_unlock(®ister_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(®ister_mutex); >> + return as; >> + } >> + } >> + } >> + mutex_unlock(®ister_mutex); > > I prefer having the single lock/unlock call pair, e.g. > > struct snd_usb_stream *as, *ret; > > ret = NULL; > mutex_lock(®ister_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(®ister_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. > > Sure will do, thanks for taking a look. Thanks Wesley Cheng
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(®ister_mutex); >>>> + chip = usb_chip[card_idx]; >>>> + if (!chip) { >>>> + mutex_unlock(®ister_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(®ister_mutex); >>>> + return as; >>>> + } >>>> + } >>>> + } >>>> + mutex_unlock(®ister_mutex); >>> >>> I prefer having the single lock/unlock call pair, e.g. >>> >>> struct snd_usb_stream *as, *ret; >>> >>> ret = NULL; >>> mutex_lock(®ister_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(®ister_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
diff --git a/sound/usb/card.c b/sound/usb/card.c index c0b312e264bf..11b827b7a2a5 100644 --- a/sound/usb/card.c +++ b/sound/usb/card.c @@ -162,6 +162,46 @@ int snd_usb_unregister_platform_ops(void) } EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops); +/* + * Checks to see if requested audio profile, i.e sample rate, # of + * channels, etc... is supported by the substream associated to the + * USB audio device. + */ +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(®ister_mutex); + chip = usb_chip[card_idx]; + if (!chip) { + mutex_unlock(®ister_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(®ister_mutex); + return as; + } + } + } + mutex_unlock(®ister_mutex); + + return NULL; +} +EXPORT_SYMBOL_GPL(snd_usb_find_suppported_substream); + /* * disconnect streams * called from usb_audio_disconnect() diff --git a/sound/usb/card.h b/sound/usb/card.h index 02e4ea898db5..ed4a664e24e5 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -217,4 +217,15 @@ struct snd_usb_platform_ops { int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops); int snd_usb_unregister_platform_ops(void); + +#if IS_ENABLED(CONFIG_SND_USB_AUDIO) +struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, + struct snd_pcm_hw_params *params, int direction); +#else +static struct snd_usb_stream *snd_usb_find_suppported_substream(int card_idx, + struct snd_pcm_hw_params *params, int direction) +{ + return NULL; +} +#endif /* IS_ENABLED(CONFIG_SND_USB_AUDIO) */ #endif /* __USBAUDIO_CARD_H */
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> --- sound/usb/card.c | 40 ++++++++++++++++++++++++++++++++++++++++ sound/usb/card.h | 11 +++++++++++ 2 files changed, 51 insertions(+)