On Fri, Mar 12, 2021 at 2:01 AM Wesley Cheng <wch...@codeaurora.org> wrote: > > In the situations where the DWC3 gadget stops active transfers, once > calling the dwc3_gadget_giveback(), there is a chance where a function > driver can queue a new USB request in between the time where the dwc3 > lock has been released and re-aquired. This occurs after we've already > issued an ENDXFER command. When the stop active transfers continues > to remove USB requests from all dep lists, the newly added request will > also be removed, while controller still has an active TRB for it. > This can lead to the controller accessing an unmapped memory address. > > Fix this by ensuring parameters to prevent EP queuing are set before > calling the stop active transfers API.
commit f09ddcfcb8c569675066337adac2ac205113471f Author: Wesley Cheng <wch...@codeaurora.org> Date: Thu Mar 11 15:59:02 2021 -0800 usb: dwc3: gadget: Prevent EP queuing while stopping transfers effectively broke my gadget setup. The output of the kernel (followed by non responsive state of USB controller): [ 195.228586] using random self ethernet address [ 195.233104] using random host ethernet address [ 195.245306] usb0: HOST MAC aa:bb:cc:dd:ee:f2 [ 195.249732] usb0: MAC aa:bb:cc:dd:ee:f1 # [ 195.773594] IPv6: ADDRCONF(NETDEV_CHANGE): usb0: link becomes ready [ 195.780585] ------------[ cut here ]------------ [ 195.785217] dwc3 dwc3.0.auto: No resource for ep2in [ 195.790162] WARNING: CPU: 0 PID: 217 at drivers/usb/dwc3/gadget.c:360 dwc3_send_gadget_ep_cmd+0x4b9/0x670 [ 195.799760] Modules linked in: usb_f_eem u_ether libcomposite brcmfmac brcmutil mmc_block pwm_lpss_pci pwm_lps s snd_sof_pci_intel_tng snd_sof_pci snd_sof_acpi_intel_byt snd_sof_intel_ipc snd_sof_acpi snd_sof snd_sof_nocodec spi_pxa2xx_platform snd_sof_xtensa_dsp spi_pxa2xx_pci extcon_intel_mrfld intel_mrfld_adc sdhci_pci cqhci sdhci m mc_core intel_mrfld_pwrbtn intel_soc_pmic_mrfld hci_uart btbcm btintel [ 195.835604] CPU: 0 PID: 217 Comm: irq/16-dwc3 Not tainted 5.12.0-rc4+ #60 [ 195.842403] Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 2015.01.21:18.19.48 [ 195.851191] RIP: 0010:dwc3_send_gadget_ep_cmd+0x4b9/0x670 [ 195.856608] Code: cd 00 00 00 44 89 44 24 20 48 89 4c 24 18 e8 ee f7 e4 ff 48 8b 4c 24 18 4c 89 f2 48 c7 c7 b9 ed 4f a0 48 89 c6 e8 ef 24 43 00 <0f> 0b 41 be ea ff ff ff 44 8b 44 24 20 e9 80 fc ff ff 41 83 fe 92 [ 195.875381] RSP: 0000:ffffa53c00373ba8 EFLAGS: 00010086 [ 195.880617] RAX: 0000000000000000 RBX: 0000000000001387 RCX: 00000000ffffdfff [ 195.887755] RDX: 00000000ffffdfff RSI: 00000000ffffffea RDI: 0000000000000000 [ 195.894893] RBP: ffff9ce8c8f2b028 R08: ffffffffa0732288 R09: 0000000000009ffb [ 195.902034] R10: 00000000ffffe000 R11: 3fffffffffffffff R12: 0000000000041006 [ 195.909170] R13: ffffa53c00373c24 R14: ffff9ce8c11dadb0 R15: ffff9ce8c2861700 [ 195.916310] FS: 0000000000000000(0000) GS:ffff9ce8fe200000(0000) knlGS:0000000000000000 [ 195.924409] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 195.930161] CR2: 00000000f7f694a0 CR3: 0000000038e0c000 CR4: 00000000001006f0 [ 195.937300] Call Trace: [ 195.939755] __dwc3_gadget_ep_enable+0x2d4/0x4e0 [ 195.944393] ? dwc3_remove_requests.constprop.0+0x86/0x170 [ 195.949897] dwc3_gadget_ep_enable+0x5d/0x120 [ 195.954274] usb_ep_enable+0x27/0x80 [ 195.957869] gether_connect+0x32/0x1f0 [u_ether] [ 195.962512] eem_set_alt+0x6d/0x140 [usb_f_eem] [ 195.967061] composite_setup+0x224/0x1ba0 [libcomposite] [ 195.972405] ? debug_dma_unmap_page+0x79/0x80 [ 195.976782] ? configfs_composite_setup+0x6b/0x90 [libcomposite] [ 195.982811] configfs_composite_setup+0x6b/0x90 [libcomposite] [ 195.988668] dwc3_ep0_interrupt+0x459/0xa50 [ 195.992869] dwc3_thread_interrupt+0x8e2/0xee0 [ 195.997327] ? __schedule+0x237/0x6d0 [ 196.001005] ? disable_irq_nosync+0x10/0x10 [ 196.005200] irq_thread_fn+0x1b/0x60 [ 196.008789] irq_thread+0xd6/0x170 [ 196.012202] ? irq_thread_check_affinity+0x70/0x70 [ 196.017004] ? irq_forced_thread_fn+0x70/0x70 [ 196.021373] kthread+0x116/0x130 [ 196.024617] ? kthread_create_worker_on_cpu+0x60/0x60 [ 196.029680] ret_from_fork+0x22/0x30 [ 196.033272] ---[ end trace 8dd104a950d8d248 ]--- Revert helps (I'm on v5.12-rc4 now with a revert). The script to enable gadget: #!/bin/sh -efu # Mounting CONFIGFS grep -q -w /sys/kernel/config /proc/mounts || mount -t configfs none /sys/kernel/config # Addresses and files readonly GADGET_BASE_DIR="/sys/kernel/config/usb_gadget/g1" readonly DEV_ETH_ADDR="aa:bb:cc:dd:ee:f1" readonly HOST_ETH_ADDR="aa:bb:cc:dd:ee:f2" readonly USBDISK="/usbdisk.img" # Insert modules modprobe libcomposite # Create directory structure mkdir "${GADGET_BASE_DIR}" cd "${GADGET_BASE_DIR}" mkdir -p configs/c.1/strings/0x409 mkdir -p strings/0x409 # Ethernet device mkdir functions/eem.usb0 echo "${DEV_ETH_ADDR}" > functions/eem.usb0/dev_addr echo "${HOST_ETH_ADDR}" > functions/eem.usb0/host_addr ln -s functions/eem.usb0 configs/c.1/ # Composite Gadget Setup echo 0x1d6b > idVendor # Linux Foundation echo 0x0104 > idProduct # Multifunction Composite Gadget echo 0x0100 > bcdDevice # v1.0.0 echo 0x0200 > bcdUSB # USB2 echo "0123456789abcdef" > strings/0x409/serialnumber echo "USBArmory" > strings/0x409/manufacturer echo "USBArmory Gadget" > strings/0x409/product echo "Conf1" > configs/c.1/strings/0x409/configuration echo 120 > configs/c.1/MaxPower # Activate gadgets echo dwc3.0.auto > UDC Please, tell me how to fix this, otherwise I will have to send a revert. > Fixes: ae7e86108b12 ("usb: dwc3: Stop active transfers before halting the > controller") > Signed-off-by: Wesley Cheng <wch...@codeaurora.org> > --- > Changes since V2: > - Removed duplicate dwc->connected = false setting in pullup routine > > Changes since V1: > - Added Fixes tag to point to the commit this is addressing > > drivers/usb/dwc3/gadget.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 4780983..2c94cc9 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -783,8 +783,6 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep) > > trace_dwc3_gadget_ep_disable(dep); > > - dwc3_remove_requests(dwc, dep); > - > /* make sure HW endpoint isn't stalled */ > if (dep->flags & DWC3_EP_STALL) > __dwc3_gadget_ep_set_halt(dep, 0, false); > @@ -803,6 +801,8 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep) > dep->endpoint.desc = NULL; > } > > + dwc3_remove_requests(dwc, dep); > + > return 0; > } > > @@ -1617,7 +1617,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, > struct dwc3_request *req) > { > struct dwc3 *dwc = dep->dwc; > > - if (!dep->endpoint.desc || !dwc->pullups_connected) { > + if (!dep->endpoint.desc || !dwc->pullups_connected || > !dwc->connected) { > dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n", > dep->name); > return -ESHUTDOWN; > @@ -2247,6 +2247,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int > is_on) > if (!is_on) { > u32 count; > > + dwc->connected = false; > /* > * In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a > * Section 4.1.8 Table 4-7, it states that for a > device-initiated > @@ -2271,7 +2272,6 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int > is_on) > dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) % > dwc->ev_buf->length; > } > - dwc->connected = false; > } else { > __dwc3_gadget_start(dwc); > } > @@ -3329,8 +3329,6 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 > *dwc) > { > u32 reg; > > - dwc->connected = true; > - > /* > * WORKAROUND: DWC3 revisions <1.88a have an issue which > * would cause a missing Disconnect Event if there's a > @@ -3370,6 +3368,7 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 > *dwc) > * transfers." > */ > dwc3_stop_active_transfers(dwc); > + dwc->connected = true; > > reg = dwc3_readl(dwc->regs, DWC3_DCTL); > reg &= ~DWC3_DCTL_TSTCTRL_MASK; > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- With Best Regards, Andy Shevchenko