[PATCH v2 0/3] Revert "usb: gadget: u_audio: add real feedback implementation"

[PATCH v2 0/3] Revert "usb: gadget: u_audio: add real feedback implementation"

[Ferry Toth]

Although there is a patch resolving this issue (see 
https://lore.kernel.org/linux-usb/1jilzsy8g7.fsf@starbuckisacylon.baylibre.com/T/#u)
it needs a little work and will not be ready for v5.14.0 release. Until then
revert the series.

v2:
- Added SoB (Balbi)


Ferry Toth (3):
  Revert "usb: gadget: u_audio: add real feedback implementation"
  Revert "usb: gadget: f_uac2: add adaptive sync support for capture"
  Revert "usb: gadget: f_uac2/u_audio: add feedback endpoint support"

 .../ABI/testing/configfs-usb-gadget-uac2      |   2 -
 Documentation/usb/gadget-testing.rst          |   2 -
 drivers/usb/gadget/function/f_uac2.c          | 144 +----------
 drivers/usb/gadget/function/u_audio.c         | 225 +-----------------
 drivers/usb/gadget/function/u_audio.h         |  12 -
 drivers/usb/gadget/function/u_uac2.h          |   4 -
 6 files changed, 6 insertions(+), 383 deletions(-)

-- 
2.30.2

[PATCH v2 1/3] Revert "usb: gadget: u_audio: add real feedback implementation"

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

Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
---
 .../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

[PATCH v2 2/3] Revert "usb: gadget: f_uac2: add adaptive sync support for capture"

[Ferry Toth]

This reverts commit 40c73b30546e759bedcec607fedc2d4be954508f.

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.

Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
---
 .../ABI/testing/configfs-usb-gadget-uac2      |   1 -
 Documentation/usb/gadget-testing.rst          |   1 -
 drivers/usb/gadget/function/f_uac2.c          | 100 ++----------------
 drivers/usb/gadget/function/u_uac2.h          |   2 -
 4 files changed, 9 insertions(+), 95 deletions(-)

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uac2 b/Documentation/ABI/testing/configfs-usb-gadget-uac2
index e7e59d7fb12f..d4356c8b8cd6 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uac2
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uac2
@@ -8,7 +8,6 @@ Description:
 		c_chmask   capture channel mask
 		c_srate    capture sampling rate
 		c_ssize    capture sample size (bytes)
-		c_sync     capture synchronization type (async/adaptive)
 		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 f5a12667bd41..2085e7b24eeb 100644
--- a/Documentation/usb/gadget-testing.rst
+++ b/Documentation/usb/gadget-testing.rst
@@ -728,7 +728,6 @@ The uac2 function provides these attributes in its function directory:
 	c_chmask	capture channel mask
 	c_srate		capture sampling rate
 	c_ssize		capture sample size (bytes)
-	c_sync		capture synchronization type (async/adaptive)
 	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 321e6c05ba93..5d63244ba319 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -44,7 +44,6 @@
 
 #define EPIN_EN(_opts) ((_opts)->p_chmask != 0)
 #define EPOUT_EN(_opts) ((_opts)->c_chmask != 0)
-#define EPOUT_FBACK_IN_EN(_opts) ((_opts)->c_sync == USB_ENDPOINT_SYNC_ASYNC)
 
 struct f_uac2 {
 	struct g_audio g_audio;
@@ -241,7 +240,7 @@ static struct usb_interface_descriptor std_as_out_if1_desc = {
 	.bDescriptorType = USB_DT_INTERFACE,
 
 	.bAlternateSetting = 1,
-	.bNumEndpoints = 1,
+	.bNumEndpoints = 2,
 	.bInterfaceClass = USB_CLASS_AUDIO,
 	.bInterfaceSubClass = USB_SUBCLASS_AUDIOSTREAMING,
 	.bInterfaceProtocol = UAC_VERSION_2,
@@ -274,7 +273,7 @@ static struct usb_endpoint_descriptor fs_epout_desc = {
 	.bDescriptorType = USB_DT_ENDPOINT,
 
 	.bEndpointAddress = USB_DIR_OUT,
-	/* .bmAttributes = DYNAMIC */
+	.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
 	/* .wMaxPacketSize = DYNAMIC */
 	.bInterval = 1,
 };
@@ -283,7 +282,7 @@ static struct usb_endpoint_descriptor hs_epout_desc = {
 	.bLength = USB_DT_ENDPOINT_SIZE,
 	.bDescriptorType = USB_DT_ENDPOINT,
 
-	/* .bmAttributes = DYNAMIC */
+	.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
 	/* .wMaxPacketSize = DYNAMIC */
 	.bInterval = 4,
 };
@@ -293,7 +292,7 @@ static struct usb_endpoint_descriptor ss_epout_desc = {
 	.bDescriptorType = USB_DT_ENDPOINT,
 
 	.bEndpointAddress = USB_DIR_OUT,
-	/* .bmAttributes = DYNAMIC */
+	.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
 	/* .wMaxPacketSize = DYNAMIC */
 	.bInterval = 4,
 };
@@ -650,9 +649,7 @@ static void setup_headers(struct f_uac2_opts *opts,
 			headers[i++] = USBDHDR(epout_desc_comp);
 
 		headers[i++] = USBDHDR(&as_iso_out_desc);
-
-		if (EPOUT_FBACK_IN_EN(opts))
-			headers[i++] = USBDHDR(epin_fback_desc);
+		headers[i++] = USBDHDR(epin_fback_desc);
 	}
 	if (EPIN_EN(opts)) {
 		headers[i++] = USBDHDR(&std_as_in_if0_desc);
@@ -823,23 +820,6 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
 		std_as_out_if1_desc.bInterfaceNumber = ret;
 		uac2->as_out_intf = ret;
 		uac2->as_out_alt = 0;
-
-		if (EPOUT_FBACK_IN_EN(uac2_opts)) {
-			fs_epout_desc.bmAttributes =
-			  USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC;
-			hs_epout_desc.bmAttributes =
-			  USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC;
-			ss_epout_desc.bmAttributes =
-			  USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC;
-			std_as_out_if1_desc.bNumEndpoints++;
-		} else {
-			fs_epout_desc.bmAttributes =
-			  USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ADAPTIVE;
-			hs_epout_desc.bmAttributes =
-			  USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ADAPTIVE;
-			ss_epout_desc.bmAttributes =
-			  USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ADAPTIVE;
-		}
 	}
 
 	if (EPIN_EN(uac2_opts)) {
@@ -903,14 +883,11 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
 			dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
 			return -ENODEV;
 		}
-		if (EPOUT_FBACK_IN_EN(uac2_opts)) {
-			agdev->in_ep_fback = usb_ep_autoconfig(gadget,
+		agdev->in_ep_fback = usb_ep_autoconfig(gadget,
 						       &fs_epin_fback_desc);
-			if (!agdev->in_ep_fback) {
-				dev_err(dev, "%s:%d Error!\n",
-					__func__, __LINE__);
-				return -ENODEV;
-			}
+		if (!agdev->in_ep_fback) {
+			dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
+			return -ENODEV;
 		}
 	}
 
@@ -1265,68 +1242,11 @@ end:									\
 									\
 CONFIGFS_ATTR(f_uac2_opts_, name)
 
-#define UAC2_ATTRIBUTE_SYNC(name)					\
-static ssize_t f_uac2_opts_##name##_show(struct config_item *item,	\
-					 char *page)			\
-{									\
-	struct f_uac2_opts *opts = to_f_uac2_opts(item);		\
-	int result;							\
-	char *str;							\
-									\
-	mutex_lock(&opts->lock);					\
-	switch (opts->name) {						\
-	case USB_ENDPOINT_SYNC_ASYNC:					\
-		str = "async";						\
-		break;							\
-	case USB_ENDPOINT_SYNC_ADAPTIVE:				\
-		str = "adaptive";					\
-		break;							\
-	default:							\
-		str = "unknown";					\
-		break;							\
-	}								\
-	result = sprintf(page, "%s\n", str);				\
-	mutex_unlock(&opts->lock);					\
-									\
-	return result;							\
-}									\
-									\
-static ssize_t f_uac2_opts_##name##_store(struct config_item *item,	\
-					  const char *page, size_t len)	\
-{									\
-	struct f_uac2_opts *opts = to_f_uac2_opts(item);		\
-	int ret = 0;							\
-									\
-	mutex_lock(&opts->lock);					\
-	if (opts->refcnt) {						\
-		ret = -EBUSY;						\
-		goto end;						\
-	}								\
-									\
-	if (!strncmp(page, "async", 5))					\
-		opts->name = USB_ENDPOINT_SYNC_ASYNC;			\
-	else if (!strncmp(page, "adaptive", 8))				\
-		opts->name = USB_ENDPOINT_SYNC_ADAPTIVE;		\
-	else {								\
-		ret = -EINVAL;						\
-		goto end;						\
-	}								\
-									\
-	ret = len;							\
-									\
-end:									\
-	mutex_unlock(&opts->lock);					\
-	return ret;							\
-}									\
-									\
-CONFIGFS_ATTR(f_uac2_opts_, name)
-
 UAC2_ATTRIBUTE(p_chmask);
 UAC2_ATTRIBUTE(p_srate);
 UAC2_ATTRIBUTE(p_ssize);
 UAC2_ATTRIBUTE(c_chmask);
 UAC2_ATTRIBUTE(c_srate);
-UAC2_ATTRIBUTE_SYNC(c_sync);
 UAC2_ATTRIBUTE(c_ssize);
 UAC2_ATTRIBUTE(req_number);
 
@@ -1337,7 +1257,6 @@ static struct configfs_attribute *f_uac2_attrs[] = {
 	&f_uac2_opts_attr_c_chmask,
 	&f_uac2_opts_attr_c_srate,
 	&f_uac2_opts_attr_c_ssize,
-	&f_uac2_opts_attr_c_sync,
 	&f_uac2_opts_attr_req_number,
 	NULL,
 };
@@ -1376,7 +1295,6 @@ static struct usb_function_instance *afunc_alloc_inst(void)
 	opts->c_chmask = UAC2_DEF_CCHMASK;
 	opts->c_srate = UAC2_DEF_CSRATE;
 	opts->c_ssize = UAC2_DEF_CSSIZE;
-	opts->c_sync = UAC2_DEF_CSYNC;
 	opts->req_number = UAC2_DEF_REQ_NUM;
 	return &opts->func_inst;
 }
diff --git a/drivers/usb/gadget/function/u_uac2.h b/drivers/usb/gadget/function/u_uac2.h
index 13589c3c805c..b5035711172d 100644
--- a/drivers/usb/gadget/function/u_uac2.h
+++ b/drivers/usb/gadget/function/u_uac2.h
@@ -21,7 +21,6 @@
 #define UAC2_DEF_CCHMASK 0x3
 #define UAC2_DEF_CSRATE 64000
 #define UAC2_DEF_CSSIZE 2
-#define UAC2_DEF_CSYNC		USB_ENDPOINT_SYNC_ASYNC
 #define UAC2_DEF_REQ_NUM 2
 
 struct f_uac2_opts {
@@ -32,7 +31,6 @@ struct f_uac2_opts {
 	int				c_chmask;
 	int				c_srate;
 	int				c_ssize;
-	int				c_sync;
 	int				req_number;
 	bool				bound;
 
-- 
2.30.2

[PATCH v2 3/3] Revert "usb: gadget: f_uac2/u_audio: add feedback endpoint support"

[Ferry Toth]

This reverts commit 24f779dac8f3efb9629adc0e486914d93dc45517.

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.

Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
---
 drivers/usb/gadget/function/f_uac2.c  |  49 +----------
 drivers/usb/gadget/function/u_audio.c | 119 +-------------------------
 drivers/usb/gadget/function/u_audio.h |   3 -
 3 files changed, 3 insertions(+), 168 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
index 5d63244ba319..7aa4c8bc5a1a 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -240,7 +240,7 @@ static struct usb_interface_descriptor std_as_out_if1_desc = {
 	.bDescriptorType = USB_DT_INTERFACE,
 
 	.bAlternateSetting = 1,
-	.bNumEndpoints = 2,
+	.bNumEndpoints = 1,
 	.bInterfaceClass = USB_CLASS_AUDIO,
 	.bInterfaceSubClass = USB_SUBCLASS_AUDIOSTREAMING,
 	.bInterfaceProtocol = UAC_VERSION_2,
@@ -317,37 +317,6 @@ static struct uac2_iso_endpoint_descriptor as_iso_out_desc = {
 	.wLockDelay = 0,
 };
 
-/* STD AS ISO IN Feedback Endpoint */
-static struct usb_endpoint_descriptor fs_epin_fback_desc = {
-	.bLength = USB_DT_ENDPOINT_SIZE,
-	.bDescriptorType = USB_DT_ENDPOINT,
-
-	.bEndpointAddress = USB_DIR_IN,
-	.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_USAGE_FEEDBACK,
-	.wMaxPacketSize = cpu_to_le16(3),
-	.bInterval = 1,
-};
-
-static struct usb_endpoint_descriptor hs_epin_fback_desc = {
-	.bLength = USB_DT_ENDPOINT_SIZE,
-	.bDescriptorType = USB_DT_ENDPOINT,
-
-	.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_USAGE_FEEDBACK,
-	.wMaxPacketSize = cpu_to_le16(4),
-	.bInterval = 4,
-};
-
-static struct usb_endpoint_descriptor ss_epin_fback_desc = {
-	.bLength = USB_DT_ENDPOINT_SIZE,
-	.bDescriptorType = USB_DT_ENDPOINT,
-
-	.bEndpointAddress = USB_DIR_IN,
-	.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_USAGE_FEEDBACK,
-	.wMaxPacketSize = cpu_to_le16(4),
-	.bInterval = 4,
-};
-
-
 /* Audio Streaming IN Interface - Alt0 */
 static struct usb_interface_descriptor std_as_in_if0_desc = {
 	.bLength = sizeof std_as_in_if0_desc,
@@ -462,7 +431,6 @@ static struct usb_descriptor_header *fs_audio_desc[] = {
 	(struct usb_descriptor_header *)&as_out_fmt1_desc,
 	(struct usb_descriptor_header *)&fs_epout_desc,
 	(struct usb_descriptor_header *)&as_iso_out_desc,
-	(struct usb_descriptor_header *)&fs_epin_fback_desc,
 
 	(struct usb_descriptor_header *)&std_as_in_if0_desc,
 	(struct usb_descriptor_header *)&std_as_in_if1_desc,
@@ -493,7 +461,6 @@ static struct usb_descriptor_header *hs_audio_desc[] = {
 	(struct usb_descriptor_header *)&as_out_fmt1_desc,
 	(struct usb_descriptor_header *)&hs_epout_desc,
 	(struct usb_descriptor_header *)&as_iso_out_desc,
-	(struct usb_descriptor_header *)&hs_epin_fback_desc,
 
 	(struct usb_descriptor_header *)&std_as_in_if0_desc,
 	(struct usb_descriptor_header *)&std_as_in_if1_desc,
@@ -525,7 +492,6 @@ static struct usb_descriptor_header *ss_audio_desc[] = {
 	(struct usb_descriptor_header *)&ss_epout_desc,
 	(struct usb_descriptor_header *)&ss_epout_desc_comp,
 	(struct usb_descriptor_header *)&as_iso_out_desc,
-	(struct usb_descriptor_header *)&ss_epin_fback_desc,
 
 	(struct usb_descriptor_header *)&std_as_in_if0_desc,
 	(struct usb_descriptor_header *)&std_as_in_if1_desc,
@@ -602,26 +568,22 @@ static void setup_headers(struct f_uac2_opts *opts,
 	struct usb_ss_ep_comp_descriptor *epin_desc_comp = NULL;
 	struct usb_endpoint_descriptor *epout_desc;
 	struct usb_endpoint_descriptor *epin_desc;
-	struct usb_endpoint_descriptor *epin_fback_desc;
 	int i;
 
 	switch (speed) {
 	case USB_SPEED_FULL:
 		epout_desc = &fs_epout_desc;
 		epin_desc = &fs_epin_desc;
-		epin_fback_desc = &fs_epin_fback_desc;
 		break;
 	case USB_SPEED_HIGH:
 		epout_desc = &hs_epout_desc;
 		epin_desc = &hs_epin_desc;
-		epin_fback_desc = &hs_epin_fback_desc;
 		break;
 	default:
 		epout_desc = &ss_epout_desc;
 		epin_desc = &ss_epin_desc;
 		epout_desc_comp = &ss_epout_desc_comp;
 		epin_desc_comp = &ss_epin_desc_comp;
-		epin_fback_desc = &ss_epin_fback_desc;
 	}
 
 	i = 0;
@@ -649,7 +611,6 @@ static void setup_headers(struct f_uac2_opts *opts,
 			headers[i++] = USBDHDR(epout_desc_comp);
 
 		headers[i++] = USBDHDR(&as_iso_out_desc);
-		headers[i++] = USBDHDR(epin_fback_desc);
 	}
 	if (EPIN_EN(opts)) {
 		headers[i++] = USBDHDR(&std_as_in_if0_desc);
@@ -883,12 +844,6 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
 			dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
 			return -ENODEV;
 		}
-		agdev->in_ep_fback = usb_ep_autoconfig(gadget,
-						       &fs_epin_fback_desc);
-		if (!agdev->in_ep_fback) {
-			dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
-			return -ENODEV;
-		}
 	}
 
 	if (EPIN_EN(uac2_opts)) {
@@ -912,10 +867,8 @@ afunc_bind(struct usb_configuration *cfg, struct usb_function *fn)
 				le16_to_cpu(ss_epout_desc.wMaxPacketSize));
 
 	hs_epout_desc.bEndpointAddress = fs_epout_desc.bEndpointAddress;
-	hs_epin_fback_desc.bEndpointAddress = fs_epin_fback_desc.bEndpointAddress;
 	hs_epin_desc.bEndpointAddress = fs_epin_desc.bEndpointAddress;
 	ss_epout_desc.bEndpointAddress = fs_epout_desc.bEndpointAddress;
-	ss_epin_fback_desc.bEndpointAddress = fs_epin_fback_desc.bEndpointAddress;
 	ss_epin_desc.bEndpointAddress = fs_epin_desc.bEndpointAddress;
 
 	setup_descriptor(uac2_opts);
diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index f637ebec80b0..5fbceee897a3 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -38,10 +38,6 @@ struct uac_rtd_params {
 	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 */
 };
 
 struct snd_uac_chip {
@@ -74,32 +70,6 @@ static const struct snd_pcm_hardware uac_pcm_hardware = {
 	.periods_min = MIN_PERIODS,
 };
 
-static void u_audio_set_fback_frequency(enum usb_device_speed speed,
-					unsigned int freq, void *buf)
-{
-	u32 ff = 0;
-
-	if (speed == USB_SPEED_FULL) {
-		/*
-		 * Full-speed feedback endpoints report frequency
-		 * in samples/microframe
-		 * Format is encoded in Q10.10 left-justified in the 24 bits,
-		 * so that it has a Q10.14 format.
-		 */
-		ff = DIV_ROUND_UP((freq << 14), 1000);
-	} else {
-		/*
-		 * High-speed feedback endpoints report frequency
-		 * in samples/microframe.
-		 * 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 = DIV_ROUND_UP((freq << 13), 1000);
-	}
-	*(__le32 *)buf = cpu_to_le32(ff);
-}
-
 static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req)
 {
 	unsigned int pending;
@@ -203,34 +173,6 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req)
 		dev_err(uac->card->dev, "%d Error!\n", __LINE__);
 }
 
-static void u_audio_iso_fback_complete(struct usb_ep *ep,
-				       struct usb_request *req)
-{
-	struct uac_rtd_params *prm = req->context;
-	struct snd_uac_chip *uac = prm->uac;
-	struct g_audio *audio_dev = uac->audio_dev;
-	int status = req->status;
-	unsigned long flags;
-
-	/* i/f shutting down */
-	if (!prm->fb_ep_enabled || req->status == -ESHUTDOWN)
-		return;
-
-	/*
-	 * We can't really do much about bad xfers.
-	 * Afterall, the ISOCH xfers could fail legitimately.
-	 */
-	if (status)
-		pr_debug("%s: iso_complete status(%d) %d/%d\n",
-			__func__, status, req->actual, req->length);
-
-	u_audio_set_fback_frequency(audio_dev->gadget->speed,
-				    prm->ffback, req->buf);
-
-	if (usb_ep_queue(ep, req, GFP_ATOMIC))
-		dev_err(uac->card->dev, "%d Error!\n", __LINE__);
-}
-
 static int uac_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 {
 	struct snd_uac_chip *uac = snd_pcm_substream_chip(substream);
@@ -393,33 +335,14 @@ static inline void free_ep(struct uac_rtd_params *prm, struct usb_ep *ep)
 		dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__);
 }
 
-static inline void free_ep_fback(struct uac_rtd_params *prm, struct usb_ep *ep)
-{
-	struct snd_uac_chip *uac = prm->uac;
-
-	if (!prm->fb_ep_enabled)
-		return;
-
-	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);
-		prm->req_fback = NULL;
-	}
-
-	if (usb_ep_disable(ep))
-		dev_err(uac->card->dev, "%s:%d Error!\n", __func__, __LINE__);
-}
 
 int u_audio_start_capture(struct g_audio *audio_dev)
 {
 	struct snd_uac_chip *uac = audio_dev->uac;
 	struct usb_gadget *gadget = audio_dev->gadget;
 	struct device *dev = &gadget->dev;
-	struct usb_request *req, *req_fback;
-	struct usb_ep *ep, *ep_fback;
+	struct usb_request *req;
+	struct usb_ep *ep;
 	struct uac_rtd_params *prm;
 	struct uac_params *params = &audio_dev->params;
 	int req_len, i;
@@ -451,42 +374,6 @@ int u_audio_start_capture(struct g_audio *audio_dev)
 			dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
 	}
 
-	ep_fback = audio_dev->in_ep_fback;
-	if (!ep_fback)
-		return 0;
-
-	/* Setup feedback endpoint */
-	config_ep_by_speed(gadget, &audio_dev->func, ep_fback);
-	prm->fb_ep_enabled = true;
-	usb_ep_enable(ep_fback);
-	req_len = ep_fback->maxpacket;
-
-	req_fback = usb_ep_alloc_request(ep_fback, GFP_ATOMIC);
-	if (req_fback == NULL)
-		return -ENOMEM;
-
-	prm->req_fback = req_fback;
-	req_fback->zero = 0;
-	req_fback->context = prm;
-	req_fback->length = req_len;
-	req_fback->complete = u_audio_iso_fback_complete;
-
-	req_fback->buf = kzalloc(req_len, GFP_ATOMIC);
-	if (!req_fback->buf)
-		return -ENOMEM;
-
-	/*
-	 * Configure the feedback endpoint's reported frequency.
-	 * Always start with original frequency since its deviation can't
-	 * be meauserd at start of playback
-	 */
-	prm->ffback = params->c_srate;
-	u_audio_set_fback_frequency(audio_dev->gadget->speed,
-				    prm->ffback, req_fback->buf);
-
-	if (usb_ep_queue(ep_fback, req_fback, GFP_ATOMIC))
-		dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(u_audio_start_capture);
@@ -495,8 +382,6 @@ void u_audio_stop_capture(struct g_audio *audio_dev)
 {
 	struct snd_uac_chip *uac = audio_dev->uac;
 
-	if (audio_dev->in_ep_fback)
-		free_ep_fback(&uac->c_prm, audio_dev->in_ep_fback);
 	free_ep(&uac->c_prm, audio_dev->out_ep);
 }
 EXPORT_SYMBOL_GPL(u_audio_stop_capture);
diff --git a/drivers/usb/gadget/function/u_audio.h b/drivers/usb/gadget/function/u_audio.h
index 53e6baf55cbf..5ea6b86f1fda 100644
--- a/drivers/usb/gadget/function/u_audio.h
+++ b/drivers/usb/gadget/function/u_audio.h
@@ -30,10 +30,7 @@ struct g_audio {
 	struct usb_gadget *gadget;
 
 	struct usb_ep *in_ep;
-
 	struct usb_ep *out_ep;
-	/* feedback IN endpoint corresponding to out_ep */
-	struct usb_ep *in_ep_fback;
 
 	/* Max packet size for all in_ep possible speeds */
 	unsigned int in_ep_maxpsize;
-- 
2.30.2

Re: [PATCH v2 0/3] Revert "usb: gadget: u_audio: add real feedback implementation"

[Jerome Brunet]

On Thu 26 Aug 2021 at 20:57, Ferry Toth <ftoth@exalondelft.nl> wrote:

> Although there is a patch resolving this issue (see 
> https://lore.kernel.org/linux-usb/1jilzsy8g7.fsf@starbuckisacylon.baylibre.com/T/#u)
> it needs a little work and will not be ready for v5.14.0 release. Until then
> revert the series.

Seems like a quite messy solution when the fix available :/
Change with the updated commit description is avaialable BTW [0]

[0]: https://lore.kernel.org/20210827075853.266912-1-jbrunet@baylibre.com

>
> v2:
> - Added SoB (Balbi)
>
>
> Ferry Toth (3):
>   Revert "usb: gadget: u_audio: add real feedback implementation"
>   Revert "usb: gadget: f_uac2: add adaptive sync support for capture"
>   Revert "usb: gadget: f_uac2/u_audio: add feedback endpoint support"
>
>  .../ABI/testing/configfs-usb-gadget-uac2      |   2 -
>  Documentation/usb/gadget-testing.rst          |   2 -
>  drivers/usb/gadget/function/f_uac2.c          | 144 +----------
>  drivers/usb/gadget/function/u_audio.c         | 225 +-----------------
>  drivers/usb/gadget/function/u_audio.h         |  12 -
>  drivers/usb/gadget/function/u_uac2.h          |   4 -
>  6 files changed, 6 insertions(+), 383 deletions(-)

Re: [PATCH v2 0/3] Revert "usb: gadget: u_audio: add real feedback implementation"

[Greg Kroah-Hartman]

On Thu, Aug 26, 2021 at 08:57:36PM +0200, Ferry Toth wrote:
> Although there is a patch resolving this issue (see 
> https://lore.kernel.org/linux-usb/1jilzsy8g7.fsf@starbuckisacylon.baylibre.com/T/#u)
> it needs a little work and will not be ready for v5.14.0 release. Until then
> revert the series.

revert the series for what?  5.14-final needs these reverts?  Or are
these for 5.15-rc1?

confused,

greg k-h

Re: [PATCH v2 1/3] Revert "usb: gadget: u_audio: add real feedback implementation"

[Greg Kroah-Hartman]

On Thu, Aug 26, 2021 at 08:57:37PM +0200, Ferry Toth wrote:
> 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.

Are these normal configuration options in the wild, or is this just
something that you "can do"?

> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>

I need an ack from the original authors to revert all this...

thanks,

greg k-h

Re: [PATCH v2 0/3] Revert "usb: gadget: u_audio: add real feedback implementation"

[Andy Shevchenko]

On Fri, Aug 27, 2021 at 11:05 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Thu 26 Aug 2021 at 20:57, Ferry Toth <ftoth@exalondelft.nl> wrote:
>
> > Although there is a patch resolving this issue (see
> > https://lore.kernel.org/linux-usb/1jilzsy8g7.fsf@starbuckisacylon.baylibre.com/T/#u)
> > it needs a little work and will not be ready for v5.14.0 release. Until then
> > revert the series.
>
> Seems like a quite messy solution when the fix available :/

Is it? AFAIR Thing pointed out to some problems with the proposed
solution. Have those been addressed?

> Change with the updated commit description is avaialable BTW [0]

> [0]: https://lore.kernel.org/20210827075853.266912-1-jbrunet@baylibre.com

Dunno, but this gives me 404.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] Revert "usb: gadget: u_audio: add real feedback implementation"

[Ferry Toth]

Op 27-08-2021 om 10:29 schreef Greg Kroah-Hartman:
> On Thu, Aug 26, 2021 at 08:57:37PM +0200, Ferry Toth wrote:
>> 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.
> 
> Are these normal configuration options in the wild, or is this just
> something that you "can do"?

It's a work around suggested by Pavel, a line that can be added to 
configfs script.

>> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl>
> 
> I need an ack from the original authors to revert all this...

A fix has appeared:
https://lore.kernel.org/linux-usb/1jfsuvw817.fsf@starbuckisacylon.baylibre.com/T/#m922149b7e74204c8ed1bed2c624910ab4eafd43c

This has been acked by Felipe. If we can get that in 5.14.0 it would be 
preferable to reverting this series.

> thanks,
> 
> greg k-h
> 

Re: [PATCH v2 0/3] Revert "usb: gadget: u_audio: add real feedback implementation"

[Ferry Toth]

Op 27-08-2021 om 11:15 schreef Andy Shevchenko:
> On Fri, Aug 27, 2021 at 11:05 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>> On Thu 26 Aug 2021 at 20:57, Ferry Toth <ftoth@exalondelft.nl> wrote:
>>
>>> Although there is a patch resolving this issue (see
>>> https://lore.kernel.org/linux-usb/1jilzsy8g7.fsf@starbuckisacylon.baylibre.com/T/#u)
>>> it needs a little work and will not be ready for v5.14.0 release. Until then
>>> revert the series.
>>
>> Seems like a quite messy solution when the fix available :/
> 
> Is it? AFAIR Thing pointed out to some problems with the proposed
> solution. Have those been addressed?
> 
>> Change with the updated commit description is avaialable BTW [0]
> 
>> [0]: https://lore.kernel.org/20210827075853.266912-1-jbrunet@baylibre.com
> 
> Dunno, but this gives me 404.
> 
I think:
https://lore.kernel.org/linux-usb/1jfsuvw817.fsf@starbuckisacylon.baylibre.com/T/#m922149b7e74204c8ed1bed2c624910ab4eafd43c

Re: [PATCH v2 0/3] Revert "usb: gadget: u_audio: add real feedback implementation"

[Ferry Toth]

Hi
Op 27-08-2021 om 10:27 schreef Greg Kroah-Hartman:
> On Thu, Aug 26, 2021 at 08:57:36PM +0200, Ferry Toth wrote:
>> Although there is a patch resolving this issue (see
>> https://lore.kernel.org/linux-usb/1jilzsy8g7.fsf@starbuckisacylon.baylibre.com/T/#u)
>> it needs a little work and will not be ready for v5.14.0 release. Until then
>> revert the series.
> 
> revert the series for what?  5.14-final needs these reverts?  Or are
> these for 5.15-rc1?
> 
> confused,

Apologies. Yes, the idea was to revert for 5.14-final as the series 
creates a regression.

However, an updated patch "usb: gadget: f_uac2: fixup feedback endpoint 
stop"
https://lore.kernel.org/linux-usb/1jfsuvw817.fsf@starbuckisacylon.baylibre.com/T/#m922149b7e74204c8ed1bed2c624910ab4eafd43c
now has been acked by Felipe.

If we can get that it for 5.14-final it would be less messy.

Can we?

> greg k-h
>