[PATCH v3] usb: dwc3: gadget: trb_dequeue is not updated properly

[PATCH v3] usb: dwc3: gadget: trb_dequeue is not updated properly

[fei.yang]

From: Fei Yang <fei.yang@intel.com>

If scatter-gather operation is allowed, a large USB request is split into
multiple TRBs. These TRBs are chained up by setting DWC3_TRB_CTRL_CHN bit
except the last one which has DWC3_TRB_CTRL_IOC bit set instead.
Since only the last TRB has IOC set for the whole USB request, the
dwc3_gadget_ep_reclaim_trb_sg() gets called only once after the last TRB
completes and all the TRBs allocated for this request are supposed to be
reclaimed. However that is not what the current code does.

dwc3_gadget_ep_reclaim_trb_sg() is trying to reclaim all the TRBs in the
following for-loop,
	for_each_sg(sg, s, pending, i) {
		trb = &dep->trb_pool[dep->trb_dequeue];

                if (trb->ctrl & DWC3_TRB_CTRL_HWO)
                        break;

                req->sg = sg_next(s);
                req->num_pending_sgs--;

                ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
                                trb, event, status, chain);
                if (ret)
                        break;
        }
but since the interrupt comes only after the last TRB completes, the
event->status has DEPEVT_STATUS_IOC bit set, so that the for-loop ends for
the first TRB due to dwc3_gadget_ep_reclaim_completed_trb() returns 1.
	if (event->status & DEPEVT_STATUS_IOC)
		return 1;

This patch addresses the issue by checking each TRB in function
dwc3_gadget_ep_reclaim_trb_sg() and maing sure the chained ones are properly
reclaimed. dwc3_gadget_ep_reclaim_completed_trb() will return 1 Only for the
last TRB.

Signed-off-by: Fei Yang <fei.yang@intel.com>
Cc: stable <stable@vger.kernel.org>
---
v2: Better solution is to reclaim chained TRBs in dwc3_gadget_ep_reclaim_trb_sg()
    and leave the last TRB to the dwc3_gadget_ep_reclaim_completed_trb().
v3: Checking DWC3_TRB_CTRL_CHN bit for each TRB instead, and making sure that
    dwc3_gadget_ep_reclaim_completed_trb() returns 1 only for the last TRB.
---
 drivers/usb/dwc3/gadget.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 173f532..88eed49 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2394,7 +2394,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
 	if (event->status & DEPEVT_STATUS_SHORT && !chain)
 		return 1;
 
-	if (event->status & DEPEVT_STATUS_IOC)
+	if (event->status & DEPEVT_STATUS_IOC && !chain)
 		return 1;
 
 	return 0;
@@ -2404,11 +2404,12 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
 		struct dwc3_request *req, const struct dwc3_event_depevt *event,
 		int status)
 {
-	struct dwc3_trb *trb = &dep->trb_pool[dep->trb_dequeue];
+	struct dwc3_trb *trb;
 	struct scatterlist *sg = req->sg;
 	struct scatterlist *s;
 	unsigned int pending = req->num_pending_sgs;
 	unsigned int i;
+	int chain = false;
 	int ret = 0;
 
 	for_each_sg(sg, s, pending, i) {
@@ -2419,9 +2420,13 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
 
 		req->sg = sg_next(s);
 		req->num_pending_sgs--;
+		if (trb->ctrl & DWC3_TRB_CTRL_CHN)
+			chain = true;
+		else
+			chain = false;
 
 		ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
-				trb, event, status, true);
+				trb, event, status, chain);
 		if (ret)
 			break;
 	}
-- 
2.7.4

Re: [PATCH v3] usb: dwc3: gadget: trb_dequeue is not updated properly

[Thinh Nguyen]

Hi,

fei.yang@intel.com wrote:
> From: Fei Yang <fei.yang@intel.com>
>
> If scatter-gather operation is allowed, a large USB request is split into
> multiple TRBs. These TRBs are chained up by setting DWC3_TRB_CTRL_CHN bit
> except the last one which has DWC3_TRB_CTRL_IOC bit set instead.
> Since only the last TRB has IOC set for the whole USB request, the
> dwc3_gadget_ep_reclaim_trb_sg() gets called only once after the last TRB
> completes and all the TRBs allocated for this request are supposed to be
> reclaimed. However that is not what the current code does.
>
> dwc3_gadget_ep_reclaim_trb_sg() is trying to reclaim all the TRBs in the
> following for-loop,
> 	for_each_sg(sg, s, pending, i) {
> 		trb = &dep->trb_pool[dep->trb_dequeue];
>
>                 if (trb->ctrl & DWC3_TRB_CTRL_HWO)
>                         break;
>
>                 req->sg = sg_next(s);
>                 req->num_pending_sgs--;
>
>                 ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
>                                 trb, event, status, chain);
>                 if (ret)
>                         break;
>         }
> but since the interrupt comes only after the last TRB completes, the
> event->status has DEPEVT_STATUS_IOC bit set, so that the for-loop ends for
> the first TRB due to dwc3_gadget_ep_reclaim_completed_trb() returns 1.
> 	if (event->status & DEPEVT_STATUS_IOC)
> 		return 1;
>
> This patch addresses the issue by checking each TRB in function
> dwc3_gadget_ep_reclaim_trb_sg() and maing sure the chained ones are properly
> reclaimed. dwc3_gadget_ep_reclaim_completed_trb() will return 1 Only for the
> last TRB.
>
> Signed-off-by: Fei Yang <fei.yang@intel.com>
> Cc: stable <stable@vger.kernel.org>
> ---
> v2: Better solution is to reclaim chained TRBs in dwc3_gadget_ep_reclaim_trb_sg()
>     and leave the last TRB to the dwc3_gadget_ep_reclaim_completed_trb().
> v3: Checking DWC3_TRB_CTRL_CHN bit for each TRB instead, and making sure that
>     dwc3_gadget_ep_reclaim_completed_trb() returns 1 only for the last TRB.
> ---
>  drivers/usb/dwc3/gadget.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 173f532..88eed49 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2394,7 +2394,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>  	if (event->status & DEPEVT_STATUS_SHORT && !chain)
>  		return 1;
>  
> -	if (event->status & DEPEVT_STATUS_IOC)
> +	if (event->status & DEPEVT_STATUS_IOC && !chain)
>  		return 1;
>  
>  	return 0;
> @@ -2404,11 +2404,12 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>  		struct dwc3_request *req, const struct dwc3_event_depevt *event,
>  		int status)
>  {
> -	struct dwc3_trb *trb = &dep->trb_pool[dep->trb_dequeue];
> +	struct dwc3_trb *trb;
>  	struct scatterlist *sg = req->sg;
>  	struct scatterlist *s;
>  	unsigned int pending = req->num_pending_sgs;
>  	unsigned int i;
> +	int chain = false;
>  	int ret = 0;
>  
>  	for_each_sg(sg, s, pending, i) {
> @@ -2419,9 +2420,13 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>  
>  		req->sg = sg_next(s);
>  		req->num_pending_sgs--;
> +		if (trb->ctrl & DWC3_TRB_CTRL_CHN)
> +			chain = true;
> +		else
> +			chain = false;
>  
>  		ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
> -				trb, event, status, true);
> +				trb, event, status, chain);
>  		if (ret)
>  			break;
>  	}

There was already a fix a long time ago by Anurag. But it never made it
to the kernel mainline. You can check this out:
https://patchwork.kernel.org/patch/10640137/

Hi Felipe,

Maybe you can review and cherry-pick that patch?

Thanks,
Thinh

Re: [PATCH v3] usb: dwc3: gadget: trb_dequeue is not updated properly

[John Stultz]

On Thu, Jul 18, 2019 at 6:12 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> fei.yang@intel.com wrote:
> > From: Fei Yang <fei.yang@intel.com>
> >
> > If scatter-gather operation is allowed, a large USB request is split into
> > multiple TRBs. These TRBs are chained up by setting DWC3_TRB_CTRL_CHN bit
> > except the last one which has DWC3_TRB_CTRL_IOC bit set instead.
> > Since only the last TRB has IOC set for the whole USB request, the
> > dwc3_gadget_ep_reclaim_trb_sg() gets called only once after the last TRB
> > completes and all the TRBs allocated for this request are supposed to be
> > reclaimed. However that is not what the current code does.
> >
> > dwc3_gadget_ep_reclaim_trb_sg() is trying to reclaim all the TRBs in the
> > following for-loop,
> >       for_each_sg(sg, s, pending, i) {
> >               trb = &dep->trb_pool[dep->trb_dequeue];
> >
> >                 if (trb->ctrl & DWC3_TRB_CTRL_HWO)
> >                         break;
> >
> >                 req->sg = sg_next(s);
> >                 req->num_pending_sgs--;
> >
> >                 ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
> >                                 trb, event, status, chain);
> >                 if (ret)
> >                         break;
> >         }
> > but since the interrupt comes only after the last TRB completes, the
> > event->status has DEPEVT_STATUS_IOC bit set, so that the for-loop ends for
> > the first TRB due to dwc3_gadget_ep_reclaim_completed_trb() returns 1.
> >       if (event->status & DEPEVT_STATUS_IOC)
> >               return 1;
> >
> > This patch addresses the issue by checking each TRB in function
> > dwc3_gadget_ep_reclaim_trb_sg() and maing sure the chained ones are properly
> > reclaimed. dwc3_gadget_ep_reclaim_completed_trb() will return 1 Only for the
> > last TRB.
> >
> > Signed-off-by: Fei Yang <fei.yang@intel.com>
> > Cc: stable <stable@vger.kernel.org>
> > ---
> > v2: Better solution is to reclaim chained TRBs in dwc3_gadget_ep_reclaim_trb_sg()
> >     and leave the last TRB to the dwc3_gadget_ep_reclaim_completed_trb().
> > v3: Checking DWC3_TRB_CTRL_CHN bit for each TRB instead, and making sure that
> >     dwc3_gadget_ep_reclaim_completed_trb() returns 1 only for the last TRB.
> > ---
> >  drivers/usb/dwc3/gadget.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 173f532..88eed49 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -2394,7 +2394,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
> >       if (event->status & DEPEVT_STATUS_SHORT && !chain)
> >               return 1;
> >
> > -     if (event->status & DEPEVT_STATUS_IOC)
> > +     if (event->status & DEPEVT_STATUS_IOC && !chain)
> >               return 1;
> >
> >       return 0;
> > @@ -2404,11 +2404,12 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
> >               struct dwc3_request *req, const struct dwc3_event_depevt *event,
> >               int status)
> >  {
> > -     struct dwc3_trb *trb = &dep->trb_pool[dep->trb_dequeue];
> > +     struct dwc3_trb *trb;
> >       struct scatterlist *sg = req->sg;
> >       struct scatterlist *s;
> >       unsigned int pending = req->num_pending_sgs;
> >       unsigned int i;
> > +     int chain = false;
> >       int ret = 0;
> >
> >       for_each_sg(sg, s, pending, i) {
> > @@ -2419,9 +2420,13 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
> >
> >               req->sg = sg_next(s);
> >               req->num_pending_sgs--;
> > +             if (trb->ctrl & DWC3_TRB_CTRL_CHN)
> > +                     chain = true;
> > +             else
> > +                     chain = false;
> >
> >               ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
> > -                             trb, event, status, true);
> > +                             trb, event, status, chain);
> >               if (ret)
> >                       break;
> >       }
>
> There was already a fix a long time ago by Anurag. But it never made it
> to the kernel mainline. You can check this out:
> https://patchwork.kernel.org/patch/10640137/

So, back from a vacation last week, and just validated that both Fei's
patch and a forward ported version of this patch Thinh pointed out
both seem to resolve the usb stalls I've been seeing sinice 4.20 w/
dwc3 hardware on both hikey960 and dragonboard 845c.

Felipe: Does Anurag's patch above make more sense as a proper fix?

thanks
-john

Re: [PATCH v3] usb: dwc3: gadget: trb_dequeue is not updated properly

[Felipe Balbi]

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


Hi,

John Stultz <john.stultz@linaro.org> writes:
> On Thu, Jul 18, 2019 at 6:12 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>> fei.yang@intel.com wrote:
>> > From: Fei Yang <fei.yang@intel.com>
>> >
>> > If scatter-gather operation is allowed, a large USB request is split into
>> > multiple TRBs. These TRBs are chained up by setting DWC3_TRB_CTRL_CHN bit
>> > except the last one which has DWC3_TRB_CTRL_IOC bit set instead.
>> > Since only the last TRB has IOC set for the whole USB request, the
>> > dwc3_gadget_ep_reclaim_trb_sg() gets called only once after the last TRB
>> > completes and all the TRBs allocated for this request are supposed to be
>> > reclaimed. However that is not what the current code does.
>> >
>> > dwc3_gadget_ep_reclaim_trb_sg() is trying to reclaim all the TRBs in the
>> > following for-loop,
>> >       for_each_sg(sg, s, pending, i) {
>> >               trb = &dep->trb_pool[dep->trb_dequeue];
>> >
>> >                 if (trb->ctrl & DWC3_TRB_CTRL_HWO)
>> >                         break;
>> >
>> >                 req->sg = sg_next(s);
>> >                 req->num_pending_sgs--;
>> >
>> >                 ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
>> >                                 trb, event, status, chain);
>> >                 if (ret)
>> >                         break;
>> >         }
>> > but since the interrupt comes only after the last TRB completes, the
>> > event->status has DEPEVT_STATUS_IOC bit set, so that the for-loop ends for
>> > the first TRB due to dwc3_gadget_ep_reclaim_completed_trb() returns 1.
>> >       if (event->status & DEPEVT_STATUS_IOC)
>> >               return 1;
>> >
>> > This patch addresses the issue by checking each TRB in function
>> > dwc3_gadget_ep_reclaim_trb_sg() and maing sure the chained ones are properly
>> > reclaimed. dwc3_gadget_ep_reclaim_completed_trb() will return 1 Only for the
>> > last TRB.
>> >
>> > Signed-off-by: Fei Yang <fei.yang@intel.com>
>> > Cc: stable <stable@vger.kernel.org>
>> > ---
>> > v2: Better solution is to reclaim chained TRBs in dwc3_gadget_ep_reclaim_trb_sg()
>> >     and leave the last TRB to the dwc3_gadget_ep_reclaim_completed_trb().
>> > v3: Checking DWC3_TRB_CTRL_CHN bit for each TRB instead, and making sure that
>> >     dwc3_gadget_ep_reclaim_completed_trb() returns 1 only for the last TRB.
>> > ---
>> >  drivers/usb/dwc3/gadget.c | 11 ++++++++---
>> >  1 file changed, 8 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> > index 173f532..88eed49 100644
>> > --- a/drivers/usb/dwc3/gadget.c
>> > +++ b/drivers/usb/dwc3/gadget.c
>> > @@ -2394,7 +2394,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>> >       if (event->status & DEPEVT_STATUS_SHORT && !chain)
>> >               return 1;
>> >
>> > -     if (event->status & DEPEVT_STATUS_IOC)
>> > +     if (event->status & DEPEVT_STATUS_IOC && !chain)
>> >               return 1;
>> >
>> >       return 0;
>> > @@ -2404,11 +2404,12 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>> >               struct dwc3_request *req, const struct dwc3_event_depevt *event,
>> >               int status)
>> >  {
>> > -     struct dwc3_trb *trb = &dep->trb_pool[dep->trb_dequeue];
>> > +     struct dwc3_trb *trb;
>> >       struct scatterlist *sg = req->sg;
>> >       struct scatterlist *s;
>> >       unsigned int pending = req->num_pending_sgs;
>> >       unsigned int i;
>> > +     int chain = false;
>> >       int ret = 0;
>> >
>> >       for_each_sg(sg, s, pending, i) {
>> > @@ -2419,9 +2420,13 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>> >
>> >               req->sg = sg_next(s);
>> >               req->num_pending_sgs--;
>> > +             if (trb->ctrl & DWC3_TRB_CTRL_CHN)
>> > +                     chain = true;
>> > +             else
>> > +                     chain = false;
>> >
>> >               ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
>> > -                             trb, event, status, true);
>> > +                             trb, event, status, chain);
>> >               if (ret)
>> >                       break;
>> >       }
>>
>> There was already a fix a long time ago by Anurag. But it never made it
>> to the kernel mainline. You can check this out:
>> https://patchwork.kernel.org/patch/10640137/
>
> So, back from a vacation last week, and just validated that both Fei's
> patch and a forward ported version of this patch Thinh pointed out
> both seem to resolve the usb stalls I've been seeing sinice 4.20 w/
> dwc3 hardware on both hikey960 and dragonboard 845c.
>
> Felipe: Does Anurag's patch above make more sense as a proper fix?

I think it's enough to check only the TRB. We won't get events for bits
we didn't enable on the TRB. The only problem here is when we get IOC
event for multiple TRBs where only the last one has IOC.

So, instead of checking:

	if (event->status & IOC && trb->ctrl & IOC)

It's probably enough to check:

	if (tbc->ctrl & IOC)

Could you check that?

Cheers

-- 
balbi

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

[PATCH] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields

[John Stultz]

From: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>

The present code in dwc3_gadget_ep_reclaim_completed_trb() will check
for IOC/LST bit in the event->status and returns if IOC/LST bit is
set. This logic doesn't work if multiple TRBs are queued per
request and the IOC/LST bit is set on the last TRB of that request.
Consider an example where a queued request has multiple queued TRBs
and IOC/LST bit is set only for the last TRB. In this case, the Core
generates XferComplete/XferInProgress events only for the last TRB
(since IOC/LST are set only for the last TRB). As per the logic in
dwc3_gadget_ep_reclaim_completed_trb() event->status is checked for
IOC/LST bit and returns on the first TRB. This makes the remaining
TRBs left unhandled.
To aviod this, changed the code to check for IOC/LST bits in both
event->status & TRB->ctrl. This patch does the same.

At a practical level, this patch resolves USB transfer stalls seen
with adb on dwc3 based Android devices after functionfs gadget
added scatter-gather support around v4.20.

Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Fei Yang <fei.yang@intel.com>
Cc: Thinh Nguyen <thinhn@synopsys.com>
Cc: Tejas Joglekar <tejas.joglekar@synopsys.com>
Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Linux USB List <linux-usb@vger.kernel.org>
Cc: stable <stable@vger.kernel.org>
Tested-By: Tejas Joglekar <tejas.joglekar@synopsys.com>
Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
[jstultz: forward ported to mainline, added note to commit log]
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
Just wanted to send this out so we're all looking at the same thing.
Not sure if its correct, but it seems to solve the adb stalls I've
been seeing for awhile.

 thanks
 -john

 drivers/usb/dwc3/gadget.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index c9cecb3a9670..1d9701dde69b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2394,7 +2394,12 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
 	if (event->status & DEPEVT_STATUS_SHORT && !chain)
 		return 1;
 
-	if (event->status & DEPEVT_STATUS_IOC)
+	if ((event->status & DEPEVT_STATUS_IOC) &&
+	    (trb->ctrl & DWC3_TRB_CTRL_IOC))
+		return 1;
+
+	if ((event->status & DEPEVT_STATUS_LST) &&
+	    (trb->ctrl & DWC3_TRB_CTRL_LST))
 		return 1;
 
 	return 0;
-- 
2.17.1

Re: [PATCH] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields

[John Stultz]

On Tue, Jul 23, 2019 at 1:27 PM John Stultz <john.stultz@linaro.org> wrote:
>
> From: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
>
> The present code in dwc3_gadget_ep_reclaim_completed_trb() will check
> for IOC/LST bit in the event->status and returns if IOC/LST bit is
> set. This logic doesn't work if multiple TRBs are queued per
> request and the IOC/LST bit is set on the last TRB of that request.
> Consider an example where a queued request has multiple queued TRBs
> and IOC/LST bit is set only for the last TRB. In this case, the Core
> generates XferComplete/XferInProgress events only for the last TRB
> (since IOC/LST are set only for the last TRB). As per the logic in
> dwc3_gadget_ep_reclaim_completed_trb() event->status is checked for
> IOC/LST bit and returns on the first TRB. This makes the remaining
> TRBs left unhandled.
> To aviod this, changed the code to check for IOC/LST bits in both
> event->status & TRB->ctrl. This patch does the same.
>
> At a practical level, this patch resolves USB transfer stalls seen
> with adb on dwc3 based Android devices after functionfs gadget
> added scatter-gather support around v4.20.
>
> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
> Cc: Fei Yang <fei.yang@intel.com>
> Cc: Thinh Nguyen <thinhn@synopsys.com>
> Cc: Tejas Joglekar <tejas.joglekar@synopsys.com>
> Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Cc: Linux USB List <linux-usb@vger.kernel.org>
> Cc: stable <stable@vger.kernel.org>
> Tested-By: Tejas Joglekar <tejas.joglekar@synopsys.com>
> Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> [jstultz: forward ported to mainline, added note to commit log]
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> Just wanted to send this out so we're all looking at the same thing.
> Not sure if its correct, but it seems to solve the adb stalls I've
> been seeing for awhile.

Felipe: Any thoughts on this patch?

thanks
-john

Re: [PATCH v3] usb: dwc3: gadget: trb_dequeue is not updated properly

[Felipe Balbi]

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


Hi,

fei.yang@intel.com writes:
> From: Fei Yang <fei.yang@intel.com>
>
> If scatter-gather operation is allowed, a large USB request is split into
> multiple TRBs. These TRBs are chained up by setting DWC3_TRB_CTRL_CHN bit
> except the last one which has DWC3_TRB_CTRL_IOC bit set instead.
> Since only the last TRB has IOC set for the whole USB request, the
> dwc3_gadget_ep_reclaim_trb_sg() gets called only once after the last TRB
> completes and all the TRBs allocated for this request are supposed to be
> reclaimed. However that is not what the current code does.
>
> dwc3_gadget_ep_reclaim_trb_sg() is trying to reclaim all the TRBs in the
> following for-loop,
> 	for_each_sg(sg, s, pending, i) {
> 		trb = &dep->trb_pool[dep->trb_dequeue];
>
>                 if (trb->ctrl & DWC3_TRB_CTRL_HWO)
>                         break;
>
>                 req->sg = sg_next(s);
>                 req->num_pending_sgs--;
>
>                 ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
>                                 trb, event, status, chain);
>                 if (ret)
>                         break;
>         }
> but since the interrupt comes only after the last TRB completes, the
> event->status has DEPEVT_STATUS_IOC bit set, so that the for-loop ends for
> the first TRB due to dwc3_gadget_ep_reclaim_completed_trb() returns 1.
> 	if (event->status & DEPEVT_STATUS_IOC)
> 		return 1;
>
> This patch addresses the issue by checking each TRB in function
> dwc3_gadget_ep_reclaim_trb_sg() and maing sure the chained ones are properly
> reclaimed. dwc3_gadget_ep_reclaim_completed_trb() will return 1 Only for the
> last TRB.
>
> Signed-off-by: Fei Yang <fei.yang@intel.com>
> Cc: stable <stable@vger.kernel.org>
> ---
> v2: Better solution is to reclaim chained TRBs in dwc3_gadget_ep_reclaim_trb_sg()
>     and leave the last TRB to the dwc3_gadget_ep_reclaim_completed_trb().
> v3: Checking DWC3_TRB_CTRL_CHN bit for each TRB instead, and making sure that
>     dwc3_gadget_ep_reclaim_completed_trb() returns 1 only for the last TRB.
> ---
>  drivers/usb/dwc3/gadget.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 173f532..88eed49 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2394,7 +2394,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>  	if (event->status & DEPEVT_STATUS_SHORT && !chain)
>  		return 1;
>  
> -	if (event->status & DEPEVT_STATUS_IOC)
> +	if (event->status & DEPEVT_STATUS_IOC && !chain)
>  		return 1;

This will break the situation when we have more SGs than available
TRBs. In that case we set IOC before the last so we have time to update
transfer to append more TRBs.

Please, send me tracepoints

> @@ -2404,11 +2404,12 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>  		struct dwc3_request *req, const struct dwc3_event_depevt *event,
>  		int status)
>  {
> -	struct dwc3_trb *trb = &dep->trb_pool[dep->trb_dequeue];
> +	struct dwc3_trb *trb;

should be part of another patch. This is a cleanup that has nothing to
do with this fix.

>  	struct scatterlist *sg = req->sg;
>  	struct scatterlist *s;
>  	unsigned int pending = req->num_pending_sgs;
>  	unsigned int i;
> +	int chain = false;

this could be defined inside for_each_sg() loop like this:

	int chain = trb->ctrl & DWC3_TRB_CTRL_CHN;

> @@ -2419,9 +2420,13 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>  
>  		req->sg = sg_next(s);
>  		req->num_pending_sgs--;
> +		if (trb->ctrl & DWC3_TRB_CTRL_CHN)
> +			chain = true;
> +		else
> +			chain = false;
>  
>  		ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
> -				trb, event, status, true);
> +				trb, event, status, chain);

this is definitely a valid fix :-) I'm not convinced about that IOC &&
!chain above, however. Also, if "chain" is always trb->ctrl &
DWC3_TRB_CTRL_CHN, we can get rid of that argument altogether and have
the callee handle it internally, but that's something else, subject to
another patch.

-- 
balbi

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

RE: [PATCH v3] usb: dwc3: gadget: trb_dequeue is not updated properly

[Yang, Fei]

>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c 
>> index 173f532..88eed49 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2394,7 +2394,7 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>>  	if (event->status & DEPEVT_STATUS_SHORT && !chain)
>>  		return 1;
>>  
>> -	if (event->status & DEPEVT_STATUS_IOC)
>> +	if (event->status & DEPEVT_STATUS_IOC && !chain)
>>  		return 1;
>
> This will break the situation when we have more SGs than available TRBs. In that case we set IOC before the last so we have time to update transfer to append more TRBs.
What's your opinion on https://patchwork.kernel.org/patch/10640137/? Checking condition "(event->status & DEPEVT_STATUS_IOC) && (trb->ctrl & DWC3_TRB_CTRL_IOC)"
won't cause problem handling TRB shortage cases, right?

> Please, send me tracepoints
I sent you the tracepoints last Friday, any new findings?