RE: [PATCH] usb: dwc3: gadget: Correct the logic for queuing sgs

[Anurag Kumar Vulisha]

Hi Felipe,

Thanks for providing your inputs on this patch. Will send v2 with all your
suggestions added.

Thanks,
Anurag Kumar Vulisha

>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@xxxxxxxxxx]
>Sent: Friday, March 23, 2018 4:59 PM
>To: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>; Greg Kroah-Hartman
><gregkh@xxxxxxxxxxxxxxxxxxx>
>Cc: v.anuragkumar@xxxxxxxxx; Ajay Yugalkishore Pandey
><APANDEY@xxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx
>Subject: RE: [PATCH] usb: dwc3: gadget: Correct the logic for queuing sgs
>
>
>(please configure your email client to break lines at 80 columns ;-)
>
>Hi,
>
>Anurag Kumar Vulisha <anuragku@xxxxxxxxxx> writes:
>> Hi Felipe,
>>
>> Thanks for reviewing the patch , please find my comments inline
>
>no issues :-)
>
>>>Anurag Kumar Vulisha <anurag.kumar.vulisha@xxxxxxxxxx> writes:
>>>> This patch fixes two issues
>>>>
>>>> 1. The code logic in dwc3_prepare_one_trb() incorrectly uses the
>>>> address and length given in req packet even for scattergather lists.
>>>> This patch correct's the code to use sg->address and sg->length when
>>>> scattergather lists are present.
>>>>
>>>> 2. The present code correctly fetches the req's which were not
>>>> queued from the started_list but fails to start from the sg where it
>>>> previously stopped queuing because of the unavailable TRB's. This
>>>> patch correct's the code to start queuing from the correct sg in sglist.
>>>
>>>these two should be in separate patches, then.
>>>
>> Will create separate patches in next version
>
>thanks, that helps :-)
>
>>>> Signed-off-by: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>
>>>> ---
>>>>  drivers/usb/dwc3/core.h   |  4 ++++
>>>>  drivers/usb/dwc3/gadget.c | 42
>>>> ++++++++++++++++++++++++++++++++++++------
>>>>  2 files changed, 40 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index
>>>> 860d2bc..2779e58 100644
>>>> --- a/drivers/usb/dwc3/core.h
>>>> +++ b/drivers/usb/dwc3/core.h
>>>> @@ -718,7 +718,9 @@ struct dwc3_hwparams {
>>>>   * @list: a list_head used for request queueing
>>>>   * @dep: struct dwc3_ep owning this request
>>>>   * @sg: pointer to first incomplete sg
>>>> + * @sg_to_start: pointer to the sg which should be queued next
>>>>   * @num_pending_sgs: counter to pending sgs
>>>> + * @num_queued_sgs: counter to the number of sgs which already got
>>>> + queued
>>>
>>>this is the same as num_pending_sgs.
>>
>> num_pending_sgs is initially pointing to num_mapped_sgs, which gets
>> decremented in dwc3_cleanup_done_reqs().
>>
>> Consider a case where the driver failed to queue all sgs into TRBs
>> because of insufficient TRB number. For example, lets assume req has 5
>> sgs and only 3 got queued. In this scenario ,when the
>> dwc3_cleanup_done_reqs() gets called the value of num_pending_sgs = 5.
>> Since the value of num_pending_sgs is greater than 0,
>> __dwc3_gadget_kick_transfer() gets called with num_pending_sgs = 5-1 =
>> 4.
>>
>> Eventually __dwc3_gadget_kick_transfer() calls
>> dwc3_prepare_one_trb_sg() which has for_each_sg() call which loops for
>> num_pending_sgs times (4 times in our example). This is incorrect,
>> ideally it should be called only for 2 times because we have only 2
>> sgs which previously were not queued.
>>
>> So, we added an extra flag num_queued_sgs which counts the number of
>> sgs that got queued successfully and make for_each_sg() loop for
>> num_mapped_sgs - num_queued_sgs times only . In our example case with
>> the updated logic, it will loop for 5 - 3 = 2 times only. Because of
>> this reason added num_queued_sgs flag. Please correct me if I am
>> wrong.
>
>That's true. Seems like we do need a new counter.
>
>>>> static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep
>>>> *dep,
>>>>
>>>>                 req->request.actual = length - req->remaining;
>>>>
>>>> -               if ((req->request.actual < length) && req->num_pending_sgs)
>>>> -                       return __dwc3_gadget_kick_transfer(dep);
>>>> +               if (req->request.actual < length ||
>>>> + req->num_pending_sgs) {
>>>
>>>why do you think this needs to be || instead of &&? If actual ==
>>>length we're done, there's nothing more left to do. If there is either
>>>host sent more data than it should, or we miscalculated
>>>num_pending_sgs, or we had the wrong length somewhere in some TRBs.
>>>Either of those cases is an error condition we don't want to hide. We want
>things to fail in that case.
>>>
>>
>> Consider the same example that we had previously discussed, among the
>> 5 sgs only 3 sgs got queued and all 3 sgs got completed successfully.
>> The req->remaining field represents the size of TRB which was not
>> transferred. In this example , as 3 sgs got completed successfully the
>> req-> remaining = 0. So , request.actual = length - 0
>> (req->remaining) which means request.actual == length.  Because of
>> this , the condition check if ((req->request.actual < length) &&
>> req->num_pending_sgs) ) fails even though we have req->num_pending_sgs
>> > 0.  So, corrected the logic to always kick transfer for two
>> conditions 1) if req->num_pending_sgs > 0 2) if ((req->request.actual
>> < length) && req->num_pending_sgs)) condition satisfies. Please
>> correct me If my understanding is wrong.
>
>fair enough, but then we miss an important (IMO, that is) error case. If
>req->num_pending_sgs > 0 && actual == length, then something is super
>wrong. We may just add it as an extra check:
>
>	dev_WARN_ONCE(.... actual == len && num_pending_sgs);
>
>--
>balbi