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

[Felipe Balbi]

(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

Attachment: signature.asc
Description: PGP signature