BUG: Race condition in SPI driver

[Mike Voytovich]

Hi,

I noticed that I'd occasionally hit an error when calling into nrf_drv_spi_transfer using the SPIM driver (SPI0_USE_EASY_DMA set). nrf_drv_spi_transfer was returning NRF_ERROR_BUSY:

if (p_cb->transfer_in_progress)
{
    return NRF_ERROR_BUSY;
}

The problem turned out to be a race condition, where transfer_in_progress was set after the SPIM module was started; thus, occasionally it would complete the SPI transaction (and clear transfer_in_progress and call the callback) before the transfer_in_progress flag was set. Thus, transfer_in_progress was essentially getting set when the transaction was complete, thus preventing further transactions from occurring.

I could reproduce this issue very easily by setting the compiler optimization to -O0. It would occasionally happen with -O3.

Attached is a patch which I believe fixes the issue - it sets the flag before initiating the transfer.

If you could, please confirm this is the correct patch - thanks! diff.txt

[Papyrus]

So, as per my comment to the other answer, The patch did not solve this for me, albeit it's possible I did not apply it correctly. (see comment above).

BUT, I did discover a simpler fix that solved this for me, and that's simply inserting one line of code:

p_cb->transfer_in_progress = false;

Right before the last line of nrf_drv_spi_transfer as shown below:

ret_code_t nrf_drv_spi_transfer(nrf_drv_spi_t const * const p_instance,
                                uint8_t const * p_tx_buffer,
                                uint8_t         tx_buffer_length,
                                uint8_t       * p_rx_buffer,
                                uint8_t         rx_buffer_length)

    //SPI stuff...
    //SPI stuff...
    //SPI stuff...
    
    p_cb->transfer_in_progress = false; 
    return NRF_SUCCESS;
}

[Mike Voytovich]

The patch in your first comment looks correct to me. Note that if you're not setting p_cb->transfer_in_progress = true; before triggering the event, the driver and your application will never know when the SPIM transfer is actually in progress, so I don't believe that's a legitimate patch...