LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] usb: gadget: f_uac2: fixup feedback endpoint stop
@ 2021-08-27  7:58 Jerome Brunet
  2021-08-27  8:05 ` Felipe Balbi
  0 siblings, 1 reply; 11+ messages in thread
From: Jerome Brunet @ 2021-08-27  7:58 UTC (permalink / raw)
  To: Ruslan Bilovol, Felipe Balbi, Pavel Hofman
  Cc: Jerome Brunet, Greg Kroah-Hartman, Jack Pham, linux-kernel,
	linux-usb, Thinh Nguyen, Ferry Toth

When the uac2 function is stopped, there seems to be an issue reported on
some platforms (Intel Merrifield at least)

BUG: kernel NULL pointer dereference, address: 0000000000000008
...
RIP: 0010:dwc3_gadget_del_and_unmap_request+0x19/0xe0
...
Call Trace:
 dwc3_remove_requests.constprop.0+0x12f/0x170
 __dwc3_gadget_ep_disable+0x7a/0x160
 dwc3_gadget_ep_disable+0x3d/0xd0
 usb_ep_disable+0x1c/0x70
 u_audio_stop_capture+0x79/0x120 [u_audio]
 afunc_set_alt+0x73/0x80 [usb_f_uac2]
 composite_setup+0x224/0x1b90 [libcomposite]

The issue happens only when the gadget is using the sync type "async", not
"adaptive". This indicates that problem is coming from the feedback
endpoint, which is only used with async synchronization mode.

The problem is that request is freed regardless of usb_ep_dequeue(), which
ends up badly if the request is not actually dequeued yet.

Update the feedback endpoint free function to release the endpoint the same
way it is done for the data endpoint, which takes care of the problem.

Reported-by: Ferry Toth <ftoth@exalondelft.nl>
Tested-by: Ferry Toth <ftoth@exalondelft.nl>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 Hi Felipe,

 This solves the issue reported here [0] and makes revert [1]
 unnecessary.

 [0]: https://lore.kernel.org/r/20210824201433.11385-1-ftoth@exalondelft.nl
 [1]: https://lore.kernel.org/r/20210826185739.3868-1-ftoth@exalondelft.nl

 drivers/usb/gadget/function/u_audio.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index 018dd0978995..63d9340f008e 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -230,7 +230,13 @@ static void u_audio_iso_fback_complete(struct usb_ep *ep,
 	int status = req->status;
 
 	/* i/f shutting down */
-	if (!prm->fb_ep_enabled || req->status == -ESHUTDOWN)
+	if (!prm->fb_ep_enabled) {
+		kfree(req->buf);
+		usb_ep_free_request(ep, req);
+		return;
+	}
+
+	if (req->status == -ESHUTDOWN)
 		return;
 
 	/*
@@ -421,9 +427,10 @@ static inline void free_ep_fback(struct uac_rtd_params *prm, struct usb_ep *ep)
 	prm->fb_ep_enabled = false;
 
 	if (prm->req_fback) {
-		usb_ep_dequeue(ep, prm->req_fback);
-		kfree(prm->req_fback->buf);
-		usb_ep_free_request(ep, prm->req_fback);
+		if (usb_ep_dequeue(ep, prm->req_fback)) {
+			kfree(prm->req_fback->buf);
+			usb_ep_free_request(ep, prm->req_fback);
+		}
 		prm->req_fback = NULL;
 	}
 
-- 
2.33.0


^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH v1 1/3] Revert "usb: gadget: u_audio: add real feedback implementation"
@ 2021-08-24 20:14 Ferry Toth
  2021-08-25  9:21 ` [PATCH] usb: gadget: f_uac2: fixup feedback endpoint stop Jerome Brunet
  0 siblings, 1 reply; 11+ messages in thread
From: Ferry Toth @ 2021-08-24 20:14 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jerome Brunet, Ruslan Bilovol, Oded Gabbay,
	Cezary Rojewski, Ferry Toth, Mauro Carvalho Chehab,
	Pawel Laszczak, Felipe Balbi, Jack Pham, linux-kernel, linux-usb,
	linux-doc
  Cc: Jonathan Corbet, Lorenzo Colitti, Wesley Cheng, robh+dt, agross,
	bjorn.andersson, frowand.list, devicetree, linux-arm-msm,
	heikki.krogerus, Thinh Nguyen, Andy Shevchenko, Pavel Hofman,
	Ferry Toth

This reverts commit e89bb4288378b85c82212b60dc98ecda6b3d3a70.

The commit is part of a series with commit
24f779dac8f3efb9629adc0e486914d93dc45517 causing a BUG on dwc3
hardware, at least on Intel Merrifield platform when configured
through configfs:
BUG: kernel NULL pointer dereference, address: 0000000000000008
...
RIP: 0010:dwc3_gadget_del_and_unmap_request+0x19/0xe0
...
Call Trace:
 dwc3_remove_requests.constprop.0+0x12f/0x170
 __dwc3_gadget_ep_disable+0x7a/0x160
 dwc3_gadget_ep_disable+0x3d/0xd0
 usb_ep_disable+0x1c/0x70
 u_audio_stop_capture+0x79/0x120 [u_audio]
 afunc_set_alt+0x73/0x80 [usb_f_uac2]
 composite_setup+0x224/0x1b90 [libcomposite]

Pavel's suggestion to add
`echo "adaptive" > functions/uac2.usb0/c_sync` to the configfs script
resolves the issue.
Thinh suggests "the crash is probably because of f_uac2 prematurely
freeing feedback request before its completion. usb_ep_dequeue() is
asynchronous. dwc2() may treat it as a synchronous call so you didn't
get a crash."

Revert as this is a regression and the kernel shouldn't crash depending
on configuration parameters.

Reported-by: Ferry Toth <fntoth@gmail.com>
---
 .../ABI/testing/configfs-usb-gadget-uac2      |   1 -
 Documentation/usb/gadget-testing.rst          |   1 -
 drivers/usb/gadget/function/f_uac2.c          |   9 +-
 drivers/usb/gadget/function/u_audio.c         | 124 ++----------------
 drivers/usb/gadget/function/u_audio.h         |   9 --
 drivers/usb/gadget/function/u_uac2.h          |   2 -
 6 files changed, 10 insertions(+), 136 deletions(-)

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uac2 b/Documentation/ABI/testing/configfs-usb-gadget-uac2
index 26fb8e9b4e61..e7e59d7fb12f 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uac2
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uac2
@@ -9,7 +9,6 @@ Description:
 		c_srate    capture sampling rate
 		c_ssize    capture sample size (bytes)
 		c_sync     capture synchronization type (async/adaptive)
-		fb_max     maximum extra bandwidth in async mode
 		p_chmask   playback channel mask
 		p_srate    playback sampling rate
 		p_ssize    playback sample size (bytes)
diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst
index 9d6276f82774..f5a12667bd41 100644
--- a/Documentation/usb/gadget-testing.rst
+++ b/Documentation/usb/gadget-testing.rst
@@ -729,7 +729,6 @@ The uac2 function provides these attributes in its function directory:
 	c_srate		capture sampling rate
 	c_ssize		capture sample size (bytes)
 	c_sync		capture synchronization type (async/adaptive)
-	fb_max          maximum extra bandwidth in async mode
 	p_chmask	playback channel mask
 	p_srate		playback sampling rate
 	p_ssize		playback sample size (bytes)
diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
index ae29ff2b2b68..321e6c05ba93 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -584,11 +584,8 @@ static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
 		ssize = uac2_opts->c_ssize;
 	}
 
-	if (!is_playback && (uac2_opts->c_sync == USB_ENDPOINT_SYNC_ASYNC))
-		srate = srate * (1000 + uac2_opts->fb_max) / 1000;
-
 	max_size_bw = num_channels(chmask) * ssize *
-		DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
+		((srate / (factor / (1 << (ep_desc->bInterval - 1)))) + 1);
 	ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw,
 						    max_size_ep));
 
@@ -960,7 +957,6 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
 	agdev->params.c_srate = uac2_opts->c_srate;
 	agdev->params.c_ssize = uac2_opts->c_ssize;
 	agdev->params.req_number = uac2_opts->req_number;
-	agdev->params.fb_max = uac2_opts->fb_max;
 	ret = g_audio_setup(agdev, "UAC2 PCM", "UAC2_Gadget");
 	if (ret)
 		goto err_free_descs;
@@ -1333,7 +1329,6 @@ UAC2_ATTRIBUTE(c_srate);
 UAC2_ATTRIBUTE_SYNC(c_sync);
 UAC2_ATTRIBUTE(c_ssize);
 UAC2_ATTRIBUTE(req_number);
-UAC2_ATTRIBUTE(fb_max);
 
 static struct configfs_attribute *f_uac2_attrs[] = {
 	&f_uac2_opts_attr_p_chmask,
@@ -1344,7 +1339,6 @@ static struct configfs_attribute *f_uac2_attrs[] = {
 	&f_uac2_opts_attr_c_ssize,
 	&f_uac2_opts_attr_c_sync,
 	&f_uac2_opts_attr_req_number,
-	&f_uac2_opts_attr_fb_max,
 	NULL,
 };
 
@@ -1384,7 +1378,6 @@ static struct usb_function_instance *afunc_alloc_inst(void)
 	opts->c_ssize = UAC2_DEF_CSSIZE;
 	opts->c_sync = UAC2_DEF_CSYNC;
 	opts->req_number = UAC2_DEF_REQ_NUM;
-	opts->fb_max = UAC2_DEF_FB_MAX;
 	return &opts->func_inst;
 }
 
diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index 018dd0978995..f637ebec80b0 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -16,7 +16,6 @@
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
-#include <sound/control.h>
 
 #include "u_audio.h"
 
@@ -36,12 +35,12 @@ struct uac_rtd_params {
 
 	void *rbuf;
 
-	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 */
+	unsigned int ffback; /* Real frequency reported by feedback endpoint */
 	bool fb_ep_enabled; /* if the ep is enabled */
 };
 
@@ -76,29 +75,18 @@ static const struct snd_pcm_hardware uac_pcm_hardware = {
 };
 
 static void u_audio_set_fback_frequency(enum usb_device_speed speed,
-					unsigned long long freq,
-					unsigned int pitch,
-					void *buf)
+					unsigned int freq, void *buf)
 {
 	u32 ff = 0;
 
-	/*
-	 * Because the pitch base is 1000000, the final divider here
-	 * will be 1000 * 1000000 = 1953125 << 9
-	 *
-	 * Instead of dealing with big numbers lets fold this 9 left shift
-	 */
-
 	if (speed == USB_SPEED_FULL) {
 		/*
 		 * Full-speed feedback endpoints report frequency
-		 * in samples/frame
+		 * in samples/microframe
 		 * Format is encoded in Q10.10 left-justified in the 24 bits,
 		 * so that it has a Q10.14 format.
-		 *
-		 * ff = (freq << 14) / 1000
 		 */
-		freq <<= 5;
+		ff = DIV_ROUND_UP((freq << 14), 1000);
 	} else {
 		/*
 		 * High-speed feedback endpoints report frequency
@@ -106,14 +94,9 @@ static void u_audio_set_fback_frequency(enum usb_device_speed speed,
 		 * Format is encoded in Q12.13 fitted into four bytes so that
 		 * the binary point is located between the second and the third
 		 * byte fromat (that is Q16.16)
-		 *
-		 * ff = (freq << 16) / 8000
 		 */
-		freq <<= 4;
+		ff = DIV_ROUND_UP((freq << 13), 1000);
 	}
-
-	ff = DIV_ROUND_CLOSEST_ULL((freq * pitch), 1953125);
-
 	*(__le32 *)buf = cpu_to_le32(ff);
 }
 
@@ -226,8 +209,8 @@ static void u_audio_iso_fback_complete(struct usb_ep *ep,
 	struct uac_rtd_params *prm = req->context;
 	struct snd_uac_chip *uac = prm->uac;
 	struct g_audio *audio_dev = uac->audio_dev;
-	struct uac_params *params = &audio_dev->params;
 	int status = req->status;
+	unsigned long flags;
 
 	/* i/f shutting down */
 	if (!prm->fb_ep_enabled || req->status == -ESHUTDOWN)
@@ -242,8 +225,7 @@ static void u_audio_iso_fback_complete(struct usb_ep *ep,
 			__func__, status, req->actual, req->length);
 
 	u_audio_set_fback_frequency(audio_dev->gadget->speed,
-				    params->c_srate, prm->pitch,
-				    req->buf);
+				    prm->ffback, req->buf);
 
 	if (usb_ep_queue(ep, req, GFP_ATOMIC))
 		dev_err(uac->card->dev, "%d Error!\n", __LINE__);
@@ -498,10 +480,9 @@ int u_audio_start_capture(struct g_audio *audio_dev)
 	 * Always start with original frequency since its deviation can't
 	 * be meauserd at start of playback
 	 */
-	prm->pitch = 1000000;
+	prm->ffback = params->c_srate;
 	u_audio_set_fback_frequency(audio_dev->gadget->speed,
-				    params->c_srate, prm->pitch,
-				    req_fback->buf);
+				    prm->ffback, req_fback->buf);
 
 	if (usb_ep_queue(ep_fback, req_fback, GFP_ATOMIC))
 		dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
@@ -597,82 +578,12 @@ void u_audio_stop_playback(struct g_audio *audio_dev)
 }
 EXPORT_SYMBOL_GPL(u_audio_stop_playback);
 
-static int u_audio_pitch_info(struct snd_kcontrol *kcontrol,
-				   struct snd_ctl_elem_info *uinfo)
-{
-	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
-	struct snd_uac_chip *uac = prm->uac;
-	struct g_audio *audio_dev = uac->audio_dev;
-	struct uac_params *params = &audio_dev->params;
-	unsigned int pitch_min, pitch_max;
-
-	pitch_min = (1000 - FBACK_SLOW_MAX) * 1000;
-	pitch_max = (1000 + params->fb_max) * 1000;
-
-	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
-	uinfo->count = 1;
-	uinfo->value.integer.min = pitch_min;
-	uinfo->value.integer.max = pitch_max;
-	uinfo->value.integer.step = 1;
-	return 0;
-}
-
-static int u_audio_pitch_get(struct snd_kcontrol *kcontrol,
-				   struct snd_ctl_elem_value *ucontrol)
-{
-	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
-
-	ucontrol->value.integer.value[0] = prm->pitch;
-
-	return 0;
-}
-
-static int u_audio_pitch_put(struct snd_kcontrol *kcontrol,
-				  struct snd_ctl_elem_value *ucontrol)
-{
-	struct uac_rtd_params *prm = snd_kcontrol_chip(kcontrol);
-	struct snd_uac_chip *uac = prm->uac;
-	struct g_audio *audio_dev = uac->audio_dev;
-	struct uac_params *params = &audio_dev->params;
-	unsigned int val;
-	unsigned int pitch_min, pitch_max;
-	int change = 0;
-
-	pitch_min = (1000 - FBACK_SLOW_MAX) * 1000;
-	pitch_max = (1000 + params->fb_max) * 1000;
-
-	val = ucontrol->value.integer.value[0];
-
-	if (val < pitch_min)
-		val = pitch_min;
-	if (val > pitch_max)
-		val = pitch_max;
-
-	if (prm->pitch != val) {
-		prm->pitch = val;
-		change = 1;
-	}
-
-	return change;
-}
-
-static const struct snd_kcontrol_new u_audio_controls[]  = {
-{
-	.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,
-},
-};
-
 int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 					const char *card_name)
 {
 	struct snd_uac_chip *uac;
 	struct snd_card *card;
 	struct snd_pcm *pcm;
-	struct snd_kcontrol *kctl;
 	struct uac_params *params;
 	int p_chmask, c_chmask;
 	int err;
@@ -760,23 +671,6 @@ int g_audio_setup(struct g_audio *g_audio, const char *pcm_name,
 	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &uac_pcm_ops);
 	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &uac_pcm_ops);
 
-	if (c_chmask && g_audio->in_ep_fback) {
-		strscpy(card->mixername, card_name, sizeof(card->driver));
-
-		kctl = snd_ctl_new1(&u_audio_controls[0], &uac->c_prm);
-		if (!kctl) {
-			err = -ENOMEM;
-			goto snd_fail;
-		}
-
-		kctl->id.device = pcm->device;
-		kctl->id.subdevice = 0;
-
-		err = snd_ctl_add(card, kctl);
-		if (err < 0)
-			goto snd_fail;
-	}
-
 	strscpy(card->driver, card_name, sizeof(card->driver));
 	strscpy(card->shortname, card_name, sizeof(card->shortname));
 	sprintf(card->longname, "%s %i", card_name, card->dev->id);
diff --git a/drivers/usb/gadget/function/u_audio.h b/drivers/usb/gadget/function/u_audio.h
index a218cdf771fe..53e6baf55cbf 100644
--- a/drivers/usb/gadget/function/u_audio.h
+++ b/drivers/usb/gadget/function/u_audio.h
@@ -11,14 +11,6 @@
 
 #include <linux/usb/composite.h>
 
-/*
- * Same maximum frequency deviation on the slower side as in
- * sound/usb/endpoint.c. Value is expressed in per-mil deviation.
- * The maximum deviation on the faster side will be provided as
- * parameter, as it impacts the endpoint required bandwidth.
- */
-#define FBACK_SLOW_MAX	250
-
 struct uac_params {
 	/* playback */
 	int p_chmask;	/* channel mask */
@@ -31,7 +23,6 @@ struct uac_params {
 	int c_ssize;	/* sample size */
 
 	int req_number; /* number of preallocated requests */
-	int fb_max;	/* upper frequency drift feedback limit per-mil */
 };
 
 struct g_audio {
diff --git a/drivers/usb/gadget/function/u_uac2.h b/drivers/usb/gadget/function/u_uac2.h
index 179d3ef6a195..13589c3c805c 100644
--- a/drivers/usb/gadget/function/u_uac2.h
+++ b/drivers/usb/gadget/function/u_uac2.h
@@ -23,7 +23,6 @@
 #define UAC2_DEF_CSSIZE 2
 #define UAC2_DEF_CSYNC		USB_ENDPOINT_SYNC_ASYNC
 #define UAC2_DEF_REQ_NUM 2
-#define UAC2_DEF_FB_MAX 5
 
 struct f_uac2_opts {
 	struct usb_function_instance	func_inst;
@@ -35,7 +34,6 @@ struct f_uac2_opts {
 	int				c_ssize;
 	int				c_sync;
 	int				req_number;
-	int				fb_max;
 	bool				bound;
 
 	struct mutex			lock;
-- 
2.30.2


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-08-27 23:30 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-27  7:58 [PATCH] usb: gadget: f_uac2: fixup feedback endpoint stop Jerome Brunet
2021-08-27  8:05 ` Felipe Balbi
2021-08-27  9:26   ` Jerome Brunet
  -- strict thread matches above, loose matches on Subject: below --
2021-08-24 20:14 [PATCH v1 1/3] Revert "usb: gadget: u_audio: add real feedback implementation" Ferry Toth
2021-08-25  9:21 ` [PATCH] usb: gadget: f_uac2: fixup feedback endpoint stop Jerome Brunet
2021-08-25 20:07   ` Ferry Toth
2021-08-25 21:42     ` Thinh Nguyen
2021-08-26  7:25       ` Jerome Brunet
2021-08-27  0:50         ` Thinh Nguyen
2021-08-27  8:38           ` Jerome Brunet
2021-08-27 23:30             ` Thinh Nguyen
2021-08-26  7:23     ` Jerome Brunet

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).