hathach / tinyusb

An open source cross-platform USB stack for embedded system
https://www.tinyusb.org
MIT License
4.85k stars 1.03k forks source link

RNF52/NRF53 incorrect size DMA transfers due to hardware undocumented behavior #1474

Closed kasjer closed 2 years ago

kasjer commented 2 years ago

Operating System

Linux

Board

pca10095 or pca10056 or other NRF52/NRF53

Firmware

Latest master can be 0.13.0. mynewt/nimble build with BTH and MSC enabled

What happened ?

When DMA is started for one OUT endpoint values read from SIZE.EPOUT[n] for other endpoint are not valid. This behavior is not documented (yet) by Nordic but it can be observed on NRF52/NRF53. When two packets arrive on two different endpoints single interrupt will detect and try to handle both. It calls function xact_out_dma() for both endpoints one after the other. For the first endpoint (one with lower number) DMA will start transferring data from endpoint to RAM. Second call xact_out_dma() for next ready endpoint will not start DMA right away since it's already started, it will call usbd_defer_func() with task start register. This looks like correct sequence however there is problem in following code inside xact_out_dma()

    // Following line may compute incorrect value !!!!!!!!!!!!!
    xact_len = tu_min16((uint16_t) NRF_USBD->SIZE.EPOUT[epnum], xfer->total_len - xfer->actual_len);
    // Trigger DMA move data from Endpoint -> SRAM
    NRF_USBD->EPOUT[epnum].PTR = (uint32_t) xfer->buffer;
    NRF_USBD->EPOUT[epnum].MAXCNT = xact_len; // <- Possible wrong value

    edpt_dma_start(&NRF_USBD->TASKS_STARTEPOUT[epnum]);

first line can incorrectly computes xact_len while DMA is ongoing. Read from NRF_USBD->SIZE.EPOUT[epnum] yields incorrect values. This incorrect value is then set to NRF_USBD->EPOUT[epnum].MAXCNT and later DMA will copy less bytes that it should and resulting in broken protocols.

Following code could be used to trap error

// helper to start DMA
static void start_dma(volatile uint32_t* reg_startep)
{
  for (int i = 0; i < 8; ++i) stored_size_epout1[i] = NRF_USBD->SIZE.EPOUT[i];
  (*reg_startep) = 1;
  __ISB(); __DSB();
  for (int i = 0; i < 8; ++i) stored_size_epout2[i] = NRF_USBD->SIZE.EPOUT[i];

  if (memcmp(stored_size_epout1, stored_size_epout2, 8))
    TU_BREAKPOINT();

How to reproduce ?

I think it maybe it could be reproduce in cdc_msc modified sample (at least two OUT endpoint are needed) and some massive transfer on both endpoints.

Debug Log as txt file

No response

Screenshots

No response

hathach commented 2 years ago

thank you for the issue and the fix.