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

[Philip Odom]

Your patch looks good for the SPIM code (probably that's what you're using, @mikevoyt). I found that a similar fix is needed for the SPI code as well. This may be the problem you're having, @papyrus.

Fixed code for SPI:

CODE_FOR_SPI
(
    NRF_SPI_Type * p_spi = p_instance->p_registers;

    p_cb->p_tx_buffer       = p_tx_buffer;
    p_cb->tx_buffer_length  = tx_buffer_length;
    p_cb->p_rx_buffer       = p_rx_buffer;
    p_cb->rx_buffer_length  = rx_buffer_length;
    p_cb->bytes_transferred = 0;

			//set transfer_in_progress before doing anything to events
			//fixes a race condition that was causing issues with transfer
			//completing before flag being set.
			//see: devzone.nordicsemi.com/.../
    if (p_cb->handler)
    {
        p_cb->transfer_in_progress = true;
    }

    nrf_spi_event_clear(p_spi, NRF_SPI_EVENT_READY);

    // Start the transfer by writing some byte to the TXD register;
    // if TX buffer is not empty, take the first byte from this buffer,
    // otherwise - use over-run character.
    nrf_spi_txd_set(p_spi,
        (tx_buffer_length > 0 ?  p_tx_buffer[0] : p_cb->orc));
    // TXD register is double buffered, so next byte to be transmitted can
    // be written immediately, if needed, i.e. if TX or RX transfer is to
    // be more that 1 byte long. Again - if there is something more in TX
    // buffer send it, otherwise use over-run character.
    if (tx_buffer_length > 1)
    {
        nrf_spi_txd_set(p_spi, p_tx_buffer[1]);
    }
    else if (rx_buffer_length > 1)
    {
        nrf_spi_txd_set(p_spi, p_cb->orc);
    }

    // For blocking mode (user handler not provided) wait here for READY
    // events (indicating that the byte from TXD register was transmitted
    // and a new incoming byte was moved to the RXD register) and continue
    // transaction until all requested bytes are transferred.
    // In non-blocking mode - IRQ service routine will do this stuff.
    if (!p_cb->handler)
    {
        do {
            while (!nrf_spi_event_check(p_spi, NRF_SPI_EVENT_READY)) {}
            nrf_spi_event_clear(p_spi, NRF_SPI_EVENT_READY);
        } while (transfer_byte(p_spi, p_cb));
    }
)