| Message ID | 1444677889-4758-1-git-send-email-balbi@ti.com |
|---|---|
| State | Accepted |
| Commit | e5f68b4a3e7b006f209aba078d6a5ff3a78dd783 |
| Headers | show |
On 10/12/2015 12:25 PM, Felipe Balbi wrote: > This reverts commit 70f3a9caa11665e9f9aace581d85d8483716a4c8. > > That commit was causing a lockdep splat with g_ether and that > was interfering with proper functionality. > > Signed-off-by: Felipe Balbi <balbi@ti.com> > --- > drivers/usb/dwc3/gadget.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index cca806e09e5b..81bfb9ad1e2e 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2642,15 +2642,16 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3 *dwc, u32 buf) > static irqreturn_t dwc3_thread_interrupt(int irq, void *_dwc) > { > struct dwc3 *dwc = _dwc; > + unsigned long flags; > irqreturn_t ret = IRQ_NONE; > int i; > > - spin_lock(&dwc->lock); > + spin_lock_irqsave(&dwc->lock, flags); > > for (i = 0; i < dwc->num_event_buffers; i++) > ret |= dwc3_process_event_buf(dwc, i); > > - spin_unlock(&dwc->lock); > + spin_unlock_irqrestore(&dwc->lock, flags); > > return ret; > } > Hi Felipe, This seems related to a problem we've been tracking down the past few days. This commit, 70f3a9ca, and commit a66c275b both cause regression on one of our systems running mass-storage gadget. a66c275b was introduced first, and until 70f3a9ca is introduced if we revert just that, it works fine. 70f3a9ca causes similar issues. So we must revert both in order to get back to a working state. Failure happens during enumeration and appears to be a race condition in the event handling. Attached are driver logs/traces for the failure with a66c275b. John
Hi, John Youn <John.Youn@synopsys.com> writes: > On 10/12/2015 12:25 PM, Felipe Balbi wrote: >> This reverts commit 70f3a9caa11665e9f9aace581d85d8483716a4c8. >> >> That commit was causing a lockdep splat with g_ether and that >> was interfering with proper functionality. >> >> Signed-off-by: Felipe Balbi <balbi@ti.com> >> --- >> drivers/usb/dwc3/gadget.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index cca806e09e5b..81bfb9ad1e2e 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2642,15 +2642,16 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3 *dwc, u32 buf) >> static irqreturn_t dwc3_thread_interrupt(int irq, void *_dwc) >> { >> struct dwc3 *dwc = _dwc; >> + unsigned long flags; >> irqreturn_t ret = IRQ_NONE; >> int i; >> >> - spin_lock(&dwc->lock); >> + spin_lock_irqsave(&dwc->lock, flags); >> >> for (i = 0; i < dwc->num_event_buffers; i++) >> ret |= dwc3_process_event_buf(dwc, i); >> >> - spin_unlock(&dwc->lock); >> + spin_unlock_irqrestore(&dwc->lock, flags); >> >> return ret; >> } >> > > Hi Felipe, > > This seems related to a problem we've been tracking down the past > few days. This commit, 70f3a9ca, and commit a66c275b both cause > regression on one of our systems running mass-storage gadget. I can't see how a66c275b could cause any issues. Our top half runs in hardirq context with IRQs disabled; this has been discussed several times. > a66c275b was introduced first, and until 70f3a9ca is introduced > if we revert just that, it works fine. 70f3a9ca causes similar > issues. So we must revert both in order to get back to a working > state. > > Failure happens during enumeration and appears to be a race > condition in the event handling. > > Attached are driver logs/traces for the failure with a66c275b. All I see is a bunch of Start Transfer commands which failed. That's odd. Which version of the core are you using ? Dwc3 or dwc31 ?
On 10/13/2015 9:05 AM, Felipe Balbi wrote: > > Hi, > > John Youn <John.Youn@synopsys.com> writes: >> On 10/12/2015 12:25 PM, Felipe Balbi wrote: >>> This reverts commit 70f3a9caa11665e9f9aace581d85d8483716a4c8. >>> >>> That commit was causing a lockdep splat with g_ether and that >>> was interfering with proper functionality. >>> >>> Signed-off-by: Felipe Balbi <balbi@ti.com> >>> --- >>> drivers/usb/dwc3/gadget.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index cca806e09e5b..81bfb9ad1e2e 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -2642,15 +2642,16 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3 *dwc, u32 buf) >>> static irqreturn_t dwc3_thread_interrupt(int irq, void *_dwc) >>> { >>> struct dwc3 *dwc = _dwc; >>> + unsigned long flags; >>> irqreturn_t ret = IRQ_NONE; >>> int i; >>> >>> - spin_lock(&dwc->lock); >>> + spin_lock_irqsave(&dwc->lock, flags); >>> >>> for (i = 0; i < dwc->num_event_buffers; i++) >>> ret |= dwc3_process_event_buf(dwc, i); >>> >>> - spin_unlock(&dwc->lock); >>> + spin_unlock_irqrestore(&dwc->lock, flags); >>> >>> return ret; >>> } >>> >> >> Hi Felipe, >> >> This seems related to a problem we've been tracking down the past >> few days. This commit, 70f3a9ca, and commit a66c275b both cause >> regression on one of our systems running mass-storage gadget. > > I can't see how a66c275b could cause any issues. Our top half runs in > hardirq context with IRQs disabled; this has been discussed several times. > Yes I followed that and I'm also not sure how this can affect it. I was hoping you might have some idea :) I'll look into it in more detail later and let you know. >> a66c275b was introduced first, and until 70f3a9ca is introduced >> if we revert just that, it works fine. 70f3a9ca causes similar >> issues. So we must revert both in order to get back to a working >> state. >> >> Failure happens during enumeration and appears to be a race >> condition in the event handling. >> >> Attached are driver logs/traces for the failure with a66c275b. > > All I see is a bunch of Start Transfer commands which failed. That's > odd. Which version of the core are you using ? Dwc3 or dwc31 ? > USB 3.0 John -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, John Youn <John.Youn@synopsys.com> writes: > On 10/13/2015 9:05 AM, Felipe Balbi wrote: >>> John Youn <John.Youn@synopsys.com> writes: >>> On 10/12/2015 12:25 PM, Felipe Balbi wrote: >>>> This reverts commit 70f3a9caa11665e9f9aace581d85d8483716a4c8. >>>> >>>> That commit was causing a lockdep splat with g_ether and that >>>> was interfering with proper functionality. >>>> >>>> Signed-off-by: Felipe Balbi <balbi@ti.com> >>>> --- >>>> drivers/usb/dwc3/gadget.c | 5 +++-- >>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>> index cca806e09e5b..81bfb9ad1e2e 100644 >>>> --- a/drivers/usb/dwc3/gadget.c >>>> +++ b/drivers/usb/dwc3/gadget.c >>>> @@ -2642,15 +2642,16 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3 *dwc, u32 buf) >>>> static irqreturn_t dwc3_thread_interrupt(int irq, void *_dwc) >>>> { >>>> struct dwc3 *dwc = _dwc; >>>> + unsigned long flags; >>>> irqreturn_t ret = IRQ_NONE; >>>> int i; >>>> >>>> - spin_lock(&dwc->lock); >>>> + spin_lock_irqsave(&dwc->lock, flags); >>>> >>>> for (i = 0; i < dwc->num_event_buffers; i++) >>>> ret |= dwc3_process_event_buf(dwc, i); >>>> >>>> - spin_unlock(&dwc->lock); >>>> + spin_unlock_irqrestore(&dwc->lock, flags); >>>> >>>> return ret; >>>> } >>>> >>> >>> Hi Felipe, >>> >>> This seems related to a problem we've been tracking down the past >>> few days. This commit, 70f3a9ca, and commit a66c275b both cause >>> regression on one of our systems running mass-storage gadget. >> >> I can't see how a66c275b could cause any issues. Our top half runs in >> hardirq context with IRQs disabled; this has been discussed several times. >> > > Yes I followed that and I'm also not sure how this can affect > it. I was hoping you might have some idea :) > > I'll look into it in more detail later and let you know. okay. >>> a66c275b was introduced first, and until 70f3a9ca is introduced >>> if we revert just that, it works fine. 70f3a9ca causes similar >>> issues. So we must revert both in order to get back to a working >>> state. >>> >>> Failure happens during enumeration and appears to be a race >>> condition in the event handling. >>> >>> Attached are driver logs/traces for the failure with a66c275b. >> >> All I see is a bunch of Start Transfer commands which failed. That's >> odd. Which version of the core are you using ? Dwc3 or dwc31 ? >> > > USB 3.0 silicon, fpga or virtual model ? If this is virtual model, could it be a bug in the interrupt controller core model or something like that ? Any chance you can get your same dwc3 in FPGA with Synopsys PCIe controller and stick it into a desktop PC and see if you can get the same behavior? If you want, I could try it myself, but I only have HAPS-61 and would need sd-card contents (bitfile and conf file) with the core revision you're using. I only have an old 1.83a bitfile around.
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index cca806e09e5b..81bfb9ad1e2e 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2642,15 +2642,16 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3 *dwc, u32 buf) static irqreturn_t dwc3_thread_interrupt(int irq, void *_dwc) { struct dwc3 *dwc = _dwc; + unsigned long flags; irqreturn_t ret = IRQ_NONE; int i; - spin_lock(&dwc->lock); + spin_lock_irqsave(&dwc->lock, flags); for (i = 0; i < dwc->num_event_buffers; i++) ret |= dwc3_process_event_buf(dwc, i); - spin_unlock(&dwc->lock); + spin_unlock_irqrestore(&dwc->lock, flags); return ret; }
This reverts commit 70f3a9caa11665e9f9aace581d85d8483716a4c8. That commit was causing a lockdep splat with g_ether and that was interfering with proper functionality. Signed-off-by: Felipe Balbi <balbi@ti.com> --- drivers/usb/dwc3/gadget.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)