Preventing race conditions with multiple bus users

[jnohlgard]

I have multiple drivers that are using the same bus (SPI_0) for communicating with their hardware. Has anyone considered or experienced the possible race conditions between different device drivers communicating over the bus without mutual exclusion?

I would like to introduce one mutex per bus device (SPI_0, SPI_1, ... , I2C_0, I2C_1 etc) that must be acquired before using the bus in order to prevent multiple chip select signals being asserted causing all kinds of trouble with the hardware devices.

Example taken from at86rf231 driver:

uint8_t at86rf231_reg_read(uint8_t addr)
{
    char value;

    /* Start the SPI transfer */
    gpio_clear(AT86RF231_CS);
    /* read from register */
    spi_transfer_reg(AT86RF231_SPI, AT86RF231_ACCESS_REG | AT86RF231_ACCESS_READ | addr, 0, &value);
    /* End the SPI transfer */
    gpio_set(AT86RF231_CS);
    return (uint8_t)value;
}

Running the above with interrupts enabled and getting a context switch anywhere between gpio_clear and gpio_set opens up for the potential that another SPI device driver will attempt to write something to the bus, but ends up sending it to both the at86rf231 as well as the originally intended hardware device.

Adding mutex_lock(&SPI_0_MUTEX) before calling gpio_clear(), and mutex_unlock(&SPI_0_MUTEX) after gpio_set() (or similar) will make sure only one device driver can access the bus at any given time.

The same race condition can also occur on i2c busses (which have no chip select line) where the device driver needs to send more than a single byte (or whatever the atomic size is for the i2c hardware).

It would be necessary to update all drivers using I2C or SPI busses to actually achieve anything with the change. I would be glad for any comments and suggestions on my proposed solution.

ChibiOS has a similar design where a bus user must first call spiAcquireBus and spiReleaseBus in order to ensure exclusive access to the bus.

I recommend adopting something similar to what ChibiOS does in order to allow multiple bus users to coexist safely.

[haukepetersen]

@gebart You are completely right, the above mentioned behavior is (at least for me) a known issue that is not resolved in RIOT so far. I have been thinking about the scenarios you describe, but I got never to actually design a solution for it.

At first sight, I see two solutions:

1. making the peripheral drivers thread safe internally, by introducing and managing a Mutex internally in the driver implementation
   Pros: users don't have to worry about (un-)locking
   Cons: as in aboves example, a task switch could happen between gpio_clear() and spi_transfer()...
2. or making peripherals lock-able externally:
   Pros: completely thread safe, for simple applications you don't have to use locking to save cycles
   Cons: users have to worry about locking

Both solutions have the drawback, that they will use 8 additional bytes (on 32-bit systems) for each peripheral that utilizes the locking mechanism.

Concluding, I think we definitely need some kind of locking for (bus) peripherals. I would opt for the second option, as I think it is safer and more flexible. How about a solution as in #2290 ?

[PeterKietzmann]

Even if it's a bit uncomfortable to handle the (un-)locking externally I would also vote for this solution because when going for a "safer" solution let's do a "really safe" one.

[thomaseichinger]

I think using acquire and lease functions in upper drivers is the way to go especially for SPI, we don't handle chip select internally for, which could run into trouble with more than one CS line active.

[PeterKietzmann]

@thomaseichinger I think I didn't understand your comment. Are you fine with the solution in @2290 or not? What kind of problems do you see with more than one CS line? This situation should 1. be avoided when interrupt based SPI-commands are called or 2. be handled (manually) when multiple devices on the bus should get the same message. Did I get something wrong?

[haukepetersen]

@PeterKietzmann I read @thomaseichingers comment that he is fine with the proposed soultion...

[thomaseichinger]

@PeterKietzmann Yes I'm fine with the solution proposed in #2290.

[jnohlgard]

I am closing this issue in favour of the tracker at #2314