diff mbox series

[v3,09/28] sound: usb: card: Introduce USB SND platform op callbacks

Message ID 20230308235751.495-10-quic_wcheng@quicinc.com
State New
Headers
Series Introduce QC USB SND audio offloading support |

Commit Message

Wesley Cheng March 8, 2023, 11:57 p.m. UTC 
  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(+)

Comments

Greg KH March 9, 2023, 6:44 a.m. UTC | #1 
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(&register_mutex);
> +	platform_ops = ops;
> +	mutex_unlock(&register_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
Oliver Neukum March 9, 2023, 11:16 a.m. UTC | #2 
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(&register_mutex);
> +	platform_ops = ops;
> +	mutex_unlock(&register_mutex);

This looks _highly_ suspicious.
Why does the assignment need a lock, but the check does not?

	Regards
		Oliver
Takashi Iwai March 9, 2023, 2:10 p.m. UTC | #3 
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 mbox series

Patch

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(&register_mutex);
+	platform_ops = ops;
+	mutex_unlock(&register_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_usb_register_platform_ops);
+
+int snd_usb_unregister_platform_ops(void)
+{
+	mutex_lock(&register_mutex);
+	platform_ops = NULL;
+	mutex_unlock(&register_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(&register_mutex);
+
 	return 0;
 
  __error:
@@ -946,6 +974,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;
@@ -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(&register_mutex);
+	if (platform_ops && platform_ops->suspend_cb)
+		platform_ops->suspend_cb(intf, message);
+	mutex_unlock(&register_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 */