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

[Felipe Balbi]

Hi,

Anurag Kumar Vulisha <anurag.kumar.vulisha@xxxxxxxxxx> writes:
> This patch fixes two issues
>
> 1. The code logic in dwc3_prepare_one_trb() incorrectly uses the address
> and length given in req packet even for scattergather lists. This patch
> correct's the code to use sg->address and sg->length when scattergather
> lists are present.
>
> 2. The present code correctly fetches the req's which were not queued from
> the started_list but fails to start from the sg where it previously stopped
> queuing because of the unavailable TRB's. This patch correct's the code to
> start queuing from the correct sg in sglist.

these two should be in separate patches, then.

> Signed-off-by: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>
> ---
>  drivers/usb/dwc3/core.h   |  4 ++++
>  drivers/usb/dwc3/gadget.c | 42 ++++++++++++++++++++++++++++++++++++------
>  2 files changed, 40 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 860d2bc..2779e58 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -718,7 +718,9 @@ struct dwc3_hwparams {
>   * @list: a list_head used for request queueing
>   * @dep: struct dwc3_ep owning this request
>   * @sg: pointer to first incomplete sg
> + * @sg_to_start: pointer to the sg which should be queued next
>   * @num_pending_sgs: counter to pending sgs
> + * @num_queued_sgs: counter to the number of sgs which already got queued

this is the same as num_pending_sgs.

>   * @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: signature.asc
Description: PGP signature