| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <MWHPR02MB2816D32AE891F329232DA730A7D40@MWHPR02MB2816.namprd02.prod.outlook.com>
Date: Mon, 19 Mar 2018 14:12:59 +0000
From: Anurag Kumar Vulisha <anuragku@...inx.com>
To: Felipe Balbi <balbi@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC: "v.anuragkumar@...il.com" <v.anuragkumar@...il.com>,
"Ajay Yugalkishore Pandey" <APANDEY@...inx.com>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] usb: dwc3: gadget: Correct the logic for queuing sgs
Hi Felipe,
Thanks for reviewing the patch , please find my comments inline
>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@...nel.org]
>Sent: Monday, March 19, 2018 2:21 PM
>To: Anurag Kumar Vulisha <anuragku@...inx.com>; Greg Kroah-Hartman
><gregkh@...uxfoundation.org>
>Cc: v.anuragkumar@...il.com; Ajay Yugalkishore Pandey
><APANDEY@...inx.com>; linux-usb@...r.kernel.org; linux-
>kernel@...r.kernel.org; Anurag Kumar Vulisha <anuragku@...inx.com>
>Subject: Re: [PATCH] usb: dwc3: gadget: Correct the logic for queuing sgs
>
>
>Hi,
>
>Anurag Kumar Vulisha <anurag.kumar.vulisha@...inx.com> 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@...inx.com>
>> ---
>> 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
Powered by blists - more mailing lists