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

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

[Anurag Kumar Vulisha]

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 <anuragku@xilinx.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
  * @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.

usb: dwc3: gadget: Correct the logic for queuing sgs

[Anurag Kumar Vulisha]

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 <anuragku@xilinx.com>
---
 drivers/usb/dwc3/core.h   |  4 ++++
 drivers/usb/dwc3/gadget.c | 42 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 40 insertions(+), 6 deletions(-)

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

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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 7482 bytes --]


Hi,

Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.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.

> Signed-off-by: Anurag Kumar Vulisha <anuragku@xilinx.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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

usb: dwc3: gadget: Correct the logic for queuing sgs

[Felipe Balbi]

Hi,

Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.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.

> Signed-off-by: Anurag Kumar Vulisha <anuragku@xilinx.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.

>   * @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.

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

[Anurag Kumar Vulisha]

Hi Felipe,

Thanks for reviewing the patch , please find my comments inline

>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@kernel.org]
>Sent: Monday, March 19, 2018 2:21 PM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>
>Cc: v.anuragkumar@gmail.com; Ajay Yugalkishore Pandey
><APANDEY@xilinx.com>; linux-usb@vger.kernel.org; linux-
>kernel@vger.kernel.org; Anurag Kumar Vulisha <anuragku@xilinx.com>
>Subject: Re: [PATCH] usb: dwc3: gadget: Correct the logic for queuing sgs
>
>
>Hi,
>
>Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.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@xilinx.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

usb: dwc3: gadget: Correct the logic for queuing sgs

[Anurag Kumar Vulisha]

Hi Felipe,

Thanks for reviewing the patch , please find my comments inline

>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@kernel.org]
>Sent: Monday, March 19, 2018 2:21 PM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>
>Cc: v.anuragkumar@gmail.com; Ajay Yugalkishore Pandey
><APANDEY@xilinx.com>; linux-usb@vger.kernel.org; linux-
>kernel@vger.kernel.org; Anurag Kumar Vulisha <anuragku@xilinx.com>
>Subject: Re: [PATCH] usb: dwc3: gadget: Correct the logic for queuing sgs
>
>
>Hi,
>
>Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.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@xilinx.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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 5031 bytes --]


(please configure your email client to break lines at 80 columns ;-)

Hi,

Anurag Kumar Vulisha <anuragku@xilinx.com> writes:
> Hi Felipe,
>
> Thanks for reviewing the patch , please find my comments inline

no issues :-)

>>Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.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

thanks, that helps :-)

>>> Signed-off-by: Anurag Kumar Vulisha <anuragku@xilinx.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.

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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@xilinx.com> writes:
> Hi Felipe,
>
> Thanks for reviewing the patch , please find my comments inline

no issues :-)

>>Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.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

thanks, that helps :-)

>>> Signed-off-by: Anurag Kumar Vulisha <anuragku@xilinx.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.

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

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

[Anurag Kumar Vulisha]

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@kernel.org]
>Sent: Friday, March 23, 2018 4:59 PM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>
>Cc: v.anuragkumar@gmail.com; Ajay Yugalkishore Pandey
><APANDEY@xilinx.com>; linux-usb@vger.kernel.org; linux-
>kernel@vger.kernel.org
>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@xilinx.com> writes:
>> Hi Felipe,
>>
>> Thanks for reviewing the patch , please find my comments inline
>
>no issues :-)
>
>>>Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.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
>
>thanks, that helps :-)
>
>>>> Signed-off-by: Anurag Kumar Vulisha <anuragku@xilinx.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.
>
>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

usb: dwc3: gadget: Correct the logic for queuing sgs

[Anurag Kumar Vulisha]

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@kernel.org]
>Sent: Friday, March 23, 2018 4:59 PM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>
>Cc: v.anuragkumar@gmail.com; Ajay Yugalkishore Pandey
><APANDEY@xilinx.com>; linux-usb@vger.kernel.org; linux-
>kernel@vger.kernel.org
>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@xilinx.com> writes:
>> Hi Felipe,
>>
>> Thanks for reviewing the patch , please find my comments inline
>
>no issues :-)
>
>>>Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.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
>
>thanks, that helps :-)
>
>>>> Signed-off-by: Anurag Kumar Vulisha <anuragku@xilinx.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.
>
>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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html