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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
