Re: [PATCH 18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize
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.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize
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;
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize
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 ba...@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 ba...@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);
+
+ 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);
+ }
}
}
--
2.1.0.GIT
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize
On Thu, 23 Oct 2014, Felipe Balbi wrote:
here's v2:
8--
From 1080b54d66e3e77410b41732e76746ed8e2c01c7 Mon Sep 17 00:00:00 2001
From: Felipe Balbi ba...@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 ba...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize
Hi,
On Thu, Oct 23, 2014 at 10:18:27AM -0400, 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 ba...@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 ba...@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.
yeah, this is desirable but it would require a bigger rework of how we
handle TRBs in dwc3. Currently, for ep0, we have a single TRB and adding
another wouldn't fit into an -rc release. I'll see what I can do to make
this better but as of now we need this bug fix.
+
+ 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.
and here I was assuming IS_ALIGNED() would handle that case. Here's v3.
8--
From 36f84ffb45c75ff10442a9f8f2fd91dcf6c6f5dd Mon Sep 17 00:00:00 2001
From: Felipe Balbi ba...@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 ba...@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..711b230 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-length ur-zero) {
+ int ret;
+
+
RE: [PATCH 18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize
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 ba...@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 ba...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize
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 ba...@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 ba...@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
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize
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 ba...@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 ba...@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.
--
balbi
signature.asc
Description: Digital signature
RE: [PATCH 18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize
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 ba...@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 ba...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize
From: Alan Stern From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-ow...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize
Hi, On Mon, 20 Oct 2014, Paul Zimmerman wrote: From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb- ow...@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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize
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 ba...@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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize
From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-ow...@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. -- Paul -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 18/28] usb: dwc3: ep0: fix Data Phase for transfer sizes aligned to wMaxPacketSize
On Mon, 20 Oct 2014, Paul Zimmerman wrote: From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-ow...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 18/28] 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 ba...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
