[4/4] usb: dwc3: gadget: check if dep->frame_number is still valid

[Felipe Balbi <felipe.balbi@linux.intel.com>]

Hi,

Thinh Nguyen <thinh.nguyen@synopsys.com> writes:
>>> If it fails for over 5 times in a row, then we should probably wait for
>>> the next XferNotReady to get the frame_number again. Also 5 is an
>>> arbitrary number I came up without any testing, we can probably decide
>>> on a better number. What do you think?
>> I have this now:
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 131028501752..3390fa46ea30 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -37,6 +37,7 @@
>>  #define DWC3_EP0_SETUP_SIZE    512
>>  #define DWC3_ENDPOINTS_NUM     32
>>  #define DWC3_XHCI_RESOURCES_NUM        2
>> +#define DWC3_ISOC_MAX_RETRIES  5
>>  
>>  #define DWC3_SCRATCHBUF_SIZE   4096    /* each buffer is assumed to be 4KiB */
>>  #define DWC3_EVENT_BUFFERS_SIZE        4096
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index d8c7ad0c22e8..1590516735cb 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -27,7 +27,7 @@
>>  #include "gadget.h"
>>  #include "io.h"
>>  
>> -#define DWC3_ALIGN_FRAME(d)    (((d)->frame_number + (d)->interval) \
>> +#define DWC3_ALIGN_FRAME(d, n) (((d)->frame_number + ((d)->interval * (n))) \
>>                                         & ~((d)->interval - 1))
>>  
>>  /**
>> @@ -1268,13 +1268,24 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>>  
>>  static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>>  {
>> +       int retries;
>> +       int ret;
>> +       int i;
>> +
>>         if (list_empty(&dep->pending_list)) {
>>                 dep->flags |= DWC3_EP_PENDING_REQUEST;
>>                 return -EAGAIN;
>>         }
>>  
>> -       dep->frame_number = DWC3_ALIGN_FRAME(dep);
>> -       return __dwc3_gadget_kick_transfer(dep);
>> +       for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
>> +               dep->frame_number = DWC3_ALIGN_FRAME(dep, i);
>  
> Shouldn't this be like this?
> dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);

good catch, I'll fix.

>> +
>> +               ret = __dwc3_gadget_kick_transfer(dep);
>> +               if (ret != -EAGAIN)
>> +                       break;
>> +       }
>> +
>> +       return ret;
>>  }
>>  
>>  static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>>
>> This means we will increment in full intervals, up to 5 intervals in the
>> future. If it still fails, then there's not much we can do.
>>
> I agree. Also, depending on the application requirement, we may want to
> giveback/cleanup request(s) every time we fail so that the data won't be
> pushed back/delayed for too long.
> We could experiment and decide on the maximum delay time (base on the
> service interval length and number of retries) to be considered
> acceptable to start giveback stale requests.

That's something we can consider in the future. I'd say that the gadget
driver should, then, tell us how much slack to give.