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