On Thu, Jul 21, 2016 at 05:40:15PM -0400, Rich Felker wrote: > The spi master device has a "bus lock flag" that's used in lieu of a > recursive mutex, but it seems to be used entirely incorrectly. The > logic I'm seeing in spi.c is essentially: I am entirely unclear why you believe there is a recursive mutex here, that's not the purpose of the spi_bus_lock() API at all. > if (some_task_has_spi_bus_locked) > use_spi_master_without_locking(); > There seems to be no check that the task that's using the master > without locking is the same one that locked it! That's because there is no requirement that something that takes a bus lock be executing from a single context. It is just granting exclusive access to the bus, the caller is free to do those accesses in any manner it sees fit. > Also, master->bus_lock_flag is modified completely unsynchonized from > read access to master->bus_lock_flag. There is no locking of > master->bus_lock_mutex or master->bus_lock_spinlock around the read > accesses to master->bus_lock_flag, and in one place the write (of 0) > to master->bus_lock_flag takes place without holding the spinlock. The critical read access is in spi_async where we exclude users that don't hold the lock which *is* spinlocked; we should probably hold the spinlock on release but it's not so important since we know that we hold the lock when we call it and the mutex ends up providing protection. spi_sync() shouldn't be reading the flag in the first place as far as I can tell, it should unconditionally have the flag clear as locked callers have to use spi_sync_locked() but that will give us a fast path that doesn't check for the bus lock which starts to reveal the underlying issue. This underlying issue is that we are trying to use one mutex for two purposes, the existing mutex is mainly being used to serialize access to the physical bus but the externally visible bus lock is there to ensure that only one caller is able to queue new transfers which is not the same thing at all. It is completely irrelevant when we are pushing messages to the bus if we are doing so with the bus lock held. > The obvious fix would be using an actual recursive mutex to obtain > recursive mutex semantics. This would be broken since we need to be able to start transfers from atomic context.
Attachment:
signature.asc
Description: PGP signature
