Hi,
On 5/11/2017 1:22 AM, Felipe Balbi wrote:
>
> Hi,
>
> John Youn <John.Youn@xxxxxxxxxxxx> writes:
>>>>>> The device hangs at the end of the log.
>>>>
>>>> I repeated the test with USB 3.0 IP version 2.90a and USB 3.1 IP version
>>>> 1.60, and I found the same result where the TH is occasionally called
>>>> twice back-to-back.
>>>>
>>>>>
>>>>> Right, found this in the logs. So the only thing that I can think of
>>>>> here, is that we need to flush posted writes. Does this help?
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>> index 79e7a3480d51..6f06738273f2 100644
>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>> @@ -3099,6 +3099,10 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt)
>>>>>
>>>>> dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count);
>>>>>
>>>>> + /* flush posted writes */
>>>>> + dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
>>>>> + dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
>>>>> +
>>>>> return IRQ_WAKE_THREAD;
>>>>> }
>>>>>
>>>>>
>>>>
>>>> With the change above, I don't see the TH get called twice in in a row.
>>>> However, I also tested with replacing the code above with a udelay(4),
>>>> from reading the kernel trace, to take in account the latency of reading
>>>> the registers. I also don't see TH get called twice in a row.
>>>>
>>>> The extra latency from reading the registers above can mask this issue.
>>>
>>> okay, so it is a problem with posted writes. Can you send above as a
>>> formal patch? That's something we can backport to every stable
>>> kernel. And the real reason here _IS_ caused by posted writes. There's
>>> no spurious interrupt happening.
>>>
>>
>> Hi Felipe,
>>
>> I only mentioned spurious interrupts as something the driver needs to
>> account for anyways. They are not the cause of this issue. And I don't
>> think the PCIe posted writes are either becuase of what I said before
>> about the interrupts being packets and the latency in assertion and
>> detection.
>>
>> But anyways, after thorough testing we don't see this in our system
>> anymore with the read-back (probably due to the 4 us delay that's
>> introduced). We can submit a patch for this for stable.
>
> please do :-)
>
>> However I don't think this is a sufficient fix for mainline, and dwc3
>> should be able to handle multiple interrupts before BH, whatever the
>> reason for it.
>>
>> How do you want to handle mainline? Either Thinh's original patch,
>> create a queue for the events, or some other way? We can work on this
>> fix too if you want.
>
> Can't we just check for the PENDING flag? something like:
>
> @@ -3052,6 +3052,9 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt)
> u32 count;
> u32 reg;
>
> + if (evt->flags & DWC3_EVENT_PENDING)
> + return IRQ_HANDLED;
> +
> if (pm_runtime_suspended(dwc->dev)) {
> pm_runtime_get(dwc->dev);
> disable_irq_nosync(dwc->irq_gadget);
>
>
That will work also. I just resubmitted the patch with this change.
Thanks,
Thinh
--
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
