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

[Anurag Kumar Vulisha]

Hi Felipe,

Thanks for reviewing the patch , please find my comments inline

>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@xxxxxxxxxx]
>Sent: Monday, March 19, 2018 2:21 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; Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>
>Subject: Re: [PATCH] usb: dwc3: gadget: Correct the logic for queuing sgs
>
>
>Hi,
>
>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

>> 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.

>
>>   * @remaining: amount of data remaining
>>   * @epnum: endpoint number to which this request refers
>>   * @trb: pointer to struct dwc3_trb
>> @@ -734,8 +736,10 @@ struct dwc3_request {
>>         struct list_head        list;
>>         struct dwc3_ep          *dep;
>>         struct scatterlist      *sg;
>> +       struct scatterlist      *sg_to_start;
>
>indeed, we seem to need something like this.
>
>>         unsigned                num_pending_sgs;
>> +       unsigned int            num_queued_sgs;
>
>this should be unnecessary.

The explanation given above is valid for this comment also. Please refer the explanation given above

>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 2bda4eb..1cffed5 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -978,11 +978,20 @@ static void dwc3_prepare_one_trb(struct dwc3_ep
>*dep,
>>                 struct dwc3_request *req, unsigned chain, unsigned
>> node)  {
>>         struct dwc3_trb         *trb;
>> -       unsigned                length = req->request.length;
>> +       unsigned int            length;
>> +       dma_addr_t              dma;
>>         unsigned                stream_id = req->request.stream_id;
>>         unsigned                short_not_ok = req->request.short_not_ok;
>>         unsigned                no_interrupt = req->request.no_interrupt;
>> -       dma_addr_t              dma = req->request.dma;
>> +
>> +       if (req->request.num_sgs > 0) {
>> +               /* Use scattergather list address and length */
>
>unnecessary comment

Will remove the comment

>
>> +               length = sg_dma_len(req->sg_to_start);
>> +               dma = sg_dma_address(req->sg_to_start);
>> +       } else {
>> +               length = req->request.length;
>> +               dma = req->request.dma;
>> +       }
>>
>>         trb = &dep->trb_pool[dep->trb_enqueue];
>>
>> @@ -1048,11 +1057,14 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep
>> *dep)  static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
>>                 struct dwc3_request *req)  {
>> -       struct scatterlist *sg = req->sg;
>> +       struct scatterlist *sg = req->sg_to_start;
>>         struct scatterlist *s;
>>         int             i;
>>
>> -       for_each_sg(sg, s, req->num_pending_sgs, i) {
>> +       unsigned int remaining = req->request.num_mapped_sgs
>> +               - req->num_queued_sgs;
>
>already tracked as num_pending_sgs

The explanation given above is valid for this comment also. Please refer the above mentioned explanation. 
>
>> +       for_each_sg(sg, s, remaining, i) {
>>                 unsigned int length = req->request.length;
>>                 unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
>>                 unsigned int rem = length % maxp; @@ -1081,6 +1093,16
>> @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
>>                         dwc3_prepare_one_trb(dep, req, chain, i);
>>                 }
>>
>> +               /* In the case where not able to queue trbs for all
>> + sgs in
>
>wrong comment style
>

Will fix this in next version of patch

>> +                * request because of trb not available, update sg_to_start
>> +                * to next sg from which we can start queing trbs once trbs
>> +                * availbale
>                     ^^^^^^^^^
>                     available
>
>sg_to_start is too long and awkward to type. Sure, I'm nitpicking, but start_sg
>would be better.

I agree with you, start_sg looks better. Will fix this in next series of patch
 
>
>> +                */
>> +               if (chain)
>> +                       req->sg_to_start = sg_next(s);
>> +
>> +               req->num_queued_sgs++;
>> +
>>                 if (!dwc3_calc_trbs_left(dep))
>>                         break;
>>         }
>> @@ -1171,6 +1193,8 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
>>                         return;
>>
>>                 req->sg                 = req->request.sg;
>> +               req->sg_to_start        = req->sg;
>> +               req->num_queued_sgs     = 0;
>
>num_queued_sgs is unnecessary.

As said in previous explanation, we use num_queued_sgs to  correctly maintain the incomplete sgs. So, clearing it to 0.

>
>>                 req->num_pending_sgs    = req->request.num_mapped_sgs;
>>
>>                 if (req->num_pending_sgs > 0) @@ -2327,8 +2351,14 @@
>> 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.

>> +                       /* There could be cases where the whole req
>> + can't be
>
>wrong comment style.
>
Will correct this in next series of patches

>> +                        * mapped into TRB's available. In that case, we need
>> +                        * to kick transfer again if (req->num_pending_sgs > 0)
>> +                        */
>
>also, the code has already been trying to do that. It just wasn't correct. We don't
>need to add this comment.

Added the comment to describe what is happening here. Please let me know if any more information needs to be added or deleted.

>
>> +                       if (req->num_pending_sgs)
>> +                               return
>> + __dwc3_gadget_kick_transfer(dep);
>
>another num_pending_sgs check? Why?

As explained in the previous comment, changed the logic to make it work for two conditions 1) if req->num_pending_sgs > 0  2) if ((req->request.actual < length) && req->num_pending_sgs)) . 

>
>> This email and any attachments are intended for the sole use of the
>> named recipient(s) and contain(s) confidential information that may be
>> proprietary, privileged or copyrighted under applicable law. If you
>> are not the intended recipient, do not read, copy, or forward this
>> email message or any attachments. Delete this email message and any
>> attachments immediately.
>
>I can't accept ANY patches from you until you remove this footer. In fact, I'm not
>in the To field, so I'm not a "named recipient" and therefore, I'm deleting your
>email. Talk to your IT department about contributing to public mailing lists.
>
Sorry for the inconvenience caused. Unknowingly , this message got added. I will ensure that this message won't appear in future emails.

Thanks,
Anurag Kumar Vulisha

>Email, now, deleted.
>
>--
>balbi