fastboot: Fix OUT transaction length alignment

PATCH New Semen Protsenko 5 participants 14 messages Apr 13, 2016 – Apr 19, 2016 USB

lore

Share & tools

lore

Semen Protsenko

author

2016-04-13 12:01 UTC #0 lore raw

PATCH 1/1 fastboot: Fix OUT transaction length alignment

From: Sam Protsenko <[email protected]>

Some UDC controllers may require buffer size to be aligned to wMaxPacketSize. It's indicated by gadget->quirk_ep_out_aligned_size field being set to "true" (in UDC driver code). In that case rx_bytes_expected must be aligned to wMaxPacket size, otherwise stuck on transaction will happen. For example, it's required by DWC3 controller data manual:

section 8.2.3.3 Buffer Size Rules and Zero-Length Packets:

For OUT endpoints, the following rules apply:
- The BUFSIZ field must be ? 1 byte.
- The total size of a Buffer Descriptor must be a multiple of MaxPacketSize
- A received zero-length packet still requires a MaxPacketSize buffer. Therefore, if the expected amount of data to be received is a multiple of MaxPacketSize, software should add MaxPacketSize bytes to the buffer to sink a possible zero-length packet at the end of the transfer.

But other UDC controllers don't need such alignment, so mentioned field is set to "false". If buffer size is aligned to wMaxPacketSize, those controllers may stuck on transaction. The example is DWC2.

This patch checks gadget->quirk_ep_out_aligned_size field and aligns rx_bytes_expected to wMaxPacketSize only when it's needed.

Signed-off-by Sam Protsenko <[email protected]>

---
 drivers/usb/gadget/f_fastboot.c | 9 +++++++++
 1 file changed, 9 insertions(+)

drivers/usb/gadget/f_fastboot.c +9 -0

index 2e87fee..54dcce0 100644
@@ -58,6 +58,7 @@ static unsigned int fastboot_flash_session_id;
 static unsigned int download_size;
 static unsigned int download_bytes;
 static bool is_high_speed;
+static bool quirk_ep_out_aligned_size;
 
 static struct usb_endpoint_descriptor fs_ep_in = {
 	.bLength            = USB_DT_ENDPOINT_SIZE,

@@ -240,6 +241,8 @@ static int fastboot_set_alt(struct usb_function *f,
 	debug("%s: func: %s intf: %d alt: %d\n",
 	      __func__, f->name, interface, alt);
 
+	quirk_ep_out_aligned_size = gadget->quirk_ep_out_aligned_size;
+
 	/* make sure we don't enable the ep twice */
 	if (gadget->speed == USB_SPEED_HIGH) {
 		ret = usb_ep_enable(f_fb->out_ep, &hs_ep_out);

@@ -435,12 +438,18 @@ static unsigned int rx_bytes_expected(unsigned int maxpacket)
 		return 0;
 	if (rx_remain > EP_BUFFER_SIZE)
 		return EP_BUFFER_SIZE;
+
+	if (!quirk_ep_out_aligned_size)
+		goto out;
+
 	if (rx_remain < maxpacket) {
 		rx_remain = maxpacket;
 	} else if (rx_remain % maxpacket != 0) {
 		rem = rx_remain % maxpacket;
 		rx_remain = rx_remain + (maxpacket - rem);
 	}
+
+out:
 	return rx_remain;
 }

Signature · 1 line

2.8.0.rc3

Sam Protsenko

author

2016-04-13 12:02 UTC #1 lore raw

PATCH 1/1 fastboot: Fix OUT transaction length alignment

Quoted 73 lines · Semen Protsenko

From: Sam Protsenko <[email protected]>
Some UDC controllers may require buffer size to be aligned to wMaxPacketSize. It's indicated by gadget->quirk_ep_out_aligned_size field being set to "true" (in UDC driver code). In that case rx_bytes_expected must be aligned to wMaxPacket size, otherwise stuck on transaction will happen. For example, it's required by DWC3 controller data manual:
section 8.2.3.3 Buffer Size Rules and Zero-Length Packets:
For OUT endpoints, the following rules apply:
- The BUFSIZ field must be ? 1 byte.
- The total size of a Buffer Descriptor must be a multiple of MaxPacketSize
- A received zero-length packet still requires a MaxPacketSize buffer. Therefore, if the expected amount of data to be received is a multiple of MaxPacketSize, software should add MaxPacketSize bytes to the buffer to sink a possible zero-length packet at the end of the transfer.
But other UDC controllers don't need such alignment, so mentioned field is set to "false". If buffer size is aligned to wMaxPacketSize, those controllers may stuck on transaction. The example is DWC2.
This patch checks gadget->quirk_ep_out_aligned_size field and aligns rx_bytes_expected to wMaxPacketSize only when it's needed.
Signed-off-by Sam Protsenko <[email protected]>
---
 drivers/usb/gadget/f_fastboot.c | 9 +++++++++
 1 file changed, 9 insertions(+)
drivers/usb/gadget/f_fastboot.c +9 -1
index 2e87fee..54dcce0 100644
@@ -58,6 +58,7 @@ static unsigned int fastboot_flash_session_id;
 static unsigned int download_size;
 static unsigned int download_bytes;
 static bool is_high_speed;
+static bool quirk_ep_out_aligned_size;
 
 static struct usb_endpoint_descriptor fs_ep_in = {
        .bLength            = USB_DT_ENDPOINT_SIZE,
@@ -240,6 +241,8 @@ static int fastboot_set_alt(struct usb_function *f,
        debug("%s: func: %s intf: %d alt: %d\n",
              __func__, f->name, interface, alt);
 
+       quirk_ep_out_aligned_size = gadget->quirk_ep_out_aligned_size;
+
        /* make sure we don't enable the ep twice */
        if (gadget->speed == USB_SPEED_HIGH) {
                ret = usb_ep_enable(f_fb->out_ep, &hs_ep_out);
@@ -435,12 +438,18 @@ static unsigned int rx_bytes_expected(unsigned int maxpacket)
                return 0;
        if (rx_remain > EP_BUFFER_SIZE)
                return EP_BUFFER_SIZE;
+
+       if (!quirk_ep_out_aligned_size)
+               goto out;
+
        if (rx_remain < maxpacket) {
                rx_remain = maxpacket;
        } else if (rx_remain % maxpacket != 0) {
                rem = rx_remain % maxpacket;
                rx_remain = rx_remain + (maxpacket - rem);
        }
+
+out:
        return rx_remain;
 }
 
--

Quoted 2 lines

2.8.0.rc3

Steve,

Can you please check if it fixes "fastboot flash" on your boards?

Thanks.

Tom Rini

2016-04-13 12:26 UTC #2 lore raw

PATCH 1/1 fastboot: Fix OUT transaction length alignment

In reply to Semen Protsenko

Quoted 26 lines · Semen Protsenko

From: Sam Protsenko <[email protected]>
Some UDC controllers may require buffer size to be aligned to wMaxPacketSize. It's indicated by gadget->quirk_ep_out_aligned_size field being set to "true" (in UDC driver code). In that case rx_bytes_expected must be aligned to wMaxPacket size, otherwise stuck on transaction will happen. For example, it's required by DWC3 controller data manual:
section 8.2.3.3 Buffer Size Rules and Zero-Length Packets:
For OUT endpoints, the following rules apply:
- The BUFSIZ field must be ? 1 byte.
- The total size of a Buffer Descriptor must be a multiple of MaxPacketSize
- A received zero-length packet still requires a MaxPacketSize buffer. Therefore, if the expected amount of data to be received is a multiple of MaxPacketSize, software should add MaxPacketSize bytes to the buffer to sink a possible zero-length packet at the end of the transfer.
But other UDC controllers don't need such alignment, so mentioned field is set to "false". If buffer size is aligned to wMaxPacketSize, those controllers may stuck on transaction. The example is DWC2.
This patch checks gadget->quirk_ep_out_aligned_size field and aligns rx_bytes_expected to wMaxPacketSize only when it's needed.

[snip]

Quoted 7 lines

drivers/usb/gadget/f_fastboot.c +3 -0

@@ -435,12 +438,18 @@ static unsigned int rx_bytes_expected(unsigned int maxpacket)
 		return 0;
 	if (rx_remain > EP_BUFFER_SIZE)
 		return EP_BUFFER_SIZE;
+
+	if (!quirk_ep_out_aligned_size)
+		goto out;

drivers/usb/gadget/f_fastboot.c static unsigned int rx_bytes_expected(unsigned int maxpacket) · line 453

Quoted 10 lines

drivers/usb/gadget/f_fastboot.c +3 -0

+
 	if (rx_remain < maxpacket) {
 		rx_remain = maxpacket;
 	} else if (rx_remain % maxpacket != 0) {
 		rem = rx_remain % maxpacket;
 		rx_remain = rx_remain + (maxpacket - rem);
 	}
+
+out:
 	return rx_remain;

Can we do:
/* Only continue on conntrollers that require additional padding */ if (!quirk_ep_out_aligned_size)

	return rx_remain;

Or similar comment, so it's clear in the code why we're doing this and no need for a new goto here. Thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160413/dcccccec/attachment.sig>

Roger Quadros

2016-04-13 12:32 UTC #3 lore raw

PATCH 1/1 fastboot: Fix OUT transaction length alignment

In reply to Semen Protsenko

Hi,

Quoted 63 lines · Semen Protsenko

From: Sam Protsenko <[email protected]>
Some UDC controllers may require buffer size to be aligned to wMaxPacketSize. It's indicated by gadget->quirk_ep_out_aligned_size field being set to "true" (in UDC driver code). In that case rx_bytes_expected must be aligned to wMaxPacket size, otherwise stuck on transaction will happen. For example, it's required by DWC3 controller data manual:
section 8.2.3.3 Buffer Size Rules and Zero-Length Packets:
For OUT endpoints, the following rules apply:
- The BUFSIZ field must be ? 1 byte.
- The total size of a Buffer Descriptor must be a multiple of MaxPacketSize
- A received zero-length packet still requires a MaxPacketSize buffer. Therefore, if the expected amount of data to be received is a multiple of MaxPacketSize, software should add MaxPacketSize bytes to the buffer to sink a possible zero-length packet at the end of the transfer.
But other UDC controllers don't need such alignment, so mentioned field is set to "false". If buffer size is aligned to wMaxPacketSize, those controllers may stuck on transaction. The example is DWC2.
This patch checks gadget->quirk_ep_out_aligned_size field and aligns rx_bytes_expected to wMaxPacketSize only when it's needed.
Signed-off-by Sam Protsenko <[email protected]>
---
 drivers/usb/gadget/f_fastboot.c | 9 +++++++++
 1 file changed, 9 insertions(+)
drivers/usb/gadget/f_fastboot.c +7 -0
index 2e87fee..54dcce0 100644
@@ -58,6 +58,7 @@ static unsigned int fastboot_flash_session_id;
 static unsigned int download_size;
 static unsigned int download_bytes;
 static bool is_high_speed;
+static bool quirk_ep_out_aligned_size;
 
 static struct usb_endpoint_descriptor fs_ep_in = {
 	.bLength            = USB_DT_ENDPOINT_SIZE,
@@ -240,6 +241,8 @@ static int fastboot_set_alt(struct usb_function *f,
 	debug("%s: func: %s intf: %d alt: %d\n",
 	      __func__, f->name, interface, alt);
 
+	quirk_ep_out_aligned_size = gadget->quirk_ep_out_aligned_size;
+
 	/* make sure we don't enable the ep twice */
 	if (gadget->speed == USB_SPEED_HIGH) {
 		ret = usb_ep_enable(f_fb->out_ep, &hs_ep_out);
@@ -435,12 +438,18 @@ static unsigned int rx_bytes_expected(unsigned int maxpacket)
 		return 0;
 	if (rx_remain > EP_BUFFER_SIZE)
 		return EP_BUFFER_SIZE;
+
+	if (!quirk_ep_out_aligned_size)
+		goto out;
+
 	if (rx_remain < maxpacket) {

drivers/usb/gadget/f_fastboot.c static unsigned int rx_bytes_expected(unsigned int maxpacket) · line 455

Quoted 10 lines

drivers/usb/gadget/f_fastboot.c +2 -0

 		rx_remain = maxpacket;
 	} else if (rx_remain % maxpacket != 0) {
 		rem = rx_remain % maxpacket;
 		rx_remain = rx_remain + (maxpacket - rem);
 	}
+
+out:
 	return rx_remain;
 }

Why do we need a special flag for this driver if other drivers e.g. mass storage are doing perfectly fine without it?

This patch is just covering up the real problem, by bypassing the faulty code with a flag.

The buffer size is EP_BUFFER_SIZE and is already aligned to wMaxPacketSize so the problem shouldn't have happened in the first place. But it is happening indicating something else is wrong.

Why is the code using wMaxPacketSize for the check. why can't it just use usb_ep->maxpacket?

cheers,
-roger

Sam Protsenko

author

2016-04-13 16:56 UTC #4 lore raw

PATCH 1/1 fastboot: Fix OUT transaction length alignment

Quoted 71 lines · Roger Quadros

Hi,
Quoted 68 lines · Semen Protsenko
From: Sam Protsenko <[email protected]>
Some UDC controllers may require buffer size to be aligned to wMaxPacketSize. It's indicated by gadget->quirk_ep_out_aligned_size field being set to "true" (in UDC driver code). In that case rx_bytes_expected must be aligned to wMaxPacket size, otherwise stuck on transaction will happen. For example, it's required by DWC3 controller data manual:
section 8.2.3.3 Buffer Size Rules and Zero-Length Packets:
For OUT endpoints, the following rules apply:
- The BUFSIZ field must be ? 1 byte.
- The total size of a Buffer Descriptor must be a multiple of MaxPacketSize
- A received zero-length packet still requires a MaxPacketSize buffer. Therefore, if the expected amount of data to be received is a multiple of MaxPacketSize, software should add MaxPacketSize bytes to the buffer to sink a possible zero-length packet at the end of the transfer.
But other UDC controllers don't need such alignment, so mentioned field is set to "false". If buffer size is aligned to wMaxPacketSize, those controllers may stuck on transaction. The example is DWC2.
This patch checks gadget->quirk_ep_out_aligned_size field and aligns rx_bytes_expected to wMaxPacketSize only when it's needed.
Signed-off-by Sam Protsenko <[email protected]>
---
 drivers/usb/gadget/f_fastboot.c | 9 +++++++++
 1 file changed, 9 insertions(+)
drivers/usb/gadget/f_fastboot.c +8 -0
index 2e87fee..54dcce0 100644
@@ -58,6 +58,7 @@ static unsigned int fastboot_flash_session_id;
 static unsigned int download_size;
 static unsigned int download_bytes;
 static bool is_high_speed;
+static bool quirk_ep_out_aligned_size;
 
 static struct usb_endpoint_descriptor fs_ep_in = {
      .bLength            = USB_DT_ENDPOINT_SIZE,
@@ -240,6 +241,8 @@ static int fastboot_set_alt(struct usb_function *f,
      debug("%s: func: %s intf: %d alt: %d\n",
            __func__, f->name, interface, alt);
 
+     quirk_ep_out_aligned_size = gadget->quirk_ep_out_aligned_size;
+
      /* make sure we don't enable the ep twice */
      if (gadget->speed == USB_SPEED_HIGH) {
              ret = usb_ep_enable(f_fb->out_ep, &hs_ep_out);
@@ -435,12 +438,18 @@ static unsigned int rx_bytes_expected(unsigned int maxpacket)
              return 0;
      if (rx_remain > EP_BUFFER_SIZE)
              return EP_BUFFER_SIZE;
+
+     if (!quirk_ep_out_aligned_size)
+             goto out;
+
      if (rx_remain < maxpacket) {
              rx_remain = maxpacket;
      } else if (rx_remain % maxpacket != 0) {
              rem = rx_remain % maxpacket;
              rx_remain = rx_remain + (maxpacket - rem);
      }
+

Quoted 9 lines

drivers/usb/gadget/f_fastboot.c static unsigned int rx_bytes_expected(unsigned int maxpacket) · line 455
Quoted 5 lines
drivers/usb/gadget/f_fastboot.c +1 -0
+out:
      return rx_remain;
 }
 
Why do we need a special flag for this driver if other drivers e.g. mass storage are doing perfectly fine without it?

I don't know how it works in other gadgets, but please see this patch in kernel: [1]. That patch is doing just the same as I did (and also in gadget code), using usb_ep_align_maybe() function for alignment.

Quoted 7 lines

This patch is just covering up the real problem, by bypassing the faulty code with a flag.
The buffer size is EP_BUFFER_SIZE and is already aligned to wMaxPacketSize so the problem shouldn't have happened in the first place. But it is happening indicating something else is wrong.

There is what I'm observing on platform with DWC3 controller:
- when doing "fastboot flash xloader MLO":
- the whole size of data to send is 70964 bytes
- the size of all packets (except of last packet) is 4096 bytes
- so those are being sent just fine (in req->complete, which is
rx_handler_dl_image() function)
- but last packet has size of 1332 bytes (because 70964 % 4096 = 1332)
- when its req->length is aligned to wMaxPacketSize (so it's 1536
bytes): after we send it using usb_ep_queue(), the req->complete callback is called one last time and we see that transmission is finished (download_bytes >= download_size)
- but when its req->length is not aligned to wMaxPacketSize (so it's
1332 bytes): req->complete callback is not called last time, so transaction is not finished and we are stuck in "fastboot flash"

So I guess the issue is real and related to DWC3 quirk. If you have any thoughts regarding other possible causes of this problem -- please share. I can't predict all possible causes as I'm not USB expert.

Quoted 2 lines

Why is the code using wMaxPacketSize for the check. why can't it just use usb_ep->maxpacket?

I just reused already existed code. Sure we can use usb_ep->maxpacket instead, it's also 512 in my case (I believe it's assigned in DWC3 code).

[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=219580e64f035bb9018dbb08d340f90b0ac50f8c

Quoted 2 lines

cheers,
-roger

Steve Rae

2016-04-14 01:06 UTC #5 lore raw

PATCH 1/1 fastboot: Fix OUT transaction length alignment

Quoted 74 lines · Sam Protsenko

Quoted 19 lines · Roger Quadros
Hi,
Quoted 16 lines · Semen Protsenko
From: Sam Protsenko <[email protected]>
Some UDC controllers may require buffer size to be aligned to wMaxPacketSize. It's indicated by gadget->quirk_ep_out_aligned_size field being set to "true" (in UDC driver code). In that case rx_bytes_expected must be aligned to wMaxPacket size, otherwise stuck on transaction will happen. For example, it's required by DWC3 controller data manual:
section 8.2.3.3 Buffer Size Rules and Zero-Length Packets:
For OUT endpoints, the following rules apply:
- The BUFSIZ field must be ? 1 byte.
- The total size of a Buffer Descriptor must be a multiple of MaxPacketSize
- A received zero-length packet still requires a MaxPacketSize
buffer.
Quoted 1 line
Quoted 1 line
Therefore, if the expected amount of data to be received is a
multiple
Quoted 1 line
Quoted 1 line
of MaxPacketSize, software should add MaxPacketSize bytes to the
buffer
Quoted 9 lines
Quoted 9 lines
to sink a possible zero-length packet at the end of the transfer.
But other UDC controllers don't need such alignment, so mentioned field is set to "false". If buffer size is aligned to wMaxPacketSize, those controllers may stuck on transaction. The example is DWC2.
This patch checks gadget->quirk_ep_out_aligned_size field and aligns rx_bytes_expected to wMaxPacketSize only when it's needed.
Quoted 6 lines
Review
Quoted 6 lines
Signed-off-by Sam Protsenko <[email protected]>
---
 drivers/usb/gadget/f_fastboot.c | 9 +++++++++
 1 file changed, 9 insertions(+)
(quoted diff) +0 -0
b/drivers/usb/gadget/f_fastboot.c
Quoted 15 lines
Quoted 15 lines
index 2e87fee..54dcce0 100644
drivers/usb/gadget/f_fastboot.c +1 -0
@@ -58,6 +58,7 @@ static unsigned int fastboot_flash_session_id;
 static unsigned int download_size;
 static unsigned int download_bytes;
 static bool is_high_speed;
+static bool quirk_ep_out_aligned_size;
 
 static struct usb_endpoint_descriptor fs_ep_in = {
      .bLength            = USB_DT_ENDPOINT_SIZE,
@@ -240,6 +241,8 @@ static int fastboot_set_alt(struct usb_function *f,
      debug("%s: func: %s intf: %d alt: %d\n",
            __func__, f->name, interface, alt);
 
Quoted 6 lines
drivers/usb/gadget/f_fastboot.c static unsigned int rx_bytes_expected(unsigned
Quoted 6 lines
drivers/usb/gadget/f_fastboot.c +2 -0
+     quirk_ep_out_aligned_size = gadget->quirk_ep_out_aligned_size;
+
      /* make sure we don't enable the ep twice */
      if (gadget->speed == USB_SPEED_HIGH) {
              ret = usb_ep_enable(f_fb->out_ep, &hs_ep_out);
@@ -435,12 +438,18 @@ static unsigned int rx_bytes_expected(unsigned
int maxpacket)
Quoted 11 lines
Quoted 11 lines
drivers/usb/gadget/f_fastboot.c +4 -0
              return 0;
      if (rx_remain > EP_BUFFER_SIZE)
              return EP_BUFFER_SIZE;
+
+     if (!quirk_ep_out_aligned_size)
+             goto out;
+
      if (rx_remain < maxpacket) {
              rx_remain = maxpacket;
      } else if (rx_remain % maxpacket != 0) {
              rem = rx_remain % maxpacket;

Quoted 9 lines

Quoted 8 lines
drivers/usb/gadget/f_fastboot.c static unsigned int rx_bytes_expected(unsigned int maxpacket) · line 455
Quoted 8 lines
drivers/usb/gadget/f_fastboot.c +2 -0
              rx_remain = rx_remain + (maxpacket - rem);
      }
+
+out:
      return rx_remain;
 }
 

yes -- anything that skips those 6 lines will work on my boards....

Quoted 55 lines

Quoted 2 lines
Why do we need a special flag for this driver if other drivers e.g. mass
storage
Quoted 2 lines
are doing perfectly fine without it?
I don't know how it works in other gadgets, but please see this patch in kernel: [1]. That patch is doing just the same as I did (and also in gadget code), using usb_ep_align_maybe() function for alignment.
Quoted 1 line
This patch is just covering up the real problem, by bypassing the faulty
code
Quoted 3 lines
with a flag.
The buffer size is EP_BUFFER_SIZE and is already aligned to
wMaxPacketSize so
Quoted 1 line
the problem shouldn't have happened in the first place. But it is
happening
Quoted 2 lines
indicating something else is wrong.
There is what I'm observing on platform with DWC3 controller:
- when doing "fastboot flash xloader MLO":
- the whole size of data to send is 70964 bytes
- the size of all packets (except of last packet) is 4096 bytes
- so those are being sent just fine (in req->complete, which is
rx_handler_dl_image() function)
- but last packet has size of 1332 bytes (because 70964 % 4096 = 1332)
- when its req->length is aligned to wMaxPacketSize (so it's 1536
bytes): after we send it using usb_ep_queue(), the req->complete callback is called one last time and we see that transmission is finished (download_bytes >= download_size)
- but when its req->length is not aligned to wMaxPacketSize (so it's
1332 bytes): req->complete callback is not called last time, so transaction is not finished and we are stuck in "fastboot flash"
So I guess the issue is real and related to DWC3 quirk. If you have any thoughts regarding other possible causes of this problem -- please share. I can't predict all possible causes as I'm not USB expert.
Quoted 1 line
Why is the code using wMaxPacketSize for the check. why can't it just
use usb_ep->maxpacket?
Quoted 1 line
I just reused already existed code. Sure we can use usb_ep->maxpacket instead, it's also 512 in my case (I believe it's assigned in DWC3 code).
[1]
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=219580e64f035bb9018dbb08d340f90b0ac50f8c
Quoted 2 lines
cheers,
-roger

Roger Quadros

2016-04-14 10:18 UTC #6 lore raw

PATCH 1/1 fastboot: Fix OUT transaction length alignment

In reply to Sam Protsenko

Hi,

Quoted 82 lines · Sam Protsenko

Quoted 71 lines · Roger Quadros
Hi,
Quoted 68 lines · Semen Protsenko
From: Sam Protsenko <[email protected]>
Some UDC controllers may require buffer size to be aligned to wMaxPacketSize. It's indicated by gadget->quirk_ep_out_aligned_size field being set to "true" (in UDC driver code). In that case rx_bytes_expected must be aligned to wMaxPacket size, otherwise stuck on transaction will happen. For example, it's required by DWC3 controller data manual:
section 8.2.3.3 Buffer Size Rules and Zero-Length Packets:
For OUT endpoints, the following rules apply:
- The BUFSIZ field must be ? 1 byte.
- The total size of a Buffer Descriptor must be a multiple of MaxPacketSize
- A received zero-length packet still requires a MaxPacketSize buffer. Therefore, if the expected amount of data to be received is a multiple of MaxPacketSize, software should add MaxPacketSize bytes to the buffer to sink a possible zero-length packet at the end of the transfer.
But other UDC controllers don't need such alignment, so mentioned field is set to "false". If buffer size is aligned to wMaxPacketSize, those controllers may stuck on transaction. The example is DWC2.
This patch checks gadget->quirk_ep_out_aligned_size field and aligns rx_bytes_expected to wMaxPacketSize only when it's needed.
Signed-off-by Sam Protsenko <[email protected]>
---
 drivers/usb/gadget/f_fastboot.c | 9 +++++++++
 1 file changed, 9 insertions(+)
drivers/usb/gadget/f_fastboot.c +8 -0
index 2e87fee..54dcce0 100644
@@ -58,6 +58,7 @@ static unsigned int fastboot_flash_session_id;
 static unsigned int download_size;
 static unsigned int download_bytes;
 static bool is_high_speed;
+static bool quirk_ep_out_aligned_size;
 
 static struct usb_endpoint_descriptor fs_ep_in = {
      .bLength            = USB_DT_ENDPOINT_SIZE,
@@ -240,6 +241,8 @@ static int fastboot_set_alt(struct usb_function *f,
      debug("%s: func: %s intf: %d alt: %d\n",
            __func__, f->name, interface, alt);
 
+     quirk_ep_out_aligned_size = gadget->quirk_ep_out_aligned_size;
+
      /* make sure we don't enable the ep twice */
      if (gadget->speed == USB_SPEED_HIGH) {
              ret = usb_ep_enable(f_fb->out_ep, &hs_ep_out);
@@ -435,12 +438,18 @@ static unsigned int rx_bytes_expected(unsigned int maxpacket)
              return 0;
      if (rx_remain > EP_BUFFER_SIZE)
              return EP_BUFFER_SIZE;
+
+     if (!quirk_ep_out_aligned_size)
+             goto out;
+
      if (rx_remain < maxpacket) {
              rx_remain = maxpacket;
      } else if (rx_remain % maxpacket != 0) {
              rem = rx_remain % maxpacket;
              rx_remain = rx_remain + (maxpacket - rem);
      }
+
Quoted 9 lines
drivers/usb/gadget/f_fastboot.c static unsigned int rx_bytes_expected(unsigned int maxpacket) · line 455
Quoted 5 lines
drivers/usb/gadget/f_fastboot.c +1 -0
+out:
      return rx_remain;
 }
 
Why do we need a special flag for this driver if other drivers e.g. mass storage are doing perfectly fine without it?

Quoted 3 lines

I don't know how it works in other gadgets, but please see this patch in kernel: [1]. That patch is doing just the same as I did (and also in gadget code), using usb_ep_align_maybe() function for alignment.

NOTE: there haven't been such quirks in the kernel drivers except for that one driver that has a user mode interface and needs more moral policing for user provided buffers. So that example is not something we should be using as reference. Our buffers are alreay aligned to maxpacket size. The only thing we need to worry about is the length of the last transaction that is not integral multiple of maxpacket size.

If my understanding is right all USB controllers should work fine with bulk OUT requests that are integral multiple of maxpacket size. So we shouldn't be needing any quirk flags.

Quoted 24 lines

Quoted 8 lines
This patch is just covering up the real problem, by bypassing the faulty code with a flag.
The buffer size is EP_BUFFER_SIZE and is already aligned to wMaxPacketSize so the problem shouldn't have happened in the first place. But it is happening indicating something else is wrong.
There is what I'm observing on platform with DWC3 controller:
- when doing "fastboot flash xloader MLO":
- the whole size of data to send is 70964 bytes
- the size of all packets (except of last packet) is 4096 bytes
- so those are being sent just fine (in req->complete, which is
rx_handler_dl_image() function)
- but last packet has size of 1332 bytes (because 70964 % 4096 = 1332)
- when its req->length is aligned to wMaxPacketSize (so it's 1536
bytes): after we send it using usb_ep_queue(), the req->complete callback is called one last time and we see that transmission is finished (download_bytes >= download_size)
- but when its req->length is not aligned to wMaxPacketSize (so it's
1332 bytes): req->complete callback is not called last time, so transaction is not finished and we are stuck in "fastboot flash"

Quoted 3 lines

So I guess the issue is real and related to DWC3 quirk. If you have any thoughts regarding other possible causes of this problem -- please share. I can't predict all possible causes as I'm not USB expert.

I've tried to clean up the bulk out handling code in the below patch. Note you will need to apply this on top of the 2 patches I sent earlier.
https://patchwork.ozlabs.org/patch/609417/
https://patchwork.ozlabs.org/patch/609896/

Steve, do let me know if it works for you. If it doesn't then we need to figure out why. Can you please share details about the USB controller on your board?
Does it not like OUT requests that are aligned to maxpacket size?
What does ep->maxpacket show for your board?

drivers/usb/gadget/f_fastboot.c +23 ~~-27~~

index 7961231..28b244a 100644
@@ -39,6 +39,11 @@
 #define TX_ENDPOINT_MAXIMUM_PACKET_SIZE      (0x0040)
 
 #define EP_BUFFER_SIZE			4096
+/*
+ * EP_BUFFER_SIZE must always be an integral multiple of maxpacket size
+ * (64 or 512 or 1024), else we break on certain controllers like DWC3
+ * that expect bulk OUT requests to be divisible by maxpacket size.
+ */
 
 struct f_fastboot {
 	struct usb_function usb_function;

@@ -57,7 +62,6 @@ static struct f_fastboot *fastboot_func;
 static unsigned int fastboot_flash_session_id;
 static unsigned int download_size;
 static unsigned int download_bytes;
-static bool is_high_speed;
 
 static struct usb_endpoint_descriptor fs_ep_in = {
 	.bLength            = USB_DT_ENDPOINT_SIZE,

@@ -269,11 +273,6 @@ static int fastboot_set_alt(struct usb_function *f,
 	debug("%s: func: %s intf: %d alt: %d\n",
 	      __func__, f->name, interface, alt);
 
-	if (gadget->speed == USB_SPEED_HIGH)
-		is_high_speed = true;
-	else
-		is_high_speed = false;
-
 	d = fb_ep_desc(gadget, &fs_ep_out, &hs_ep_out);
 	ret = usb_ep_enable(f_fb->out_ep, d);
 	if (ret) {

@@ -455,20 +454,27 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req)
 	fastboot_tx_write_str(response);
 }
 
-static unsigned int rx_bytes_expected(unsigned int maxpacket)
+static unsigned int rx_bytes_expected(struct usb_ep *ep)
 {
 	int rx_remain = download_size - download_bytes;
-	int rem = 0;
-	if (rx_remain < 0)
+	unsigned int rem;
+	unsigned int maxpacket = ep->maxpacket;
+
+	if (rx_remain <= 0)
 		return 0;
-	if (rx_remain > EP_BUFFER_SIZE)
+	else if (rx_remain > EP_BUFFER_SIZE)
 		return EP_BUFFER_SIZE;
-	if (rx_remain < maxpacket) {
-		rx_remain = maxpacket;
-	} else if (rx_remain % maxpacket != 0) {
-		rem = rx_remain % maxpacket;
+
+	/*
+	 * Some controllers e.g. DWC3 don't like OUT transfers to be
+	 * not ending in maxpacket boundary. So just make them happy by
+	 * always requesting for integral multiple of maxpackets.
+	 * This shouldn't bother controllers that don't care about it.
+	 */
+	rem = rx_remain % maxpacket;
+	if (rem > 0)
 		rx_remain = rx_remain + (maxpacket - rem);
-	}
+
 	return rx_remain;
 }

@@ -480,7 +486,6 @@ static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request *req)
 	const unsigned char *buffer = req->buf;
 	unsigned int buffer_size = req->actual;
 	unsigned int pre_dot_num, now_dot_num;
-	unsigned int max;
 
 	if (req->status != 0) {
 		printf("Bad status: %d\n", req->status);

@@ -518,11 +523,7 @@ static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request *req)
 
 		printf("\ndownloading of %d bytes finished\n", download_bytes);
 	} else {
-		max = is_high_speed ? hs_ep_out.wMaxPacketSize :
-				fs_ep_out.wMaxPacketSize;
-		req->length = rx_bytes_expected(max);
-		if (req->length < ep->maxpacket)
-			req->length = ep->maxpacket;
+		req->length = rx_bytes_expected(ep);
 	}
 
 	req->actual = 0;

@@ -533,7 +534,6 @@ static void cb_download(struct usb_ep *ep, struct usb_request *req)
 {
 	char *cmd = req->buf;
 	char response[FASTBOOT_RESPONSE_LEN];
-	unsigned int max;
 
 	strsep(&cmd, ":");
 	download_size = simple_strtoul(cmd, NULL, 16);

@@ -549,11 +549,7 @@ static void cb_download(struct usb_ep *ep, struct usb_request *req)
 	} else {
 		sprintf(response, "DATA%08x", download_size);
 		req->complete = rx_handler_dl_image;
-		max = is_high_speed ? hs_ep_out.wMaxPacketSize :
-			fs_ep_out.wMaxPacketSize;
-		req->length = rx_bytes_expected(max);
-		if (req->length < ep->maxpacket)
-			req->length = ep->maxpacket;
+		req->length = rx_bytes_expected(ep);
 	}
 	fastboot_tx_write_str(response);
 }

Signature · 1 line

2.5.0

Steve Rae

2016-04-15 19:44 UTC #7 lore raw

PATCH 1/1 fastboot: Fix OUT transaction length alignment

Quoted 148 lines · Roger Quadros

Hi,
Quoted 10 lines · Sam Protsenko
Quoted 9 lines · Roger Quadros
Hi,
Quoted 6 lines · Semen Protsenko
From: Sam Protsenko <[email protected]>
Some UDC controllers may require buffer size to be aligned to wMaxPacketSize. It's indicated by gadget->quirk_ep_out_aligned_size field being set to "true" (in UDC driver code). In that case rx_bytes_expected must be aligned to wMaxPacket size, otherwise stuck
on
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
transaction will happen. For example, it's required by DWC3 controller data manual:
section 8.2.3.3 Buffer Size Rules and Zero-Length Packets:
For OUT endpoints, the following rules apply:
- The BUFSIZ field must be ? 1 byte.
- The total size of a Buffer Descriptor must be a multiple of MaxPacketSize
- A received zero-length packet still requires a MaxPacketSize
buffer.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Therefore, if the expected amount of data to be received is a
multiple
Quoted 1 line
Quoted 1 line
Quoted 1 line
of MaxPacketSize, software should add MaxPacketSize bytes to the
buffer
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
to sink a possible zero-length packet at the end of the transfer.
But other UDC controllers don't need such alignment, so mentioned field is set to "false". If buffer size is aligned to wMaxPacketSize, those controllers may stuck on transaction. The example is DWC2.
This patch checks gadget->quirk_ep_out_aligned_size field and aligns rx_bytes_expected to wMaxPacketSize only when it's needed.
Quoted 6 lines
Quoted 6 lines
Review
Quoted 6 lines
Signed-off-by Sam Protsenko <[email protected]>
---
 drivers/usb/gadget/f_fastboot.c | 9 +++++++++
 1 file changed, 9 insertions(+)
(quoted diff) +0 -0
b/drivers/usb/gadget/f_fastboot.c
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
index 2e87fee..54dcce0 100644
drivers/usb/gadget/f_fastboot.c +1 -0
@@ -58,6 +58,7 @@ static unsigned int fastboot_flash_session_id;
 static unsigned int download_size;
 static unsigned int download_bytes;
 static bool is_high_speed;
+static bool quirk_ep_out_aligned_size;
 
 static struct usb_endpoint_descriptor fs_ep_in = {
      .bLength            = USB_DT_ENDPOINT_SIZE,
@@ -240,6 +241,8 @@ static int fastboot_set_alt(struct usb_function *f,
      debug("%s: func: %s intf: %d alt: %d\n",
            __func__, f->name, interface, alt);
 
Quoted 6 lines
Quoted 6 lines
drivers/usb/gadget/f_fastboot.c static unsigned int rx_bytes_expected(unsigned
Quoted 6 lines
drivers/usb/gadget/f_fastboot.c +2 -0
+     quirk_ep_out_aligned_size = gadget->quirk_ep_out_aligned_size;
+
      /* make sure we don't enable the ep twice */
      if (gadget->speed == USB_SPEED_HIGH) {
              ret = usb_ep_enable(f_fb->out_ep, &hs_ep_out);
@@ -435,12 +438,18 @@ static unsigned int rx_bytes_expected(unsigned
int maxpacket)
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
drivers/usb/gadget/f_fastboot.c +4 -0
              return 0;
      if (rx_remain > EP_BUFFER_SIZE)
              return EP_BUFFER_SIZE;
+
+     if (!quirk_ep_out_aligned_size)
+             goto out;
+
      if (rx_remain < maxpacket) {
              rx_remain = maxpacket;
      } else if (rx_remain % maxpacket != 0) {
              rem = rx_remain % maxpacket;
              rx_remain = rx_remain + (maxpacket - rem);
      }
Quoted 8 lines
Quoted 8 lines
drivers/usb/gadget/f_fastboot.c static void cb_getvar(struct usb_ep *ep, struct usb_request *req) · line 480
Quoted 6 lines
drivers/usb/gadget/f_fastboot.c +2 -0
+
+out:
      return rx_remain;
 }
 
Why do we need a special flag for this driver if other drivers e.g.
mass storage
Quoted 6 lines
Quoted 2 lines
are doing perfectly fine without it?
I don't know how it works in other gadgets, but please see this patch in kernel: [1]. That patch is doing just the same as I did (and also in gadget code), using usb_ep_align_maybe() function for alignment.
NOTE: there haven't been such quirks in the kernel drivers except for that one
driver that has a user mode interface and needs more moral policing for user
provided buffers. So that example is not something we should be using as reference.
Our buffers are alreay aligned to maxpacket size. The only thing we need to worry
about is the length of the last transaction that is not integral multiple of
maxpacket size.
If my understanding is right all USB controllers should work fine with bulk OUT requests that are integral multiple of maxpacket size. So we shouldn't be needing any quirk flags.
Quoted 2 lines
Quoted 2 lines
This patch is just covering up the real problem, by bypassing the
faulty code
Quoted 3 lines
Quoted 3 lines
with a flag.
The buffer size is EP_BUFFER_SIZE and is already aligned to
wMaxPacketSize so
Quoted 1 line
Quoted 1 line
the problem shouldn't have happened in the first place. But it is
happening
Quoted 18 lines
Quoted 2 lines
indicating something else is wrong.
There is what I'm observing on platform with DWC3 controller:
- when doing "fastboot flash xloader MLO":
- the whole size of data to send is 70964 bytes
- the size of all packets (except of last packet) is 4096 bytes
- so those are being sent just fine (in req->complete, which is
rx_handler_dl_image() function)
- but last packet has size of 1332 bytes (because 70964 % 4096 = 1332)
- when its req->length is aligned to wMaxPacketSize (so it's 1536
bytes): after we send it using usb_ep_queue(), the req->complete callback is called one last time and we see that transmission is finished (download_bytes >= download_size)
- but when its req->length is not aligned to wMaxPacketSize (so it's
1332 bytes): req->complete callback is not called last time, so transaction is not finished and we are stuck in "fastboot flash"
Quoted 3 lines
So I guess the issue is real and related to DWC3 quirk. If you have any thoughts regarding other possible causes of this problem -- please share. I can't predict all possible causes as I'm not USB expert.
I've tried to clean up the bulk out handling code in the below patch. Note you will need to apply this on top of the 2 patches I sent earlier.
https://patchwork.ozlabs.org/patch/609417/
https://patchwork.ozlabs.org/patch/609896/

Quoted 6 lines

Steve, do let me know if it works for you. If it doesn't then we need to figure out why.
Can you please share details about the USB controller on your board? Does it not like OUT requests that are aligned to maxpacket size? What does ep->maxpacket show for your board?

This (still) does not work....
- using the standard U-Boot DWC2 driver,
- do not know if it doesn't like "OUT requests that are aligned to maxpacket size" -- perhaps @Lukasz could answer this....
- ep->maxpacket is 512

with logging in "drivers/usb/gadget/dwc2_udc_otg.c" enabled, output is (for the last transactions...):

*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK :
0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003
*** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000

        EP2-OUT : DOEPINT = 0x2011

complete_rx: RX DMA done : ep = 2, rx bytes = 4096/4096, is_short = 0, DOEPTSIZ = 0x0, remained bytes = 4096
complete_rx: Next Rx request start...
setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x401000, DOEPCTL = 0x80098200

        buf = 0xffb84f80, pktcnt = 8, xfersize = 4096

*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK :
0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003
*** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000

        EP2-OUT : DOEPINT = 0x2011

complete_rx: RX DMA done : ep = 2, rx bytes = 3968/4096, is_short = 0, DOEPTSIZ = 0x80, remained bytes = 3968
setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb85f00,DOEPTSIZ = 0x80080, DOEPCTL = 0x80098200

        buf = 0xffb85f00, pktcnt = 1, xfersize = 128

Quoted 130 lines

Quoted 1 line

Quoted 1 line
Quoted 1 line
and it hangs here!!!

drivers/usb/gadget/f_fastboot.c +21 ~~-16~~

index 7961231..28b244a 100644
@@ -39,6 +39,11 @@
 #define TX_ENDPOINT_MAXIMUM_PACKET_SIZE      (0x0040)
 
 #define EP_BUFFER_SIZE                 4096
+/*
+ * EP_BUFFER_SIZE must always be an integral multiple of maxpacket size
+ * (64 or 512 or 1024), else we break on certain controllers like DWC3
+ * that expect bulk OUT requests to be divisible by maxpacket size.
+ */
 
 struct f_fastboot {
        struct usb_function usb_function;

@@ -57,7 +62,6 @@ static struct f_fastboot *fastboot_func;
 static unsigned int fastboot_flash_session_id;
 static unsigned int download_size;
 static unsigned int download_bytes;
-static bool is_high_speed;
 
 static struct usb_endpoint_descriptor fs_ep_in = {
        .bLength            = USB_DT_ENDPOINT_SIZE,

@@ -269,11 +273,6 @@ static int fastboot_set_alt(struct usb_function *f,
        debug("%s: func: %s intf: %d alt: %d\n",
              __func__, f->name, interface, alt);
 
-       if (gadget->speed == USB_SPEED_HIGH)
-               is_high_speed = true;
-       else
-               is_high_speed = false;
-
        d = fb_ep_desc(gadget, &fs_ep_out, &hs_ep_out);
        ret = usb_ep_enable(f_fb->out_ep, d);
        if (ret) {

@@ -455,20 +454,27 @@ static void cb_getvar(struct usb_ep *ep, struct
 usb_request *req)
        fastboot_tx_write_str(response);
 }
 
-static unsigned int rx_bytes_expected(unsigned int maxpacket)
+static unsigned int rx_bytes_expected(struct usb_ep *ep)
 {
        int rx_remain = download_size - download_bytes;
-       int rem = 0;
-       if (rx_remain < 0)
+       unsigned int rem;
+       unsigned int maxpacket = ep->maxpacket;
+
+       if (rx_remain <= 0)
                return 0;
-       if (rx_remain > EP_BUFFER_SIZE)
+       else if (rx_remain > EP_BUFFER_SIZE)
                return EP_BUFFER_SIZE;
-       if (rx_remain < maxpacket) {
-               rx_remain = maxpacket;
-       } else if (rx_remain % maxpacket != 0) {
-               rem = rx_remain % maxpacket;
+
+       /*
+        * Some controllers e.g. DWC3 don't like OUT transfers to be
+        * not ending in maxpacket boundary. So just make them happy by
+        * always requesting for integral multiple of maxpackets.
+        * This shouldn't bother controllers that don't care about it.
+        */
+       rem = rx_remain % maxpacket;
+       if (rem > 0)
                rx_remain = rx_remain + (maxpacket - rem);
-       }
+
        return rx_remain;
 }

@@ -480,7 +486,6 @@ static void rx_handler_dl_image(struct usb_ep *ep,
 struct usb_request *req)
        const unsigned char *buffer = req->buf;
        unsigned int buffer_size = req->actual;
        unsigned int pre_dot_num, now_dot_num;
-       unsigned int max;
 
        if (req->status != 0) {

                printf("Bad status: %d\n", req->status);

drivers/usb/gadget/f_fastboot.c +1 -5

@@ -518,11 +523,7 @@ static void rx_handler_dl_image(struct usb_ep *ep,
 struct usb_request *req)
 
                printf("\ndownloading of %d bytes finished\n",
 download_bytes);
        } else {
-               max = is_high_speed ? hs_ep_out.wMaxPacketSize :
-                               fs_ep_out.wMaxPacketSize;
-               req->length = rx_bytes_expected(max);
-               if (req->length < ep->maxpacket)
-                       req->length = ep->maxpacket;
+               req->length = rx_bytes_expected(ep);
        }

        req->actual = 0;

drivers/usb/gadget/f_fastboot.c +0 -1

@@ -533,7 +534,6 @@ static void cb_download(struct usb_ep *ep, struct
 usb_request *req)
 {
        char *cmd = req->buf;
        char response[FASTBOOT_RESPONSE_LEN];
-       unsigned int max;
 
        strsep(&cmd, ":");

        download_size = simple_strtoul(cmd, NULL, 16);

drivers/usb/gadget/f_fastboot.c +1 -5

@@ -549,11 +549,7 @@ static void cb_download(struct usb_ep *ep, struct
 usb_request *req)
        } else {
                sprintf(response, "DATA%08x", download_size);
                req->complete = rx_handler_dl_image;
-               max = is_high_speed ? hs_ep_out.wMaxPacketSize :
-                       fs_ep_out.wMaxPacketSize;
-               req->length = rx_bytes_expected(max);
-               if (req->length < ep->maxpacket)
-                       req->length = ep->maxpacket;
+               req->length = rx_bytes_expected(ep);
        }
        fastboot_tx_write_str(response);

}
--
2.5.0

Roger Quadros

2016-04-18 07:56 UTC #8 lore raw

PATCH 1/1 fastboot: Fix OUT transaction length alignment

Quoted 150 lines · Steve Rae

On Thu, Apr 14, 2016 at 3:18 AM, Roger Quadros <rogerq at ti.com <mailto:[email protected]>> wrote:

    Hi,

    On 13/04/16 19:56, Sam Protsenko wrote:
    > On Wed, Apr 13, 2016 at 3:32 PM, Roger Quadros <rogerq at ti.com <mailto:[email protected]>> wrote:
    >> Hi,
    >>
    >> On 13/04/16 15:01, Semen Protsenko wrote:
    >>> From: Sam Protsenko <semen.protsenko at linaro.org <mailto:[email protected]>>
    >>>
    >>> Some UDC controllers may require buffer size to be aligned to
    >>> wMaxPacketSize. It's indicated by gadget->quirk_ep_out_aligned_size
    >>> field being set to "true" (in UDC driver code). In that case
    >>> rx_bytes_expected must be aligned to wMaxPacket size, otherwise stuck on
    >>> transaction will happen. For example, it's required by DWC3 controller
    >>> data manual:
    >>>
    >>>     section 8.2.3.3 Buffer Size Rules and Zero-Length Packets:
    >>>
    >>>     For OUT endpoints, the following rules apply:
    >>>     - The BUFSIZ field must be ? 1 byte.
    >>>     - The total size of a Buffer Descriptor must be a multiple of
    >>>       MaxPacketSize
    >>>     - A received zero-length packet still requires a MaxPacketSize buffer.
    >>>       Therefore, if the expected amount of data to be received is a multiple
    >>>       of MaxPacketSize, software should add MaxPacketSize bytes to the buffer
    >>>       to sink a possible zero-length packet at the end of the transfer.
    >>>
    >>> But other UDC controllers don't need such alignment, so mentioned field
    >>> is set to "false". If buffer size is aligned to wMaxPacketSize, those
    >>> controllers may stuck on transaction. The example is DWC2.
    >>>
    >>> This patch checks gadget->quirk_ep_out_aligned_size field and aligns
    >>> rx_bytes_expected to wMaxPacketSize only when it's needed.
    >>>
    >>> Signed-off-by: Sam Protsenko <semen.protsenko at linaro.org <mailto:[email protected]>>
    >>> ---
    >>>  drivers/usb/gadget/f_fastboot.c | 9 +++++++++
    >>>  1 file changed, 9 insertions(+)
    >>>
    >>> diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
    >>> index 2e87fee..54dcce0 100644
    >>> --- a/drivers/usb/gadget/f_fastboot.c
    >>> +++ b/drivers/usb/gadget/f_fastboot.c
    >>> @@ -58,6 +58,7 @@ static unsigned int fastboot_flash_session_id;
    >>>  static unsigned int download_size;
    >>>  static unsigned int download_bytes;
    >>>  static bool is_high_speed;
    >>> +static bool quirk_ep_out_aligned_size;
    >>>
    >>>  static struct usb_endpoint_descriptor fs_ep_in = {
    >>>       .bLength            = USB_DT_ENDPOINT_SIZE,
    >>> @@ -240,6 +241,8 @@ static int fastboot_set_alt(struct usb_function *f,
    >>>       debug("%s: func: %s intf: %d alt: %d\n",
    >>>             __func__, f->name, interface, alt);
    >>>
    >>> +     quirk_ep_out_aligned_size = gadget->quirk_ep_out_aligned_size;
    >>> +
    >>>       /* make sure we don't enable the ep twice */
    >>>       if (gadget->speed == USB_SPEED_HIGH) {
    >>>               ret = usb_ep_enable(f_fb->out_ep, &hs_ep_out);
    >>> @@ -435,12 +438,18 @@ static unsigned int rx_bytes_expected(unsigned int maxpacket)
    >>>               return 0;
    >>>       if (rx_remain > EP_BUFFER_SIZE)
    >>>               return EP_BUFFER_SIZE;
    >>> +
    >>> +     if (!quirk_ep_out_aligned_size)
    >>> +             goto out;
    >>> +
    >>>       if (rx_remain < maxpacket) {
    >>>               rx_remain = maxpacket;
    >>>       } else if (rx_remain % maxpacket != 0) {
    >>>               rem = rx_remain % maxpacket;
    >>>               rx_remain = rx_remain + (maxpacket - rem);
    >>>       }
    >>> +
    >>> +out:
    >>>       return rx_remain;
    >>>  }
    >>>
    >>>
    >>
    >> Why do we need a special flag for this driver if other drivers e.g. mass storage
    >> are doing perfectly fine without it?
    >>
    >
    > I don't know how it works in other gadgets, but please see this patch
    > in kernel: [1]. That patch is doing just the same as I did (and also
    > in gadget code), using usb_ep_align_maybe() function for alignment.

    NOTE: there haven't been such quirks in the kernel drivers except for that one
    driver that has a user mode interface and needs more moral policing for user
    provided buffers. So that example is not something we should be using as reference.
    Our buffers are alreay aligned to maxpacket size. The only thing we need to worry
    about is the length of the last transaction that is not integral multiple of
    maxpacket size.

    If my understanding is right all USB controllers should work fine with
    bulk OUT requests that are integral multiple of maxpacket size.
    So we shouldn't be needing any quirk flags.

    >
    >> This patch is just covering up the real problem, by bypassing the faulty code
    >> with a flag.
    >>
    >> The buffer size is EP_BUFFER_SIZE and is already aligned to wMaxPacketSize so
    >> the problem shouldn't have happened in the first place. But it is happening
    >> indicating something else is wrong.
    >>
    >
    > There is what I'm observing on platform with DWC3 controller:
    >  - when doing "fastboot flash xloader MLO":
    >  - the whole size of data to send is 70964 bytes
    >  - the size of all packets (except of last packet) is 4096 bytes
    >  - so those are being sent just fine (in req->complete, which is
    > rx_handler_dl_image() function)
    >  - but last packet has size of 1332 bytes (because 70964 % 4096 = 1332)
    >  - when its req->length is aligned to wMaxPacketSize (so it's 1536
    > bytes): after we send it using usb_ep_queue(), the req->complete
    > callback is called one last time and we see that transmission is
    > finished (download_bytes >= download_size)
    >  - but when its req->length is not aligned to wMaxPacketSize (so it's
    > 1332 bytes): req->complete callback is not called last time, so
    > transaction is not finished and we are stuck in "fastboot flash"
    >
    > So I guess the issue is real and related to DWC3 quirk. If you have
    > any thoughts regarding other possible causes of this problem -- please
    > share. I can't predict all possible causes as I'm not USB expert.

    I've tried to clean up the bulk out handling code in the below patch.
    Note you will need to apply this on top of the 2 patches I sent earlier.
    https://patchwork.ozlabs.org/patch/609417/
    https://patchwork.ozlabs.org/patch/609896/

    Steve, do let me know if it works for you. If it doesn't then we need to figure out why.
    Can you please share details about the USB controller on your board?
    Does it not like OUT requests that are aligned to maxpacket size?
    What does ep->maxpacket show for your board?

with logging in "drivers/usb/gadget/dwc2_udc_otg.c" enabled, output is (for the last transactions...):

Quoted 7 lines

*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000
        EP2-OUT : DOEPINT = 0x2011
complete_rx: RX DMA done : ep = 2, rx bytes = 4096/4096, is_short = 0, DOEPTSIZ = 0x0, remained bytes = 4096
complete_rx: Next Rx request start...
setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x401000, DOEPCTL = 0x80098200
        buf = 0xffb84f80, pktcnt = 8, xfersize = 4096

OK so we asked for 4096 bytes and looks like we received 3968 bytes, which is probably the end of transfer.

Quoted 9 lines

*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000
        EP2-OUT : DOEPINT = 0x2011
complete_rx: RX DMA done : ep = 2, rx bytes = 3968/4096, is_short = 0, DOEPTSIZ = 0x80, remained bytes = 3968
setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb85f00,DOEPTSIZ = 0x80080, DOEPCTL = 0x80098200
        buf = 0xffb85f00, pktcnt = 1, xfersize = 128
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
and it hangs here!!!

The dwc2 driver should have returned then and not queued another 128 bytes. IMO there is a bug in the dwc2 driver.

3968 = 7 x 512 + 384
This means the last packet (384 bytes) was a short packet and it signals end of transfer so the dwc2 driver shouldn't be queuing another transfer. It should end the usb ep_queue request.

So, question to dwc2 mantainers.
Can we modify dwc2 driver to not automatically queue a pending transfer if the transfer ended in a short packet?

cheers,
-roger

Quoted 123 lines




    diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
    index 7961231..28b244a 100644
    --- a/drivers/usb/gadget/f_fastboot.c
    +++ b/drivers/usb/gadget/f_fastboot.c
    @@ -39,6 +39,11 @@
     #define TX_ENDPOINT_MAXIMUM_PACKET_SIZE      (0x0040)

     #define EP_BUFFER_SIZE                 4096
    +/*
    + * EP_BUFFER_SIZE must always be an integral multiple of maxpacket size
    + * (64 or 512 or 1024), else we break on certain controllers like DWC3
    + * that expect bulk OUT requests to be divisible by maxpacket size.
    + */

     struct f_fastboot {
            struct usb_function usb_function;
    @@ -57,7 +62,6 @@ static struct f_fastboot *fastboot_func;
     static unsigned int fastboot_flash_session_id;
     static unsigned int download_size;
     static unsigned int download_bytes;
    -static bool is_high_speed;

     static struct usb_endpoint_descriptor fs_ep_in = {
            .bLength            = USB_DT_ENDPOINT_SIZE,
    @@ -269,11 +273,6 @@ static int fastboot_set_alt(struct usb_function *f,
            debug("%s: func: %s intf: %d alt: %d\n",
                  __func__, f->name, interface, alt);

    -       if (gadget->speed == USB_SPEED_HIGH)
    -               is_high_speed = true;
    -       else
    -               is_high_speed = false;
    -
            d = fb_ep_desc(gadget, &fs_ep_out, &hs_ep_out);
            ret = usb_ep_enable(f_fb->out_ep, d);
            if (ret) {
    @@ -455,20 +454,27 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req)
            fastboot_tx_write_str(response);
     }

    -static unsigned int rx_bytes_expected(unsigned int maxpacket)
    +static unsigned int rx_bytes_expected(struct usb_ep *ep)
     {
            int rx_remain = download_size - download_bytes;
    -       int rem = 0;
    -       if (rx_remain < 0)
    +       unsigned int rem;
    +       unsigned int maxpacket = ep->maxpacket;
    +
    +       if (rx_remain <= 0)
                    return 0;
    -       if (rx_remain > EP_BUFFER_SIZE)
    +       else if (rx_remain > EP_BUFFER_SIZE)
                    return EP_BUFFER_SIZE;
    -       if (rx_remain < maxpacket) {
    -               rx_remain = maxpacket;
    -       } else if (rx_remain % maxpacket != 0) {
    -               rem = rx_remain % maxpacket;
    +
    +       /*
    +        * Some controllers e.g. DWC3 don't like OUT transfers to be
    +        * not ending in maxpacket boundary. So just make them happy by
    +        * always requesting for integral multiple of maxpackets.
    +        * This shouldn't bother controllers that don't care about it.
    +        */
    +       rem = rx_remain % maxpacket;
    +       if (rem > 0)
                    rx_remain = rx_remain + (maxpacket - rem);
    -       }
    +
            return rx_remain;
     }

    @@ -480,7 +486,6 @@ static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request *req)
            const unsigned char *buffer = req->buf;
            unsigned int buffer_size = req->actual;
            unsigned int pre_dot_num, now_dot_num;
    -       unsigned int max;

            if (req->status != 0) {
                    printf("Bad status: %d\n", req->status);
    @@ -518,11 +523,7 @@ static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request *req)

                    printf("\ndownloading of %d bytes finished\n", download_bytes);
            } else {
    -               max = is_high_speed ? hs_ep_out.wMaxPacketSize :
    -                               fs_ep_out.wMaxPacketSize;
    -               req->length = rx_bytes_expected(max);
    -               if (req->length < ep->maxpacket)
    -                       req->length = ep->maxpacket;
    +               req->length = rx_bytes_expected(ep);
            }

            req->actual = 0;
    @@ -533,7 +534,6 @@ static void cb_download(struct usb_ep *ep, struct usb_request *req)
     {
            char *cmd = req->buf;
            char response[FASTBOOT_RESPONSE_LEN];
    -       unsigned int max;

            strsep(&cmd, ":");
            download_size = simple_strtoul(cmd, NULL, 16);
    @@ -549,11 +549,7 @@ static void cb_download(struct usb_ep *ep, struct usb_request *req)
            } else {
                    sprintf(response, "DATA%08x", download_size);
                    req->complete = rx_handler_dl_image;
    -               max = is_high_speed ? hs_ep_out.wMaxPacketSize :
    -                       fs_ep_out.wMaxPacketSize;
    -               req->length = rx_bytes_expected(max);
    -               if (req->length < ep->maxpacket)
    -                       req->length = ep->maxpacket;
    +               req->length = rx_bytes_expected(ep);
            }
            fastboot_tx_write_str(response);
     }
    --
    2.5.0

Roger Quadros

2016-04-18 11:39 UTC #9 lore raw

PATCH 1/1 fastboot: Fix OUT transaction length alignment

+Lukazs

Quoted 175 lines · Roger Quadros

Quoted 150 lines · Steve Rae

On Thu, Apr 14, 2016 at 3:18 AM, Roger Quadros <rogerq at ti.com <mailto:[email protected]>> wrote:

    Hi,

    On 13/04/16 19:56, Sam Protsenko wrote:
    > On Wed, Apr 13, 2016 at 3:32 PM, Roger Quadros <rogerq at ti.com <mailto:[email protected]>> wrote:
    >> Hi,
    >>
    >> On 13/04/16 15:01, Semen Protsenko wrote:
    >>> From: Sam Protsenko <semen.protsenko at linaro.org <mailto:[email protected]>>
    >>>
    >>> Some UDC controllers may require buffer size to be aligned to
    >>> wMaxPacketSize. It's indicated by gadget->quirk_ep_out_aligned_size
    >>> field being set to "true" (in UDC driver code). In that case
    >>> rx_bytes_expected must be aligned to wMaxPacket size, otherwise stuck on
    >>> transaction will happen. For example, it's required by DWC3 controller
    >>> data manual:
    >>>
    >>>     section 8.2.3.3 Buffer Size Rules and Zero-Length Packets:
    >>>
    >>>     For OUT endpoints, the following rules apply:
    >>>     - The BUFSIZ field must be ? 1 byte.
    >>>     - The total size of a Buffer Descriptor must be a multiple of
    >>>       MaxPacketSize
    >>>     - A received zero-length packet still requires a MaxPacketSize buffer.
    >>>       Therefore, if the expected amount of data to be received is a multiple
    >>>       of MaxPacketSize, software should add MaxPacketSize bytes to the buffer
    >>>       to sink a possible zero-length packet at the end of the transfer.
    >>>
    >>> But other UDC controllers don't need such alignment, so mentioned field
    >>> is set to "false". If buffer size is aligned to wMaxPacketSize, those
    >>> controllers may stuck on transaction. The example is DWC2.
    >>>
    >>> This patch checks gadget->quirk_ep_out_aligned_size field and aligns
    >>> rx_bytes_expected to wMaxPacketSize only when it's needed.
    >>>
    >>> Signed-off-by: Sam Protsenko <semen.protsenko at linaro.org <mailto:[email protected]>>
    >>> ---
    >>>  drivers/usb/gadget/f_fastboot.c | 9 +++++++++
    >>>  1 file changed, 9 insertions(+)
    >>>
    >>> diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
    >>> index 2e87fee..54dcce0 100644
    >>> --- a/drivers/usb/gadget/f_fastboot.c
    >>> +++ b/drivers/usb/gadget/f_fastboot.c
    >>> @@ -58,6 +58,7 @@ static unsigned int fastboot_flash_session_id;
    >>>  static unsigned int download_size;
    >>>  static unsigned int download_bytes;
    >>>  static bool is_high_speed;
    >>> +static bool quirk_ep_out_aligned_size;
    >>>
    >>>  static struct usb_endpoint_descriptor fs_ep_in = {
    >>>       .bLength            = USB_DT_ENDPOINT_SIZE,
    >>> @@ -240,6 +241,8 @@ static int fastboot_set_alt(struct usb_function *f,
    >>>       debug("%s: func: %s intf: %d alt: %d\n",
    >>>             __func__, f->name, interface, alt);
    >>>
    >>> +     quirk_ep_out_aligned_size = gadget->quirk_ep_out_aligned_size;
    >>> +
    >>>       /* make sure we don't enable the ep twice */
    >>>       if (gadget->speed == USB_SPEED_HIGH) {
    >>>               ret = usb_ep_enable(f_fb->out_ep, &hs_ep_out);
    >>> @@ -435,12 +438,18 @@ static unsigned int rx_bytes_expected(unsigned int maxpacket)
    >>>               return 0;
    >>>       if (rx_remain > EP_BUFFER_SIZE)
    >>>               return EP_BUFFER_SIZE;
    >>> +
    >>> +     if (!quirk_ep_out_aligned_size)
    >>> +             goto out;
    >>> +
    >>>       if (rx_remain < maxpacket) {
    >>>               rx_remain = maxpacket;
    >>>       } else if (rx_remain % maxpacket != 0) {
    >>>               rem = rx_remain % maxpacket;
    >>>               rx_remain = rx_remain + (maxpacket - rem);
    >>>       }
    >>> +
    >>> +out:
    >>>       return rx_remain;
    >>>  }
    >>>
    >>>
    >>
    >> Why do we need a special flag for this driver if other drivers e.g. mass storage
    >> are doing perfectly fine without it?
    >>
    >
    > I don't know how it works in other gadgets, but please see this patch
    > in kernel: [1]. That patch is doing just the same as I did (and also
    > in gadget code), using usb_ep_align_maybe() function for alignment.

    NOTE: there haven't been such quirks in the kernel drivers except for that one
    driver that has a user mode interface and needs more moral policing for user
    provided buffers. So that example is not something we should be using as reference.
    Our buffers are alreay aligned to maxpacket size. The only thing we need to worry
    about is the length of the last transaction that is not integral multiple of
    maxpacket size.

    If my understanding is right all USB controllers should work fine with
    bulk OUT requests that are integral multiple of maxpacket size.
    So we shouldn't be needing any quirk flags.

    >
    >> This patch is just covering up the real problem, by bypassing the faulty code
    >> with a flag.
    >>
    >> The buffer size is EP_BUFFER_SIZE and is already aligned to wMaxPacketSize so
    >> the problem shouldn't have happened in the first place. But it is happening
    >> indicating something else is wrong.
    >>
    >
    > There is what I'm observing on platform with DWC3 controller:
    >  - when doing "fastboot flash xloader MLO":
    >  - the whole size of data to send is 70964 bytes
    >  - the size of all packets (except of last packet) is 4096 bytes
    >  - so those are being sent just fine (in req->complete, which is
    > rx_handler_dl_image() function)
    >  - but last packet has size of 1332 bytes (because 70964 % 4096 = 1332)
    >  - when its req->length is aligned to wMaxPacketSize (so it's 1536
    > bytes): after we send it using usb_ep_queue(), the req->complete
    > callback is called one last time and we see that transmission is
    > finished (download_bytes >= download_size)
    >  - but when its req->length is not aligned to wMaxPacketSize (so it's
    > 1332 bytes): req->complete callback is not called last time, so
    > transaction is not finished and we are stuck in "fastboot flash"
    >
    > So I guess the issue is real and related to DWC3 quirk. If you have
    > any thoughts regarding other possible causes of this problem -- please
    > share. I can't predict all possible causes as I'm not USB expert.

    I've tried to clean up the bulk out handling code in the below patch.
    Note you will need to apply this on top of the 2 patches I sent earlier.
    https://patchwork.ozlabs.org/patch/609417/
    https://patchwork.ozlabs.org/patch/609896/

    Steve, do let me know if it works for you. If it doesn't then we need to figure out why.
    Can you please share details about the USB controller on your board?
    Does it not like OUT requests that are aligned to maxpacket size?
    What does ep->maxpacket show for your board?

with logging in "drivers/usb/gadget/dwc2_udc_otg.c" enabled, output is (for the last transactions...):

Quoted 7 lines

*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000
        EP2-OUT : DOEPINT = 0x2011
complete_rx: RX DMA done : ep = 2, rx bytes = 4096/4096, is_short = 0, DOEPTSIZ = 0x0, remained bytes = 4096
complete_rx: Next Rx request start...
setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x401000, DOEPCTL = 0x80098200
        buf = 0xffb84f80, pktcnt = 8, xfersize = 4096

OK so we asked for 4096 bytes and looks like we received 3968 bytes, which is probably the end of transfer.

Quoted 9 lines

*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000
        EP2-OUT : DOEPINT = 0x2011
complete_rx: RX DMA done : ep = 2, rx bytes = 3968/4096, is_short = 0, DOEPTSIZ = 0x80, remained bytes = 3968
setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb85f00,DOEPTSIZ = 0x80080, DOEPCTL = 0x80098200
        buf = 0xffb85f00, pktcnt = 1, xfersize = 128
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
and it hangs here!!!

The dwc2 driver should have returned then and not queued another 128 bytes. IMO there is a bug in the dwc2 driver.

Quoted 8 lines

3968 = 7 x 512 + 384
This means the last packet (384 bytes) was a short packet and it signals end of transfer so the dwc2 driver shouldn't be queuing another transfer. It should end the usb ep_queue request.
So, question to dwc2 mantainers.
Can we modify dwc2 driver to not automatically queue a pending transfer if the transfer ended in a short packet?

BTW, this is mentioned in the USB Specification.
Ref: USB2.0 Secification: Section. 5.3.2 Pipes

"An IRP (I/O request packet) may require multiple data payloads to move the client data over the bus. The data payloads for such a multiple data payload IRP are expected to be of the maximum packet size until the last data payload that contains the remainder of the overall IRP. See the description of each transfer type for more details. For such an IRP, short packets (i.e., less than maximum-sized data payloads) on input that do not completely fill an IRP data buffer can have one of two possible meanings, depending upon the expectations of a client:
? A client can expect a variable-sized amount of data in an IRP. In this case, a short packet that does not fill an IRP data buffer can be used simply as an in-band delimiter to indicate ?end of unit of data.? The IRP should be retired without error and the Host Controller should advance to the next IRP.
? A client can expect a specific-sized amount of data. In this case, a short packet that does not fill an IRP data buffer is an indication of an error. The IRP should be retired, the pipe should be stalled, and any pending IRPs associated with the pipe should also be retired."

I think we want the controller to behave as the first case since we haven't set the short_not_ok flag in the USB request.

So the below patch to the dwc2 driver should fix it.

--
cheers,
-roger

drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c +1 -1

index bce9c30..a31d875 100644
@@ -229,7 +229,7 @@ static void complete_rx(struct dwc2_udc *dev, u8 ep_num)
 				ROUND(xfer_size, CONFIG_SYS_CACHELINE_SIZE));
 
 	req->req.actual += min(xfer_size, req->req.length - req->req.actual);
-	is_short = (xfer_size < ep->ep.maxpacket);
+	is_short = xfer_size % ep->ep.maxpacket;
 
 	debug_cond(DEBUG_OUT_EP != 0,
 		   "%s: RX DMA done : ep = %d, rx bytes = %d/%d, "

Lukasz Majewski

2016-04-18 13:47 UTC #10 lore raw

PATCH 1/1 fastboot: Fix OUT transaction length alignment

Hi Roger, Steve,

Quoted 184 lines

+Lukazs
Quoted 181 lines · Roger Quadros
Quoted 179 lines · Steve Rae
On Thu, Apr 14, 2016 at 3:18 AM, Roger Quadros <[email protected] <mailto:[email protected]>> wrote:
    Hi,

    On 13/04/16 19:56, Sam Protsenko wrote:
    > On Wed, Apr 13, 2016 at 3:32 PM, Roger Quadros
    > <rogerq at ti.com <mailto:[email protected]>> wrote:
    >> Hi,
    >>
    >> On 13/04/16 15:01, Semen Protsenko wrote:
    >>> From: Sam Protsenko <[email protected]
    >>> <mailto:[email protected]>>
    >>>
    >>> Some UDC controllers may require buffer size to be aligned
    >>> to wMaxPacketSize. It's indicated by
    >>> gadget->quirk_ep_out_aligned_size field being set to
    >>> "true" (in UDC driver code). In that case
    >>> rx_bytes_expected must be aligned to wMaxPacket size,
    >>> otherwise stuck on transaction will happen. For example,
    >>> it's required by DWC3 controller data manual:
    >>>
    >>>     section 8.2.3.3 Buffer Size Rules and Zero-Length
    >>> Packets:
    >>>
    >>>     For OUT endpoints, the following rules apply:
    >>>     - The BUFSIZ field must be ? 1 byte.
    >>>     - The total size of a Buffer Descriptor must be a
    >>> multiple of MaxPacketSize
    >>>     - A received zero-length packet still requires a
    >>> MaxPacketSize buffer. Therefore, if the expected amount of
    >>> data to be received is a multiple of MaxPacketSize,
    >>> software should add MaxPacketSize bytes to the buffer to
    >>> sink a possible zero-length packet at the end of the
    >>> transfer.
    >>>
    >>> But other UDC controllers don't need such alignment, so
    >>> mentioned field is set to "false". If buffer size is
    >>> aligned to wMaxPacketSize, those controllers may stuck on
    >>> transaction. The example is DWC2.
    >>>
    >>> This patch checks gadget->quirk_ep_out_aligned_size field
    >>> and aligns rx_bytes_expected to wMaxPacketSize only when
    >>> it's needed.
    >>>
    >>> Signed-off-by: Sam Protsenko <[email protected]
    >>> <mailto:[email protected]>> ---
    >>>  drivers/usb/gadget/f_fastboot.c | 9 +++++++++
    >>>  1 file changed, 9 insertions(+)
    >>>
    >>> diff --git a/drivers/usb/gadget/f_fastboot.c
    >>> b/drivers/usb/gadget/f_fastboot.c index 2e87fee..54dcce0
    >>> 100644 --- a/drivers/usb/gadget/f_fastboot.c
    >>> +++ b/drivers/usb/gadget/f_fastboot.c
    >>> @@ -58,6 +58,7 @@ static unsigned int
    >>> fastboot_flash_session_id; static unsigned int
    >>> download_size; static unsigned int download_bytes;
    >>>  static bool is_high_speed;
    >>> +static bool quirk_ep_out_aligned_size;
    >>>
    >>>  static struct usb_endpoint_descriptor fs_ep_in = {
    >>>       .bLength            = USB_DT_ENDPOINT_SIZE,
    >>> @@ -240,6 +241,8 @@ static int fastboot_set_alt(struct
    >>> usb_function *f, debug("%s: func: %s intf: %d alt: %d\n",
    >>>             __func__, f->name, interface, alt);
    >>>
    >>> +     quirk_ep_out_aligned_size =
    >>> gadget->quirk_ep_out_aligned_size; +
    >>>       /* make sure we don't enable the ep twice */
    >>>       if (gadget->speed == USB_SPEED_HIGH) {
    >>>               ret = usb_ep_enable(f_fb->out_ep,
    >>> &hs_ep_out); @@ -435,12 +438,18 @@ static unsigned int
    >>> rx_bytes_expected(unsigned int maxpacket) return 0;
    >>>       if (rx_remain > EP_BUFFER_SIZE)
    >>>               return EP_BUFFER_SIZE;
    >>> +
    >>> +     if (!quirk_ep_out_aligned_size)
    >>> +             goto out;
    >>> +
    >>>       if (rx_remain < maxpacket) {
    >>>               rx_remain = maxpacket;
    >>>       } else if (rx_remain % maxpacket != 0) {
    >>>               rem = rx_remain % maxpacket;
    >>>               rx_remain = rx_remain + (maxpacket - rem);
    >>>       }
    >>> +
    >>> +out:
    >>>       return rx_remain;
    >>>  }
    >>>
    >>>
    >>
    >> Why do we need a special flag for this driver if other
    >> drivers e.g. mass storage are doing perfectly fine without
    >> it?
    >>
    >
    > I don't know how it works in other gadgets, but please see
    > this patch in kernel: [1]. That patch is doing just the same
    > as I did (and also in gadget code), using
    > usb_ep_align_maybe() function for alignment.

    NOTE: there haven't been such quirks in the kernel drivers
except for that one driver that has a user mode interface and needs more moral policing for user provided buffers. So that example is not something we should be using as reference. Our buffers are alreay aligned to maxpacket size. The only thing we need to worry about is the length of the last transaction that is not integral multiple of maxpacket size.
If my understanding is right all USB controllers should work
fine with bulk OUT requests that are integral multiple of maxpacket size. So we shouldn't be needing any quirk flags.
Quoted 29 lines
Quoted 9 lines
This patch is just covering up the real problem, by bypassing the faulty code with a flag.
The buffer size is EP_BUFFER_SIZE and is already aligned to wMaxPacketSize so the problem shouldn't have happened in the first place. But it is happening indicating something else is wrong.
There is what I'm observing on platform with DWC3 controller:
- when doing "fastboot flash xloader MLO":
- the whole size of data to send is 70964 bytes
- the size of all packets (except of last packet) is 4096
bytes
- so those are being sent just fine (in req->complete,
which is rx_handler_dl_image() function)
- but last packet has size of 1332 bytes (because 70964 %
4096 = 1332)
- when its req->length is aligned to wMaxPacketSize (so
it's 1536 bytes): after we send it using usb_ep_queue(), the req->complete callback is called one last time and we see that transmission is finished (download_bytes >= download_size)
- but when its req->length is not aligned to wMaxPacketSize
(so it's 1332 bytes): req->complete callback is not called last time, so transaction is not finished and we are stuck in "fastboot flash"
Quoted 4 lines
So I guess the issue is real and related to DWC3 quirk. If you have any thoughts regarding other possible causes of this problem -- please share. I can't predict all possible causes as I'm not USB expert.
I've tried to clean up the bulk out handling code in the below
patch. Note you will need to apply this on top of the 2 patches I sent earlier. https://patchwork.ozlabs.org/patch/609417/
https://patchwork.ozlabs.org/patch/609896/
Steve, do let me know if it works for you. If it doesn't then
we need to figure out why. Can you please share details about the USB controller on your board? Does it not like OUT requests that are aligned to maxpacket size? What does ep->maxpacket show for your board?
This (still) does not work....
- using the standard U-Boot DWC2 driver,
- do not know if it doesn't like "OUT requests that are aligned to maxpacket size" -- perhaps @Lukasz could answer this....
- ep->maxpacket is 512
with logging in "drivers/usb/gadget/dwc2_udc_otg.c" enabled, output is (for the last transactions...):
*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 ***
process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 EP2-OUT :
DOEPINT = 0x2011 complete_rx: RX DMA done : ep = 2, rx bytes = 4096/4096, is_short = 0, DOEPTSIZ = 0x0, remained bytes = 4096
complete_rx: Next Rx request start... setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x401000, DOEPCTL = 0x80098200 buf = 0xffb84f80, pktcnt = 8, xfersize = 4096

It is _real_ hard for me to debug protocol which I'm not using on HW which I don't posses.

However, I will do my best to fix this bug. Hence please be patient with my questions.

Steve, how much bytes do you send?

Quoted 2 lines

Quoted 2 lines
OK so we asked for 4096 bytes and looks like we received 3968 bytes, which is probably the end of transfer.

I think that you refer to the below code.

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 ***
process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 EP2-OUT :
DOEPINT = 0x2011 complete_rx: RX DMA done : ep = 2, rx bytes = 3968/4096,

So we have following situation here:
We received 3968B from 4096B (missing 128B)

Quoted 1 line

is_short = 0

This should be equal to 1.

Quoted 1 line

, DOEPTSIZ = 0x80,

We are supposed (now) to receive 128 B more.

Quoted 1 line

remained bytes = 3968

This is totally wrong. Here we should have 128 B.

Quoted 40 lines

Quoted 9 lines
Quoted 5 lines
setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb85f00,DOEPTSIZ = 0x80080, DOEPCTL = 0x80098200 buf = 0xffb85f00, pktcnt = 1, xfersize = 128
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
and it hangs here!!!
The dwc2 driver should have returned then and not queued another 128 bytes. IMO there is a bug in the dwc2 driver.
Quoted 8 lines
3968 = 7 x 512 + 384
This means the last packet (384 bytes) was a short packet and it signals end of transfer so the dwc2 driver shouldn't be queuing another transfer. It should end the usb ep_queue request.
So, question to dwc2 mantainers.
Can we modify dwc2 driver to not automatically queue a pending transfer if the transfer ended in a short packet?
BTW, this is mentioned in the USB Specification.
Ref: USB2.0 Secification: Section. 5.3.2 Pipes
"An IRP (I/O request packet) may require multiple data payloads to move the client data over the bus. The data payloads for such a multiple data payload IRP are expected to be of the maximum packet size until the last data payload that contains the remainder of the overall IRP. See the description of each transfer type for more details. For such an IRP, short packets (i.e., less than maximum-sized data payloads)
on input that do not completely fill an
IRP data buffer can have one of two possible meanings, depending upon the expectations of a client: ? A client can expect a variable-sized amount of data in an IRP. In this case, a short packet that does not fill an IRP data buffer can be used simply as an in-band delimiter to indicate ?end of unit of data.? The IRP should be retired without error and the Host Controller should advance to the next IRP. ? A client can expect a specific-sized amount of data. In this case, a short packet that does not fill an IRP data buffer is an indication of an error. The IRP should be retired, the pipe should be stalled, and any pending IRPs associated with the pipe should also be retired."

Quoted 2 lines

I think we want the controller to behave as the first case since we haven't set the short_not_ok flag in the USB request.

short_not_ok flag is used solely in USB Mass Storage gadget (which is correct for it).

However, dwc2 UDC driver is not explicitly supporting it (but unfortunately does it implicitly).

This of course should be fixed.

One question to Steve - could you check if below Roger's change fixes your problem?

I will also test it - however, current tests aren't covering this situation:

1. DFU uses EP0 (not EPn bulk) - this works since we can send or receive e.g. 65 B

2. USB Mass Storage expects (UMS) transmission to be a multiple of wMaxPacketSize (there is some caching done) and uses short_not_ok flag.

3. THOR protocol is padded to LBA size (512 B) as it is convenient for eMMC flashing.

I do need to come up with new one.

Roger, thanks for your support and effort.

Quoted 20 lines

So the below patch to the dwc2 driver should fix it.
--
cheers,
(quoted diff) +0 -1
-roger
(quoted diff) +0 -0
b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c index bce9c30..a31d875 100644 --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c +1 -1
@@ -229,7 +229,7 @@ static void complete_rx(struct dwc2_udc *dev, u8
 ep_num) ROUND(xfer_size, CONFIG_SYS_CACHELINE_SIZE));
 
 	req->req.actual += min(xfer_size, req->req.length -
 req->req.actual);
-	is_short = (xfer_size < ep->ep.maxpacket);
+	is_short = xfer_size % ep->ep.maxpacket;
 
 	debug_cond(DEBUG_OUT_EP != 0,

Quoted 2 lines

"%s: RX DMA done : ep = %d, rx bytes = %d/%d, "

I will test this change.

Signature · 5 lines

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Roger Quadros

2016-04-18 13:55 UTC #11 lore raw

PATCH 1/1 fastboot: Fix OUT transaction length alignment

Quoted 208 lines · Lukasz Majewski

Hi Roger, Steve,
Quoted 184 lines
+Lukazs
Quoted 181 lines · Roger Quadros
Quoted 179 lines · Steve Rae
On Thu, Apr 14, 2016 at 3:18 AM, Roger Quadros <[email protected] <mailto:[email protected]>> wrote:
    Hi,

    On 13/04/16 19:56, Sam Protsenko wrote:
    > On Wed, Apr 13, 2016 at 3:32 PM, Roger Quadros
    > <rogerq at ti.com <mailto:[email protected]>> wrote:
    >> Hi,
    >>
    >> On 13/04/16 15:01, Semen Protsenko wrote:
    >>> From: Sam Protsenko <[email protected]
    >>> <mailto:[email protected]>>
    >>>
    >>> Some UDC controllers may require buffer size to be aligned
    >>> to wMaxPacketSize. It's indicated by
    >>> gadget->quirk_ep_out_aligned_size field being set to
    >>> "true" (in UDC driver code). In that case
    >>> rx_bytes_expected must be aligned to wMaxPacket size,
    >>> otherwise stuck on transaction will happen. For example,
    >>> it's required by DWC3 controller data manual:
    >>>
    >>>     section 8.2.3.3 Buffer Size Rules and Zero-Length
    >>> Packets:
    >>>
    >>>     For OUT endpoints, the following rules apply:
    >>>     - The BUFSIZ field must be ? 1 byte.
    >>>     - The total size of a Buffer Descriptor must be a
    >>> multiple of MaxPacketSize
    >>>     - A received zero-length packet still requires a
    >>> MaxPacketSize buffer. Therefore, if the expected amount of
    >>> data to be received is a multiple of MaxPacketSize,
    >>> software should add MaxPacketSize bytes to the buffer to
    >>> sink a possible zero-length packet at the end of the
    >>> transfer.
    >>>
    >>> But other UDC controllers don't need such alignment, so
    >>> mentioned field is set to "false". If buffer size is
    >>> aligned to wMaxPacketSize, those controllers may stuck on
    >>> transaction. The example is DWC2.
    >>>
    >>> This patch checks gadget->quirk_ep_out_aligned_size field
    >>> and aligns rx_bytes_expected to wMaxPacketSize only when
    >>> it's needed.
    >>>
    >>> Signed-off-by: Sam Protsenko <[email protected]
    >>> <mailto:[email protected]>> ---
    >>>  drivers/usb/gadget/f_fastboot.c | 9 +++++++++
    >>>  1 file changed, 9 insertions(+)
    >>>
    >>> diff --git a/drivers/usb/gadget/f_fastboot.c
    >>> b/drivers/usb/gadget/f_fastboot.c index 2e87fee..54dcce0
    >>> 100644 --- a/drivers/usb/gadget/f_fastboot.c
    >>> +++ b/drivers/usb/gadget/f_fastboot.c
    >>> @@ -58,6 +58,7 @@ static unsigned int
    >>> fastboot_flash_session_id; static unsigned int
    >>> download_size; static unsigned int download_bytes;
    >>>  static bool is_high_speed;
    >>> +static bool quirk_ep_out_aligned_size;
    >>>
    >>>  static struct usb_endpoint_descriptor fs_ep_in = {
    >>>       .bLength            = USB_DT_ENDPOINT_SIZE,
    >>> @@ -240,6 +241,8 @@ static int fastboot_set_alt(struct
    >>> usb_function *f, debug("%s: func: %s intf: %d alt: %d\n",
    >>>             __func__, f->name, interface, alt);
    >>>
    >>> +     quirk_ep_out_aligned_size =
    >>> gadget->quirk_ep_out_aligned_size; +
    >>>       /* make sure we don't enable the ep twice */
    >>>       if (gadget->speed == USB_SPEED_HIGH) {
    >>>               ret = usb_ep_enable(f_fb->out_ep,
    >>> &hs_ep_out); @@ -435,12 +438,18 @@ static unsigned int
    >>> rx_bytes_expected(unsigned int maxpacket) return 0;
    >>>       if (rx_remain > EP_BUFFER_SIZE)
    >>>               return EP_BUFFER_SIZE;
    >>> +
    >>> +     if (!quirk_ep_out_aligned_size)
    >>> +             goto out;
    >>> +
    >>>       if (rx_remain < maxpacket) {
    >>>               rx_remain = maxpacket;
    >>>       } else if (rx_remain % maxpacket != 0) {
    >>>               rem = rx_remain % maxpacket;
    >>>               rx_remain = rx_remain + (maxpacket - rem);
    >>>       }
    >>> +
    >>> +out:
    >>>       return rx_remain;
    >>>  }
    >>>
    >>>
    >>
    >> Why do we need a special flag for this driver if other
    >> drivers e.g. mass storage are doing perfectly fine without
    >> it?
    >>
    >
    > I don't know how it works in other gadgets, but please see
    > this patch in kernel: [1]. That patch is doing just the same
    > as I did (and also in gadget code), using
    > usb_ep_align_maybe() function for alignment.

    NOTE: there haven't been such quirks in the kernel drivers
except for that one driver that has a user mode interface and needs more moral policing for user provided buffers. So that example is not something we should be using as reference. Our buffers are alreay aligned to maxpacket size. The only thing we need to worry about is the length of the last transaction that is not integral multiple of maxpacket size.
If my understanding is right all USB controllers should work
fine with bulk OUT requests that are integral multiple of maxpacket size. So we shouldn't be needing any quirk flags.
Quoted 29 lines
Quoted 9 lines
This patch is just covering up the real problem, by bypassing the faulty code with a flag.
The buffer size is EP_BUFFER_SIZE and is already aligned to wMaxPacketSize so the problem shouldn't have happened in the first place. But it is happening indicating something else is wrong.
There is what I'm observing on platform with DWC3 controller:
- when doing "fastboot flash xloader MLO":
- the whole size of data to send is 70964 bytes
- the size of all packets (except of last packet) is 4096
bytes
- so those are being sent just fine (in req->complete,
which is rx_handler_dl_image() function)
- but last packet has size of 1332 bytes (because 70964 %
4096 = 1332)
- when its req->length is aligned to wMaxPacketSize (so
it's 1536 bytes): after we send it using usb_ep_queue(), the req->complete callback is called one last time and we see that transmission is finished (download_bytes >= download_size)
- but when its req->length is not aligned to wMaxPacketSize
(so it's 1332 bytes): req->complete callback is not called last time, so transaction is not finished and we are stuck in "fastboot flash"
Quoted 4 lines
So I guess the issue is real and related to DWC3 quirk. If you have any thoughts regarding other possible causes of this problem -- please share. I can't predict all possible causes as I'm not USB expert.
I've tried to clean up the bulk out handling code in the below
patch. Note you will need to apply this on top of the 2 patches I sent earlier. https://patchwork.ozlabs.org/patch/609417/
https://patchwork.ozlabs.org/patch/609896/
Steve, do let me know if it works for you. If it doesn't then
we need to figure out why. Can you please share details about the USB controller on your board? Does it not like OUT requests that are aligned to maxpacket size? What does ep->maxpacket show for your board?
This (still) does not work....
- using the standard U-Boot DWC2 driver,
- do not know if it doesn't like "OUT requests that are aligned to maxpacket size" -- perhaps @Lukasz could answer this....
- ep->maxpacket is 512
with logging in "drivers/usb/gadget/dwc2_udc_otg.c" enabled, output is (for the last transactions...):
*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 ***
process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 EP2-OUT :
DOEPINT = 0x2011 complete_rx: RX DMA done : ep = 2, rx bytes = 4096/4096, is_short = 0, DOEPTSIZ = 0x0, remained bytes = 4096
complete_rx: Next Rx request start... setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x401000, DOEPCTL = 0x80098200 buf = 0xffb84f80, pktcnt = 8, xfersize = 4096
It is _real_ hard for me to debug protocol which I'm not using on HW which I don't posses.
However, I will do my best to fix this bug. Hence please be patient with my questions.
Steve, how much bytes do you send?
Quoted 2 lines
Quoted 2 lines
OK so we asked for 4096 bytes and looks like we received 3968 bytes, which is probably the end of transfer.
I think that you refer to the below code.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 ***
process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 EP2-OUT :
DOEPINT = 0x2011 complete_rx: RX DMA done : ep = 2, rx bytes = 3968/4096,

Quoted 6 lines

So we have following situation here:
We received 3968B from 4096B (missing 128B)
Quoted 1 line
is_short = 0
This should be equal to 1.

This should be fixed by the patch I sent inline.

Quoted 8 lines

Quoted 2 lines
, DOEPTSIZ = 0x80,
We are supposed (now) to receive 128 B more.
Quoted 1 line
remained bytes = 3968
This is totally wrong. Here we should have 128 B.

Exactly. The debug print is using the old xfersize to print remaining size. That must be fixed as well.
I can do that along with the official patch once Steve confirms that it works.

Quoted 66 lines

Quoted 41 lines
Quoted 10 lines
Quoted 6 lines
setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb85f00,DOEPTSIZ = 0x80080, DOEPCTL = 0x80098200 buf = 0xffb85f00, pktcnt = 1, xfersize = 128
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
and it hangs here!!!
The dwc2 driver should have returned then and not queued another 128 bytes. IMO there is a bug in the dwc2 driver.
Quoted 8 lines
3968 = 7 x 512 + 384
This means the last packet (384 bytes) was a short packet and it signals end of transfer so the dwc2 driver shouldn't be queuing another transfer. It should end the usb ep_queue request.
So, question to dwc2 mantainers.
Can we modify dwc2 driver to not automatically queue a pending transfer if the transfer ended in a short packet?
BTW, this is mentioned in the USB Specification.
Ref: USB2.0 Secification: Section. 5.3.2 Pipes
"An IRP (I/O request packet) may require multiple data payloads to move the client data over the bus. The data payloads for such a multiple data payload IRP are expected to be of the maximum packet size until the last data payload that contains the remainder of the overall IRP. See the description of each transfer type for more details. For such an IRP, short packets (i.e., less than maximum-sized data payloads)
on input that do not completely fill an
IRP data buffer can have one of two possible meanings, depending upon the expectations of a client: ? A client can expect a variable-sized amount of data in an IRP. In this case, a short packet that does not fill an IRP data buffer can be used simply as an in-band delimiter to indicate ?end of unit of data.? The IRP should be retired without error and the Host Controller should advance to the next IRP. ? A client can expect a specific-sized amount of data. In this case, a short packet that does not fill an IRP data buffer is an indication of an error. The IRP should be retired, the pipe should be stalled, and any pending IRPs associated with the pipe should also be retired."
Quoted 2 lines
I think we want the controller to behave as the first case since we haven't set the short_not_ok flag in the USB request.
short_not_ok flag is used solely in USB Mass Storage gadget (which is correct for it).
However, dwc2 UDC driver is not explicitly supporting it (but unfortunately does it implicitly).
This of course should be fixed.
One question to Steve - could you check if below Roger's change fixes your problem?
I will also test it - however, current tests aren't covering this situation:
1. DFU uses EP0 (not EPn bulk) - this works since we can send or receive e.g. 65 B
2. USB Mass Storage expects (UMS) transmission to be a multiple of wMaxPacketSize (there is some caching done) and uses short_not_ok flag.

Quoted 7 lines

3. THOR protocol is padded to LBA size (512 B) as it is convenient for eMMC flashing.
I do need to come up with new one.
Roger, thanks for your support and effort.

No problem :).

Quoted 20 lines

Quoted 20 lines
So the below patch to the dwc2 driver should fix it.
--
cheers,
(quoted diff) +0 -1
-roger
(quoted diff) +0 -0
b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c index bce9c30..a31d875 100644 --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c +1 -1
@@ -229,7 +229,7 @@ static void complete_rx(struct dwc2_udc *dev, u8
 ep_num) ROUND(xfer_size, CONFIG_SYS_CACHELINE_SIZE));
 
 	req->req.actual += min(xfer_size, req->req.length -
 req->req.actual);
-	is_short = (xfer_size < ep->ep.maxpacket);
+	is_short = xfer_size % ep->ep.maxpacket;
 

Quoted 6 lines

Quoted 3 lines
drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c +0 -0
 	debug_cond(DEBUG_OUT_EP != 0,
"%s: RX DMA done : ep = %d, rx bytes = %d/%d, "
I will test this change.

Thanks.

cheers,
-roger

Steve Rae

2016-04-18 15:43 UTC #12 lore raw

PATCH 1/1 fastboot: Fix OUT transaction length alignment

Quoted 321 lines · Roger Quadros

Quoted 208 lines · Lukasz Majewski
Hi Roger, Steve,
Quoted 184 lines
+Lukazs
Quoted 181 lines · Roger Quadros
Quoted 179 lines · Steve Rae
On Thu, Apr 14, 2016 at 3:18 AM, Roger Quadros <[email protected] <mailto:[email protected]>> wrote:
    Hi,

    On 13/04/16 19:56, Sam Protsenko wrote:
    > On Wed, Apr 13, 2016 at 3:32 PM, Roger Quadros
    > <rogerq at ti.com <mailto:[email protected]>> wrote:
    >> Hi,
    >>
    >> On 13/04/16 15:01, Semen Protsenko wrote:
    >>> From: Sam Protsenko <[email protected]
    >>> <mailto:[email protected]>>
    >>>
    >>> Some UDC controllers may require buffer size to be aligned
    >>> to wMaxPacketSize. It's indicated by
    >>> gadget->quirk_ep_out_aligned_size field being set to
    >>> "true" (in UDC driver code). In that case
    >>> rx_bytes_expected must be aligned to wMaxPacket size,
    >>> otherwise stuck on transaction will happen. For example,
    >>> it's required by DWC3 controller data manual:
    >>>
    >>>     section 8.2.3.3 Buffer Size Rules and Zero-Length
    >>> Packets:
    >>>
    >>>     For OUT endpoints, the following rules apply:
    >>>     - The BUFSIZ field must be ? 1 byte.
    >>>     - The total size of a Buffer Descriptor must be a
    >>> multiple of MaxPacketSize
    >>>     - A received zero-length packet still requires a
    >>> MaxPacketSize buffer. Therefore, if the expected amount of
    >>> data to be received is a multiple of MaxPacketSize,
    >>> software should add MaxPacketSize bytes to the buffer to
    >>> sink a possible zero-length packet at the end of the
    >>> transfer.
    >>>
    >>> But other UDC controllers don't need such alignment, so
    >>> mentioned field is set to "false". If buffer size is
    >>> aligned to wMaxPacketSize, those controllers may stuck on
    >>> transaction. The example is DWC2.
    >>>
    >>> This patch checks gadget->quirk_ep_out_aligned_size field
    >>> and aligns rx_bytes_expected to wMaxPacketSize only when
    >>> it's needed.
    >>>
    >>> Signed-off-by: Sam Protsenko <[email protected]
    >>> <mailto:[email protected]>> ---
    >>>  drivers/usb/gadget/f_fastboot.c | 9 +++++++++
    >>>  1 file changed, 9 insertions(+)
    >>>
    >>> diff --git a/drivers/usb/gadget/f_fastboot.c
    >>> b/drivers/usb/gadget/f_fastboot.c index 2e87fee..54dcce0
    >>> 100644 --- a/drivers/usb/gadget/f_fastboot.c
    >>> +++ b/drivers/usb/gadget/f_fastboot.c
    >>> @@ -58,6 +58,7 @@ static unsigned int
    >>> fastboot_flash_session_id; static unsigned int
    >>> download_size; static unsigned int download_bytes;
    >>>  static bool is_high_speed;
    >>> +static bool quirk_ep_out_aligned_size;
    >>>
    >>>  static struct usb_endpoint_descriptor fs_ep_in = {
    >>>       .bLength            = USB_DT_ENDPOINT_SIZE,
    >>> @@ -240,6 +241,8 @@ static int fastboot_set_alt(struct
    >>> usb_function *f, debug("%s: func: %s intf: %d alt: %d\n",
    >>>             __func__, f->name, interface, alt);
    >>>
    >>> +     quirk_ep_out_aligned_size =
    >>> gadget->quirk_ep_out_aligned_size; +
    >>>       /* make sure we don't enable the ep twice */
    >>>       if (gadget->speed == USB_SPEED_HIGH) {
    >>>               ret = usb_ep_enable(f_fb->out_ep,
    >>> &hs_ep_out); @@ -435,12 +438,18 @@ static unsigned int
    >>> rx_bytes_expected(unsigned int maxpacket) return 0;
    >>>       if (rx_remain > EP_BUFFER_SIZE)
    >>>               return EP_BUFFER_SIZE;
    >>> +
    >>> +     if (!quirk_ep_out_aligned_size)
    >>> +             goto out;
    >>> +
    >>>       if (rx_remain < maxpacket) {
    >>>               rx_remain = maxpacket;
    >>>       } else if (rx_remain % maxpacket != 0) {
    >>>               rem = rx_remain % maxpacket;
    >>>               rx_remain = rx_remain + (maxpacket - rem);
    >>>       }
    >>> +
    >>> +out:
    >>>       return rx_remain;
    >>>  }
    >>>
    >>>
    >>
    >> Why do we need a special flag for this driver if other
    >> drivers e.g. mass storage are doing perfectly fine without
    >> it?
    >>
    >
    > I don't know how it works in other gadgets, but please see
    > this patch in kernel: [1]. That patch is doing just the same
    > as I did (and also in gadget code), using
    > usb_ep_align_maybe() function for alignment.

    NOTE: there haven't been such quirks in the kernel drivers
except for that one driver that has a user mode interface and needs more moral policing for user provided buffers. So that example is not something we should be using as reference. Our buffers are alreay aligned to maxpacket size. The only thing we need to worry about is the length of the last transaction that is not integral multiple of maxpacket size.
If my understanding is right all USB controllers should work
fine with bulk OUT requests that are integral multiple of maxpacket size. So we shouldn't be needing any quirk flags.
Quoted 29 lines
Quoted 9 lines
This patch is just covering up the real problem, by bypassing the faulty code with a flag.
The buffer size is EP_BUFFER_SIZE and is already aligned to wMaxPacketSize so the problem shouldn't have happened in the first place. But it is happening indicating something else is wrong.
There is what I'm observing on platform with DWC3 controller:
- when doing "fastboot flash xloader MLO":
- the whole size of data to send is 70964 bytes
- the size of all packets (except of last packet) is 4096
bytes
- so those are being sent just fine (in req->complete,
which is rx_handler_dl_image() function)
- but last packet has size of 1332 bytes (because 70964 %
4096 = 1332)
- when its req->length is aligned to wMaxPacketSize (so
it's 1536 bytes): after we send it using usb_ep_queue(), the req->complete callback is called one last time and we see that transmission is finished (download_bytes >= download_size)
- but when its req->length is not aligned to wMaxPacketSize
(so it's 1332 bytes): req->complete callback is not called last time, so transaction is not finished and we are stuck in "fastboot flash"
Quoted 4 lines
So I guess the issue is real and related to DWC3 quirk. If you have any thoughts regarding other possible causes of this problem -- please share. I can't predict all possible causes as I'm not USB expert.
I've tried to clean up the bulk out handling code in the below
patch. Note you will need to apply this on top of the 2 patches I sent earlier. https://patchwork.ozlabs.org/patch/609417/
https://patchwork.ozlabs.org/patch/609896/
Steve, do let me know if it works for you. If it doesn't then
we need to figure out why. Can you please share details about the USB controller on your board? Does it not like OUT requests that are aligned to maxpacket size? What does ep->maxpacket show for your board?
This (still) does not work....
- using the standard U-Boot DWC2 driver,
- do not know if it doesn't like "OUT requests that are aligned to maxpacket size" -- perhaps @Lukasz could answer this....
- ep->maxpacket is 512
with logging in "drivers/usb/gadget/dwc2_udc_otg.c" enabled, output is (for the last transactions...):
*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 ***
process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 EP2-OUT :
DOEPINT = 0x2011 complete_rx: RX DMA done : ep = 2, rx bytes = 4096/4096, is_short = 0, DOEPTSIZ = 0x0, remained bytes = 4096
complete_rx: Next Rx request start... setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x401000, DOEPCTL = 0x80098200 buf = 0xffb84f80, pktcnt = 8, xfersize = 4096
It is _real_ hard for me to debug protocol which I'm not using on HW which I don't posses.
However, I will do my best to fix this bug. Hence please be patient with my questions.
Steve, how much bytes do you send?
Quoted 2 lines
Quoted 2 lines
OK so we asked for 4096 bytes and looks like we received 3968 bytes, which is probably the end of transfer.
I think that you refer to the below code.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 ***
process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 EP2-OUT :
DOEPINT = 0x2011 complete_rx: RX DMA done : ep = 2, rx bytes = 3968/4096,
Quoted 6 lines
So we have following situation here:
We received 3968B from 4096B (missing 128B)
Quoted 1 line
is_short = 0
This should be equal to 1.
This should be fixed by the patch I sent inline.
Quoted 8 lines
Quoted 2 lines
, DOEPTSIZ = 0x80,
We are supposed (now) to receive 128 B more.
Quoted 1 line
remained bytes = 3968
This is totally wrong. Here we should have 128 B.
Exactly. The debug print is using the old xfersize to print remaining size. That must be fixed as well.
I can do that along with the official patch once Steve confirms that it works.
Quoted 66 lines
Quoted 41 lines
Quoted 10 lines
Quoted 6 lines
setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb85f00,DOEPTSIZ = 0x80080, DOEPCTL = 0x80098200 buf = 0xffb85f00, pktcnt = 1, xfersize = 128
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
and it hangs here!!!
The dwc2 driver should have returned then and not queued another 128 bytes. IMO there is a bug in the dwc2 driver.
Quoted 8 lines
3968 = 7 x 512 + 384
This means the last packet (384 bytes) was a short packet and it signals end of transfer so the dwc2 driver shouldn't be queuing another transfer. It should end the usb ep_queue request.
So, question to dwc2 mantainers.
Can we modify dwc2 driver to not automatically queue a pending transfer if the transfer ended in a short packet?
BTW, this is mentioned in the USB Specification.
Ref: USB2.0 Secification: Section. 5.3.2 Pipes
"An IRP (I/O request packet) may require multiple data payloads to move the client data over the bus. The data payloads for such a multiple data payload IRP are expected to be of the maximum packet size until the last data payload that contains the remainder of the overall IRP. See the description of each transfer type for more details. For such an IRP, short packets (i.e., less than maximum-sized data payloads)
on input that do not completely fill an
IRP data buffer can have one of two possible meanings, depending upon the expectations of a client: ? A client can expect a variable-sized amount of data in an IRP. In this case, a short packet that does not fill an IRP data buffer can be used simply as an in-band delimiter to indicate ?end of unit of data.? The IRP should be retired without error and the Host Controller should advance to the next IRP. ? A client can expect a specific-sized amount of data. In this case, a short packet that does not fill an IRP data buffer is an indication of an error. The IRP should be retired, the pipe should be stalled, and any pending IRPs associated with the pipe should also be retired."
Quoted 2 lines
I think we want the controller to behave as the first case since we haven't set the short_not_ok flag in the USB request.
short_not_ok flag is used solely in USB Mass Storage gadget (which is correct for it).
However, dwc2 UDC driver is not explicitly supporting it (but unfortunately does it implicitly).
This of course should be fixed.
One question to Steve - could you check if below Roger's change fixes your problem?
I will also test it - however, current tests aren't covering this situation:
1. DFU uses EP0 (not EPn bulk) - this works since we can send or receive e.g. 65 B
2. USB Mass Storage expects (UMS) transmission to be a multiple of wMaxPacketSize (there is some caching done) and uses short_not_ok flag.
Quoted 7 lines
3. THOR protocol is padded to LBA size (512 B) as it is convenient for eMMC flashing.
I do need to come up with new one.
Roger, thanks for your support and effort.
No problem :).
Quoted 15 lines
Quoted 15 lines
So the below patch to the dwc2 driver should fix it.
--
cheers,
(quoted diff) +0 -1
-roger
(quoted diff) +0 -0
b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c index bce9c30..a31d875 100644 --- a/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c +++ b/drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c
(quoted diff) +0 -0
@@ -229,7 +229,7 @@ static void complete_rx(struct dwc2_udc *dev, u8
 ep_num) ROUND(xfer_size, CONFIG_SYS_CACHELINE_SIZE));
 

Quoted 8 lines

drivers/usb/gadget/dwc2_udc_otg_xfer_dma.c +1 -1

     req->req.actual += min(xfer_size, req->req.length -
 req->req.actual);
-    is_short = (xfer_size < ep->ep.maxpacket);
+    is_short = xfer_size % ep->ep.maxpacket;
 
     debug_cond(DEBUG_OUT_EP != 0,

                "%s: RX DMA done : ep = %d, rx bytes = %d/%d, "

Roger, Lukasz - this gets past the "hang" on my boards, and it completes successfully! BTW, the last packet is now:

+++++ SRAE: (new) rx_remain=520, ep->maxpacket=512

complete_rx: Next Rx request start...
setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x100400, DOEPCTL = 0x80098200

        buf = 0xffb84f80, pktcnt = 2, xfersize = 1024

*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000

        EP2-OUT : DOEPINT = 0x2011

complete_rx: RX DMA done : ep = 2, rx bytes = 520/1024, is_short = 8, DOEPTSIZ = 0x1f8, remained bytes = 520
dwc2_queue: ep_is_in, DWC2_UDC_OTG_GINTSTS=0x14008028 setdma_tx:EP1 TX DMA start : DIEPDMA0 = 0xffb85fc0,DIEPTSIZ0 = 0x80004, DIEPCTL0 = 0x80498200

        buf = 0xffb85fc0, pktcnt = 1, xfersize = 4

downloading of 258568 bytes finished

Quoted 3 lines

Quoted 3 lines
I will test this change.

Lukasz: would you _like_ to be able to test fastboot?
- do you have a board that uses a GPT table, and has USB-OTG support?
- do you want a Linux (Ubuntu) version of the fastboot client (Windows is much more difficult - but it does work...)
Let me know, and I'll help you get it setup....

Quoted 5 lines

Thanks.
cheers,
-roger

Lukasz Majewski

2016-04-19 08:46 UTC #13 lore raw

PATCH 1/1 fastboot: Fix OUT transaction length alignment

Hi Steve,

Quoted 5 lines

Quoted 3 lines
Quoted 3 lines
I will test this change.
Lukasz: would you _like_ to be able to test fastboot?

Ok, I will setup this.

Quoted 1 line

- do you have a board that uses a GPT table, and has USB-OTG support?

I think that Trats2/Trats is such a board. I suppose that USB device support is enough? (We have USB-OTG capable HW, but we only support USB device)

Quoted 2 lines

- do you want a Linux (Ubuntu) version of the fastboot client (Windows is much more difficult - but it does work...)

I do have debian linux.

Quoted 1 line

Let me know, and I'll help you get it setup....

Thanks for offering your help.

Signature · 5 lines

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group