diff mbox

[18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize

Message ID 20141023134903.GB13573@saruman
State New
Headers show

Commit Message

Felipe Balbi Oct. 23, 2014, 1:49 p.m. UTC
Hi,

On Thu, Oct 23, 2014 at 08:48:08AM -0500, Felipe Balbi wrote:
> On Wed, Oct 22, 2014 at 10:14:54AM -0400, Alan Stern wrote:
> > On Wed, 22 Oct 2014, Anton Tikhomirov wrote:
> > 
> > > > That's right, and it's true for USB-2 as well.  A ZLP is needed only in
> > > > cases where the host otherwise wouldn't know the transfer is over,
> > > > i.e., when the transfer length is a nonzero multiple of the maxpacket
> > > > size and is smaller than wLength.
> > > 
> > > Shall we use/check struct usb_request's zero flag for this?
> > 
> > Of course; we have to.  There's no other way for the UDC driver to know 
> > whether the transfer is shorter than the host expects.
> 
> alright, so I take it this incremental diff is enough ?
> 
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 0a34e71..ce6b0c7 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -830,7 +830,8 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
>  	} else {
>  		dwc3_gadget_giveback(ep0, r, 0);
>  
> -		if (IS_ALIGNED(ur->length, ep0->endpoint.maxpacket)) {
> +		if (IS_ALIGNED(ur->length, ep0->endpoint.maxpacket) &&
> +				ur->zero) {
>  			int ret;
>  
>  			dwc->ep0_next_event = DWC3_EP0_COMPLETE;
> 

here's v2:

8<--------------------------------------------------------------

From 1080b54d66e3e77410b41732e76746ed8e2c01c7 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <balbi@ti.com>
Date: Tue, 30 Sep 2014 10:39:14 -0500
Subject: [PATCH] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to
 wMaxPacketSize

According to Section 8.5.3.2 of the USB 2.0 specification,
a USB device must terminate a Data Phase with either a
short packet or a ZLP (if the previous transfer was
a multiple of wMaxPacketSize).

For reference, here's what the USB 2.0 specification, section
8.5.3.2 says:

"
8.5.3.2 Variable-length Data Stage

A control pipe may have a variable-length data phase
in which the host requests more data than is contained
in the specified data structure. When all of the data
structure is returned to the host, the function should
indicate that the Data stage is ended by returning a
packet that is shorter than the MaxPacketSize for the
pipe. If the data structure is an exact multiple of
wMaxPacketSize for the pipe, the function will return
a zero-length packet to indicate the end of the Data
stage.
"

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/usb/dwc3/ep0.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

Comments

Alan Stern Oct. 23, 2014, 2:18 p.m. UTC | #1
On Thu, 23 Oct 2014, Felipe Balbi wrote:

> here's v2:
> 
> 8<--------------------------------------------------------------
> 
> From 1080b54d66e3e77410b41732e76746ed8e2c01c7 Mon Sep 17 00:00:00 2001
> From: Felipe Balbi <balbi@ti.com>
> Date: Tue, 30 Sep 2014 10:39:14 -0500
> Subject: [PATCH] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to
>  wMaxPacketSize
> 
> According to Section 8.5.3.2 of the USB 2.0 specification,
> a USB device must terminate a Data Phase with either a
> short packet or a ZLP (if the previous transfer was
> a multiple of wMaxPacketSize).
> 
> For reference, here's what the USB 2.0 specification, section
> 8.5.3.2 says:
> 
> "
> 8.5.3.2 Variable-length Data Stage
> 
> A control pipe may have a variable-length data phase
> in which the host requests more data than is contained
> in the specified data structure. When all of the data
> structure is returned to the host, the function should
> indicate that the Data stage is ended by returning a
> packet that is shorter than the MaxPacketSize for the
> pipe. If the data structure is an exact multiple of
> wMaxPacketSize for the pipe, the function will return
> a zero-length packet to indicate the end of the Data
> stage.
> "
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  drivers/usb/dwc3/ep0.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index a47cc1e..ce6b0c7 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -828,12 +828,19 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
>  
>  		dwc3_ep0_stall_and_restart(dwc);
>  	} else {
> -		/*
> -		 * handle the case where we have to send a zero packet. This
> -		 * seems to be case when req.length > maxpacket. Could it be?
> -		 */
> -		if (r)
> -			dwc3_gadget_giveback(ep0, r, 0);
> +		dwc3_gadget_giveback(ep0, r, 0);

Don't you want to wait until the ZLP has completed before doing the 
giveback?

In fact, shouldn't all this ZLP code run when the transfer is 
submitted, rather than when it completes?  The idea is that you get a 
request, you queue up all the necessary TRBs or whatever, and if an 
extra ZLP is needed then you queue up an extra TRB.

> +
> +		if (IS_ALIGNED(ur->length, ep0->endpoint.maxpacket) &&
> +				ur->zero) {

Is this correct in the case where ur->length is 0?  When that happens,
there should be only one ZLP, not two.

> +			int ret;
> +
> +			dwc->ep0_next_event = DWC3_EP0_COMPLETE;
> +
> +			ret = dwc3_ep0_start_trans(dwc, epnum,
> +					dwc->ctrl_req_addr, 0,
> +					DWC3_TRBCTL_CONTROL_DATA);
> +			WARN_ON(ret < 0);
> +		}
>  	}
>  }

Alan Stern

--
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
Anton Tikhomirov Oct. 24, 2014, 1:14 a.m. UTC | #2
Hi,

On Thu, 23 Oct 2014, Alan Stern wrote:
> On Thu, 23 Oct 2014, Felipe Balbi wrote:
> 
> > here's v2:
> >
> > 8<--------------------------------------------------------------
> >
> > From 1080b54d66e3e77410b41732e76746ed8e2c01c7 Mon Sep 17 00:00:00
> 2001
> > From: Felipe Balbi <balbi@ti.com>
> > Date: Tue, 30 Sep 2014 10:39:14 -0500
> > Subject: [PATCH] usb: dwc3: ep0: fix Data Phase for transfer sizes
> aligned to
> >  wMaxPacketSize
> >
> > According to Section 8.5.3.2 of the USB 2.0 specification,
> > a USB device must terminate a Data Phase with either a
> > short packet or a ZLP (if the previous transfer was
> > a multiple of wMaxPacketSize).
> >
> > For reference, here's what the USB 2.0 specification, section
> > 8.5.3.2 says:
> >
> > "
> > 8.5.3.2 Variable-length Data Stage
> >
> > A control pipe may have a variable-length data phase
> > in which the host requests more data than is contained
> > in the specified data structure. When all of the data
> > structure is returned to the host, the function should
> > indicate that the Data stage is ended by returning a
> > packet that is shorter than the MaxPacketSize for the
> > pipe. If the data structure is an exact multiple of
> > wMaxPacketSize for the pipe, the function will return
> > a zero-length packet to indicate the end of the Data
> > stage.
> > "
> >
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> >  drivers/usb/dwc3/ep0.c | 19 +++++++++++++------
> >  1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> > index a47cc1e..ce6b0c7 100644
> > --- a/drivers/usb/dwc3/ep0.c
> > +++ b/drivers/usb/dwc3/ep0.c
> > @@ -828,12 +828,19 @@ static void dwc3_ep0_complete_data(struct dwc3
> *dwc,
> >
> >  		dwc3_ep0_stall_and_restart(dwc);
> >  	} else {
> > -		/*
> > -		 * handle the case where we have to send a zero packet.
> This
> > -		 * seems to be case when req.length > maxpacket. Could it
> be?
> > -		 */
> > -		if (r)
> > -			dwc3_gadget_giveback(ep0, r, 0);
> > +		dwc3_gadget_giveback(ep0, r, 0);
> 
> Don't you want to wait until the ZLP has completed before doing the
> giveback?
> 
> In fact, shouldn't all this ZLP code run when the transfer is
> submitted, rather than when it completes?  The idea is that you get a
> request, you queue up all the necessary TRBs or whatever, and if an
> extra ZLP is needed then you queue up an extra TRB.

You may want to check my patch one more time. I sent it for review 10
monthes ago:

[PATCH] usb: dwc3: ep0: Handle variable-length Data Stage

It works just fine for us in our custom kernel.

> 
> > +
> > +		if (IS_ALIGNED(ur->length, ep0->endpoint.maxpacket) &&
> > +				ur->zero) {
> 
> Is this correct in the case where ur->length is 0?  When that happens,
> there should be only one ZLP, not two.
> 
> > +			int ret;
> > +
> > +			dwc->ep0_next_event = DWC3_EP0_COMPLETE;
> > +
> > +			ret = dwc3_ep0_start_trans(dwc, epnum,
> > +					dwc->ctrl_req_addr, 0,
> > +					DWC3_TRBCTL_CONTROL_DATA);
> > +			WARN_ON(ret < 0);
> > +		}
> >  	}
> >  }
> 
> Alan Stern

--
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
Felipe Balbi Oct. 24, 2014, 3:18 a.m. UTC | #3
HI,

On Fri, Oct 24, 2014 at 10:14:19AM +0900, Anton Tikhomirov wrote:
> 
> Hi,
> 
> On Thu, 23 Oct 2014, Alan Stern wrote:
> > On Thu, 23 Oct 2014, Felipe Balbi wrote:
> > 
> > > here's v2:
> > >
> > > 8<--------------------------------------------------------------
> > >
> > > From 1080b54d66e3e77410b41732e76746ed8e2c01c7 Mon Sep 17 00:00:00
> > 2001
> > > From: Felipe Balbi <balbi@ti.com>
> > > Date: Tue, 30 Sep 2014 10:39:14 -0500
> > > Subject: [PATCH] usb: dwc3: ep0: fix Data Phase for transfer sizes
> > aligned to
> > >  wMaxPacketSize
> > >
> > > According to Section 8.5.3.2 of the USB 2.0 specification,
> > > a USB device must terminate a Data Phase with either a
> > > short packet or a ZLP (if the previous transfer was
> > > a multiple of wMaxPacketSize).
> > >
> > > For reference, here's what the USB 2.0 specification, section
> > > 8.5.3.2 says:
> > >
> > > "
> > > 8.5.3.2 Variable-length Data Stage
> > >
> > > A control pipe may have a variable-length data phase
> > > in which the host requests more data than is contained
> > > in the specified data structure. When all of the data
> > > structure is returned to the host, the function should
> > > indicate that the Data stage is ended by returning a
> > > packet that is shorter than the MaxPacketSize for the
> > > pipe. If the data structure is an exact multiple of
> > > wMaxPacketSize for the pipe, the function will return
> > > a zero-length packet to indicate the end of the Data
> > > stage.
> > > "
> > >
> > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > ---
> > >  drivers/usb/dwc3/ep0.c | 19 +++++++++++++------
> > >  1 file changed, 13 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> > > index a47cc1e..ce6b0c7 100644
> > > --- a/drivers/usb/dwc3/ep0.c
> > > +++ b/drivers/usb/dwc3/ep0.c
> > > @@ -828,12 +828,19 @@ static void dwc3_ep0_complete_data(struct dwc3
> > *dwc,
> > >
> > >  		dwc3_ep0_stall_and_restart(dwc);
> > >  	} else {
> > > -		/*
> > > -		 * handle the case where we have to send a zero packet.
> > This
> > > -		 * seems to be case when req.length > maxpacket. Could it
> > be?
> > > -		 */
> > > -		if (r)
> > > -			dwc3_gadget_giveback(ep0, r, 0);
> > > +		dwc3_gadget_giveback(ep0, r, 0);
> > 
> > Don't you want to wait until the ZLP has completed before doing the
> > giveback?
> > 
> > In fact, shouldn't all this ZLP code run when the transfer is
> > submitted, rather than when it completes?  The idea is that you get a
> > request, you queue up all the necessary TRBs or whatever, and if an
> > extra ZLP is needed then you queue up an extra TRB.
> 
> You may want to check my patch one more time. I sent it for review 10
> monthes ago:
> 
> [PATCH] usb: dwc3: ep0: Handle variable-length Data Stage
> 
> It works just fine for us in our custom kernel.

you also said you'd send another version (see [1]) and never did. 10
months passed and I had even forgotten about this issue until I decided
to run all gadget drivers through USB[23]0CV and found that g_ncm falls
into this same bug, so I wrote the patch above.

considering you never sent another version even after 10 months, I'll
just go ahead with this one and work on improving TRB handling on this
driver so that when req->zero is true we can actually allocate a
separate TRB (as has been discussed on this and previous threads).

I'll add a note to commit log stating that you provided the original
patch but failed to provide a follow up.

[1] http://www.spinics.net/lists/linux-usb/msg99809.html
Felipe Balbi Oct. 24, 2014, 3:21 a.m. UTC | #4
On Thu, Oct 23, 2014 at 10:18:41PM -0500, Felipe Balbi wrote:
> HI,
> 
> On Fri, Oct 24, 2014 at 10:14:19AM +0900, Anton Tikhomirov wrote:
> > 
> > Hi,
> > 
> > On Thu, 23 Oct 2014, Alan Stern wrote:
> > > On Thu, 23 Oct 2014, Felipe Balbi wrote:
> > > 
> > > > here's v2:
> > > >
> > > > 8<--------------------------------------------------------------
> > > >
> > > > From 1080b54d66e3e77410b41732e76746ed8e2c01c7 Mon Sep 17 00:00:00
> > > 2001
> > > > From: Felipe Balbi <balbi@ti.com>
> > > > Date: Tue, 30 Sep 2014 10:39:14 -0500
> > > > Subject: [PATCH] usb: dwc3: ep0: fix Data Phase for transfer sizes
> > > aligned to
> > > >  wMaxPacketSize
> > > >
> > > > According to Section 8.5.3.2 of the USB 2.0 specification,
> > > > a USB device must terminate a Data Phase with either a
> > > > short packet or a ZLP (if the previous transfer was
> > > > a multiple of wMaxPacketSize).
> > > >
> > > > For reference, here's what the USB 2.0 specification, section
> > > > 8.5.3.2 says:
> > > >
> > > > "
> > > > 8.5.3.2 Variable-length Data Stage
> > > >
> > > > A control pipe may have a variable-length data phase
> > > > in which the host requests more data than is contained
> > > > in the specified data structure. When all of the data
> > > > structure is returned to the host, the function should
> > > > indicate that the Data stage is ended by returning a
> > > > packet that is shorter than the MaxPacketSize for the
> > > > pipe. If the data structure is an exact multiple of
> > > > wMaxPacketSize for the pipe, the function will return
> > > > a zero-length packet to indicate the end of the Data
> > > > stage.
> > > > "
> > > >
> > > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > > ---
> > > >  drivers/usb/dwc3/ep0.c | 19 +++++++++++++------
> > > >  1 file changed, 13 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> > > > index a47cc1e..ce6b0c7 100644
> > > > --- a/drivers/usb/dwc3/ep0.c
> > > > +++ b/drivers/usb/dwc3/ep0.c
> > > > @@ -828,12 +828,19 @@ static void dwc3_ep0_complete_data(struct dwc3
> > > *dwc,
> > > >
> > > >  		dwc3_ep0_stall_and_restart(dwc);
> > > >  	} else {
> > > > -		/*
> > > > -		 * handle the case where we have to send a zero packet.
> > > This
> > > > -		 * seems to be case when req.length > maxpacket. Could it
> > > be?
> > > > -		 */
> > > > -		if (r)
> > > > -			dwc3_gadget_giveback(ep0, r, 0);
> > > > +		dwc3_gadget_giveback(ep0, r, 0);
> > > 
> > > Don't you want to wait until the ZLP has completed before doing the
> > > giveback?
> > > 
> > > In fact, shouldn't all this ZLP code run when the transfer is
> > > submitted, rather than when it completes?  The idea is that you get a
> > > request, you queue up all the necessary TRBs or whatever, and if an
> > > extra ZLP is needed then you queue up an extra TRB.
> > 
> > You may want to check my patch one more time. I sent it for review 10
> > monthes ago:
> > 
> > [PATCH] usb: dwc3: ep0: Handle variable-length Data Stage
> > 
> > It works just fine for us in our custom kernel.
> 
> you also said you'd send another version (see [1]) and never did. 10
> months passed and I had even forgotten about this issue until I decided
> to run all gadget drivers through USB[23]0CV and found that g_ncm falls
> into this same bug, so I wrote the patch above.
> 
> considering you never sent another version even after 10 months, I'll
> just go ahead with this one and work on improving TRB handling on this
> driver so that when req->zero is true we can actually allocate a
> separate TRB (as has been discussed on this and previous threads).
> 
> I'll add a note to commit log stating that you provided the original
> patch but failed to provide a follow up.

actually, I can't do that anymore as I have already moved this commit to
my 'fixes' branch.
Anton Tikhomirov Oct. 24, 2014, 3:50 a.m. UTC | #5
Hi,

> On Thu, Oct 23, 2014 at 10:18:41PM -0500, Felipe Balbi wrote:
> > HI,
> >
> > On Fri, Oct 24, 2014 at 10:14:19AM +0900, Anton Tikhomirov wrote:
> > >
> > > Hi,
> > >
> > > On Thu, 23 Oct 2014, Alan Stern wrote:
> > > > On Thu, 23 Oct 2014, Felipe Balbi wrote:
> > > >
> > > > > here's v2:
> > > > >
> > > > > 8<-------------------------------------------------------------
> -
> > > > >
> > > > > From 1080b54d66e3e77410b41732e76746ed8e2c01c7 Mon Sep 17
> 00:00:00
> > > > 2001
> > > > > From: Felipe Balbi <balbi@ti.com>
> > > > > Date: Tue, 30 Sep 2014 10:39:14 -0500
> > > > > Subject: [PATCH] usb: dwc3: ep0: fix Data Phase for transfer
> sizes
> > > > aligned to
> > > > >  wMaxPacketSize
> > > > >
> > > > > According to Section 8.5.3.2 of the USB 2.0 specification,
> > > > > a USB device must terminate a Data Phase with either a
> > > > > short packet or a ZLP (if the previous transfer was
> > > > > a multiple of wMaxPacketSize).
> > > > >
> > > > > For reference, here's what the USB 2.0 specification, section
> > > > > 8.5.3.2 says:
> > > > >
> > > > > "
> > > > > 8.5.3.2 Variable-length Data Stage
> > > > >
> > > > > A control pipe may have a variable-length data phase
> > > > > in which the host requests more data than is contained
> > > > > in the specified data structure. When all of the data
> > > > > structure is returned to the host, the function should
> > > > > indicate that the Data stage is ended by returning a
> > > > > packet that is shorter than the MaxPacketSize for the
> > > > > pipe. If the data structure is an exact multiple of
> > > > > wMaxPacketSize for the pipe, the function will return
> > > > > a zero-length packet to indicate the end of the Data
> > > > > stage.
> > > > > "
> > > > >
> > > > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > > > ---
> > > > >  drivers/usb/dwc3/ep0.c | 19 +++++++++++++------
> > > > >  1 file changed, 13 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> > > > > index a47cc1e..ce6b0c7 100644
> > > > > --- a/drivers/usb/dwc3/ep0.c
> > > > > +++ b/drivers/usb/dwc3/ep0.c
> > > > > @@ -828,12 +828,19 @@ static void dwc3_ep0_complete_data(struct
> dwc3
> > > > *dwc,
> > > > >
> > > > >  		dwc3_ep0_stall_and_restart(dwc);
> > > > >  	} else {
> > > > > -		/*
> > > > > -		 * handle the case where we have to send a zero
> packet.
> > > > This
> > > > > -		 * seems to be case when req.length > maxpacket.
> Could it
> > > > be?
> > > > > -		 */
> > > > > -		if (r)
> > > > > -			dwc3_gadget_giveback(ep0, r, 0);
> > > > > +		dwc3_gadget_giveback(ep0, r, 0);
> > > >
> > > > Don't you want to wait until the ZLP has completed before doing
> the
> > > > giveback?
> > > >
> > > > In fact, shouldn't all this ZLP code run when the transfer is
> > > > submitted, rather than when it completes?  The idea is that you
> get a
> > > > request, you queue up all the necessary TRBs or whatever, and if
> an
> > > > extra ZLP is needed then you queue up an extra TRB.
> > >
> > > You may want to check my patch one more time. I sent it for review
> 10
> > > monthes ago:
> > >
> > > [PATCH] usb: dwc3: ep0: Handle variable-length Data Stage
> > >
> > > It works just fine for us in our custom kernel.
> >
> > you also said you'd send another version (see [1]) and never did. 10
> > months passed and I had even forgotten about this issue until I
> decided
> > to run all gadget drivers through USB[23]0CV and found that g_ncm
> falls
> > into this same bug, so I wrote the patch above.

I'm sorry about this. I was busy with current project at that time and
finally forgot about this issue too.

> >
> > considering you never sent another version even after 10 months, I'll
> > just go ahead with this one and work on improving TRB handling on
> this
> > driver so that when req->zero is true we can actually allocate a
> > separate TRB (as has been discussed on this and previous threads).
> >
> > I'll add a note to commit log stating that you provided the original
> > patch but failed to provide a follow up.
> 
> actually, I can't do that anymore as I have already moved this commit
> to
> my 'fixes' branch.

It's ok

> 
> --
> 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
Felipe Balbi Oct. 24, 2014, 3:10 p.m. UTC | #6
Hi,

On Fri, Oct 24, 2014 at 12:50:44PM +0900, Anton Tikhomirov wrote:
> > > > > >  		dwc3_ep0_stall_and_restart(dwc);
> > > > > >  	} else {
> > > > > > -		/*
> > > > > > -		 * handle the case where we have to send a zero
> > packet.
> > > > > This
> > > > > > -		 * seems to be case when req.length > maxpacket.
> > Could it
> > > > > be?
> > > > > > -		 */
> > > > > > -		if (r)
> > > > > > -			dwc3_gadget_giveback(ep0, r, 0);
> > > > > > +		dwc3_gadget_giveback(ep0, r, 0);
> > > > >
> > > > > Don't you want to wait until the ZLP has completed before doing
> > the
> > > > > giveback?
> > > > >
> > > > > In fact, shouldn't all this ZLP code run when the transfer is
> > > > > submitted, rather than when it completes?  The idea is that you
> > get a
> > > > > request, you queue up all the necessary TRBs or whatever, and if
> > an
> > > > > extra ZLP is needed then you queue up an extra TRB.
> > > >
> > > > You may want to check my patch one more time. I sent it for review
> > 10
> > > > monthes ago:
> > > >
> > > > [PATCH] usb: dwc3: ep0: Handle variable-length Data Stage
> > > >
> > > > It works just fine for us in our custom kernel.
> > >
> > > you also said you'd send another version (see [1]) and never did. 10
> > > months passed and I had even forgotten about this issue until I
> > decided
> > > to run all gadget drivers through USB[23]0CV and found that g_ncm
> > falls
> > > into this same bug, so I wrote the patch above.
> 
> I'm sorry about this. I was busy with current project at that time and
> finally forgot about this issue too.

I also apologize that I completely forgot about your patch, I should've
at least mentioned your work. Anyway, I'll send Greg a pull request
today. Finally finished testing everything on top of v3.18-rc1.
diff mbox

Patch

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index a47cc1e..ce6b0c7 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -828,12 +828,19 @@  static void dwc3_ep0_complete_data(struct dwc3 *dwc,
 
 		dwc3_ep0_stall_and_restart(dwc);
 	} else {
-		/*
-		 * handle the case where we have to send a zero packet. This
-		 * seems to be case when req.length > maxpacket. Could it be?
-		 */
-		if (r)
-			dwc3_gadget_giveback(ep0, r, 0);
+		dwc3_gadget_giveback(ep0, r, 0);
+
+		if (IS_ALIGNED(ur->length, ep0->endpoint.maxpacket) &&
+				ur->zero) {
+			int ret;
+
+			dwc->ep0_next_event = DWC3_EP0_COMPLETE;
+
+			ret = dwc3_ep0_start_trans(dwc, epnum,
+					dwc->ctrl_req_addr, 0,
+					DWC3_TRBCTL_CONTROL_DATA);
+			WARN_ON(ret < 0);
+		}
 	}
 }