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
