Re: [PATCH v26 24/33] ALSA: usb-audio: Introduce USB SND platform op callbacks

[Wesley Cheng <quic_wcheng@quicinc.com>]

Hi Pierre,

On 8/30/2024 2:38 AM, Pierre-Louis Bossart wrote:
>
> On 8/29/24 21:40, Wesley Cheng wrote:
>> Allow for different platforms to be notified on USB SND connect/disconnect
>> sequences.  This allows for platform USB SND modules to properly initialize
>> and populate internal structures with references to the USB SND chip
>> device.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>  sound/usb/card.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  sound/usb/card.h | 10 +++++++++
>>  2 files changed, 63 insertions(+)
>>
>> diff --git a/sound/usb/card.c b/sound/usb/card.c
>> index 1f9dfcd8f336..7f120aa006c0 100644
>> --- a/sound/usb/card.c
>> +++ b/sound/usb/card.c
>> @@ -118,6 +118,42 @@ MODULE_PARM_DESC(skip_validation, "Skip unit descriptor validation (default: no)
>>  static DEFINE_MUTEX(register_mutex);
>>  static struct snd_usb_audio *usb_chip[SNDRV_CARDS];
>>  static struct usb_driver usb_audio_driver;
>> +static struct snd_usb_platform_ops *platform_ops;
>> +
>> +/*
>> + * Register platform specific operations that will be notified on events
>> + * which occur in USB SND.  The platform driver can utilize this path to
>> + * enable features, such as USB audio offloading, which allows for audio data
>> + * to be queued by an audio DSP.
>> + *
>> + * Only one set of platform operations can be registered to USB SND.  The
>> + * platform register operation is protected by the register_mutex.
>> + */
>> +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops)
>> +{
>> +	guard(mutex)(&register_mutex);
>> +	if (platform_ops)
>> +		return -EEXIST;
>> +
>> +	platform_ops = ops;
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_usb_register_platform_ops);
>> +
>> +/*
>> + * Unregisters the current set of platform operations.  This allows for
> Unregister?
Will fix.
>> + * a new set to be registered if required.
>> + *
>> + * The platform unregister operation is protected by the register_mutex.
>> + */
>> +int snd_usb_unregister_platform_ops(void)
>> +{
>> +	guard(mutex)(&register_mutex);
>> +	platform_ops = NULL;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops);
>>  
>>  /*
>>   * Checks to see if requested audio profile, i.e sample rate, # of
>> @@ -946,7 +982,11 @@ static int usb_audio_probe(struct usb_interface *intf,
>>  	chip->num_interfaces++;
>>  	usb_set_intfdata(intf, chip);
>>  	atomic_dec(&chip->active);
>> +
>> +	if (platform_ops && platform_ops->connect_cb)
>> +		platform_ops->connect_cb(chip);
>>  	mutex_unlock(&register_mutex);
>> +
>>  	return 0;
>>  
>>   __error:
>> @@ -983,6 +1023,9 @@ static void usb_audio_disconnect(struct usb_interface *intf)
>>  	card = chip->card;
>>  
>>  	mutex_lock(&register_mutex);
>> +	if (platform_ops && platform_ops->disconnect_cb)
>> +		platform_ops->disconnect_cb(chip);
>> +
>>  	if (atomic_inc_return(&chip->shutdown) == 1) {
>>  		struct snd_usb_stream *as;
>>  		struct snd_usb_endpoint *ep;
>> @@ -1130,6 +1173,11 @@ static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message)
>>  		chip->system_suspend = chip->num_suspended_intf;
>>  	}
>>  
>> +	mutex_lock(&register_mutex);
>> +	if (platform_ops && platform_ops->suspend_cb)
>> +		platform_ops->suspend_cb(intf, message);
>> +	mutex_unlock(&register_mutex);
>> +
>>  	return 0;
>>  }
>>  
>> @@ -1170,6 +1218,11 @@ static int usb_audio_resume(struct usb_interface *intf)
>>  
>>  	snd_usb_midi_v2_resume_all(chip);
>>  
>> +	mutex_lock(&register_mutex);
>> +	if (platform_ops && platform_ops->resume_cb)
>> +		platform_ops->resume_cb(intf);
>> +	mutex_unlock(&register_mutex);
>> +
>>   out:
>>  	if (chip->num_suspended_intf == chip->system_suspend) {
>>  		snd_power_change_state(chip->card, SNDRV_CTL_POWER_D0);
>> diff --git a/sound/usb/card.h b/sound/usb/card.h
>> index 4f4f3f39b7fa..23d9e6fc69e7 100644
>> --- a/sound/usb/card.h
>> +++ b/sound/usb/card.h
>> @@ -207,7 +207,17 @@ struct snd_usb_stream {
>>  	struct list_head list;
>>  };
>>  
>> +struct snd_usb_platform_ops {
>> +	void (*connect_cb)(struct snd_usb_audio *chip);
>> +	void (*disconnect_cb)(struct snd_usb_audio *chip);
>> +	void (*suspend_cb)(struct usb_interface *intf, pm_message_t message);
>> +	void (*resume_cb)(struct usb_interface *intf);
>> +};
>
> You're using the same mutex to protect all four callbacks, so how would
> things work if e.g. you disconnected a device during the resume operation?

We actually might be able to remove the mutex locks from the suspend and resume callbacks.  From looking at the USB core driver, whenever the USB interface is unbounded, it ensures that it is in an active/resumed state before the disconnect() is called:

static int usb_unbind_interface(struct device *dev)
{
..
    /* Autoresume for set_interface call below */
    udev = interface_to_usbdev(intf);
    error = usb_autoresume_device(udev);
..

    driver->disconnect(intf);

So this will ensure that there won't be a condition where an interface disconnect routine could run in parallel to the interface's runtime resume or runtime suspend.

Thanks

Wesley Cheng