resetting DATA PID toggling to DATA0 after SET_INTERFACE

[laf0rge]

Operating System

Linux

Board

raspberry pico (rp2040)

Firmware

any tinyusb firmware as long as it uses the usbd.c default handling of TUSB_REQ_SET_INTERFACE and doesn't override that in a driver specific control endpoint handler call-back.

What happened ?

It is my understanding that after any configuration event (to which SET INTERFACE belongs), the endpoints belonging to that interface re-start from DATA0.

See USB Spec 9.1.1.5: configuing a device or changing an alternate setting causes all of the status and configuration values associated with endpoints in the affected interfaecs to be set to their default values. This includes esetting the data toggle of any endpoint using data toggles to the value of DATA0

However, I don't see any code in tinyusb for resetting data toggling to DATA0 after a SET_INTERFACE request. tinyusb simply acknowledges any SET_INTERFACE in its generic code, but data toggles remain unaffected

If I'm creating such a situation (See below how to reproduce), I am seeing BULK out URBs never completing anymore.

If I add USB_INTS_ERROR_DATA_SEQ_BITS to the INTE (interrupts enabled) mask in dcd_rp2040.c I also notice that in this scenario, the related interrupt is raised, and the SIE_STATUS register has the USB_SIE_STATUS_DATA_SEQ_ERROR_BITS set.

I don't really think this is RP2040 specific, but in general I cannot find any code in tinyusb to set the DATA toggle back to DATA0 in case of configuration events.

How to reproduce ?

• send an uneven number of BULK OUT URBs to a device
• send a SET_INTERFACE request to the interface
• observe that any further BULK OUT requests are not succesful anymore
• perform the same without the SET_INTERFACE, and it succeeds

Debug Log as txt file (LOG/CFG_TUSB_DEBUG=2)

none

Screenshots

No response

I have checked existing issues, dicussion and documentation

• [X] I confirm I have checked existing issues, dicussion and documentation.

[laf0rge]

FYI: I tried to add related code, but I couldn't even find any dcd_* API from the low-level drivers to the generic usbd.c which would be used to reset the DATA toggle state.

The below diff illustrates how one can get notifications from the rp2040 usb device peripheral about those data toggle errors (the commented-out panic was from the hcd which interestingly does enable those error interrupts, while the dcd doesn't):

diff --git a/src/portable/raspberrypi/rp2040/dcd_rp2040.c b/src/portable/raspberrypi/rp2040/dcd_rp2040.c
index b5fa90c92..d3ee7ae4c 100644
--- a/src/portable/raspberrypi/rp2040/dcd_rp2040.c
+++ b/src/portable/raspberrypi/rp2040/dcd_rp2040.c
@@ -367,6 +367,15 @@ static void __tusb_irq_path_func(dcd_rp2040_irq)(void)
     usb_hw_clear->sie_status = USB_SIE_STATUS_RESUME_BITS;
   }
 
+  if ( status & USB_INTS_ERROR_DATA_SEQ_BITS )
+  {
+    handled |= USB_INTS_ERROR_DATA_SEQ_BITS;
+    usb_hw_clear->sie_status = USB_SIE_STATUS_DATA_SEQ_ERROR_BITS;
+    TU_LOG(3, "  Seq Error\r\n");
+    //panic("Data Seq Error \n");
+    printf("Data Seq Error\r\n");
+  }
+
   if ( status ^ handled )
   {
     panic("Unhandled IRQ 0x%x\n", (uint) (status ^ handled));
@@ -416,6 +425,7 @@ void dcd_init (uint8_t rhport)
   usb_hw->sie_ctrl = USB_SIE_CTRL_EP0_INT_1BUF_BITS;
   usb_hw->inte     = USB_INTS_BUFF_STATUS_BITS | USB_INTS_BUS_RESET_BITS | USB_INTS_SETUP_REQ_BITS |
                      USB_INTS_DEV_SUSPEND_BITS | USB_INTS_DEV_RESUME_FROM_HOST_BITS |
+                    USB_INTS_ERROR_DATA_SEQ_BITS |
                      (FORCE_VBUS_DETECT ? 0 : USB_INTS_DEV_CONN_DIS_BITS);
 
   dcd_connect(rhport);

[laf0rge]

One way of looking at the problem is the fact that the default usbd code blindly acknowledges all SET_INTERFACE requests. The USB spec chapter 9 explicitly states that for interfaces with only one altsetting the device may simply stall rather than acknowledging the request. This way the data toggling state on the host/bus presumably shouldn't change.

However, this wouldn't be a true fix for any application that actually uses multiple different altsettings (like vendor specific drivers).

I believe this doesn't show up in tinyusb as-is, as

• the only user of multiple altsettings probably is usb-audio-class, which uses isochronous endpoints, which don't do data toggling
• most standard host software/driver doesn't do a SET_INTERFACE on an itnerface with a single altsetting. However, there's nothing illegal about a host software doing that, so tinyusb should handle it gracefully.

[laf0rge]

see below for a control_xfer_cb of a custom driver which overrides the usbd.c default handling (returning true in the SETUP stage) and then later returns false in the ACK/Status stage. This way the host sees the SET_INTERFACE failing, and will not change the data toggling.

static bool cardemd_control_xfer_cb(uint8_t __unused rhport, uint8_t __unused stage, tusb_control_request_t const *request)
{
        switch (request->bmRequestType_bit.type) {
        case TUSB_REQ_TYPE_STANDARD:
                switch (request->bRequest) {
                case TUSB_REQ_SET_INTERFACE:
                        printf("SET_INTERFACE Stage: %u\r\n", stage);
                        switch (stage) {
                        case CONTROL_STAGE_SETUP:
                                tud_control_status(rhport, request);
                                return true;
                        case CONTROL_STAGE_ACK:
                                if (request->wIndex != 0) {
                                        // return RequestError
                                        return false;
                                }
                                if (request->wValue != 0) {
                                        // return RequestError
                                        return false;
                                }
                                /* from here: permitted alternate setting */
#if 0   /* global disable of any SET_INTERFACE */
                                /* reset data toggle to DATA0  after configuration operation */
                                printf("SET_INTERFACE: RESTTING DATA TOGGLE TO DATA0\r\n");
#if 0   /* try it via reset of data toggle */
                                dcd_edpt_reset_data_toggle(0, 0x01);
                                dcd_edpt_reset_data_toggle(0, 0x81);
                                dcd_edpt_reset_data_toggle(0, 0x82);
#else   /* try it via closing of endpoints */
                                dcd_edpt_close_all(rhport);
#endif
                                return true;
#else   /* global disable of any SET_INTERFACE */
                                /* return false here so that the calling function will cause a
                                 * STALL during the ACK (Status) phase of the control transfer,
                                 * making the entire request fail */
                                return false;
#endif
                        default:
                                return true;
                        }
                case TUSB_REQ_GET_INTERFACE:
                        /* let generic tinyusb usbd care about it */
                        return false;
                default:
                        /* let generic tinyusb usbd care about it */
                        return false;
                }
                break;
        default:
                /* we have no class specific control requests */
                return false;
        }
}

in the above code you can also see my various failed attempts at actually causing tinyusb to do the right thing (reset the DATA0 toggle). I couldn't get any of those to work. The only two working scenarios are:

• do not issue SET_INTERFACE from the host (defeats the purpose but avoids tinyusb running into the bug)
• reject the SET_INTERFACE by STALL in the ACK/Status phase (fixes the standar situation of a single altsetting per interface, but still defeats the purpose for any multi-altsetting interface)

[hathach]

yeah, you are right, we should reset DATA toggle when received SET_INTERFACE request, it should be done by either usbd or class driver and an additional dcd_edpt_reset_toggle() should be addded. Could you explain a bit on your configuration descriptor/driver, and how you setup/test, it would be helpful if you have an ready to build/test example/repo so that I could work on and verify the fix.

[laf0rge]

Thanks for confirming the bug.

I'm currently in the process of implementing a rp2040 based device that exposes the same protocol as the Osmocom SIMtrace2 device. The libusb based host software for this always first issues a SET INTERFACE to the desired target altsetting (see the library utility function in https://gitea.osmocom.org/osmocom/libosmocore/src/commit/d5c6651ae07fdae7e80023079c57896869ec372a/src/usb/osmo_libusb.c#L517), no matter whether or not thre are multiple altsettings, or whether or not the current altsetting != the target altsetting. The reason for that is simple: It's easier to always set it rather than to query the device and then set it.

Right now I don't really have anything that works yet in terms of a full rp2040 firmware. I have pieces of code doing pieces of stuff and other partial ports of other pieces. The current WIP is in https://gitea.osmocom.org/laforge/rp2040-playground/src/branch/laforge/wip and specifically the composite directory there. But honestly, I think at this point it's better to write a few lines of code to test this:

• small libusb host program starting with SET INTERFACE before submitting bulk out transfers (and logging any received bulk in transfers)
• small rp2040 target program echoing back anything it receives on OUT EP back to IN EP

if you then start the host program, you will see:

• upon first start, the system will work (host program continuously submitting OUT transfers, the device echoing them back as IN transfers
• stop + restart of the host program has a certain chance that it will get stuck: First OUT transfer is submitted, but it never gets echoed back to the host, and hence the host never sends any further OUT transfer either. Just keep restarting the host program and see that every so often the toggle will not match and you end up in the stuck situation.

If you have any patch/branch for me to test at some point, I'm happy to try it in my WIP application and provide feedback. Thanks.