RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields

[Anurag Kumar Vulisha]

Hi Felipe,

>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@xxxxxxxxxx]
>Sent: Monday, December 10, 2018 12:24 PM
>To: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>; Greg Kroah-Hartman
><gregkh@xxxxxxxxxxxxxxxxxxx>; Shuah Khan <shuah@xxxxxxxxxx>; Alan Stern
><stern@xxxxxxxxxxxxxxxxxxx>; Johan Hovold <johan@xxxxxxxxxx>; Jaejoong Kim
><climbbb.kim@xxxxxxxxx>; Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>;
>Roger Quadros <rogerq@xxxxxx>; Manu Gautam <mgautam@xxxxxxxxxxxxxx>;
>martin.petersen@xxxxxxxxxx; Bart Van Assche <bvanassche@xxxxxxx>; Mike
>Christie <mchristi@xxxxxxxxxx>; Matthew Wilcox <willy@xxxxxxxxxxxxx>; Colin Ian
>King <colin.king@xxxxxxxxxxxxx>
>Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>v.anuragkumar@xxxxxxxxx; Thinh Nguyen <thinhn@xxxxxxxxxxxx>; Tejas Joglekar
><tejas.joglekar@xxxxxxxxxxxx>; Ajay Yugalkishore Pandey <APANDEY@xxxxxxxxxx>
>Subject: RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status
>and TRB->ctrl fields
>
>
>Hi,
>
>Anurag Kumar Vulisha <anuragku@xxxxxxxxxx> writes:
>> HI Felipe,
>>
>>>-----Original Message-----
>>>From: Felipe Balbi [mailto:balbi@xxxxxxxxxx]
>>>Sent: Friday, December 07, 2018 11:42 AM
>>>To: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>; Greg Kroah-Hartman
>>><gregkh@xxxxxxxxxxxxxxxxxxx>; Shuah Khan <shuah@xxxxxxxxxx>; Alan Stern
>>><stern@xxxxxxxxxxxxxxxxxxx>; Johan Hovold <johan@xxxxxxxxxx>; Jaejoong Kim
>>><climbbb.kim@xxxxxxxxx>; Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>;
>>>Roger Quadros <rogerq@xxxxxx>; Manu Gautam <mgautam@xxxxxxxxxxxxxx>;
>>>martin.petersen@xxxxxxxxxx; Bart Van Assche <bvanassche@xxxxxxx>; Mike
>>>Christie <mchristi@xxxxxxxxxx>; Matthew Wilcox <willy@xxxxxxxxxxxxx>; Colin
>Ian
>>>King <colin.king@xxxxxxxxxxxxx>
>>>Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>>>v.anuragkumar@xxxxxxxxx; Thinh Nguyen <thinhn@xxxxxxxxxxxx>; Tejas
>Joglekar
>>><tejas.joglekar@xxxxxxxxxxxx>; Ajay Yugalkishore Pandey
><APANDEY@xxxxxxxxxx>
>>>Subject: RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event-
>>status
>>>and TRB->ctrl fields
>>>
>>>
>>>Hi,
>>>
>>>Anurag Kumar Vulisha <anuragku@xxxxxxxxxx> writes:
>>>>>> @@ -2286,7 +2286,12 @@ static int
>>>>>dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>>>>>>  	if (event->status & DEPEVT_STATUS_SHORT && !chain)
>>>>>>  		return 1;
>>>>>>
>>>>>> -	if (event->status & (DEPEVT_STATUS_IOC | DEPEVT_STATUS_LST))
>>>>>> +	if ((event->status & DEPEVT_STATUS_IOC) &&
>>>>>> +	    (trb->ctrl & DWC3_TRB_CTRL_IOC))
>>>>>> +		return 1;
>>>>>
>>>>>this shouldn't be necessary. According to databook, event->status
>>>>>contains the bits from the completed TRB.  Which means that
>>>>>event->status & IOC will always be equal to trb->ctrl & IOC.
>>>>>
>>>> Thanks for reviewing this patch. Lets consider an example where a
>>>> request has num_sgs > 0 and each sg is mapped to a TRB and the last
>>>> TRB has the IOC bit set. Once the controller is done with the
>>>> transfer, it  generates XferInProgress for the last TRB (since IOC bit
>>>> is set). As a part of trb reclaim process
>>>> dwc3_gadget_ep_reclaim_trb_sg() calls
>>>> dwc3_gadget_ep_reclaim_completed_trb() for req->num_sgs times. Since
>>>> the event already has the IOC bit set, the loop is exited from the
>>>> loop at the very first TRB and the remaining TRBs (mapped to the sglist) are left
>>>unhandled.
>>>> To avoid this we modified the code to exit only if both TRB & event
>>>> has the IOC bit set.
>>>
>>>Seems like IOC case should just test for chain flag as well:
>>>
>>
>> Okay. Along with this logic the code for updating chain bit should also be modified I
>guess.
>
>not really
>
>> Since the IOC bit is also set when there are not enough TRBs available, the code
>should be
>> modified to not set DWC3_TRB_CTRL_CHN bit when the IOC bit is set. I will update
>below
>> changes along with your suggestions and resend the patches.
>
>no. Actually I don't think we're allowed to split a scatter/gather like
>that. I did that quite a while ago, but I don't think we're allowed to
>do so. What we should do, in that case, is not even queue that request
>until we have enough for all members of the scatter/gather. But that's a
>separate patch, anyway.
>

Okay. I have a doubt here, not pushing the request until all sgs are mapped to enough TRBs
might remove the driver complexity but reduce the performance (since we are waiting
until enough TRBs are available). Are we okay with that?  

Thanks,
Anurag Kumar Vulisha