[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(+)
  

> +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.
  

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 */
  

Hi Greg,

On 2/10/2023 2:49 PM, Wesley Cheng wrote:
> 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.
> 

I looked at seeing how we could implement this at a per bus level, but 
the USB class driver model doesn't exactly have a good framework for 
supporting this.  Reason being is because, at the time of the USB SND 
class driver initialization, there is a big chance that there isn't a 
USB bus registered in the system, so the point of adding the operations 
is not clear.  However, we need to ensure that we've added the 
platform/driver operations before any USB SND devices are detected.

To add to the above, in case of OTG/DRD (dual role) designs, the USB 
HCD/bus isn't created until we move into the host role.  At that time, 
using DWC3 as an example, we will create the XHCI platform device, and 
probe the USB HCD, where a USB bus is created.

In general, we currently think this USB offload driver should co-exist 
with the USB SND class driver, which handles all devices connected 
across every bus.  We can add a check to the platform connect routine to 
ensure that there is a reference to the USB backend.  If so, then that 
particular USB bus/sysdev can be supported by the audio DSP.  That way, 
we do not falsely populate USB SND cards which are present on another 
USB bus/controller.

Thanks
Wesley Cheng
  

On Mon, Feb 27, 2023 at 06:59:32PM -0800, Wesley Cheng wrote:
> Hi Greg,
> 
> On 2/10/2023 2:49 PM, Wesley Cheng wrote:
> > 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.
> > 
> 
> I looked at seeing how we could implement this at a per bus level, but the
> USB class driver model doesn't exactly have a good framework for supporting
> this.  Reason being is because, at the time of the USB SND class driver
> initialization, there is a big chance that there isn't a USB bus registered
> in the system, so the point of adding the operations is not clear.  However,
> we need to ensure that we've added the platform/driver operations before any
> USB SND devices are detected.

But the offload "engine" is associated with the specific USB bus
controller instance in the system, so perhaps you are just not adding
this to the correct location?

The sound core shouldn't care about this at all, add the logic to the
USB host controller driver instead, why isn't this just another USB bus
function?

> To add to the above, in case of OTG/DRD (dual role) designs, the USB HCD/bus
> isn't created until we move into the host role.  At that time, using DWC3 as
> an example, we will create the XHCI platform device, and probe the USB HCD,
> where a USB bus is created.

Great, again, tie it to the specific xhci host controler instance.

> In general, we currently think this USB offload driver should co-exist with
> the USB SND class driver, which handles all devices connected across every
> bus.

And that is incorrect, please do not do that.

> We can add a check to the platform connect routine to ensure that
> there is a reference to the USB backend.  If so, then that particular USB
> bus/sysdev can be supported by the audio DSP.  That way, we do not falsely
> populate USB SND cards which are present on another USB bus/controller.

You should NEVER be able to populate a USB card unless the USB bus
controller has given you the USB interface structure to control, so I do
not understand how this is an issue.

thanks,

greg k-h
  

Hi Greg,

On 2/27/2023 11:30 PM, Greg KH wrote:
> On Mon, Feb 27, 2023 at 06:59:32PM -0800, Wesley Cheng wrote:
>> Hi Greg,
>>
>> On 2/10/2023 2:49 PM, Wesley Cheng wrote:
>>> 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.
>>>
>>
>> I looked at seeing how we could implement this at a per bus level, but the
>> USB class driver model doesn't exactly have a good framework for supporting
>> this.  Reason being is because, at the time of the USB SND class driver
>> initialization, there is a big chance that there isn't a USB bus registered
>> in the system, so the point of adding the operations is not clear.  However,
>> we need to ensure that we've added the platform/driver operations before any
>> USB SND devices are detected.
> 
> But the offload "engine" is associated with the specific USB bus
> controller instance in the system, so perhaps you are just not adding
> this to the correct location?
> 

There are several parts to the offload logic:
1.  XHCI interrupter/resource components - fetching addresses to the 
proper event ring and transfer rings for the audio DSP.  This is the 
part which is specific to the controller instance, and APIs are being 
directly exported from the XHCI HCD, as the offloading features utilized 
are only specific for XHCI based controllers.  This is handled in 
patches 1-6 in this series.  Each XHCI instance will have its own set of 
interrupters, and transfer resources.

2.  USB offload class driver - driver which interacts with USB SND for 
operations like UAC descriptor parsing, USB audio device support params, 
and USB endpoint setup (ie issuing SET_INTERFACE to enable the device to 
start playback this is a SETUP transaction).  It will interact with the 
USB backend and items in #1, to set up the audio playback.

> The sound core shouldn't care about this at all, add the logic to the
> USB host controller driver instead, why isn't this just another USB bus
> function?
> 

The intention of the platform ops here is to mainly keep track of USB 
SND card/pcm device creation, and access to the main "struct 
snd_usb_audio".  This structure carries all the information about the 
different substreams allocated, as well as the formats supported by the 
audio device.  This is passed onto the USB backend, which will be 
utilized in my next revision to allow userspace to specifically select 
the proper card/PCM device to enable offload on.

>> To add to the above, in case of OTG/DRD (dual role) designs, the USB HCD/bus
>> isn't created until we move into the host role.  At that time, using DWC3 as
>> an example, we will create the XHCI platform device, and probe the USB HCD,
>> where a USB bus is created.
> 
> Great, again, tie it to the specific xhci host controler instance.
> 
>> In general, we currently think this USB offload driver should co-exist with
>> the USB SND class driver, which handles all devices connected across every
>> bus.
> 
> And that is incorrect, please do not do that.
> 

To clarify, I think we can summarize that the qc_audio_offload driver 
(the one that registers the platform operations) is mainly responsible 
for USB SND card management, and communicating that to the USB backend.

>> We can add a check to the platform connect routine to ensure that
>> there is a reference to the USB backend.  If so, then that particular USB
>> bus/sysdev can be supported by the audio DSP.  That way, we do not falsely
>> populate USB SND cards which are present on another USB bus/controller.
> 
> You should NEVER be able to populate a USB card unless the USB bus
> controller has given you the USB interface structure to control, so I do
> not understand how this is an issue.
> 

This might not be so clear with the current revision.  In the next 
revision I have prepared, as I mentioned, we are proposing using the 
platform connect/disconnect ops here to build a reference to all USB SND 
card and PCM devices available in the system.

For example, if you have an external hub connected to the root hub, and 
each port on that hub has an audio device connected.  We want to allow 
userspace to select which card# and pcm# to start offload on. (versus 
userspace having to rely on trail and error, which Pierre touched on on 
why that is not desired)

Since the USB SND driver is based on udevs (not bus specific), if there 
are multiple roothubs (correlates to usb buses), then we only want to 
notify the USB backend of the USB SND cards on the controller which has 
offloading enabled.  Otherwise, if userspace selects a device on an 
unsupported controller, then that path would obviously fail.

I hope that clarifies some things.

Thanks
Wesley Cheng
  

On Tue, Feb 28, 2023 at 01:19:33AM -0800, Wesley Cheng wrote:
> Hi Greg,
> 
> On 2/27/2023 11:30 PM, Greg KH wrote:
> > On Mon, Feb 27, 2023 at 06:59:32PM -0800, Wesley Cheng wrote:
> > > Hi Greg,
> > > 
> > > On 2/10/2023 2:49 PM, Wesley Cheng wrote:
> > > > 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.
> > > > 
> > > 
> > > I looked at seeing how we could implement this at a per bus level, but the
> > > USB class driver model doesn't exactly have a good framework for supporting
> > > this.  Reason being is because, at the time of the USB SND class driver
> > > initialization, there is a big chance that there isn't a USB bus registered
> > > in the system, so the point of adding the operations is not clear.  However,
> > > we need to ensure that we've added the platform/driver operations before any
> > > USB SND devices are detected.
> > 
> > But the offload "engine" is associated with the specific USB bus
> > controller instance in the system, so perhaps you are just not adding
> > this to the correct location?
> > 
> 
> There are several parts to the offload logic:
> 1.  XHCI interrupter/resource components - fetching addresses to the proper
> event ring and transfer rings for the audio DSP.  This is the part which is
> specific to the controller instance, and APIs are being directly exported
> from the XHCI HCD, as the offloading features utilized are only specific for
> XHCI based controllers.  This is handled in patches 1-6 in this series.
> Each XHCI instance will have its own set of interrupters, and transfer
> resources.
> 
> 2.  USB offload class driver - driver which interacts with USB SND for
> operations like UAC descriptor parsing, USB audio device support params, and
> USB endpoint setup (ie issuing SET_INTERFACE to enable the device to start
> playback this is a SETUP transaction).  It will interact with the USB
> backend and items in #1, to set up the audio playback.
> 
> > The sound core shouldn't care about this at all, add the logic to the
> > USB host controller driver instead, why isn't this just another USB bus
> > function?
> > 
> 
> The intention of the platform ops here is to mainly keep track of USB SND
> card/pcm device creation, and access to the main "struct snd_usb_audio".
> This structure carries all the information about the different substreams
> allocated, as well as the formats supported by the audio device.  This is
> passed onto the USB backend, which will be utilized in my next revision to
> allow userspace to specifically select the proper card/PCM device to enable
> offload on.

Oh, I can't wait to see that user/kernel api :)

It's really hard to answer you here as I don't see any patches, and I
don't know how your hardware really works.  But in general, you should
always be working on the bus level here, and that will get rid of any
static lists or any "single controller pointers" that you all have had
in previous versions.

I'll wait for patches to be able to comment further.

thanks,

greg k-h