On Wed, Feb 17, 2010 at 11:17 AM, Ernst Schwab <[email protected]> wrote: > From: Ernst Schwab <[email protected]> > > SPI bus locking API to allow exclusive access to the SPI bus, especially, but > not limited to, for the mmc_spi driver. > > Coded according to an outline from Grant Likely; here is his > specification (accidentally swapped function names corrected):
Wow. You're fast. Thanks for turning this around so quickly. Comments below. > It requires 3 things to be added to struct spi_master. > - 1 Mutex > - 1 spin lock > - 1 flag. > > The mutex protects spi_sync, and provides sleeping "for free" > The spinlock protects the atomic spi_async call. > The flag is set when the lock is obtained, and checked while holding > the spinlock in spi_async(). If the flag is checked, then spi_async() > must fail immediately. > > The current runtime API looks like this: > spi_async(struct spi_device*, struct spi_message*); > spi_sync(struct spi_device*, struct spi_message*); > > The API needs to be extended to this: > spi_async(struct spi_device*, struct spi_message*) > spi_sync(struct spi_device*, struct spi_message*) > spi_bus_lock(struct spi_master*) /* although struct spi_device* might > be easier */ > spi_bus_unlock(struct spi_master*) > spi_async_locked(struct spi_device*, struct spi_message*) > spi_sync_locked(struct spi_device*, struct spi_message*) > > Drivers can only call the last two if they already hold the spi_master_lock(). > > spi_bus_lock() obtains the mutex, obtains the spin lock, sets the > flag, and releases the spin lock before returning. It doesn't even > need to sleep while waiting for "in-flight" spi_transactions to > complete because its purpose is to guarantee no additional > transactions are added. It does not guarantee that the bus is idle. > > spi_bus_unlock() clears the flag and releases the mutex, which will > wake up any waiters. > > The difference between spi_async() and spi_async_locked() is that the > locked version bypasses the check of the lock flag. Both versions > need to obtain the spinlock. > > The difference between spi_sync() and spi_sync_locked() is that > spi_sync() must hold the mutex while enqueuing a new transfer. > spi_sync_locked() doesn't because the mutex is already held. Note > however that spi_sync must *not* continue to hold the mutex while > waiting for the transfer to complete, otherwise only one transfer > could be queued up at a time! > > Almost no code needs to be written. The current spi_async() and > spi_sync() can probably be renamed to __spi_async() and __spi_sync() > so that spi_async(), spi_sync(), spi_async_locked() and > spi_sync_locked() can just become wrappers around the common code. > > spi_sync() is protected by a mutex because it can sleep > spi_async() needs to be protected with a flag and a spinlock because > it can be called atomically and must not sleep The usage model also needs to be documented in the .c files or somewhere in Documentation/. You can use the kernel doc format to document the usage of each function in the .c file. > > Signed-off-by: Ernst Schwab <[email protected]> > --- > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -524,6 +524,10 @@ int spi_register_master(struct spi_master *master) > dynamic = 1; > } > > + spin_lock_init(&master->bus_lock_spinlock); > + mutex_init(&master->bus_lock_mutex); > + master->bus_lock_chipselect = SPI_MASTER_BUS_UNLOCKED; The locking operations have no dependency on the spi_device. Rename bus_lock_chipselect to bus_lock_flag. > @@ -692,7 +696,7 @@ EXPORT_SYMBOL_GPL(spi_setup); > * (This rule applies equally to all the synchronous transfer calls, > * which are wrappers around this core asynchronous primitive.) > */ > -int spi_async(struct spi_device *spi, struct spi_message *message) > +static int __spi_async(struct spi_device *spi, struct spi_message *message) > { > struct spi_master *master = spi->master; > > @@ -720,7 +724,6 @@ int spi_async(struct spi_device *spi, struct spi_message > *message) > message->status = -EINPROGRESS; > return master->transfer(spi, message); > } > -EXPORT_SYMBOL_GPL(spi_async); Move the definitions of spi_async() and spi_async_locked() to here, right below the __spi_async() definition. > > > /*-------------------------------------------------------------------------*/ > @@ -756,14 +759,50 @@ static void spi_complete(void *arg) > * > * It returns zero on success, else a negative error code. > */ > + > +int spi_sync_locked(struct spi_device *spi, struct spi_message *message) > +{ > + DECLARE_COMPLETION_ONSTACK(done); > + int status; > + struct spi_master *master = spi->master; > + > + message->complete = spi_complete; > + message->context = &done; > + > + spin_lock(&master->bus_lock_spinlock); > + > + status = __spi_async(spi, message); > + > + spin_unlock(&master->bus_lock_spinlock); Instead of obtaining the spinlock here, both spi_sync() and spi_sync_locked() can call spi_async_locked() directly (assuming the chipselect # check is removed from spi_async_locked() as per my comment below. > + > + if (status == 0) { > + wait_for_completion(&done); > + status = message->status; > + } > + > + message->context = NULL; > + > + return status; > +} > +EXPORT_SYMBOL_GPL(spi_sync_locked); > + > int spi_sync(struct spi_device *spi, struct spi_message *message) > { > DECLARE_COMPLETION_ONSTACK(done); > int status; > + struct spi_master *master = spi->master; > > message->complete = spi_complete; > message->context = &done; > - status = spi_async(spi, message); > + > + mutex_lock(&master->bus_lock_mutex); > + > + spin_lock(&master->bus_lock_spinlock); > + status = __spi_async(spi, message); > + spin_unlock(&master->bus_lock_spinlock); > + > + mutex_unlock(&master->bus_lock_mutex); > + > if (status == 0) { > wait_for_completion(&done); > status = message->status; spi_sync() and spi_sync_locked() are largely idenical. Only difference is that spi_sync() needs to obtain the mutex. Rename spi_sync() to __spi_sync() and add a 'get_lock' flag parameter. Then make spi_sync() and spi_sync_locked() call __spi_sync() with the flag set appropriately. > @@ -773,6 +812,82 @@ int spi_sync(struct spi_device *spi, struct spi_message > *message) > } > EXPORT_SYMBOL_GPL(spi_sync); > > +int spi_async(struct spi_device *spi, struct spi_message *message) > +{ > + struct spi_master *master = spi->master; > + unsigned long flags; > + int ret; > + > + spin_lock_irqsave(&master->bus_lock_spinlock, flags); > + > + if (master->bus_lock_chipselect == SPI_MASTER_BUS_UNLOCKED > + || master->bus_lock_chipselect == spi->chip_select) { > + ret = __spi_async(spi, message); > + } else { > + /* REVISIT: > + * if the bus is locked to an other device, message transfer > + * fails. Maybe messages should be queued in the SPI layer > + * and be transmitted after the lock is released. > + */ > + ret = -EBUSY; > + } Don't check the cs#. There is no need for the cs to be associated with the lock. Make this simply: if (master->bus_lock_flag) ret = -EBUSY; else ret = __spi_async(spi, message); You can drop the comment here too; or at the very least move it into the header block for the function. > + > + spin_unlock_irqrestore(&master->bus_lock_spinlock, flags); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(spi_async); > + > +int spi_async_locked(struct spi_device *spi, struct spi_message *message) > +{ > + struct spi_master *master = spi->master; > + unsigned long flags; > + int ret; > + > + spin_lock_irqsave(&master->bus_lock_spinlock, flags); > + > + if (master->bus_lock_chipselect == spi->chip_select) { > + ret = __spi_async(spi, message); > + } else { > + /* API error: the SPI bus lock is not held by the caller */ > + ret = -EINVAL; > + } > + > + spin_unlock_irqrestore(&master->bus_lock_spinlock, flags); This version shouldn't check the flag at all. Just call __spi_async() unconditionally. Taking the spin lock probably isn't necessary, but it is probably wise to keep the locking model consistent. > + > + return ret; > + > +} > +EXPORT_SYMBOL_GPL(spi_async_locked); > + > +int spi_bus_lock(struct spi_device *spi) On further reflection, pass in the spi_master to this function. The lock is bus-wide, and doesn't care about what device is used, as long as the caller is the one holding the lock. ie. a caller could potentially have more than one spi_device that it would want to use while holding the bus lock. > +{ > + struct spi_master *master = spi->master; > + > + spin_lock(&master->bus_lock_spinlock); > + mutex_lock(&master->bus_lock_mutex); Always grab the mutex first. Otherwise you could get a deadlock. > + master->bus_lock_chipselect = spi->chip_select; make this simply master->bus_lock_flag = 1; > + spin_unlock(&master->bus_lock_spinlock); > + > + /* mutex remains locked until spi_bus_unlock is called */ > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(spi_bus_lock); > + > +int spi_bus_unlock(struct spi_device *spi) Ditto here, pass in spi_master* > +{ > + struct spi_master *master = spi->master; > + > + spin_lock(&master->bus_lock_spinlock); > + mutex_unlock(&master->bus_lock_mutex); > + master->bus_lock_chipselect = SPI_MASTER_BUS_UNLOCKED; > + spin_unlock(&master->bus_lock_spinlock); Unlock doesn't need to grab the spinlock. It can just clear the flag and release the mutex. > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(spi_bus_unlock); > + > /* portable code must never pass more than 32 bytes */ > #define SPI_BUFSIZ max(32,SMP_CACHE_BYTES) > > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -261,6 +261,14 @@ struct spi_master { > #define SPI_MASTER_NO_RX BIT(1) /* can't do buffer read */ > #define SPI_MASTER_NO_TX BIT(2) /* can't do buffer write */ > > + /* lock and mutex for SPI bus locking */ > + spinlock_t bus_lock_spinlock; > + struct mutex bus_lock_mutex; > + > + /* chipselect of the spi device that locked the bus for exclusive use > */ > + u8 bus_lock_chipselect; > +#define SPI_MASTER_BUS_UNLOCKED 0xFF /* value to indicate unlocked > */ > + > /* Setup mode and clock, etc (spi driver may call many times). > * > * IMPORTANT: this may be called when transfers to another > @@ -541,6 +549,8 @@ static inline void spi_message_free(struct spi_message *m) > > extern int spi_setup(struct spi_device *spi); > extern int spi_async(struct spi_device *spi, struct spi_message *message); > +extern int > +spi_async_locked(struct spi_device *spi, struct spi_message *message); Please keep return parameters and annotations on the same line as the function name (makes grep results more useful). You can break lines between parameters. > > /*---------------------------------------------------------------------------*/ > > @@ -550,6 +560,9 @@ extern int spi_async(struct spi_device *spi, struct > spi_message *message); > */ > > extern int spi_sync(struct spi_device *spi, struct spi_message *message); > +extern int spi_sync_locked(struct spi_device *spi, struct spi_message > *message); > +extern int spi_bus_lock(struct spi_device *spi); > +extern int spi_bus_unlock(struct spi_device *spi); > > /** > * spi_write - SPI synchronous write Really good work. Thanks! g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ------------------------------------------------------------------------------ SOLARIS 10 is the OS for Data Centers - provides features such as DTrace, Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW http://p.sf.net/sfu/solaris-dev2dev _______________________________________________ spi-devel-general mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/spi-devel-general
