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 <[email protected]>
---
.../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
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.
Reported-by: Ferry Toth <[email protected]>
---
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
Ferry Toth <[email protected]> writes:
> 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 <[email protected]>
this should be Signed-off-by ;-)
--
balbi
Ferry Toth <[email protected]> writes:
> 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.
>
> Reported-by: Ferry Toth <[email protected]>
Signed-off-by?
--
balbi
Hi,
Op 25-08-2021 om 07:53 schreef Felipe Balbi:
>
> Ferry Toth <[email protected]> writes:
>
>> 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 <[email protected]>
>
> this should be Signed-off-by ;-)
>
Indeed. It probably was until I re-worded the commit text.
Will resend tonight v2.
When the uac2 function is stopped, there seems to be an issue with 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 likely coming from the feedback
endpoint, which is only used with async synchronization mode.
Update the feedback endpoint free function to release the endpoint the same
way it is done for the data endpoint.
Signed-off-by: Jerome Brunet <[email protected]>
---
Hi Ferry,
Would you mind trying this before reverting the whole thing ?
The HW I have did not show the issue so far so I can't really check
if it helps. Hopefully, it does ...
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
Hi,
Op 25-08-2021 om 11:21 schreef Jerome Brunet:
> When the uac2 function is stopped, there seems to be an issue with 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 likely coming from the feedback
> endpoint, which is only used with async synchronization mode.
>
> Update the feedback endpoint free function to release the endpoint the same
> way it is done for the data endpoint.
>
> Signed-off-by: Jerome Brunet <[email protected]>
> ---
>
> Hi Ferry,
>
> Would you mind trying this before reverting the whole thing ?
> The HW I have did not show the issue so far so I can't really check
> if it helps. Hopefully, it does ...
Tested this evening and confirming that this resolves my issue. I can't
say much about the code itself, maybe Thinh?
Would be great if we could get this in instead of reverting the series.
Tested-by: Ferry Toth <[email protected]> (dwc3 / Intel Merrifield)
> 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;
> }
>
Ferry Toth wrote:
> Hi,
>
> Op 25-08-2021 om 11:21 schreef Jerome Brunet:
>> When the uac2 function is stopped, there seems to be an issue with some
>> platforms (Intel Merrifield at least)
>>
The issue isn't hardware specific.
>> 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 likely coming from the
>> feedback
>> endpoint, which is only used with async synchronization mode.
This does not describe the actual problem. The problem is that the
usb_ep_dequeue() can be an asynchronous call, and we can't free the
request until its completion (from cancellation).
>>
>> Update the feedback endpoint free function to release the endpoint the
>> same
>> way it is done for the data endpoint.
>>
>> Signed-off-by: Jerome Brunet <[email protected]>
>> ---
>>
>> Hi Ferry,
>>
>> Would you mind trying this before reverting the whole thing ?
>> The HW I have did not show the issue so far so I can't really check
>> if it helps. Hopefully, it does ...
>
> Tested this evening and confirming that this resolves my issue. I can't
> say much about the code itself, maybe Thinh?
Sure. I can take a look.
>
> Would be great if we could get this in instead of reverting the series.
>
> Tested-by: Ferry Toth <[email protected]> (dwc3 / Intel Merrifield)
>
>> 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) {
prm->fb_ep_enabled is not protected. Potential race problem here?
>> + 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;
>> }
>>
On a separate note, I notice that f_uac2 only queues a single feedback
request at a time for isoc endpoint? Even though the interval is 1ms,
this will easily cause data drop.
Also, you're ignoring other request error status and still processing
bogus data on request completion? That doesn't seem right.
BR,
Thinh
On Wed 25 Aug 2021 at 22:07, Ferry Toth <[email protected]> wrote:
> Hi,
>
> Op 25-08-2021 om 11:21 schreef Jerome Brunet:
>> When the uac2 function is stopped, there seems to be an issue with 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 likely coming from the feedback
>> endpoint, which is only used with async synchronization mode.
>>
>> Update the feedback endpoint free function to release the endpoint the same
>> way it is done for the data endpoint.
>>
>> Signed-off-by: Jerome Brunet <[email protected]>
>> ---
>>
>> Hi Ferry,
>>
>> Would you mind trying this before reverting the whole thing ?
>> The HW I have did not show the issue so far so I can't really check
>> if it helps. Hopefully, it does ...
>
> Tested this evening and confirming that this resolves my issue. I can't say
> much about the code itself, maybe Thinh?
>
> Would be great if we could get this in instead of reverting the series.
>
> Tested-by: Ferry Toth <[email protected]> (dwc3 / Intel Merrifield)
>
Thanks for reporting and testing this.
I guess we should also add
Reported-by: Ferry Toth <[email protected]>
>> 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;
>> }
>>
On Wed 25 Aug 2021 at 21:42, Thinh Nguyen <[email protected]> wrote:
> Ferry Toth wrote:
>> Hi,
>>
>> Op 25-08-2021 om 11:21 schreef Jerome Brunet:
>>> When the uac2 function is stopped, there seems to be an issue with some
>>> platforms (Intel Merrifield at least)
>>>
>
> The issue isn't hardware specific.
While the actual bug isnt, the report was (given the issue did not show
up during initial testing but did show on Ferry's HW)
Merely citing the bug report from Ferry here
>
>>> 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 likely coming from the
>>> feedback
>>> endpoint, which is only used with async synchronization mode.
>
> This does not describe the actual problem. The problem is that the
> usb_ep_dequeue() can be an asynchronous call, and we can't free the
> request until its completion (from cancellation).
Indeed. I was not sure at the time.
>
>>>
>>> Update the feedback endpoint free function to release the endpoint the
>>> same
>>> way it is done for the data endpoint.
>>>
>>> Signed-off-by: Jerome Brunet <[email protected]>
>>> ---
>>>
>>> Hi Ferry,
>>>
>>> Would you mind trying this before reverting the whole thing ?
>>> The HW I have did not show the issue so far so I can't really check
>>> if it helps. Hopefully, it does ...
>>
>> Tested this evening and confirming that this resolves my issue. I can't
>> say much about the code itself, maybe Thinh?
>
> Sure. I can take a look.
>
>>
>> Would be great if we could get this in instead of reverting the series.
>>
>> Tested-by: Ferry Toth <[email protected]> (dwc3 / Intel Merrifield)
>>
>>> 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) {
>
> prm->fb_ep_enabled is not protected. Potential race problem here?
Given how the variable is used, I don't think so.
Could you please detail ?
(I don't think this is really related to the current problem though)
>
>>> + 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;
>>> }
>>>
>
> On a separate note, I notice that f_uac2 only queues a single feedback
> request at a time for isoc endpoint? Even though the interval is 1ms,
> this will easily cause data drop.
>
> Also, you're ignoring other request error status and still processing
> bogus data on request completion? That doesn't seem right.
Gadget is sendind the feedback data, not processing it. Every data sent
is OK. Yes, packets can be missed with the current implementation,
meaning the feedback value is not reported as often as initially
intended. On slower HW, packets are also missed with 2 requests queued,
not only on the feedback endpoint but also on the playback
endpoint. Picking the approriate value is not straight forward. For the
feedback endpoint it isn't big deal because, according to the spec, if
the feedback is not sent, the host shall assume the value hasn't
changed. This why the whole thing works as it is.
I admit things still aren't perfect, but there is progress ...
>
> BR,
> Thinh
Jerome Brunet wrote:
>
> On Wed 25 Aug 2021 at 21:42, Thinh Nguyen <[email protected]> wrote:
>
>> Ferry Toth wrote:
>>> Hi,
>>>
>>> Op 25-08-2021 om 11:21 schreef Jerome Brunet:
>>>> When the uac2 function is stopped, there seems to be an issue with some
>>>> platforms (Intel Merrifield at least)
>>>>
>>
>> The issue isn't hardware specific.
>
> While the actual bug isnt, the report was (given the issue did not show
> up during initial testing but did show on Ferry's HW)
> Merely citing the bug report from Ferry here
Ok
>
>>
>>>> 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 likely coming from the
>>>> feedback
>>>> endpoint, which is only used with async synchronization mode.
>>
>> This does not describe the actual problem. The problem is that the
>> usb_ep_dequeue() can be an asynchronous call, and we can't free the
>> request until its completion (from cancellation).
>
> Indeed. I was not sure at the time.
>
>>
>>>>
>>>> Update the feedback endpoint free function to release the endpoint the
>>>> same
>>>> way it is done for the data endpoint.
>>>>
>>>> Signed-off-by: Jerome Brunet <[email protected]>
>>>> ---
>>>>
>>>> Hi Ferry,
>>>>
>>>> Would you mind trying this before reverting the whole thing ?
>>>> The HW I have did not show the issue so far so I can't really check
>>>> if it helps. Hopefully, it does ...
>>>
>>> Tested this evening and confirming that this resolves my issue. I can't
>>> say much about the code itself, maybe Thinh?
>>
>> Sure. I can take a look.
>>
>>>
>>> Would be great if we could get this in instead of reverting the series.
>>>
>>> Tested-by: Ferry Toth <[email protected]> (dwc3 / Intel Merrifield)
>>>
>>>> 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) {
>>
>> prm->fb_ep_enabled is not protected. Potential race problem here?
>
> Given how the variable is used, I don't think so.
> Could you please detail ?
>
I'm thinking of this scenario:
Since free_ep_fback() is called in a separate thread,
u_audio_iso_fback_complete() can come in the middle after
prm->fb_ep_enabled is cleared but before the usb_ep_dequeue(). So the
request may be freed before being accessed again in usb_ep_dequeue().
> (I don't think this is really related to the current problem though)
It's related to how we go about to solve the problem right?
Can we just check for the req->status for -ECONNRESET and -ESHUTDOWN and
free the request? Those 2 statuses should be for when the request is
cancelled and when the endpoint is disabled.
>
>>
>>>> + 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;
>>>> }
>>>>
>>
>> On a separate note, I notice that f_uac2 only queues a single feedback
>> request at a time for isoc endpoint? Even though the interval is 1ms,
>> this will easily cause data drop.
>>
>> Also, you're ignoring other request error status and still processing
>> bogus data on request completion? That doesn't seem right.
>
> Gadget is sendind the feedback data, not processing it. Every data sent
Ah ok.. I missed that it's IN request.
> is OK. Yes, packets can be missed with the current implementation,
> meaning the feedback value is not reported as often as initially
> intended. On slower HW, packets are also missed with 2 requests queued,
2 is still too low. From our HW testing, minimum should be 4 to minimize
data loss at 1ms interval.
> not only on the feedback endpoint but also on the playback
> endpoint. Picking the approriate value is not straight forward. For the
> feedback endpoint it isn't big deal because, according to the spec, if
> the feedback is not sent, the host shall assume the value hasn't
> changed. This why the whole thing works as it is.
Why do this when we can avoid it. We know that there will be data drop
with the way it's implemented now. There's no option to adjust the
number of feedback requests either.
>
> I admit things still aren't perfect, but there is progress ...
>
Thanks for the updates to audio/f_uac2. It's been a long time since
anyone working on it.
BR,
Thinh
On Fri 27 Aug 2021 at 00:50, Thinh Nguyen <[email protected]> wrote:
>>>>> 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) {
>>>
>>> prm->fb_ep_enabled is not protected. Potential race problem here?
>>
>> Given how the variable is used, I don't think so.
>> Could you please detail ?
>>
>
> I'm thinking of this scenario:
>
> Since free_ep_fback() is called in a separate thread,
> u_audio_iso_fback_complete() can come in the middle after
> prm->fb_ep_enabled is cleared but before the usb_ep_dequeue(). So the
> request may be freed before being accessed again in usb_ep_dequeue().
>
You are right - there is short window of opportunity.
Looks like a 2nd separate issue affecting all u_audio endpoints. (Would
be solved by reverting the feedback series)
Fix seems fairly simple.
>> (I don't think this is really related to the current problem though)
>
> It's related to how we go about to solve the problem right?
>
> Can we just check for the req->status for -ECONNRESET and -ESHUTDOWN and
> free the request? Those 2 statuses should be for when the request is
> cancelled and when the endpoint is disabled.
>
The same problem happened on the data endpoint while feedback was being
reviewed. This is why data got fixed, and the feedback endpoint did not.
As explained in the description, the fix is merely aligning what is
done for the different endpoints here.
Some of the people who have worked on this are no USB expert (myself
included) so I'm sure things can be improved. I'd suggest to take the
rework u_audio as a 2nd step.
>>
>>>
>>>>> + 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;
>>>>> }
>>>>>
>>>
>>> On a separate note, I notice that f_uac2 only queues a single feedback
>>> request at a time for isoc endpoint? Even though the interval is 1ms,
>>> this will easily cause data drop.
>>>
>>> Also, you're ignoring other request error status and still processing
>>> bogus data on request completion? That doesn't seem right.
>>
>> Gadget is sendind the feedback data, not processing it. Every data sent
>
> Ah ok.. I missed that it's IN request.
>
>> is OK. Yes, packets can be missed with the current implementation,
>> meaning the feedback value is not reported as often as initially
>> intended. On slower HW, packets are also missed with 2 requests queued,
>
> 2 is still too low. From our HW testing, minimum should be 4 to minimize
> data loss at 1ms interval.
Maybe 4 should be the default then ?
>
>> not only on the feedback endpoint but also on the playback
>> endpoint. Picking the approriate value is not straight forward. For the
>> feedback endpoint it isn't big deal because, according to the spec, if
>> the feedback is not sent, the host shall assume the value hasn't
>> changed. This why the whole thing works as it is.
>
> Why do this when we can avoid it.
Because it was not known at the time
> We know that there will be data drop
> with the way it's implemented now. There's no option to adjust the
> number of feedback requests either.
>
>>
>> I admit things still aren't perfect, but there is progress ...
>>
> Thanks for the updates to audio/f_uac2. It's been a long time since
> anyone working on it.
>
> BR,
> Thinh
Jerome Brunet wrote:
>
> On Fri 27 Aug 2021 at 00:50, Thinh Nguyen <[email protected]> wrote:
>
>>>>>> 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) {
>>>>
>>>> prm->fb_ep_enabled is not protected. Potential race problem here?
>>>
>>> Given how the variable is used, I don't think so.
>>> Could you please detail ?
>>>
>>
>> I'm thinking of this scenario:
>>
>> Since free_ep_fback() is called in a separate thread,
>> u_audio_iso_fback_complete() can come in the middle after
>> prm->fb_ep_enabled is cleared but before the usb_ep_dequeue(). So the
>> request may be freed before being accessed again in usb_ep_dequeue().
>>
>
> You are right - there is short window of opportunity.
> Looks like a 2nd separate issue affecting all u_audio endpoints. (Would
> be solved by reverting the feedback series)
>
> Fix seems fairly simple.
>
>>> (I don't think this is really related to the current problem though)
>>
>> It's related to how we go about to solve the problem right?
>>
>> Can we just check for the req->status for -ECONNRESET and -ESHUTDOWN and
>> free the request? Those 2 statuses should be for when the request is
>> cancelled and when the endpoint is disabled.
>>
>
> The same problem happened on the data endpoint while feedback was being
> reviewed. This is why data got fixed, and the feedback endpoint did not.
>
> As explained in the description, the fix is merely aligning what is
> done for the different endpoints here.
IMHO, it'd be better if we can have a proper fix to the newly introduced
feedback endpoint so that doesn't leave problems to be patched up later.
This makes it easier to backport fix patches that don't touch too many
places.
>
> Some of the people who have worked on this are no USB expert (myself
> included) so I'm sure things can be improved. I'd suggest to take the
> rework u_audio as a 2nd step.
Sure. I just wanted to bring them up as I notice them while looking
through the reported patch.
>
>>>
>>>>
>>>>>> + 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;
>>>>>> }
>>>>>>
>>>>
>>>> On a separate note, I notice that f_uac2 only queues a single feedback
>>>> request at a time for isoc endpoint? Even though the interval is 1ms,
>>>> this will easily cause data drop.
>>>>
>>>> Also, you're ignoring other request error status and still processing
>>>> bogus data on request completion? That doesn't seem right.
>>>
>>> Gadget is sendind the feedback data, not processing it. Every data sent
>>
>> Ah ok.. I missed that it's IN request.
>>
>>> is OK. Yes, packets can be missed with the current implementation,
>>> meaning the feedback value is not reported as often as initially
>>> intended. On slower HW, packets are also missed with 2 requests queued,
>>
>> 2 is still too low. From our HW testing, minimum should be 4 to minimize
>> data loss at 1ms interval.
>
> Maybe 4 should be the default then ?
Yes. Would it be a problem?
>
>>
>>> not only on the feedback endpoint but also on the playback
>>> endpoint. Picking the approriate value is not straight forward. For the
>>> feedback endpoint it isn't big deal because, according to the spec, if
>>> the feedback is not sent, the host shall assume the value hasn't
>>> changed. This why the whole thing works as it is.
>>
>> Why do this when we can avoid it.
>
> Because it was not known at the time
>
>> We know that there will be data drop
>> with the way it's implemented now. There's no option to adjust the
>> number of feedback requests either.
>>
>>>
>>> I admit things still aren't perfect, but there is progress ...
>>>
>> Thanks for the updates to audio/f_uac2. It's been a long time since
>> anyone working on it.
>>
Thanks,
Thinh