dfu download sometimes hangs

[MrJake222]

dfu-util -d 1209:b1c0 -a "iCE40 DFU (CRAM)" -D cdark.bin
dfu-util 0.11

Copyright 2005-2009 Weston Schmidt, Harald Welte and OpenMoko Inc.
Copyright 2010-2021 Tormod Volden and Stefan Schmidt
This program is Free Software and has ABSOLUTELY NO WARRANTY
Please report bugs to http://sourceforge.net/p/dfu-util/tickets/

dfu-util: Warning: Invalid DFU suffix signature
dfu-util: A valid DFU suffix will be required in a future dfu-util release
Opening DFU capable USB device...
Device ID 1209:b1c0
Run-Time device DFU version 0101
Claiming USB DFU Interface...
Setting Alternate Interface #1 ...
Determining device status...
DFU state(2) = dfuIDLE, status(0) = No error condition is present
DFU mode device DFU version 0101
Device returned transfer size 256
Copying data from PC to DFU device
Download        [=========================] 100%       104090 bytes
Download done.
DFU state(7) = dfuMANIFEST, status(0) = No error condition is present
DFU state(8) = dfuMANIFEST-WAIT-RESET, status(0) = No error condition is present
Resetting USB to switch back to runtime mode

... long hang ...
make: *** [../Makefile:85: prgcram] Error 74

Pretty annoying.

[MrJake222]

Investigation. This happens:

The endpoint buffer register (0x84 = EP0 out buffer control) is already in use (available for the hw controller for consumption). This crash occurs after Resetting USB to switch back to runtime mode. Stack trace always follows:

• ice_usb_uart_tx_from_cdc
• tud_cdc_n_read
• ...
• *ep->buffer_control & USB_BUF_CTRL_AVAIL is true

The SDK doesn't handle libusb_reset_device() well.

[josuah]

Thank you for this investigation!

It seems like someone also came upon this bug long ago: hathach/tinyusb#2556

What happens if you comment these lines? This might not be a good solution, but depending on where it crashes next, this can help understand why this happened.

I will try to collect time to reproduce the issue.

[josuah]

TL;DR: maybe we have a race condition somewhere in the pico-ice-sdk code, or in the tinyUSB driver

The buffer_control field is a hardware register:

https://github.com/hathach/tinyusb/blob/master/src/portable/raspberrypi/rp2040/rp2040_usb.h#L61-L62

typedef struct hw_endpoint
{
...
    // Buffer control register
    io_rw_32 *buffer_control;
...
};

As far as I understand the RP2040 USB driver on TinyUSB source, this flag is set here:

https://github.com/hathach/tinyusb/blob/b5d4d0f6237ecaf78b2548e5c4a08810e0957066/src/portable/raspberrypi/rp2040/rp2040_usb.c#L127

prepare_ep_buffer()
{
  uint32_t buf_ctrl = buflen | USB_BUF_CTRL_AVAIL;
  ...
  return buf_ctrl;
}

It is only called in this function:

https://github.com/hathach/tinyusb/blob/b5d4d0f6237ecaf78b2548e5c4a08810e0957066/src/portable/raspberrypi/rp2040/rp2040_usb.c#L155

hw_endpoint_start_next_buffer()
{
  uint32_t buf_ctrl = prepare_ep_buffer(ep, 0) | USB_BUF_CTRL_SEL;
...
  _hw_endpoint_buffer_control_set_value32(ep, buf_ctrl);
}

_hw_endpoint_buffer_control_set_value32() then calls _hw_endpoint_buffer_control_update32() in which the panic happens.

It does not look like this can cause a problem, as when _hw_endpoint_buffer_control_update32() returns, the flag is cleared. But if running this from the main context, and then an IRQ happens while this runs, and this is called again from the IRQ, then it becomes a race condition and the flag can be set twice (which the check detects and panic()'s).

I am not entirely sure that this is the problem, but that could be one thing I need to check where that code is used in the pico-ice-sdk...

[MrJake222]

This also mentions a race condition: hathach/tinyusb#2474

It does not look like this can cause a problem, as when _hw_endpoint_buffer_control_update32() returns, the flag is cleared. But if running this from the main context, and then an IRQ happens while this runs, and this is called again from the IRQ, then it becomes a race condition and the flag can be set twice (which the check detects and panic()'s).

I don't get where the flag is cleared. The function ends with *ep->buffer_control = value;, which can definitely leave this flag set.

I think the issue here is that while reset, the device loses its ownership of the endpoints (kinda...). With this minimal example I was able to reproduce the problem.

The issue does not appear when this line is commented out (no continuous reading).
I guess tinyusb doesn't stop the reading when init is in progress. Or we should check it somehow? "If endpoint is open then read"? The comms follow with the usb device reinitialization:

[MrJake222]

I guess tinyusb doesn't stop the reading when init is in progress. Or we should check it somehow? "If endpoint is open then read"? The comms follow with the usb device reinitialization:

Aaand I fixed it with tud_ready() haha. I'll make a pull request.

edit: here it is, against the sdk: tinyvision-ai-inc/pico-ice-sdk#69

[josuah]

Closing this now that tinyvision-ai-inc/pico-ice-sdk#69 is merged.

This will be part of the next release as soon as things are rebased.

Thanks again for the investigation!