Re: [PATCH v30 15/30] ASoC: usb: Fetch ASoC card and pcm device information

[Takashi Iwai <tiwai@xxxxxxx>]

On Wed, 06 Nov 2024 20:33:58 +0100,
Wesley Cheng wrote:
> 
> USB SND needs to know how the USB offload path is being routed.  This would
> allow for applications to open the corresponding sound card and pcm device
> when it wants to take the audio offload path.  This callback should return
> the mapped indexes based on the USB SND device information.
> 
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
> ---
>  include/sound/soc-usb.h | 16 ++++++++++++++++
>  sound/soc/soc-usb.c     | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/include/sound/soc-usb.h b/include/sound/soc-usb.h
> index 587ea07a8cf5..c3d3e8d62ac5 100644
> --- a/include/sound/soc-usb.h
> +++ b/include/sound/soc-usb.h
> @@ -36,6 +36,11 @@ struct snd_soc_usb_device {
>   * @list - list head for SND SOC struct list
>   * @component - reference to ASoC component
>   * @connection_status_cb - callback to notify connection events
> + * @update_offload_route_info - callback to fetch mapped ASoC card and pcm
> + *				device pair.  This is unrelated to the concept
> + *				of DAPM route.  The "route" argument carries
> + *				an array used for a kcontrol output and should
> + *				contain two integers, card and pcm device index
>   * @priv_data - driver data
>   **/
>  struct snd_soc_usb {
> @@ -44,6 +49,9 @@ struct snd_soc_usb {
>  	int (*connection_status_cb)(struct snd_soc_usb *usb,
>  				    struct snd_soc_usb_device *sdev,
>  				    bool connected);
> +	int (*update_offload_route_info)(struct snd_soc_component *component,
> +					 int card, int pcm, int direction,
> +					 long *route);
>  	void *priv_data;
>  };
>  
> @@ -61,6 +69,8 @@ int snd_soc_usb_setup_offload_jack(struct snd_soc_component *component,
>  int snd_soc_usb_disable_offload_jack(struct snd_soc_component *component);
>  int snd_soc_usb_enable_offload_jack(struct snd_soc_component *component,
>  				    struct snd_soc_jack *jack);
> +int snd_soc_usb_update_offload_route(struct device *dev, int card, int pcm,
> +				     int direction, long *route);
>  
>  struct snd_soc_usb *snd_soc_usb_allocate_port(struct snd_soc_component *component,
>  					      void *data);
> @@ -109,6 +119,12 @@ static inline int snd_soc_usb_enable_offload_jack(struct snd_soc_component *comp
>  	return 0;
>  }
>  
> +static int snd_soc_usb_update_offload_route(struct device *dev, int card, int pcm,
> +					    int direction, long *route)
> +{
> +	return -ENODEV;
> +}
> +
>  static inline struct snd_soc_usb *
>  snd_soc_usb_allocate_port(struct snd_soc_component *component, void *data)
>  {
> diff --git a/sound/soc/soc-usb.c b/sound/soc/soc-usb.c
> index ab914878e101..e56826f1df71 100644
> --- a/sound/soc/soc-usb.c
> +++ b/sound/soc/soc-usb.c
> @@ -145,6 +145,40 @@ int snd_soc_usb_enable_offload_jack(struct snd_soc_component *component,
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_usb_enable_offload_jack);
>  
> +/**
> + * snd_soc_usb_update_offload_route - Find active USB offload path
> + * @dev - USB device to get offload status
> + * @card - USB card index
> + * @pcm - USB PCM device index
> + * @direction - playback or capture direction
> + * @route - pointer to route output array
> + *
> + * Fetch the current status for the USB SND card and PCM device indexes
> + * specified.  The "route" argument should be an array of integers being
> + * used for a kcontrol output.  The first element should have the selected
> + * card index, and the second element should have the selected pcm device
> + * index.
> + */
> +int snd_soc_usb_update_offload_route(struct device *dev, int card, int pcm,
> +				     int direction, long *route)
> +{
> +	struct snd_soc_usb *ctx;
> +	int ret = -EINVAL;
> +
> +	ctx = snd_soc_find_usb_ctx(dev);
> +	if (!ctx)
> +		return -ENODEV;
> +
> +	mutex_lock(&ctx_mutex);
> +	if (ctx && ctx->update_offload_route_info)
> +		ret = ctx->update_offload_route_info(ctx->component, card, pcm,
> +						     direction, route);
> +	mutex_unlock(&ctx_mutex);

The second ctx check is redundant.  And the locking scheme looks
dubious -- as ctx isn't protected by ctx_mutex after its retrieval via
snd_soc_find_usb_ctx(), even if you reacquire ctx_mutex, it may point
to an already released object (in theory).

IOW, for a safer protection, you'd need to cover the whole
find-and-exec procedure via a single ctx_mutex lock action.


thanks,

Takashi