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

Message ID 1413565804-13061-19-git-send-email-balbi@ti.com
State New
Headers show

Commit Message

Felipe Balbi Oct. 17, 2014, 5:09 p.m.
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 | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

David Laight Oct. 20, 2014, 9:48 a.m. | #1
From: Felipe Balbi
> 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.
> "

If that is the same as my understanding of the USB3 spec then the
requirement for a ZLP isn't unconditional.

In particular if the data phase isn't variable length then a ZLP
must not be added.

	David
 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  drivers/usb/dwc3/ep0.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index a47cc1e..0a34e71 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -828,12 +828,18 @@ 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)) {
> +			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);
> +		}
>  	}
>  }
> 
> --
> 2.1.0.GIT
> 
> --
> 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


--
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
Paul Zimmerman Oct. 20, 2014, 7:18 p.m. | #2
> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of David Laight
> Sent: Monday, October 20, 2014 2:48 AM
> 
> From: Felipe Balbi
> > 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.
> > "
> 
> If that is the same as my understanding of the USB3 spec then the
> requirement for a ZLP isn't unconditional.
> 
> In particular if the data phase isn't variable length then a ZLP
> must not be added.

Also, in the USB 3.0 spec, the corresponding section has been modified
a bit. The last sentence has been changed to this:

"Note that if the amount of data in the data structure that is returned
to the host *is less than the amount requested by the host* and is an
exact multiple of maximum packet size then a control endpoint shall send
a zero length DP to terminate the data stage." (emphasis mine)

So I think you also need to take into account the wLength field of the
request, and only send the ZLP if the amount of data returned is less
than wLength.
Alan Stern Oct. 20, 2014, 7:52 p.m. | #3
On Mon, 20 Oct 2014, Paul Zimmerman wrote:

> > From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of David Laight
> > Sent: Monday, October 20, 2014 2:48 AM
> > 
> > From: Felipe Balbi
> > > 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.
> > > "
> > 
> > If that is the same as my understanding of the USB3 spec then the
> > requirement for a ZLP isn't unconditional.
> > 
> > In particular if the data phase isn't variable length then a ZLP
> > must not be added.
> 
> Also, in the USB 3.0 spec, the corresponding section has been modified
> a bit. The last sentence has been changed to this:
> 
> "Note that if the amount of data in the data structure that is returned
> to the host *is less than the amount requested by the host* and is an
> exact multiple of maximum packet size then a control endpoint shall send
> a zero length DP to terminate the data stage." (emphasis mine)
> 
> So I think you also need to take into account the wLength field of the
> request, and only send the ZLP if the amount of data returned is less
> than wLength.

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.

It's not clear what a "variable length" control transfer is supposed to 
be.  I guess it means that sometimes the device will send back less 
than wLength bytes of data.

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
David Laight Oct. 21, 2014, 9:03 a.m. | #4
From: Alan Stern
> > > From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of David Laight
> > > Sent: Monday, October 20, 2014 2:48 AM
> > >
> > > From: Felipe Balbi
> > > > 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.
> > > > "
> > >
> > > If that is the same as my understanding of the USB3 spec then the
> > > requirement for a ZLP isn't unconditional.
> > >
> > > In particular if the data phase isn't variable length then a ZLP
> > > must not be added.
> >
> > Also, in the USB 3.0 spec, the corresponding section has been modified
> > a bit. The last sentence has been changed to this:
> >
> > "Note that if the amount of data in the data structure that is returned
> > to the host *is less than the amount requested by the host* and is an
> > exact multiple of maximum packet size then a control endpoint shall send
> > a zero length DP to terminate the data stage." (emphasis mine)
> >
> > So I think you also need to take into account the wLength field of the
> > request, and only send the ZLP if the amount of data returned is less
> > than wLength.
> 
> 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.
> 
> It's not clear what a "variable length" control transfer is supposed to
> be.  I guess it means that sometimes the device will send back less
> than wLength bytes of data.

I take it as being one where the amount of data returned isn't known
by the host when it performs the 'read' request.

So the response to a 'disk read' request is known and a ZLP isn't required
(even though the transfer request is likely to be a multiple of the packet size).

The length that matters is that of the buffer(s) supplied by the receiving driver.
From the USB driver's point of view, only the 'application' (another driver)
knows whether the next receive message is 'variable length', so the onus
has to be on the 'application' sending the data to say whether it needs a ZLP.

Has anyone fixed xhci to send ZLP yet ?

	David



--
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. 22, 2014, 5:50 a.m. | #5
Hi,

> On Mon, 20 Oct 2014, Paul Zimmerman wrote:
> 
> > > From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-
> owner@vger.kernel.org] On Behalf Of David Laight
> > > Sent: Monday, October 20, 2014 2:48 AM
> > >
> > > From: Felipe Balbi
> > > > 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.
> > > > "
> > >
> > > If that is the same as my understanding of the USB3 spec then the
> > > requirement for a ZLP isn't unconditional.
> > >
> > > In particular if the data phase isn't variable length then a ZLP
> > > must not be added.
> >
> > Also, in the USB 3.0 spec, the corresponding section has been
> modified
> > a bit. The last sentence has been changed to this:
> >
> > "Note that if the amount of data in the data structure that is
> returned
> > to the host *is less than the amount requested by the host* and is an
> > exact multiple of maximum packet size then a control endpoint shall
> send
> > a zero length DP to terminate the data stage." (emphasis mine)
> >
> > So I think you also need to take into account the wLength field of
> the
> > request, and only send the ZLP if the amount of data returned is less
> > than wLength.
> 
> 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?

> 
> It's not clear what a "variable length" control transfer is supposed to
> be.  I guess it means that sometimes the device will send back less
> than wLength bytes of data.
> 
> 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

--
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
Alan Stern Oct. 22, 2014, 2:14 p.m. | #6
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.

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

Patch

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index a47cc1e..0a34e71 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -828,12 +828,18 @@  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)) {
+			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);
+		}
 	}
 }