Re: [PATCH] usb: dwc2: gadget: fix ISOC frame overflow handling

[Minas Harutyunyan <minas.harutyunyan@synopsys.com>]

Hi John,

On 11/9/2018 12:43 PM, Minas Harutyunyan wrote:
> Hi John,
> 
> On 11/8/2018 9:37 PM, John Keeping wrote:
>> Hi Minas,
>>
>> On Mon, 5 Nov 2018 08:28:07 +0000
>> Minas Harutyunyan <minas.harutyunyan@synopsys.com> wrote:
>>
>>> On 10/23/2018 5:43 PM, John Keeping wrote:
>>>> By clearing the overrun flag as soon as the target frame is next
>>>> incremented, we can end up incrementing the target frame more than
>>>> expected in dwc2_gadget_handle_ep_disabled() when the endpoint's
>>>> interval is greater than 1.  This happens if the target frame has
>>>> just wrapped at the point when the endpoint is disabled and the
>>>> frame number has not yet done so.
>>>>
>>>> Instead, wait until the frame number also wraps and then clear the
>>>> overrun flag.
>>>>
>>>> Signed-off-by: John Keeping <john@metanate.com>
>>>> ---
>>>>     drivers/usb/dwc2/gadget.c | 2 +-
>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>>>> index 2d6d2c8244de..8da2c052dfa1 100644
>>>> --- a/drivers/usb/dwc2/gadget.c
>>>> +++ b/drivers/usb/dwc2/gadget.c
>>>> @@ -117,7 +117,7 @@ static inline void
>>>> dwc2_gadget_incr_frame_num(struct dwc2_hsotg_ep *hs_ep) if
>>>> (hs_ep->target_frame > DSTS_SOFFN_LIMIT) { hs_ep->frame_overrun =
>>>> true; hs_ep->target_frame &= DSTS_SOFFN_LIMIT;
>>>> -	} else {
>>>> +	} else if (hs_ep->parent->frame_number <
>>>> hs_ep->target_frame) { hs_ep->frame_overrun = false;
>>>>     	}
>>>>     }
>>>>     
>>> Did you tested mentioned by you scenario? If you see issue can you
>>> provide debug log and point the issue line in the log.
>>
>> It only reproduces very occasionally so it's difficult to capture a full
>> debug log containing the error.
>>
>> I applied this patch to capture logging specifically around this
>> scenario:
>>
>> -- >8 --
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 220c0f9b89b0..3770b9d3b523 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -2722,13 +2722,20 @@ static void dwc2_gadget_handle_ep_disabled(struct dwc2_hsotg_ep *hs_ep)
>>           }
>>    
>>           do {
>> +               unsigned int target_frame = hs_ep->target_frame;
>> +               bool frame_overrun = hs_ep->frame_overrun;
>> +
>>                   hs_req = get_ep_head(hs_ep);
>>                   if (hs_req)
>>                           dwc2_hsotg_complete_request(hsotg, hs_ep, hs_req,
>>                                                       -ENODATA);
>> +
>>                   dwc2_gadget_incr_frame_num(hs_ep);
>>                   /* Update current frame number value. */
>>                   hsotg->frame_number = dwc2_hsotg_read_frameno(hsotg);
>> +
>> +               dev_warn(hsotg->dev, "%s: expiring request frame_number=0x%04x target_frame=0x%04x overrun=%u\n",
>> +                        __func__, hsotg->frame_number, target_frame, frame_overrun);
>>           } while (dwc2_gadget_target_frame_elapsed(hs_ep));
>>    
>>           dwc2_gadget_start_next_request(hs_ep);
>> -- 8< --
>>
>> and I captured this log (the first entry is a separate error and then
>> the remaining ones show this bug being triggered):
>>
>> [  562.571227] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3eb9 target_frame=0x3ec0
>> [  562.611213] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff8 target_frame=0x0008
>> [  562.611219] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff8 target_frame=0x0010
>> [  562.611223] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0018
>> [  562.611228] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0020
>> [  562.611232] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0028
>> [  562.611236] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0030
>> [  562.611240] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0038
>> [  562.611244] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0040
>> [  562.611249] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0048
>> [  562.611253] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0050
>> [  562.611257] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0058
>> [  562.611261] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0060
>> [  562.611265] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0068
>> [  562.611269] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0070
>> [  562.611274] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0078
>> [  562.611278] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0080
>> [  562.611282] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0088
>> [  562.611286] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0090
>> [  562.611290] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0098
>> [  562.611294] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x00a0
>>
> According above patch in debug log should be printed overrun flag also.
> Could you please resend log with this flag.
> 
One more request. Please add EP number to debug print.
>>
>> This was on v4.19 with an additional patch to disable descriptor DMA
>> because that seems to be causing problems on RK3288 although I haven't
>> figured out exactly why it's a problem.
> 
> In which mode you run tests Slave or Buffer DMA?
>>
>>
>> Regards,
>> John
>>
> 
> Thanks,
> Minas
> 
>