Re: [PATCH 1/3] usb: dwc3: gadget: Prevent losing events in event cache

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10 April 2017 at 20:43, John Youn <John.Youn@xxxxxxxxxxxx> wrote:
> 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.
>
Really can we have two TH calls before BH?
Interesting case :)

You suggest we have something like this:
dwc3 IRQ
   kernel irq_mask
      dwc3 TH
         mask dwc3 interrupts
         get evt->count, evt->cache
         write to hw (count)
         return WAKE_THREAD
   kernel irq_unmask
a) Before BH start we get another dwc3 IRQ?
b) CPU0 starts with BH while we get dwc3 IRQ on CPU1?

*) in normal case BH should start and in the end unmask
dwc3_interrupts and after that we could get new irq

If this could happen then for sure you need dwc->lock protection in TH
while base on your description CPU0 can execute BH and CPU1 can handle
TH - so you have to protect dwc3_event_buffer.

Could you describe this more? How to reproduce?
Do you think we introduce this when adding evt->cache in TH?
Even without evt->cache we still could overwrite evt->count - so, was
that seen before evt->cache?

BR
Janusz

> 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
--
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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux