Inherent race condition when using SPI

[mvirkkunen]

SPI uses chip select lines to select which chip to talk to but this crate only synchronizes the SPI data transaction itself, not the chip select lines, so there's a possible race condition where a transaction occurs with multiple chip select lines asserted. I was bitten by this in an actual project, and had to revert to manually synchronizing everything.

The way to do chip select lines with embedded-hal seems to be to just pass them to drivers separately from the SPI peripheral, so there is no real way to synchronize access via just the SPI trait. The only idea I had was to add a method like BusManager::acquire_spi(cs_pin) which takes ownership of the chip select OutputPin, and returns you both a BusProxy as well as a wrapper for the chip select pin, and the wrapper actually does the locking (lock when chip select is asserted). This sounds incredibly hacky though, and I think the correct solution would be to expand the embedded-hal SPI traits to include chip select support. I think some peripherals with hardware chip selects might benefit from it anyways.

[Rahix]

Yes, that is absolutely correct ... I agree, the best solution is a fix in embedded-hal. I'm busy with other things at the moment unfortunately but once I'm back, I will take a closer look at how to solve this. Thanks for the report!

[Rahix]

@ryankurte, you wrote the original patch for supporting SPI. What is your thought on this?

I'd suggest removing SPI support in its current form and then thinking about introducing something like BusManager::acquire_spi(cs_pin) as @mvirkkunen proposed. Later, if upstream embedded-hal changes the SPI trait design, we can revert back ...

[ryankurte]

Yeah this isn't ideal, sorry it took me so long to come back around to it. I'd rather not remove support (because I'm using it alright from a single threaded context), but maybe a note that it's dangerous could be useful?

I've got an open PR for transactions (rust-embedded/embedded-hal#178) which mitigates this a wee bit in that you can batch all the operations together, and we have an issue for discussion about CS traits now (rust-embedded/embedded-hal#180) if y'all would like to have any input there.

[Rahix]

Thanks for the reply, @ryankurte!

I am not sure whether I correctly understand your proposal in rust-embedded/embedded-hal#180. Maybe you can provide some (pseudo) code to elaborate?

From my point of view, the only thing we can do now is adding a new SpiBusManager type which implements the afore mentioned .acquire(cs_pin). In parallel, we can provide a dummy CS-pin type which can be passed to drivers which insist on doing CS management themselves.

Does this sound reasonable?

[Rahix]

After talking a bit in #embedded-rust, I think the way forward is to split out the proxies into different structs for I2C and SPI. Then, we can make the SPI proxy !Send + !Sync which means it can only be shared in the same thread. With all users confined to the same execution context, no races should be possible as all bus accesses are strictly serialized.

Of course, this prevents SPI usage from different execution contexts. But I think with the current embedded-hal API this is the only sane thing to do. For concurrent sharing, we'd need an SPI API which models everything from CS assertion to CS deassertion in a single call, closure API, or RAII guard. Maybe embedded-spi is exactly what we need?

[ryankurte]

My hope with embedded-spi is that when we land rust-embedded/embedded-hal#191 i can remove all the transactional stuff (as it will then be in the hal), and with a marker trait something like rust-embedded/embedded-hal#180 we can then signal when CS is managed and use this in our trait bounds (should be here but is not yet).

Completely untested example (actual impl in linked crates) but, i would expect this to look something like:

// ManagedCS specifies that all `spi` operations will be preceded by asserting the CS pin,
// and followed by de-asserting the CS pin.
// If you need to adapt from a HAL which does not provided `ManagedCS`, use`SpiWithCs::new(spi, cs_pin)` 
// to construct a wrapper object.
trait ManagedCS {}

// HAL implementer, whether Hal provides ManagedCS depends on configuration 
// (possible in linux, unlikely in other situations)
impl spi::Transactional for Hal { ... }

// SPI consumer, asserts that they expect the CS pin to be managed
impl <S: spi::Transactional + spi::ManagedCS> Driver {
  pub fn new(spi: S) -> Self { ... }
}

// SPI mux, only supported where CS is managed to avoid breaking up transactions
impl <T> spi::Transactional for T where T: spi::ManagedCS { ... }

// SPI CS Wrapper
// combines SPI and CS_PIN and manages CS assertion/de-assertion
struct SpiWithCs<S, P> {
  spi: S,
  cs_pin: P,
}

impl <S, P> SpiWithCs<S, P> {
  pub fn new(spi: S, cs_pin: P) -> Self { ... }
}

impl <S, P> ManagedSpi for SpiWithCs<S, P> {}

This lets driver authors specify their expectations about CS, HALs implement auto-cs where viable, and users adapt from unmanaged to managed instances. The only limitation that is obvious to me is that we cannot currently assert !ManagedCS for drivers that do not expect CS to be managed (though they will usually explicitly require CS to be passed in so this is still exposed in the API).

[Rahix]

Yeah, this looks good!

The only limitation that is obvious to me is that we cannot currently assert !ManagedCS for drivers that do not expect CS to be managed

I think this should not be encouraged at all as such drivers prevent bus-sharing (see this issue ...). IMO a driver shouldn't know about the CS pin at all, it should expect that whenever it interacts with its peripheral, CS will be asserted by a lower level (either your SpiWithCs wapper or the hardware itself).

[Rahix]

The unsoundness is now fixed so we can close this issue I'd say. Of course, now shared-bus no longer provides a way for sharing SPI across multiple tasks which will need the above mentioned transactional interface.