Hi, Felipe Balbi <balbi@xxxxxxxxxx> writes: > Hi, > > Baolin Wang <baolin.wang@xxxxxxxxxx> writes: >>>>> I'm thinking this is a bug in configfs interface of Gadget API, not >>>>> dwc3. The only reason for this to happen would be if we get to >>>>> ->udc_stop() with endpoints still enabled. >>>>> >>>>> Can you check if that's the case? i.e. can you check if any endpoints >>>>> are still enabled when we get here? >>>> >>>> No, it is not any endpoints are still enabled. Like I said in commit >>>> message, we will start end transfer command when disable the endpoint, >>>> if the end transfer command complete event comes after we have freed >>>> the gadget irq, it will trigger the interrupt line all the time to >>>> crash the system. >>> >>> I see what the problem is. Databook tells us we *must* set CMDIOC when >>> issuing EndTransfer command and we should always wait for Command >>> Complete IRQ. However, we took a shortcut and just delayed for 100us >>> after issuing End Transfer. >> >> Yes, but 100us delay is not enough in some scenarios, like changing >> function with configfs frequently, we will met problems. > > heh, 100us *is* enough. However we still get an IRQ because we requested > for it. If you want to fix this, then you need to find a way to get rid > of that 100us udelay() and add a proper wait_for_completion() to delay > execution until command complete IRQ fires up. I haven't tested this yet, but it shows the idea (I think we might still have a race with ep_queue if we're still waiting for End Transfer, but that's easy to sort out by checking for DWC3_EP_END_TRANSFER_PENDING before calling kick_transfer). Could you have a look and, perhaps, run a test? diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index e878366ead00..24a77e9f9bba 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -26,6 +26,7 @@ #include <linux/dma-mapping.h> #include <linux/mm.h> #include <linux/debugfs.h> +#include <linux/wait.h> #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> @@ -504,6 +505,7 @@ struct dwc3_event_buffer { * @endpoint: usb endpoint * @pending_list: list of pending requests for this endpoint * @started_list: list of started requests on this endpoint + * @wait_end_transfer: wait_queue_head_t for waiting on End Transfer complete * @lock: spinlock for endpoint request queue traversal * @regs: pointer to first endpoint register * @trb_pool: array of transaction buffers @@ -529,6 +531,8 @@ struct dwc3_ep { struct list_head pending_list; struct list_head started_list; + wait_queue_head_t wait_end_transfer; + spinlock_t lock; void __iomem *regs; @@ -545,7 +549,7 @@ struct dwc3_ep { #define DWC3_EP_BUSY (1 << 4) #define DWC3_EP_PENDING_REQUEST (1 << 5) #define DWC3_EP_MISSED_ISOC (1 << 6) - +#define DWC3_EP_END_TRANSFER_PENDING (1 << 7) /* This last one is specific to EP0 */ #define DWC3_EP0_DIR_IN (1 << 31) @@ -1047,6 +1051,9 @@ struct dwc3_event_depevt { #define DEPEVT_TRANSFER_BUS_EXPIRY 2 u32 parameters:16; + +/* For Command Complete Events */ +#define DEPEVT_PARAMETER_CMD(n) (((n) & (0xf << 24)) >> 24) } __packed; /** diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 3ba05b12e49a..8037bff43485 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -598,6 +598,8 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, reg |= DWC3_DALEPENA_EP(dep->number); dwc3_writel(dwc->regs, DWC3_DALEPENA, reg); + init_waitqueue_head(&dep->wait_end_transfer); + if (usb_endpoint_xfer_control(desc)) return 0; @@ -2156,6 +2158,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, { struct dwc3_ep *dep; u8 epnum = event->endpoint_number; + u8 cmd; dep = dwc->eps[epnum]; @@ -2200,8 +2203,13 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, return; } break; - case DWC3_DEPEVT_RXTXFIFOEVT: case DWC3_DEPEVT_EPCMDCMPLT: + cmd = DEPEVT_PARAMETER_CMD(event->parameters); + + if (cmd == DWC3_DEPCMD_ENDTRANSFER) + dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING; + break; + case DWC3_DEPEVT_RXTXFIFOEVT: break; } } @@ -2246,6 +2254,7 @@ static void dwc3_reset_gadget(struct dwc3 *dwc) } static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force) +__releases(&dwc->lock) __acquires(&dwc->lock) { struct dwc3_ep *dep; struct dwc3_gadget_ep_cmd_params params; @@ -2295,6 +2304,12 @@ static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force) memset(¶ms, 0, sizeof(params)); ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); WARN_ON_ONCE(ret); + + dep->flags |= DWC3_EP_END_TRANSFER_PENDING; + wait_event_cmd(dep->wait_end_transfer, + !(dep->flags & DWC3_EP_END_TRANSFER_PENDING), + spin_unlock_irq(&dwc->lock), spin_lock_irq(&dwc->lock)); + dep->resource_index = 0; dep->flags &= ~DWC3_EP_BUSY; -- balbi
Attachment:
signature.asc
Description: PGP signature