On 04/08/2017 12:38 PM, Janusz Dziedzic wrote: > 2017-04-08 1:57 GMT+02:00 Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>: >> The dwc3 driver can overwite its previous events if its top half IRQ >> handler gets invoked again before processing the events in the cache. We >> see this as a hang in the file transfer and the host will attempt to >> reset the device. This shouldn't be possible in the dwc3 implementation >> if the HW and the SW are in sync. However, through testing and reading >> the pcie trace, the top half IRQ handler occasionally still gets invoked >> one more time after HW interrupt deassertion. We suspect that there is a >> small detection delay in the SW. >> >> Top half IRQ handler deasserts the interrupt line after it gets the >> event count by writing DWC3_GEVNTSIZ_INTMASK to DWC3_GEVNTSIZ. There's a >> small window for a new event coming in between reading the event count >> and the interrupt deassertion. More generally, we will see 0 event >> count, which should not affect anything. >> >> To avoid this issue, we ensure that the events in the cache are >> processed before checking for the new ones. The bottom half IRQ will >> unmask the DWC3_GEVNTSIZ_INTMASK once it finishes processing the events. >> Top half IRQ handler needs to check whether the DWC3_GEVNTSIZ_INTMASK is >> still set before storing the new events. It also should return >> IRQ_HANDLED if the mask is still set since IRQ handler was invoked for >> our usb interrupt. >> >> Signed-off-by: Thinh Nguyen <thinhn@xxxxxxxxxxxx> >> --- >> drivers/usb/dwc3/gadget.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 4dc80729ae11..93d98fb7215e 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -3045,6 +3045,10 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt) >> return IRQ_HANDLED; >> } >> >> + reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0)); >> + if (reg & DWC3_GEVNTSIZ_INTMASK) >> + return IRQ_HANDLED; >> + >> count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0)); >> count &= DWC3_GEVNTCOUNT_MASK; >> if (!count) >> @@ -3054,7 +3058,6 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt) >> evt->flags |= DWC3_EVENT_PENDING; >> >> /* Mask interrupt */ >> - reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0)); >> reg |= DWC3_GEVNTSIZ_INTMASK; >> dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), reg); >> >> -- >> 2.11.0 >> > This seems like a workaround, while when we mask interrupts we should > not get IRQ before BH will done. Current driver implementation base on > this. > We are using 2.60a and didn't see problem you describe. Is it specyfic > for some HW revision? Doesn't seem to be. We can try other HW versions. > > I can imagine we can have the "last" interrupt and you will return > IRQ_HANDLED. Will HW issue this interrupt once again? If not will we > loose this IRQ (eg. disconnect event)? > Yes it will re-assert. The interrupt line will remain asserted as long events remain in the buffer and it is not masked. So when we eventually unmask, the interrupt will be reasserted again immediately. > BTW, other option here could be using (like Baolin propose some time > ago) dwc->lock in top half while this is held for BH. > Question how long spin_lock() will be held in top half... > I am not sure what option is the best. That won't help in this case since you can still have two calls top top-half before a call to bottom. The lock only prevents the two from stepping on each other, but that should already be the case without needing the lock. Regards, John -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
