| Message ID | 20230308235751.495-10-quic_wcheng@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce QC USB SND audio offloading support | expand |
On Wed, Mar 08, 2023 at 03:57:32PM -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 | 36 ++++++++++++++++++++++++++++++++++++ > sound/usb/card.h | 20 ++++++++++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/sound/usb/card.c b/sound/usb/card.c > index 26268ffb8274..9bcbaa0c0a55 100644 > --- a/sound/usb/card.c > +++ b/sound/usb/card.c > @@ -117,6 +117,30 @@ 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; As I've said before, you can not just have one of these. They need to be per-bus structure. Or per-device, something dynamic, not static like this. > +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops) > +{ > + if (platform_ops) > + return -EEXIST; > + > + mutex_lock(®ister_mutex); > + platform_ops = ops; > + mutex_unlock(®ister_mutex); Your locking is odd for a single pointer, why is it needed at all? Also you check the pointer before using the lock, which defeats the lock in the first place. thanks, greg k-h
On 09.03.23 00:57, 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 | 36 ++++++++++++++++++++++++++++++++++++ > sound/usb/card.h | 20 ++++++++++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/sound/usb/card.c b/sound/usb/card.c > index 26268ffb8274..9bcbaa0c0a55 100644 > --- a/sound/usb/card.c > +++ b/sound/usb/card.c > @@ -117,6 +117,30 @@ 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; > + > + mutex_lock(®ister_mutex); > + platform_ops = ops; > + mutex_unlock(®ister_mutex); This looks _highly_ suspicious. Why does the assignment need a lock, but the check does not? Regards Oliver
On Thu, 09 Mar 2023 07:44:13 +0100, Greg KH wrote: > > On Wed, Mar 08, 2023 at 03:57:32PM -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 | 36 ++++++++++++++++++++++++++++++++++++ > > sound/usb/card.h | 20 ++++++++++++++++++++ > > 2 files changed, 56 insertions(+) > > > > diff --git a/sound/usb/card.c b/sound/usb/card.c > > index 26268ffb8274..9bcbaa0c0a55 100644 > > --- a/sound/usb/card.c > > +++ b/sound/usb/card.c > > @@ -117,6 +117,30 @@ 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; > > As I've said before, you can not just have one of these. They need to > be per-bus structure. Or per-device, something dynamic, not static like > this. Basically the change for USB-audio driver is a kind of add-on/plugin; that is, it allows the platform_ops to override some operations of the standard USB-audio driver for supporting the offload operations. So, in that sense, this add-on itself is neither per-bus nor per-device, and it's fine as a single pointer for now, IMO, since this is the only known use case and it'd simplify the code. If we'll have more similar requirements in future, we may extend the implementation and the API to allow the registration of multiple addons, too. Though, there are a few obvious coding issues in the current patches, and they have to be addressed, sure :) thanks, Takashi
diff --git a/sound/usb/card.c b/sound/usb/card.c index 26268ffb8274..9bcbaa0c0a55 100644 --- a/sound/usb/card.c +++ b/sound/usb/card.c @@ -117,6 +117,30 @@ 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; + + mutex_lock(®ister_mutex); + platform_ops = ops; + mutex_unlock(®ister_mutex); + + return 0; +} +EXPORT_SYMBOL_GPL(snd_usb_register_platform_ops); + +int snd_usb_unregister_platform_ops(void) +{ + mutex_lock(®ister_mutex); + platform_ops = NULL; + mutex_unlock(®ister_mutex); + + return 0; +} +EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops); /* * disconnect streams @@ -909,7 +933,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(®ister_mutex); + return 0; __error: @@ -946,6 +974,9 @@ static void usb_audio_disconnect(struct usb_interface *intf) card = chip->card; mutex_lock(®ister_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; @@ -1087,6 +1118,11 @@ static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message) chip->system_suspend = chip->num_suspended_intf; } + mutex_lock(®ister_mutex); + if (platform_ops && platform_ops->suspend_cb) + platform_ops->suspend_cb(intf, message); + mutex_unlock(®ister_mutex); + return 0; } diff --git a/sound/usb/card.h b/sound/usb/card.h index 6ec95b2edf86..94c3ae7bb4a5 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -207,4 +207,24 @@ 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); +}; + +#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 */
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 | 36 ++++++++++++++++++++++++++++++++++++ sound/usb/card.h | 20 ++++++++++++++++++++ 2 files changed, 56 insertions(+)