Fix for control transfers that exceed the max packet len

[fieldofcows]

This PR fixes Issue #6 where connecting a device to a tinyusb host that has a max control transfer packet size that is less than the descriptor being transferred causes incorrect parsing of the descriptor. This normally results in execution of a never ending tight loop in the tinyusb descriptor parsing code.

The issue has been fixed by detecting the case where more DATA packets are required to complete the transfer and remaining in the DATA state until the transfer has completed and the destination buffer contains the full set of data.

[lurch]

@hathach Is this a problem only with our fork of tinyusb, or does it affect your upstream version too?

[hathach]

It looks more like the pico hcd port issue and shouldn't be fixed at usbh as proposed by this PR.

1. usbh schedule a transfer, in this case is 0x0022 bytes.
2. The pico call hcd_event_xfer_complete() with only 8 bytes, which is not Correct.

Reason is that this mouse has control packet size = 8 which is typical for mouse ( not 64 as most other). So 8 bytes here is not short packet here, and pico should continue to read more until 0x0022 bytes is read, or a short packet (len < 8) is received.

Note: control packet size can be saved by rp2040 by the hcd_edpt_open() https://github.com/hathach/tinyusb/blob/master/src/host/usbh.c#L278

Note: you will notice that host stack (usbh) initially open control endpoint with only 8 bytes for new device (addr = 0)
https://github.com/hathach/tinyusb/blob/master/src/host/usbh.c#L685 . Then once it read the device descriptor and know the exact control packet size, it will open the right size with new address https://github.com/hathach/tinyusb/blob/master/src/host/usbh.c#L769

Sumup: we should fix rp2040 hcd instead.

[ismell]

I think this might be a better fix: https://github.com/raspberrypi/pico-sdk/issues/361#issuecomment-826465401
The problem I have with my fix, is that it uses the ep->wMaxPacketSize. It's not really an ep specific value. The MaxPacketSize should be stored on some kind of device descriptor because it applies to all endpoints for that device.

[fieldofcows]

Yes, you're right. I came to a similar conclusion after reading hathach's comment above. However, although fixing the issue in that way does result in the correct use of the rp2040 host buffer, it uncovers a further problem. In my case, with the log in the issue I raised, this should result in a transfer containing three data packets of size 8, 8 and 2 bytes. The first two packets are received ok but the third packet always results in a data sequence error in the IRQ handler.

These are the values of the buffer control for the endpoint that I captured illustrating this:

Control Setup: 80 06 00 01 00 00 12 00 
on EP 00 with 8 bytes
** Write buf_ctrl = 00002408
** Read buf_ctrl = 0000A008
** Write buf_ctrl = 00000408
** Read buf_ctrl = 80080408
** Write buf_ctrl = 00006402

*** PANIC ***

Data Seq Error 

In fact, I couldn't get past this problem using the USB controller as intended. I ended up creating a workaround where I set the LAST_BUFFER flag on each packet and restarted the next packet using SIE. I suspect a bug on the RP2040 itself but would be interested to hear what others think.

I'll push the changes to this PR for inspection as the original PR solution is incorrect.

[ismell]

It looks like my original patch was also missing

@@ -151,7 +165,7 @@ void _hw_endpoint_xfer_start(struct hw_endpoint *ep, uint8_t *buffer, uint16_t t
     ep->total_len = total_len;
     ep->len = 0;
     // FIXME: What if low speed
-    ep->transfer_size = total_len > 64 ? 64 : total_len;
+    ep->transfer_size = total_len > ep->wMaxPacketSize ? ep->wMaxPacketSize : total_len;
     ep->active = true;
     ep->user_buf = buffer;
     // Recalculate if this is the last buffer

When I do that, I see the second 8 byte packet fail:

Control Setup: R: DEVICE, T: STANDARD, D: IN, R: GET_DESCRIPTOR, V: DEVICE, DescIdx: 0, Idx: 0, Len: 18
80 06 00 01 00 00 12 00
hcd_setup_send dev_addr 1
hw_endpoint_init dev 1 ep 0 out xfer 0 max 8
dev 1 ep 0 out setup buffer @ 0x50100180
endpoint control (0x50100100) <- 0xa0000180
RX: hcd_setup_send: transfer_size: 8, max: 8
hcd_port_speed_get
full speed
Sent setup packet
on EP 00 with 8 bytes
hcd_edpt_xfer dev_addr 1, ep_addr 0x80, len 18
hw_endpoint_init dev 1 ep 0 in xfer 0 max 8
dev 1 ep 0 in setup buffer @ 0x50100180
endpoint control (0x50100100) <- 0xa0000180
hw_endpoint_xfer ep 0 in total_len 18, start=1
Start transfer of total len 18 on ep 0 in, max: 8
_hw_endpoint_start_next_buffer: Buffer Control: 0x2408, Len: 8, Avail: 1, Stall: 0, Sel: 0, PID: 1, Last: 0, Full: 0
buffer control (0x50100080) <- 0x2408
hcd_port_speed_get
full speed
buf_status 0x00000001
_hw_endpoint_xfer_sync: Buffer Control: 0xa008, Len: 8, Avail: 0, Stall: 0, Sel: 0, PID: 1, Last: 0, Full: 1
rx 8 bytes (buf_ctrl 0x0000a008)
_hw_endpoint_xfer_continue: remaining_bytes: 10, ep->transfer_size: 8
_hw_endpoint_start_next_buffer: Buffer Control: 0x408, Len: 8, Avail: 1, Stall: 0, Sel: 0, PID: 0, Last: 0, Full: 0
buffer control (0x50100080) <- 0x408
buf_status 0x00000001
_hw_endpoint_xfer_sync: Buffer Control: 0x80088000, Len: 0, Avail: 0, Stall: 0, Sel: 0, PID: 0, Last: 0, Full: 1
rx 0 bytes (buf_ctrl 0x00008008)
Short rx transfer
Completed transfer of 8 bytes on ep 0 in
Stall REC

*** PANIC ***

Data Seq Error

[fieldofcows]

Yes, I was seeing something similar. Take a look at the the code around the RP2040_HOST_DATA_SEQ_WORKAROUND definition for a workaround. I could not get the hardware to behave correctly when following the procedure in the RP2040 datasheet so had to resort to a workaround.

[ismell]

So looking at my error case, my device sends a stall after the second packet. This is fine from the protocol perspective, but the pico code doesn't seem to handle it well:

Control Setup: R: DEVICE, T: STANDARD, D: IN, R: GET_DESCRIPTOR, V: DEVICE, DescIdx: 0, Idx: 0, Len: 18
80 06 00 01 00 00 12 00
hcd_setup_send dev_addr 1
hw_endpoint_init dev 1 ep 0 out xfer 0 max 8
dev 1 ep 0 out setup buffer @ 0x50100180
endpoint control (0x50100100) <- 0xa0000180
RX: hcd_setup_send: transfer_size: 8, max: 8
Sent setup packet
on EP 00 with 8 bytes
hcd_edpt_xfer dev_addr 1, ep_addr 0x80, len 18
hw_endpoint_init dev 1 ep 0 in xfer 0 max 8
dev 1 ep 0 in setup buffer @ 0x50100180
endpoint control (0x50100100) <- 0xa0000180
hw_endpoint_xfer ep 0 in total_len 18, start=1
Start transfer of total len 18 on ep 0 in, max: 8
_hw_endpoint_start_next_buffer: Buffer Control: 0x2408, Len: 8, Avail: 1, Stall: 0, Sel: 0, PID: 1, Last: 0, Full: 0
_hw_endpoint_buffer_control_update32: Clearing available: 0x2008
_hw_endpoint_buffer_control_update32: Writing: 0x2408
buffer control (0x50100080) <- 0x2408
buf_status 0x00000001
_hw_endpoint_xfer_sync: 0x2408a008: Buffer Control: 0x2408a008, Len: 8, Avail: 0, Stall: 0, Sel: 0, PID: 1, Last: 0, Full: 1
_hw_endpoint_xfer_sync: buf_sel: 1
rx 8 bytes (buf_ctrl 0x2408a008)
  0000:  12 01 00 02 FF FF FF 08                          |........|
_hw_endpoint_xfer_continue: remaining_bytes: 10, ep->transfer_size: 8
_hw_endpoint_start_next_buffer: Buffer Control: 0x408, Len: 8, Avail: 1, Stall: 0, Sel: 0, PID: 0, Last: 0, Full: 0
_hw_endpoint_buffer_control_update32: Clearing available: 0x8
_hw_endpoint_buffer_control_update32: Writing: 0x408
buffer control (0x50100080) <- 0x408
buf_status 0x00000001
_hw_endpoint_xfer_sync: 0x80088000: Buffer Control: 0x80088000, Len: 0, Avail: 0, Stall: 0, Sel: 0, PID: 0, Last: 0, Full: 1
_hw_endpoint_xfer_sync: Shifting buf_ctrl register
Buffer Control: 0x8008, Len: 8, Avail: 0, Stall: 0, Sel: 0, PID: 0, Last: 0, Full: 1
_hw_endpoint_xfer_sync: buf_sel: 0
rx 8 bytes (buf_ctrl 0x00008008)
  0000:  5E 04 8E 02 10 01 01 02                          |^.......|
_hw_endpoint_xfer_continue: remaining_bytes: 2, ep->transfer_size: 2
Last buf (2 bytes left)
_hw_endpoint_start_next_buffer: Buffer Control: 0x6402, Len: 2, Avail: 1, Stall: 0, Sel: 0, PID: 1, Last: 1, Full: 0
_hw_endpoint_buffer_control_update32: Clearing available: 0x6002
_hw_endpoint_buffer_control_update32: Writing: 0x6402
buffer control (0x50100080) <- 0x6402
Stall REC <-- We should retry the transaction here. 

*** PANIC ***

Data Seq Error

hw_xfer_complete clears the ep. It's not clear if a Data Seq error is expected when we receive a stall.

I also found a bug in _hw_endpoint_xfer_start, it calculates the uint transferred_bytes = buf_ctrl & USB_BUF_CTRL_LEN_MASK; before doing the 16 bit shift below...

[ismell]

Do we need to do the following so we get the correct rx length?

diff --git a/src/portable/raspberrypi/rp2040/rp2040_usb.c b/src/portable/raspberrypi/rp2040/rp2040_usb.c
index a44607e9..f8f0be35 100644
--- a/src/portable/raspberrypi/rp2040/rp2040_usb.c
+++ b/src/portable/raspberrypi/rp2040/rp2040_usb.c
@@ -170,7 +170,7 @@ void _hw_endpoint_xfer_sync(struct hw_endpoint *ep)
     // Get the buffer state and amount of bytes we have
     // transferred
     uint32_t buf_ctrl = _hw_endpoint_buffer_control_get_value32(ep);
-    uint transferred_bytes = buf_ctrl & USB_BUF_CTRL_LEN_MASK;
+    uint transferred_bytes;
 
 #ifdef RP2040_USB_HOST_MODE
     // tag::host_buf_sel_fix[]
@@ -184,6 +184,8 @@ void _hw_endpoint_xfer_sync(struct hw_endpoint *ep)
     // end::host_buf_sel_fix[]
 #endif
 
+    transferred_bytes = buf_ctrl & USB_BUF_CTRL_LEN_MASK;
+
     // We are continuing a transfer here. If we are TX, we have successfullly
     // sent some data can increase the length we have sent
     if (!ep->rx)

[ismell]

I tested your patch + #7 (comment) and it works!

Can you squash your two commits. The CLs touch the core tinyusb core. Once you squash them it should only touch the rp2040: code. Also can you prefix your commit with rp2040: to match the existing style.

Thanks!

[fieldofcows]

Ok, done.

[fieldofcows]

Do we need to do the following so we get the correct rx length?

No, I don't believe so. It seems the HW bug where the wrong half of the buffer control variable is updated only affects the status bits, not the length.

[ismell]

LGTM

Thanks!

[hathach]

After looking more closely with rp2040 host mode, this is not an additional hardware bug. It is due to driver incorrectly update the Data Toggle (PID). For this instance, 64 bytes is 8 packets, and the next 9th packet should not have PID toggled. There is another bug with panic already available as well (based on RP2040-E4).

These are fixed by hathach/tinyusb#862

[ismell]

I'm not sure if the latest d49938d tinyusb fixes this:

USBH DEVICE ATTACH
Open EP Control with Size = 8
Get 8 byte of Device Descriptor
Control Setup: R: DEVICE, T: STANDARD, D: IN, R: GET_DESCRIPTOR, V: DEVICE, DescIdx: 0, Idx: 0, Len: 8
80 06 00 01 00 00 08 00
hcd_setup_send dev_addr 0
on EP 00 with 8 bytes
hcd_edpt_xfer dev_addr 0, ep_addr 0x80, len 8
  0000:  12 01 00 02 FF FF FF 08                          |........|
on EP 80 with 8 bytes
Control data:
  0000:  12 01 00 02 FF FF FF 08                          |........|
Len: 18/8, Type: DEVICE, USB: 0x200, Class: 0xff, SubClass: 0xff, Protocol: 0xff, PacketSize: 8,
hcd_edpt_xfer dev_addr 0, ep_addr 0x0, len 0
on EP 00 with 0 bytes

Port reset
Set Address = 1
Control Setup: R: DEVICE, T: STANDARD, D: OUT, R: SET_ADDRESS, Val: 0x1, Idx: 0, Len: 0
00 05 01 00 00 00 00 00
hcd_setup_send dev_addr 0
on EP 00 with 8 bytes
hcd_edpt_xfer dev_addr 0, ep_addr 0x80, len 0
on EP 80 with 0 bytes

enum_set_address_complete:start
Open EP Control with Size = 8
Get Device Descriptor
Control Setup: R: DEVICE, T: STANDARD, D: IN, R: GET_DESCRIPTOR, V: DEVICE, DescIdx: 0, Idx: 0, Len: 18
80 06 00 01 00 00 12 00
hcd_setup_send dev_addr 1
on EP 00 with 8 bytes
hcd_edpt_xfer dev_addr 1, ep_addr 0x80, len 18
  0000:  12 01 00 02 FF FF FF 08                          |........|
on EP 80 with 8 bytes
Control data:
  0000:  12 01 00 02 FF FF FF 08 00 00 00 00 00 00 00 00  |................|
  0010:  00 00                                            |..|
Len: 18/18, Type: DEVICE, USB: 0x200, Class: 0xff, SubClass: 0xff, Protocol: 0xff, PacketSize: 8, VID: 0, PID: 0, Device: 0,
MfgIdx: 0, ProductIdx: 0, SerialIdx: 0, Configs: 0,
hcd_edpt_xfer dev_addr 1, ep_addr 0x0, len 0
on EP 00 with 0 bytes

Get 9 bytes of Configuration Descriptor
Control Setup: R: DEVICE, T: STANDARD, D: IN, R: GET_DESCRIPTOR, V: CONFIGURATION, DescIdx: 0, Idx: 0, Len: 9
80 06 00 02 00 00 09 00
hcd_setup_send dev_addr 1
on EP 00 with 8 bytes
hcd_edpt_xfer dev_addr 1, ep_addr 0x80, len 9
  0000:  09 02 99 00 04 01 00 A0                          |........|
on EP 80 with 8 bytes
Control data:
  0000:  09 02 99 00 04 01 00 A0 00                       |.........|
Len: 9/9, Type: CONFIGURATION, Length: 153, Interfaces: 4, Config Value: 1, Config Index: 0, Attr: 0xa0, Max Power: 0
hcd_edpt_xfer dev_addr 1, ep_addr 0x0, len 0
on EP 00 with 0 bytes

Get Configuration Descriptor
Control Setup: R: DEVICE, T: STANDARD, D: IN, R: GET_DESCRIPTOR, V: CONFIGURATION, DescIdx: 0, Idx: 0, Len: 153
80 06 00 02 00 00 99 00
hcd_setup_send dev_addr 1
on EP 00 with 8 bytes
hcd_edpt_xfer dev_addr 1, ep_addr 0x80, len 153
  0000:  09 02 99 00 04 01 00 A0                          |........|
on EP 80 with 8 bytes
Control data:
  0000:  09 02 99 00 04 01 00 A0 00 00 00 00 00 00 00 00  |................|
  0010:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  |................|
  0020:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  |................|
  0030:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  |................|
  0040:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  |................|
  0050:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  |................|
  0060:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  |................|
  0070:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  |................|
  0080:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  |................|
  0090:  00 00 00 00 00 00 00 00 00                       |.........|
Len: 9/153, Type: CONFIGURATION, Length: 153, Interfaces: 4, Config Value: 1, Config Index: 0, Attr: 0xa0, Max Power: 0
Len: 0/144, hcd_edpt_xfer dev_addr 1, ep_addr 0x0, len 0
on EP 00 with 0 bytes

I'm only getting back 8 bytes for each transfer...

I'm going to try and rebase this PR onto the latest tinyusb and see if that fixes the problem.

[ismell]

It's this line

    // Limit by packet size but not less 64 (i.e low speed 8 bytes EP0)
    ep->transfer_size = tu_min16(total_len, tu_max16(64, ep->wMaxPacketSize));

Why do we have the 64 byte limit? An endpoint can only transfer as much as the max packet size.

[hathach]

ISO can have up to 1023 bytes.

[ismell]

ISO can have up to 1023 bytes.

Right,
But if EP0 has a max packet size of 8, we shouldn't be setting the transfer size to 64. This results in short transfers as seen above. So I think this patch set is still needed. @fieldofcows would you mind rebasing this PR?