All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] usb: gadget: uac: fix indentation issues in UAC files.
@ 2021-10-12 16:53 Yunhao Tian
  2021-10-12 16:53 ` [PATCH 2/2] usb: gadget: u_audio: remove unnecessary array declaration of snd_kcontrol_new Yunhao Tian
  2021-10-13  6:54 ` [PATCH 1/2] usb: gadget: uac: fix indentation issues in UAC files Greg Kroah-Hartman
  0 siblings, 2 replies; 6+ messages in thread
From: Yunhao Tian @ 2021-10-12 16:53 UTC (permalink / raw)
  Cc: Felipe Balbi, Greg Kroah-Hartman, Ruslan Bilovol, linux-usb, Yunhao Tian

From: Yunhao Tian <t123yh.xyz@gmail.com>

Fixes: 02de698 ("usb: gadget: u_audio: add bi-directional volume and mute support")
Fixes: eaf6cbe ("usb: gadget: f_uac2: add volume and mute support")
Fixes: 0356e62 ("usb: gadget: f_uac1: add volume and mute support")

Signed-off-by: Yunhao Tian <t123yh.xyz@gmail.com>
---
 drivers/usb/gadget/function/f_uac2.c  |   2 +-
 drivers/usb/gadget/function/u_audio.c | 112 +++++++++++++-------------
 drivers/usb/gadget/function/u_uac1.h  |   8 +-
 drivers/usb/gadget/function/u_uac2.h  |  10 +--
 4 files changed, 64 insertions(+), 68 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
index ef55b8bb5870..11b604879c75 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -1232,7 +1232,7 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
 	agdev->params.fb_max = uac2_opts->fb_max;
 
 	if (FUOUT_EN(uac2_opts) || FUIN_EN(uac2_opts))
-    agdev->notify = afunc_notify;
+		agdev->notify = afunc_notify;
 
 	ret = g_audio_setup(agdev, "UAC2 PCM", "UAC2_Gadget");
 	if (ret)
diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index ad16163b5ff8..c5f39998c653 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -35,8 +35,8 @@ enum {
 
 /* Runtime data params for one stream */
 struct uac_rtd_params {
-	struct snd_uac_chip *uac; /* parent chip */
-	bool ep_enabled; /* if the ep is enabled */
+	struct snd_uac_chip *uac;	/* parent chip */
+	bool ep_enabled;		/* if the ep is enabled */
 
 	struct snd_pcm_substream *ss;
 
@@ -45,24 +45,23 @@ struct uac_rtd_params {
 
 	void *rbuf;
 
-	unsigned int pitch;	/* Stream pitch ratio to 1000000 */
-	unsigned int max_psize;	/* MaxPacketSize of endpoint */
+	unsigned int pitch;		/* Stream pitch ratio to 1000000 */
+	unsigned int max_psize;		/* MaxPacketSize of endpoint */
 
 	struct usb_request **reqs;
 
-	struct usb_request *req_fback; /* Feedback endpoint request */
-	bool fb_ep_enabled; /* if the ep is enabled */
+	struct usb_request *req_fback;	/* Feedback endpoint request */
+	bool fb_ep_enabled;		/* if the ep is enabled */
 
-  /* Volume/Mute controls and their state */
-  int fu_id; /* Feature Unit ID */
-  struct snd_kcontrol *snd_kctl_volume;
-  struct snd_kcontrol *snd_kctl_mute;
-  s16 volume_min, volume_max, volume_res;
-  s16 volume;
-  int mute;
-
-  spinlock_t lock; /* lock for control transfers */
+	/* Volume/Mute controls and their state */
+	int fu_id;			/* Feature Unit ID */
+	struct snd_kcontrol *snd_kctl_volume;
+	struct snd_kcontrol *snd_kctl_mute;
+	s16 volume_min, volume_max, volume_res;
+	s16 volume;
+	int mute;
 
+	spinlock_t lock;		/* lock for control transfers */
 };
 
 struct snd_uac_chip {
@@ -672,7 +671,7 @@ int u_audio_set_volume(struct g_audio *audio_dev, int playback, s16 val)
 
 	if (change)
 		snd_ctl_notify(uac->card, SNDRV_CTL_EVENT_MASK_VALUE,
-				&prm->snd_kctl_volume->id);
+			       &prm->snd_kctl_volume->id);
 
 	return 0;
 }
@@ -727,9 +726,8 @@ int u_audio_set_mute(struct g_audio *audio_dev, int playback, int val)
 }
 EXPORT_SYMBOL_GPL(u_audio_set_mute);
 
-
 static int u_audio_pitch_info(struct snd_kcontrol *kcontrol,
-				   struct snd_ctl_elem_info *uinfo)
+			      struct snd_ctl_elem_info *uinfo)
 {
 	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
 	struct snd_uac_chip *uac = prm->uac;
@@ -749,7 +747,7 @@ static int u_audio_pitch_info(struct snd_kcontrol *kcontrol,
 }
 
 static int u_audio_pitch_get(struct snd_kcontrol *kcontrol,
-				   struct snd_ctl_elem_value *ucontrol)
+			     struct snd_ctl_elem_value *ucontrol)
 {
 	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
 
@@ -759,7 +757,7 @@ static int u_audio_pitch_get(struct snd_kcontrol *kcontrol,
 }
 
 static int u_audio_pitch_put(struct snd_kcontrol *kcontrol,
-				  struct snd_ctl_elem_value *ucontrol)
+			     struct snd_ctl_elem_value *ucontrol)
 {
 	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
 	struct snd_uac_chip *uac = prm->uac;
@@ -788,7 +786,7 @@ static int u_audio_pitch_put(struct snd_kcontrol *kcontrol,
 }
 
 static int u_audio_mute_info(struct snd_kcontrol *kcontrol,
-				   struct snd_ctl_elem_info *uinfo)
+			     struct snd_ctl_elem_info *uinfo)
 {
 	uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
 	uinfo->count = 1;
@@ -800,7 +798,7 @@ static int u_audio_mute_info(struct snd_kcontrol *kcontrol,
 }
 
 static int u_audio_mute_get(struct snd_kcontrol *kcontrol,
-				   struct snd_ctl_elem_value *ucontrol)
+			    struct snd_ctl_elem_value *ucontrol)
 {
 	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
 	unsigned long flags;
@@ -813,7 +811,7 @@ static int u_audio_mute_get(struct snd_kcontrol *kcontrol,
 }
 
 static int u_audio_mute_put(struct snd_kcontrol *kcontrol,
-				  struct snd_ctl_elem_value *ucontrol)
+			    struct snd_ctl_elem_value *ucontrol)
 {
 	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
 	struct snd_uac_chip *uac = prm->uac;
@@ -841,7 +839,7 @@ static int u_audio_mute_put(struct snd_kcontrol *kcontrol,
  * TLV callback for mixer volume controls
  */
 static int u_audio_volume_tlv(struct snd_kcontrol *kcontrol, int op_flag,
-			 unsigned int size, unsigned int __user *_tlv)
+			      unsigned int size, unsigned int __user *_tlv)
 {
 	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
 	DECLARE_TLV_DB_MINMAX(scale, 0, 0);
@@ -859,7 +857,7 @@ static int u_audio_volume_tlv(struct snd_kcontrol *kcontrol, int op_flag,
 }
 
 static int u_audio_volume_info(struct snd_kcontrol *kcontrol,
-				   struct snd_ctl_elem_info *uinfo)
+			       struct snd_ctl_elem_info *uinfo)
 {
 	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
 
@@ -867,29 +865,29 @@ static int u_audio_volume_info(struct snd_kcontrol *kcontrol,
 	uinfo->count = 1;
 	uinfo->value.integer.min = 0;
 	uinfo->value.integer.max =
-		(prm->volume_max - prm->volume_min + prm->volume_res - 1)
-		/ prm->volume_res;
+		(prm->volume_max - prm->volume_min + prm->volume_res - 1) /
+		prm->volume_res;
 	uinfo->value.integer.step = 1;
 
 	return 0;
 }
 
 static int u_audio_volume_get(struct snd_kcontrol *kcontrol,
-				   struct snd_ctl_elem_value *ucontrol)
+			      struct snd_ctl_elem_value *ucontrol)
 {
 	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
 	unsigned long flags;
 
 	spin_lock_irqsave(&prm->lock, flags);
 	ucontrol->value.integer.value[0] =
-			(prm->volume - prm->volume_min) / prm->volume_res;
+		(prm->volume - prm->volume_min) / prm->volume_res;
 	spin_unlock_irqrestore(&prm->lock, flags);
 
 	return 0;
 }
 
 static int u_audio_volume_put(struct snd_kcontrol *kcontrol,
-				  struct snd_ctl_elem_value *ucontrol)
+			      struct snd_ctl_elem_value *ucontrol)
 {
 	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
 	struct snd_uac_chip *uac = prm->uac;
@@ -918,14 +916,14 @@ static int u_audio_volume_put(struct snd_kcontrol *kcontrol,
 
 
 static struct snd_kcontrol_new u_audio_controls[]  = {
-  [UAC_FBACK_CTRL] {
-    .iface =        SNDRV_CTL_ELEM_IFACE_PCM,
-    .name =         "Capture Pitch 1000000",
-    .info =         u_audio_pitch_info,
-    .get =          u_audio_pitch_get,
-    .put =          u_audio_pitch_put,
-  },
-  [UAC_MUTE_CTRL] {
+	[UAC_FBACK_CTRL] {
+		.iface =        SNDRV_CTL_ELEM_IFACE_PCM,
+		.name =         "Capture Pitch 1000000",
+		.info =         u_audio_pitch_info,
+		.get =          u_audio_pitch_get,
+		.put =          u_audio_pitch_put,
+	},
+	[UAC_MUTE_CTRL] {
 		.iface =	SNDRV_CTL_ELEM_IFACE_MIXER,
 		.name =		"", /* will be filled later */
 		.info =		u_audio_mute_info,
@@ -942,7 +940,7 @@ static struct snd_kcontrol_new u_audio_controls[]  = {
 };
 
 int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
-					const char *card_name)
+		  const char *card_name)
 {
 	struct snd_uac_chip *uac;
 	struct snd_card *card;
@@ -968,20 +966,19 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 	if (c_chmask) {
 		struct uac_rtd_params *prm = &uac->c_prm;
 
-    spin_lock_init(&prm->lock);
-    uac->c_prm.uac = uac;
+		spin_lock_init(&prm->lock);
+		uac->c_prm.uac = uac;
 		prm->max_psize = g_audio->out_ep_maxpsize;
 
 		prm->reqs = kcalloc(params->req_number,
-				    sizeof(struct usb_request *),
-				    GFP_KERNEL);
+				    sizeof(struct usb_request *), GFP_KERNEL);
 		if (!prm->reqs) {
 			err = -ENOMEM;
 			goto fail;
 		}
 
-		prm->rbuf = kcalloc(params->req_number, prm->max_psize,
-				GFP_KERNEL);
+		prm->rbuf =
+			kcalloc(params->req_number, prm->max_psize, GFP_KERNEL);
 		if (!prm->rbuf) {
 			prm->max_psize = 0;
 			err = -ENOMEM;
@@ -997,15 +994,14 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 		prm->max_psize = g_audio->in_ep_maxpsize;
 
 		prm->reqs = kcalloc(params->req_number,
-				    sizeof(struct usb_request *),
-				    GFP_KERNEL);
+				    sizeof(struct usb_request *), GFP_KERNEL);
 		if (!prm->reqs) {
 			err = -ENOMEM;
 			goto fail;
 		}
 
-		prm->rbuf = kcalloc(params->req_number, prm->max_psize,
-				GFP_KERNEL);
+		prm->rbuf =
+			kcalloc(params->req_number, prm->max_psize, GFP_KERNEL);
 		if (!prm->rbuf) {
 			prm->max_psize = 0;
 			err = -ENOMEM;
@@ -1025,8 +1021,8 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 	 * Create first PCM device
 	 * Create a substream only for non-zero channel streams
 	 */
-	err = snd_pcm_new(uac->card, pcm_name, 0,
-			       p_chmask ? 1 : 0, c_chmask ? 1 : 0, &pcm);
+	err = snd_pcm_new(uac->card, pcm_name, 0, p_chmask ? 1 : 0,
+			  c_chmask ? 1 : 0, &pcm);
 	if (err < 0)
 		goto snd_fail;
 
@@ -1084,8 +1080,8 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 		prm->fu_id = fu->id;
 
 		if (fu->mute_present) {
-			snprintf(ctrl_name, sizeof(ctrl_name),
-					"PCM %s Switch", direction);
+			snprintf(ctrl_name, sizeof(ctrl_name), "PCM %s Switch",
+				 direction);
 
 			u_audio_controls[UAC_MUTE_CTRL].name = ctrl_name;
 
@@ -1107,8 +1103,8 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 		}
 
 		if (fu->volume_present) {
-			snprintf(ctrl_name, sizeof(ctrl_name),
-					"PCM %s Volume", direction);
+			snprintf(ctrl_name, sizeof(ctrl_name), "PCM %s Volume",
+				 direction);
 
 			u_audio_controls[UAC_VOLUME_CTRL].name = ctrl_name;
 
@@ -1122,10 +1118,10 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 			kctl->id.device = pcm->device;
 			kctl->id.subdevice = i;
 
-
 			kctl->tlv.c = u_audio_volume_tlv;
-			kctl->vd[0].access |= SNDRV_CTL_ELEM_ACCESS_TLV_READ |
-					SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK;
+			kctl->vd[0].access |=
+				SNDRV_CTL_ELEM_ACCESS_TLV_READ |
+				SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK;
 
 			err = snd_ctl_add(card, kctl);
 			if (err < 0)
diff --git a/drivers/usb/gadget/function/u_uac1.h b/drivers/usb/gadget/function/u_uac1.h
index 589fae861141..6a06594033fe 100644
--- a/drivers/usb/gadget/function/u_uac1.h
+++ b/drivers/usb/gadget/function/u_uac1.h
@@ -36,14 +36,14 @@ struct f_uac1_opts {
 	int				p_srate;
 	int				p_ssize;
 
-	bool			p_mute_present;
-	bool			p_volume_present;
+	bool				p_mute_present;
+	bool				p_volume_present;
 	s16				p_volume_min;
 	s16				p_volume_max;
 	s16				p_volume_res;
 
-	bool			c_mute_present;
-	bool			c_volume_present;
+	bool				c_mute_present;
+	bool				c_volume_present;
 	s16				c_volume_min;
 	s16				c_volume_max;
 	s16				c_volume_res;
diff --git a/drivers/usb/gadget/function/u_uac2.h b/drivers/usb/gadget/function/u_uac2.h
index a73b35774c44..10aa6a8ed9fd 100644
--- a/drivers/usb/gadget/function/u_uac2.h
+++ b/drivers/usb/gadget/function/u_uac2.h
@@ -43,21 +43,21 @@ struct f_uac2_opts {
 	int				c_ssize;
 	int				c_sync;
 
-	bool			p_mute_present;
-	bool			p_volume_present;
+	bool				p_mute_present;
+	bool				p_volume_present;
 	s16				p_volume_min;
 	s16				p_volume_max;
 	s16				p_volume_res;
 
-	bool			c_mute_present;
-	bool			c_volume_present;
+	bool				c_mute_present;
+	bool				c_volume_present;
 	s16				c_volume_min;
 	s16				c_volume_max;
 	s16				c_volume_res;
 
 	int				req_number;
 	int				fb_max;
-	bool			bound;
+	bool				bound;
 
 	struct mutex			lock;
 	int				refcnt;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] usb: gadget: u_audio: remove unnecessary array declaration of snd_kcontrol_new
  2021-10-12 16:53 [PATCH 1/2] usb: gadget: uac: fix indentation issues in UAC files Yunhao Tian
@ 2021-10-12 16:53 ` Yunhao Tian
  2021-10-13  6:58   ` Greg Kroah-Hartman
  2021-10-13  7:31   ` Pavel Hofman
  2021-10-13  6:54 ` [PATCH 1/2] usb: gadget: uac: fix indentation issues in UAC files Greg Kroah-Hartman
  1 sibling, 2 replies; 6+ messages in thread
From: Yunhao Tian @ 2021-10-12 16:53 UTC (permalink / raw)
  Cc: Felipe Balbi, Greg Kroah-Hartman, Ruslan Bilovol, linux-usb, Yunhao Tian

From: Yunhao Tian <t123yh.xyz@gmail.com>

Currently, an array is used to contain all snd_kcontrol_new objects
of the audio gadget. This is unnecessary and possibly prone to an
(unlikely happen) race condition between the assignment of name
and call of snd_ctl_new1 if two audio gadget is being set up simutaneously.
This patch removes the global snd_kcontrol_new array and initialize
individual snd_kcontrol_new object when it's being used.

Signed-off-by: Yunhao Tian <t123yh.xyz@gmail.com>
---
 drivers/usb/gadget/function/u_audio.c | 65 +++++++++++----------------
 1 file changed, 25 insertions(+), 40 deletions(-)

diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index c5f39998c653..1f4226d75dd8 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -27,12 +27,6 @@
 #define PRD_SIZE_MAX	PAGE_SIZE
 #define MIN_PERIODS	4
 
-enum {
-	UAC_FBACK_CTRL,
-	UAC_MUTE_CTRL,
-	UAC_VOLUME_CTRL,
-};
-
 /* Runtime data params for one stream */
 struct uac_rtd_params {
 	struct snd_uac_chip *uac;	/* parent chip */
@@ -914,31 +908,6 @@ static int u_audio_volume_put(struct snd_kcontrol *kcontrol,
 	return change;
 }
 
-
-static struct snd_kcontrol_new u_audio_controls[]  = {
-	[UAC_FBACK_CTRL] {
-		.iface =        SNDRV_CTL_ELEM_IFACE_PCM,
-		.name =         "Capture Pitch 1000000",
-		.info =         u_audio_pitch_info,
-		.get =          u_audio_pitch_get,
-		.put =          u_audio_pitch_put,
-	},
-	[UAC_MUTE_CTRL] {
-		.iface =	SNDRV_CTL_ELEM_IFACE_MIXER,
-		.name =		"", /* will be filled later */
-		.info =		u_audio_mute_info,
-		.get =		u_audio_mute_get,
-		.put =		u_audio_mute_put,
-	},
-	[UAC_VOLUME_CTRL] {
-		.iface =	SNDRV_CTL_ELEM_IFACE_MIXER,
-		.name =		"", /* will be filled later */
-		.info =		u_audio_volume_info,
-		.get =		u_audio_volume_get,
-		.put =		u_audio_volume_put,
-	},
-};
-
 int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 		  const char *card_name)
 {
@@ -946,6 +915,7 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 	struct snd_card *card;
 	struct snd_pcm *pcm;
 	struct snd_kcontrol *kctl;
+	struct snd_kcontrol_new kctl_new;
 	struct uac_params *params;
 	int p_chmask, c_chmask;
 	int i, err;
@@ -1043,8 +1013,14 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 		strscpy(card->mixername, card_name, sizeof(card->driver));
 
 	if (c_chmask && g_audio->in_ep_fback) {
-		kctl = snd_ctl_new1(&u_audio_controls[UAC_FBACK_CTRL],
-				    &uac->c_prm);
+		kctl_new = (struct snd_kcontrol_new) {
+			.iface =        SNDRV_CTL_ELEM_IFACE_PCM,
+			.name =         "Capture Pitch 1000000",
+			.info =         u_audio_pitch_info,
+			.get =          u_audio_pitch_get,
+			.put =          u_audio_pitch_put,
+		};
+		kctl = snd_ctl_new1(&kctl_new, &uac->c_prm);
 		if (!kctl) {
 			err = -ENOMEM;
 			goto snd_fail;
@@ -1083,9 +1059,14 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 			snprintf(ctrl_name, sizeof(ctrl_name), "PCM %s Switch",
 				 direction);
 
-			u_audio_controls[UAC_MUTE_CTRL].name = ctrl_name;
-
-			kctl = snd_ctl_new1(&u_audio_controls[UAC_MUTE_CTRL],
+			kctl_new = (struct snd_kcontrol_new) {
+				.iface =	SNDRV_CTL_ELEM_IFACE_MIXER,
+				.name =		ctrl_name,
+				.info =		u_audio_mute_info,
+				.get =		u_audio_mute_get,
+				.put =		u_audio_mute_put,
+			},
+			kctl = snd_ctl_new1(&kctl_new,
 					    prm);
 			if (!kctl) {
 				err = -ENOMEM;
@@ -1106,10 +1087,14 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 			snprintf(ctrl_name, sizeof(ctrl_name), "PCM %s Volume",
 				 direction);
 
-			u_audio_controls[UAC_VOLUME_CTRL].name = ctrl_name;
-
-			kctl = snd_ctl_new1(&u_audio_controls[UAC_VOLUME_CTRL],
-					    prm);
+			kctl_new = (struct snd_kcontrol_new) {
+				.iface =	SNDRV_CTL_ELEM_IFACE_MIXER,
+				.name =		ctrl_name,
+				.info =		u_audio_volume_info,
+				.get =		u_audio_volume_get,
+				.put =		u_audio_volume_put,
+			};
+			kctl = snd_ctl_new1(&kctl_new, prm);
 			if (!kctl) {
 				err = -ENOMEM;
 				goto snd_fail;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] usb: gadget: uac: fix indentation issues in UAC files.
  2021-10-12 16:53 [PATCH 1/2] usb: gadget: uac: fix indentation issues in UAC files Yunhao Tian
  2021-10-12 16:53 ` [PATCH 2/2] usb: gadget: u_audio: remove unnecessary array declaration of snd_kcontrol_new Yunhao Tian
@ 2021-10-13  6:54 ` Greg Kroah-Hartman
  1 sibling, 0 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2021-10-13  6:54 UTC (permalink / raw)
  To: Yunhao Tian; +Cc: Felipe Balbi, Ruslan Bilovol, linux-usb

On Wed, Oct 13, 2021 at 12:53:13AM +0800, Yunhao Tian wrote:
> From: Yunhao Tian <t123yh.xyz@gmail.com>
> 
> Fixes: 02de698 ("usb: gadget: u_audio: add bi-directional volume and mute support")
> Fixes: eaf6cbe ("usb: gadget: f_uac2: add volume and mute support")
> Fixes: 0356e62 ("usb: gadget: f_uac1: add volume and mute support")
> 
> Signed-off-by: Yunhao Tian <t123yh.xyz@gmail.com>

I can not take patches with no changelog text at all, sorry.

Also, there does not need to be a blank line between "Fixes:" and your
signed-off-by.

Also, please make these all individual patches, one per driver.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] usb: gadget: u_audio: remove unnecessary array declaration of snd_kcontrol_new
  2021-10-12 16:53 ` [PATCH 2/2] usb: gadget: u_audio: remove unnecessary array declaration of snd_kcontrol_new Yunhao Tian
@ 2021-10-13  6:58   ` Greg Kroah-Hartman
  2021-10-13  7:54     ` Yunhao Tian
  2021-10-13  7:31   ` Pavel Hofman
  1 sibling, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2021-10-13  6:58 UTC (permalink / raw)
  To: Yunhao Tian; +Cc: Felipe Balbi, Ruslan Bilovol, linux-usb

[Meta comment, you didn't have anyone on the To: line, so that messes
with replies...]

On Wed, Oct 13, 2021 at 12:53:14AM +0800, Yunhao Tian wrote:
> From: Yunhao Tian <t123yh.xyz@gmail.com>
> 
> Currently, an array is used to contain all snd_kcontrol_new objects
> of the audio gadget. This is unnecessary and possibly prone to an
> (unlikely happen) race condition between the assignment of name
> and call of snd_ctl_new1 if two audio gadget is being set up simutaneously.
> This patch removes the global snd_kcontrol_new array and initialize
> individual snd_kcontrol_new object when it's being used.
> 
> Signed-off-by: Yunhao Tian <t123yh.xyz@gmail.com>
> ---
>  drivers/usb/gadget/function/u_audio.c | 65 +++++++++++----------------
>  1 file changed, 25 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
> index c5f39998c653..1f4226d75dd8 100644
> --- a/drivers/usb/gadget/function/u_audio.c
> +++ b/drivers/usb/gadget/function/u_audio.c
> @@ -27,12 +27,6 @@
>  #define PRD_SIZE_MAX	PAGE_SIZE
>  #define MIN_PERIODS	4
>  
> -enum {
> -	UAC_FBACK_CTRL,
> -	UAC_MUTE_CTRL,
> -	UAC_VOLUME_CTRL,
> -};
> -
>  /* Runtime data params for one stream */
>  struct uac_rtd_params {
>  	struct snd_uac_chip *uac;	/* parent chip */
> @@ -914,31 +908,6 @@ static int u_audio_volume_put(struct snd_kcontrol *kcontrol,
>  	return change;
>  }
>  
> -
> -static struct snd_kcontrol_new u_audio_controls[]  = {
> -	[UAC_FBACK_CTRL] {
> -		.iface =        SNDRV_CTL_ELEM_IFACE_PCM,
> -		.name =         "Capture Pitch 1000000",
> -		.info =         u_audio_pitch_info,
> -		.get =          u_audio_pitch_get,
> -		.put =          u_audio_pitch_put,
> -	},
> -	[UAC_MUTE_CTRL] {
> -		.iface =	SNDRV_CTL_ELEM_IFACE_MIXER,
> -		.name =		"", /* will be filled later */
> -		.info =		u_audio_mute_info,
> -		.get =		u_audio_mute_get,
> -		.put =		u_audio_mute_put,
> -	},
> -	[UAC_VOLUME_CTRL] {
> -		.iface =	SNDRV_CTL_ELEM_IFACE_MIXER,
> -		.name =		"", /* will be filled later */
> -		.info =		u_audio_volume_info,
> -		.get =		u_audio_volume_get,
> -		.put =		u_audio_volume_put,
> -	},
> -};
> -
>  int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
>  		  const char *card_name)
>  {
> @@ -946,6 +915,7 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
>  	struct snd_card *card;
>  	struct snd_pcm *pcm;
>  	struct snd_kcontrol *kctl;
> +	struct snd_kcontrol_new kctl_new;
>  	struct uac_params *params;
>  	int p_chmask, c_chmask;
>  	int i, err;
> @@ -1043,8 +1013,14 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
>  		strscpy(card->mixername, card_name, sizeof(card->driver));
>  
>  	if (c_chmask && g_audio->in_ep_fback) {
> -		kctl = snd_ctl_new1(&u_audio_controls[UAC_FBACK_CTRL],
> -				    &uac->c_prm);
> +		kctl_new = (struct snd_kcontrol_new) {
> +			.iface =        SNDRV_CTL_ELEM_IFACE_PCM,
> +			.name =         "Capture Pitch 1000000",
> +			.info =         u_audio_pitch_info,
> +			.get =          u_audio_pitch_get,
> +			.put =          u_audio_pitch_put,
> +		};
> +		kctl = snd_ctl_new1(&kctl_new, &uac->c_prm);

Did you test this code?  Now this data is on the stack and will be
removed later on, while the original code's data will persist after this
function returns.

Are you sure this is ok?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] usb: gadget: u_audio: remove unnecessary array declaration of snd_kcontrol_new
  2021-10-12 16:53 ` [PATCH 2/2] usb: gadget: u_audio: remove unnecessary array declaration of snd_kcontrol_new Yunhao Tian
  2021-10-13  6:58   ` Greg Kroah-Hartman
@ 2021-10-13  7:31   ` Pavel Hofman
  1 sibling, 0 replies; 6+ messages in thread
From: Pavel Hofman @ 2021-10-13  7:31 UTC (permalink / raw)
  To: Yunhao Tian; +Cc: Felipe Balbi, Greg Kroah-Hartman, Ruslan Bilovol, linux-usb


Dne 12. 10. 21 v 18:53 Yunhao Tian napsal(a):
> From: Yunhao Tian <t123yh.xyz@gmail.com>
> 
> Currently, an array is used to contain all snd_kcontrol_new objects
> of the audio gadget. This is unnecessary and possibly prone to an
> (unlikely happen) race condition between the assignment of name
> and call of snd_ctl_new1 if two audio gadget is being set up simutaneously.
> This patch removes the global snd_kcontrol_new array and initialize
> individual snd_kcontrol_new object when it's being used.
> 
> Signed-off-by: Yunhao Tian <t123yh.xyz@gmail.com>
> ---
>   drivers/usb/gadget/function/u_audio.c | 65 +++++++++++----------------
>   1 file changed, 25 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
> index c5f39998c653..1f4226d75dd8 100644
> --- a/drivers/usb/gadget/function/u_audio.c
> +++ b/drivers/usb/gadget/function/u_audio.c
> @@ -27,12 +27,6 @@
>   #define PRD_SIZE_MAX	PAGE_SIZE
>   #define MIN_PERIODS	4
>   
> -enum {
> -	UAC_FBACK_CTRL,
> -	UAC_MUTE_CTRL,
> -	UAC_VOLUME_CTRL,
> -};
> -
>   /* Runtime data params for one stream */
>   struct uac_rtd_params {
>   	struct snd_uac_chip *uac;	/* parent chip */
> @@ -914,31 +908,6 @@ static int u_audio_volume_put(struct snd_kcontrol *kcontrol,
>   	return change;
>   }
>   
> -
> -static struct snd_kcontrol_new u_audio_controls[]  = {
> -	[UAC_FBACK_CTRL] {
> -		.iface =        SNDRV_CTL_ELEM_IFACE_PCM,
> -		.name =         "Capture Pitch 1000000",
> -		.info =         u_audio_pitch_info,
> -		.get =          u_audio_pitch_get,
> -		.put =          u_audio_pitch_put,
> -	},
> -	[UAC_MUTE_CTRL] {
> -		.iface =	SNDRV_CTL_ELEM_IFACE_MIXER,
> -		.name =		"", /* will be filled later */
> -		.info =		u_audio_mute_info,
> -		.get =		u_audio_mute_get,
> -		.put =		u_audio_mute_put,
> -	},
> -	[UAC_VOLUME_CTRL] {
> -		.iface =	SNDRV_CTL_ELEM_IFACE_MIXER,
> -		.name =		"", /* will be filled later */
> -		.info =		u_audio_volume_info,
> -		.get =		u_audio_volume_get,
> -		.put =		u_audio_volume_put,
> -	},
> -};

Hi,

Please is this patch necessary? My patch (a fixed version of which I 
will submit today) defines another control and several other important 
controls are on their way in a few patches. My current devel version has:

enum {
	UAC_FBACK_CTRL,
	UAC_P_PITCH_CTRL,
	UAC_MUTE_CTRL,
	UAC_VOLUME_CTRL,
	UAC_CAPTURE_RATE_CTRL,
	UAC_PLAYBACK_RATE_CTRL,
	UAC_CAPTURE_REQ_CTRL,
	UAC_PLAYBACK_REQ_CTRL,
};

I actually like the current method, IMO it keeps it quite organized.

Anyway if you want to remove it, please can you wait for all the 
important patches to land first?

Thanks a lot for considering.

Pavel.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] usb: gadget: u_audio: remove unnecessary array declaration of snd_kcontrol_new
  2021-10-13  6:58   ` Greg Kroah-Hartman
@ 2021-10-13  7:54     ` Yunhao Tian
  0 siblings, 0 replies; 6+ messages in thread
From: Yunhao Tian @ 2021-10-13  7:54 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Felipe Balbi, Ruslan Bilovol, linux-usb

Greg Kroah-Hartman <gregkh@linuxfoundation.org> 于2021年10月13日周三 下午2:58写道：
>
> [Meta comment, you didn't have anyone on the To: line, so that messes
> with replies...]
>
I'll add To: line in my next email, thanks for pointing out.

> On Wed, Oct 13, 2021 at 12:53:14AM +0800, Yunhao Tian wrote:
> > From: Yunhao Tian <t123yh.xyz@gmail.com>
> >
> > Currently, an array is used to contain all snd_kcontrol_new objects
> > of the audio gadget. This is unnecessary and possibly prone to an
> > (unlikely happen) race condition between the assignment of name
> > and call of snd_ctl_new1 if two audio gadget is being set up simutaneously.
> > This patch removes the global snd_kcontrol_new array and initialize
> > individual snd_kcontrol_new object when it's being used.
> >
> > Signed-off-by: Yunhao Tian <t123yh.xyz@gmail.com>
> > ---
> >  drivers/usb/gadget/function/u_audio.c | 65 +++++++++++----------------
> >  1 file changed, 25 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
> > index c5f39998c653..1f4226d75dd8 100644
> > --- a/drivers/usb/gadget/function/u_audio.c
> > +++ b/drivers/usb/gadget/function/u_audio.c
> > @@ -27,12 +27,6 @@
> >  #define PRD_SIZE_MAX PAGE_SIZE
> >  #define MIN_PERIODS  4
> >
> > -enum {
> > -     UAC_FBACK_CTRL,
> > -     UAC_MUTE_CTRL,
> > -     UAC_VOLUME_CTRL,
> > -};
> > -
> >  /* Runtime data params for one stream */
> >  struct uac_rtd_params {
> >       struct snd_uac_chip *uac;       /* parent chip */
> > @@ -914,31 +908,6 @@ static int u_audio_volume_put(struct snd_kcontrol *kcontrol,
> >       return change;
> >  }
> >
> > -
> > -static struct snd_kcontrol_new u_audio_controls[]  = {
> > -     [UAC_FBACK_CTRL] {
> > -             .iface =        SNDRV_CTL_ELEM_IFACE_PCM,
> > -             .name =         "Capture Pitch 1000000",
> > -             .info =         u_audio_pitch_info,
> > -             .get =          u_audio_pitch_get,
> > -             .put =          u_audio_pitch_put,
> > -     },
> > -     [UAC_MUTE_CTRL] {
> > -             .iface =        SNDRV_CTL_ELEM_IFACE_MIXER,
> > -             .name =         "", /* will be filled later */
> > -             .info =         u_audio_mute_info,
> > -             .get =          u_audio_mute_get,
> > -             .put =          u_audio_mute_put,
> > -     },
> > -     [UAC_VOLUME_CTRL] {
> > -             .iface =        SNDRV_CTL_ELEM_IFACE_MIXER,
> > -             .name =         "", /* will be filled later */
> > -             .info =         u_audio_volume_info,
> > -             .get =          u_audio_volume_get,
> > -             .put =          u_audio_volume_put,
> > -     },
> > -};
> > -
> >  int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
> >                 const char *card_name)
> >  {
> > @@ -946,6 +915,7 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
> >       struct snd_card *card;
> >       struct snd_pcm *pcm;
> >       struct snd_kcontrol *kctl;
> > +     struct snd_kcontrol_new kctl_new;
> >       struct uac_params *params;
> >       int p_chmask, c_chmask;
> >       int i, err;
> > @@ -1043,8 +1013,14 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
> >               strscpy(card->mixername, card_name, sizeof(card->driver));
> >
> >       if (c_chmask && g_audio->in_ep_fback) {
> > -             kctl = snd_ctl_new1(&u_audio_controls[UAC_FBACK_CTRL],
> > -                                 &uac->c_prm);
> > +             kctl_new = (struct snd_kcontrol_new) {
> > +                     .iface =        SNDRV_CTL_ELEM_IFACE_PCM,
> > +                     .name =         "Capture Pitch 1000000",
> > +                     .info =         u_audio_pitch_info,
> > +                     .get =          u_audio_pitch_get,
> > +                     .put =          u_audio_pitch_put,
> > +             };
> > +             kctl = snd_ctl_new1(&kctl_new, &uac->c_prm);
>
> Did you test this code?  Now this data is on the stack and will be
> removed later on, while the original code's data will persist after this
> function returns.
>
> Are you sure this is ok?
>
> thanks,
>
> greg k-h

Hi greg,

snd_ctl_new1 copies all members of the kctl_new struct to its own
data structure, shown in the following code (in which ncontrol is kctl_new):

kctl->id.iface = ncontrol->iface;
kctl->id.device = ncontrol->device;
kctl->id.subdevice = ncontrol->subdevice;
if (ncontrol->name) {
    strscpy(kctl->id.name, ncontrol->name, sizeof(kctl->id.name));
    if (strcmp(ncontrol->name, kctl->id.name) != 0)
        pr_warn("ALSA: Control name '%s' truncated to '%s'\n",
                       ncontrol->name, kctl->id.name);
}
kctl->id.index = ncontrol->index;

so the data in kctl_new doesn't matter after calling snd_ctl_new1.
IMO It's fine to put kctl_new on the stack.

The patch is tested to be working.

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-13  7:55 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-12 16:53 [PATCH 1/2] usb: gadget: uac: fix indentation issues in UAC files Yunhao Tian
2021-10-12 16:53 ` [PATCH 2/2] usb: gadget: u_audio: remove unnecessary array declaration of snd_kcontrol_new Yunhao Tian
2021-10-13  6:58   ` Greg Kroah-Hartman
2021-10-13  7:54     ` Yunhao Tian
2021-10-13  7:31   ` Pavel Hofman
2021-10-13  6:54 ` [PATCH 1/2] usb: gadget: uac: fix indentation issues in UAC files Greg Kroah-Hartman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.