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.
Signed-off-by: Anurag Kumar Vulisha <[email protected]>
---
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
* @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;
unsigned num_pending_sgs;
+ unsigned int num_queued_sgs;
unsigned remaining;
u8 epnum;
struct dwc3_trb *trb;
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 */
+ 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;
+
+ 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
+ * request because of trb not available, update sg_to_start
+ * to next sg from which we can start queing trbs once trbs
+ * availbale
+ */
+ 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;
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) {
+ /* There could be cases where the whole req can't be
+ * mapped into TRB's available. In that case, we need
+ * to kick transfer again if (req->num_pending_sgs > 0)
+ */
+ if (req->num_pending_sgs)
+ return __dwc3_gadget_kick_transfer(dep);
+ }
dwc3_gadget_giveback(dep, req, status);
--
2.1.1
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.
Hi,
Anurag Kumar Vulisha <[email protected]> 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.
> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
> ---
> 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.
> * @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.
> 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
> + 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
> + 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
> + * 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.
> + */
> + 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.
> 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.
> + /* There could be cases where the whole req can't be
wrong comment style.
> + * 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.
> + if (req->num_pending_sgs)
> + return __dwc3_gadget_kick_transfer(dep);
another num_pending_sgs check? Why?
> 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.
Email, now, deleted.
--
balbi
Hi Felipe,
Thanks for reviewing the patch , please find my comments inline
>-----Original Message-----
>From: Felipe Balbi [mailto:[email protected]]
>Sent: Monday, March 19, 2018 2:21 PM
>To: Anurag Kumar Vulisha <[email protected]>; Greg Kroah-Hartman
><[email protected]>
>Cc: [email protected]; Ajay Yugalkishore Pandey
><[email protected]>; [email protected]; linux-
>[email protected]; Anurag Kumar Vulisha <[email protected]>
>Subject: Re: [PATCH] usb: dwc3: gadget: Correct the logic for queuing sgs
>
>
>Hi,
>
>Anurag Kumar Vulisha <[email protected]> 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 <[email protected]>
>> ---
>> 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
(please configure your email client to break lines at 80 columns ;-)
Hi,
Anurag Kumar Vulisha <[email protected]> writes:
> Hi Felipe,
>
> Thanks for reviewing the patch , please find my comments inline
no issues :-)
>>Anurag Kumar Vulisha <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
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:[email protected]]
>Sent: Friday, March 23, 2018 4:59 PM
>To: Anurag Kumar Vulisha <[email protected]>; Greg Kroah-Hartman
><[email protected]>
>Cc: [email protected]; Ajay Yugalkishore Pandey
><[email protected]>; [email protected]; linux-
>[email protected]
>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 <[email protected]> writes:
>> Hi Felipe,
>>
>> Thanks for reviewing the patch , please find my comments inline
>
>no issues :-)
>
>>>Anurag Kumar Vulisha <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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