[RFC,v2,12/22] sound: usb: card: Introduce USB SND platform op callbacks

Allow for different platforms to be notified on USB SND connect/disconnect
seqeunces.  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 | 28 ++++++++++++++++++++++++++++
 sound/usb/card.h | 20 ++++++++++++++++++++
 2 files changed, 48 insertions(+)

On Wed, Jan 25, 2023 at 07:14:14PM -0800, Wesley Cheng wrote:
> Allow for different platforms to be notified on USB SND connect/disconnect
> seqeunces.  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 | 28 ++++++++++++++++++++++++++++
>  sound/usb/card.h | 20 ++++++++++++++++++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/sound/usb/card.c b/sound/usb/card.c
> index 26268ffb8274..803230343c16 100644
> --- a/sound/usb/card.c
> +++ b/sound/usb/card.c
> @@ -117,6 +117,24 @@ 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;

You can not have a single "platform_ops" pointer, this HAS to be
per-bus.

And what is a "platform operations" anyway?  Shouldn't this be a driver
type or something like that?  "offload_operations"?

> +
> +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops)
> +{
> +	if (platform_ops)
> +		return -EEXIST;
> +
> +	platform_ops = ops;
> +	return 0;

No locking?  not good.

But again, this has to be per-USB-bus, it can NOT be system wide for
obvious reasons.

thanks,

greg k-h

Hi Pierre,

On 1/26/2023 7:50 AM, Pierre-Louis Bossart wrote:
> 
> 
> 
>> +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops)
>> +{
>> +	if (platform_ops)
>> +		return -EEXIST;
>> +
>> +	platform_ops = ops;
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_usb_register_platform_ops);
>> +
>> +int snd_usb_unregister_platform_ops(void)
>> +{
>> +	platform_ops = NULL;
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops);
> 
> I find this super-racy.
> 
> If the this function is called just before ...
> 
>>   
>>   /*
>>    * disconnect streams
>> @@ -910,6 +928,10 @@ static int usb_audio_probe(struct usb_interface *intf,
>>   	usb_set_intfdata(intf, chip);
>>   	atomic_dec(&chip->active);
>>   	mutex_unlock(&register_mutex);
>> +
>> +	if (platform_ops->connect_cb)
>> +		platform_ops->connect_cb(intf, chip);
>> +
> 
> ... this, then you have a risk of using a dandling pointer.
> 
> You also didn't test that the platform_ops != NULL, so there's a risk of
> dereferencing a NULL pointer.
> 
> Not so good, eh?
> 
> It's a classic (I've had the same sort of issues with SoundWire), when
> you export ops from one driver than can be removed, then additional
> protection is needed when using those callbacks.
> 
> 

Yep, will take a look at this a bit more to improve it.

Thanks
Wesley Cheng

Hi Greg,

On 1/28/2023 5:28 AM, Greg KH wrote:
> On Wed, Jan 25, 2023 at 07:14:14PM -0800, Wesley Cheng wrote:
>> Allow for different platforms to be notified on USB SND connect/disconnect
>> seqeunces.  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 | 28 ++++++++++++++++++++++++++++
>>   sound/usb/card.h | 20 ++++++++++++++++++++
>>   2 files changed, 48 insertions(+)
>>
>> diff --git a/sound/usb/card.c b/sound/usb/card.c
>> index 26268ffb8274..803230343c16 100644
>> --- a/sound/usb/card.c
>> +++ b/sound/usb/card.c
>> @@ -117,6 +117,24 @@ 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;
> 
> You can not have a single "platform_ops" pointer, this HAS to be
> per-bus.
> 

Agreed.

> And what is a "platform operations" anyway?  Shouldn't this be a driver
> type or something like that?  "offload_operations"?
> 

The reason for going with platform operations is because every platform 
may implement the offloading differently.  The offload operations term 
is more direct though in terms of explaining what the ops are going to 
be used for, so I can see the incentive of moving to that phrase.

>> +
>> +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops)
>> +{
>> +	if (platform_ops)
>> +		return -EEXIST;
>> +
>> +	platform_ops = ops;
>> +	return 0;
> 
> No locking?  not good.
> 
> But again, this has to be per-USB-bus, it can NOT be system wide for
> obvious reasons.
> 

Sure, will change that when moving to per USB bus.

Thanks
Wesley Cheng

Hi Wesley,

It looks like your audio offload driver will fetch the required
resources for a stream enable request. But we have different designs.
In the integration with your patch set, we found we still need a call
back function in card.c when the usb set interface is done, in which
we would call the new API, xhci_get_xfer_resource(), to get the EP
transfer ring address. Of course, we will try the
platform_ops->connect_cb() first to see if it is able to cover what we
need or not.


Thanks,
Albert Wang

Albert Wang | Pixel USB Software  | albertccwang@google.com | +886-918-695-245


On Thu, Jan 26, 2023 at 11:16 AM Wesley Cheng <quic_wcheng@quicinc.com> wrote:
>
> Allow for different platforms to be notified on USB SND connect/disconnect
> seqeunces.  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 | 28 ++++++++++++++++++++++++++++
>  sound/usb/card.h | 20 ++++++++++++++++++++
>  2 files changed, 48 insertions(+)
>
> diff --git a/sound/usb/card.c b/sound/usb/card.c
> index 26268ffb8274..803230343c16 100644
> --- a/sound/usb/card.c
> +++ b/sound/usb/card.c
> @@ -117,6 +117,24 @@ 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;
> +
> +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops)
> +{
> +       if (platform_ops)
> +               return -EEXIST;
> +
> +       platform_ops = ops;
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_usb_register_platform_ops);
> +
> +int snd_usb_unregister_platform_ops(void)
> +{
> +       platform_ops = NULL;
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops);
>
>  /*
>   * disconnect streams
> @@ -910,6 +928,10 @@ static int usb_audio_probe(struct usb_interface *intf,
>         usb_set_intfdata(intf, chip);
>         atomic_dec(&chip->active);
>         mutex_unlock(&register_mutex);
> +
> +       if (platform_ops->connect_cb)
> +               platform_ops->connect_cb(intf, chip);
> +
>         return 0;
>
>   __error:
> @@ -943,6 +965,9 @@ static void usb_audio_disconnect(struct usb_interface *intf)
>         if (chip == USB_AUDIO_IFACE_UNUSED)
>                 return;
>
> +       if (platform_ops->disconnect_cb)
> +               platform_ops->disconnect_cb(intf);
> +
>         card = chip->card;
>
>         mutex_lock(&register_mutex);
> @@ -1087,6 +1112,9 @@ static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message)
>                 chip->system_suspend = chip->num_suspended_intf;
>         }
>
> +       if (platform_ops->suspend_cb)
> +               platform_ops->suspend_cb(intf, message);
> +
>         return 0;
>  }
>
> diff --git a/sound/usb/card.h b/sound/usb/card.h
> index 40061550105a..2249c411c3a1 100644
> --- a/sound/usb/card.h
> +++ b/sound/usb/card.h
> @@ -206,4 +206,24 @@ struct snd_usb_stream {
>         struct list_head list;
>  };
>
> +struct snd_usb_platform_ops {
> +       void (*connect_cb)(struct usb_interface *intf, struct snd_usb_audio *chip);
> +       void (*disconnect_cb)(struct usb_interface *intf);
> +       void (*suspend_cb)(struct usb_interface *intf, pm_message_t message);
> +};
> +
> +#if IS_ENABLED(CONFIG_SND_USB_AUDIO)
> +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops);
> +int snd_usb_unregister_platform_ops(void);
> +#else
> +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
> +int snd_usb_unregister_platform_ops(void)
> +{
> +       return -EOPNOTSUPP;
> +}
> +#endif /* IS_ENABLED(CONFIG_SND_USB_AUDIO) */
>  #endif /* __USBAUDIO_CARD_H */