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

[Wesley Cheng <quic_wcheng@xxxxxxxxxxx>]

Hi Takashi,

On 11/20/2024 4:23 AM, Takashi Iwai wrote:
> 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.
>
That's fair, will make the change to move the mutexes around.

Thanks

Wesley Cheng