[PATCH 0/2] spi/mmc_spi: SPI bus locking API and mmc_spi adaptations

[PATCH 0/2] spi/mmc_spi: SPI bus locking API and mmc_spi adaptations

[Ernst Schwab]

Here's my first version of a mutex-locked SPI bus locking.
First tests on powerpc architecture with spi_mpc8xxx,
mmc_spi, at25 and at45 drivers and 12 SPI devices
sharing an SPI bus.

1 - modifications to spi.c/spi.h
2 - adaption of mmc_spi.c

Regards
Ernst

------------------------------------------------------------------------------
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

[PATCH 1/2] spi/mmc_spi: SPI bus locking API, using mutex

[Ernst Schwab]

From: Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org>

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):

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

Signed-off-by: Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org>
---
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;
+
 	/* register the device, then userspace will see it.
 	 * registration fails if the bus ID is in use.
 	 */
@@ -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);
 
 
 /*-------------------------------------------------------------------------*/
@@ -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);
+
+	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;
@@ -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;
+	}
+
+	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);
+
+	return ret;
+
+}
+EXPORT_SYMBOL_GPL(spi_async_locked);
+
+int spi_bus_lock(struct spi_device *spi)
+{
+	struct spi_master *master = spi->master;
+
+	spin_lock(&master->bus_lock_spinlock);
+	mutex_lock(&master->bus_lock_mutex);
+	master->bus_lock_chipselect = spi->chip_select;
+	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)
+{
+	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);
+
+	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);
 
 /*---------------------------------------------------------------------------*/
 
@@ -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



------------------------------------------------------------------------------
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

[PATCH 2/2] spi/mmc_spi: mmc_spi adaptations for SPI bus locking API

[Ernst Schwab]

From: Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org>

Modification of the mmc_spi driver to use the SPI bus locking API.
With this, the mmc_spi driver can be used together with other SPI
devices on the same SPI bus. The exclusive access to the SPI bus is
now managed in the SPI layer. The counting of chip selects in the probe
function is no longer needed.

Signed-off-by: Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org>
---
diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -181,7 +181,7 @@ mmc_spi_readbytes(struct mmc_spi_host *host, unsigned len)
 				host->data_dma, sizeof(*host->data),
 				DMA_FROM_DEVICE);
 
-	status = spi_sync(host->spi, &host->readback);
+	status = spi_sync_locked(host->spi, &host->readback);
 
 	if (host->dma_dev)
 		dma_sync_single_for_cpu(host->dma_dev,
@@ -540,7 +540,7 @@ mmc_spi_command_send(struct mmc_spi_host *host,
 				host->data_dma, sizeof(*host->data),
 				DMA_BIDIRECTIONAL);
 	}
-	status = spi_sync(host->spi, &host->m);
+	status = spi_sync_locked(host->spi, &host->m);
 
 	if (host->dma_dev)
 		dma_sync_single_for_cpu(host->dma_dev,
@@ -684,7 +684,7 @@ mmc_spi_writeblock(struct mmc_spi_host *host, struct spi_transfer *t,
 				host->data_dma, sizeof(*scratch),
 				DMA_BIDIRECTIONAL);
 
-	status = spi_sync(spi, &host->m);
+	status = spi_sync_locked(spi, &host->m);
 
 	if (status != 0) {
 		dev_dbg(&spi->dev, "write error (%d)\n", status);
@@ -821,7 +821,7 @@ mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t,
 				DMA_FROM_DEVICE);
 	}
 
-	status = spi_sync(spi, &host->m);
+	status = spi_sync_locked(spi, &host->m);
 
 	if (host->dma_dev) {
 		dma_sync_single_for_cpu(host->dma_dev,
@@ -1017,7 +1017,7 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct mmc_command *cmd,
 					host->data_dma, sizeof(*scratch),
 					DMA_BIDIRECTIONAL);
 
-		tmp = spi_sync(spi, &host->m);
+		tmp = spi_sync_locked(spi, &host->m);
 
 		if (host->dma_dev)
 			dma_sync_single_for_cpu(host->dma_dev,
@@ -1083,6 +1083,9 @@ static void mmc_spi_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	}
 #endif
 
+	/* request exclusive bus access */
+	spi_bus_lock(host->spi);
+
 	/* issue command; then optionally data and stop */
 	status = mmc_spi_command_send(host, mrq, mrq->cmd, mrq->data != NULL);
 	if (status == 0 && mrq->data) {
@@ -1093,6 +1096,9 @@ static void mmc_spi_request(struct mmc_host *mmc, struct mmc_request *mrq)
 			mmc_cs_off(host);
 	}
 
+	/* release the bus */
+	spi_bus_unlock(host->spi);
+
 	mmc_request_done(host->mmc, mrq);
 }
 
@@ -1289,23 +1295,6 @@ mmc_spi_detect_irq(int irq, void *mmc)
 	return IRQ_HANDLED;
 }
 
-struct count_children {
-	unsigned	n;
-	struct bus_type	*bus;
-};
-
-static int maybe_count_child(struct device *dev, void *c)
-{
-	struct count_children *ccp = c;
-
-	if (dev->bus == ccp->bus) {
-		if (ccp->n)
-			return -EBUSY;
-		ccp->n++;
-	}
-	return 0;
-}
-
 static int mmc_spi_probe(struct spi_device *spi)
 {
 	void			*ones;
@@ -1337,32 +1326,6 @@ static int mmc_spi_probe(struct spi_device *spi)
 		return status;
 	}
 
-	/* We can use the bus safely iff nobody else will interfere with us.
-	 * Most commands consist of one SPI message to issue a command, then
-	 * several more to collect its response, then possibly more for data
-	 * transfer.  Clocking access to other devices during that period will
-	 * corrupt the command execution.
-	 *
-	 * Until we have software primitives which guarantee non-interference,
-	 * we'll aim for a hardware-level guarantee.
-	 *
-	 * REVISIT we can't guarantee another device won't be added later...
-	 */
-	if (spi->master->num_chipselect > 1) {
-		struct count_children cc;
-
-		cc.n = 0;
-		cc.bus = spi->dev.bus;
-		status = device_for_each_child(spi->dev.parent, &cc,
-				maybe_count_child);
-		if (status < 0) {
-			dev_err(&spi->dev, "can't share SPI bus\n");
-			return status;
-		}
-
-		dev_warn(&spi->dev, "ASSUMING SPI bus stays unshared!\n");
-	}
-
 	/* We need a supply of ones to transmit.  This is the only time
 	 * the CPU touches these, so cache coherency isn't a concern.
 	 *



------------------------------------------------------------------------------
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

Re: [PATCH 1/2] spi/mmc_spi: SPI bus locking API, using mutex

[Mike Frysinger]

On Wed, Feb 17, 2010 at 13:17, Ernst Schwab wrote:
> 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

i dont think these new "_locked" versions are a good idea.  why cant
it be handled transparently to the caller in the core ?  the spi core
already requires the CS field of the spi device to be unique per bus.
re-use that to check ownership of the mutex.
-mike

------------------------------------------------------------------------------
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
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: [PATCH 1/2] spi/mmc_spi: SPI bus locking API, using mutex

[Grant Likely]

On Wed, Feb 17, 2010 at 11:17 AM, Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org> wrote:
> From: Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org>
>
> 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 <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org>
> ---
> 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

Re: [PATCH 1/2] spi/mmc_spi: SPI bus locking API, using mutex

[Grant Likely]

On Wed, Feb 17, 2010 at 12:03 PM, Mike Frysinger <vapier.adi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Feb 17, 2010 at 13:17, Ernst Schwab wrote:
>> 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
>
> i dont think these new "_locked" versions are a good idea.  why cant
> it be handled transparently to the caller in the core ?  the spi core
> already requires the CS field of the spi device to be unique per bus.
> re-use that to check ownership of the mutex.

No.  the bus locking operation is completely orthogonal to the spi
device being used for the transfer.  A driver could even obtain the
lock, and then use multiple spi_devices to execute transfers before
releasing it.

The API addition is definitely required.  Callers locking the bus
*absolutely* must understand what they are doing.

If anything, I'd consent to a debug option that does a WARN_ON if the
wrong function is called when the bus isn't locked.  ie. _locked
version called when bus isn't locked.

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

Re: [PATCH 2/2] spi/mmc_spi: mmc_spi adaptations for SPI bus locking API

[Grant Likely]

On Wed, Feb 17, 2010 at 11:19 AM, Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org> wrote:
> From: Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org>
>
> Modification of the mmc_spi driver to use the SPI bus locking API.
> With this, the mmc_spi driver can be used together with other SPI
> devices on the same SPI bus. The exclusive access to the SPI bus is
> now managed in the SPI layer. The counting of chip selects in the probe
> function is no longer needed.
>
> Signed-off-by: Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org>

Looks good to me.

g.

> ---
> diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> --- a/drivers/mmc/host/mmc_spi.c
> +++ b/drivers/mmc/host/mmc_spi.c
> @@ -181,7 +181,7 @@ mmc_spi_readbytes(struct mmc_spi_host *host, unsigned len)
>                                host->data_dma, sizeof(*host->data),
>                                DMA_FROM_DEVICE);
>
> -       status = spi_sync(host->spi, &host->readback);
> +       status = spi_sync_locked(host->spi, &host->readback);
>
>        if (host->dma_dev)
>                dma_sync_single_for_cpu(host->dma_dev,
> @@ -540,7 +540,7 @@ mmc_spi_command_send(struct mmc_spi_host *host,
>                                host->data_dma, sizeof(*host->data),
>                                DMA_BIDIRECTIONAL);
>        }
> -       status = spi_sync(host->spi, &host->m);
> +       status = spi_sync_locked(host->spi, &host->m);
>
>        if (host->dma_dev)
>                dma_sync_single_for_cpu(host->dma_dev,
> @@ -684,7 +684,7 @@ mmc_spi_writeblock(struct mmc_spi_host *host, struct spi_transfer *t,
>                                host->data_dma, sizeof(*scratch),
>                                DMA_BIDIRECTIONAL);
>
> -       status = spi_sync(spi, &host->m);
> +       status = spi_sync_locked(spi, &host->m);
>
>        if (status != 0) {
>                dev_dbg(&spi->dev, "write error (%d)\n", status);
> @@ -821,7 +821,7 @@ mmc_spi_readblock(struct mmc_spi_host *host, struct spi_transfer *t,
>                                DMA_FROM_DEVICE);
>        }
>
> -       status = spi_sync(spi, &host->m);
> +       status = spi_sync_locked(spi, &host->m);
>
>        if (host->dma_dev) {
>                dma_sync_single_for_cpu(host->dma_dev,
> @@ -1017,7 +1017,7 @@ mmc_spi_data_do(struct mmc_spi_host *host, struct mmc_command *cmd,
>                                        host->data_dma, sizeof(*scratch),
>                                        DMA_BIDIRECTIONAL);
>
> -               tmp = spi_sync(spi, &host->m);
> +               tmp = spi_sync_locked(spi, &host->m);
>
>                if (host->dma_dev)
>                        dma_sync_single_for_cpu(host->dma_dev,
> @@ -1083,6 +1083,9 @@ static void mmc_spi_request(struct mmc_host *mmc, struct mmc_request *mrq)
>        }
>  #endif
>
> +       /* request exclusive bus access */
> +       spi_bus_lock(host->spi);
> +
>        /* issue command; then optionally data and stop */
>        status = mmc_spi_command_send(host, mrq, mrq->cmd, mrq->data != NULL);
>        if (status == 0 && mrq->data) {
> @@ -1093,6 +1096,9 @@ static void mmc_spi_request(struct mmc_host *mmc, struct mmc_request *mrq)
>                        mmc_cs_off(host);
>        }
>
> +       /* release the bus */
> +       spi_bus_unlock(host->spi);
> +
>        mmc_request_done(host->mmc, mrq);
>  }
>
> @@ -1289,23 +1295,6 @@ mmc_spi_detect_irq(int irq, void *mmc)
>        return IRQ_HANDLED;
>  }
>
> -struct count_children {
> -       unsigned        n;
> -       struct bus_type *bus;
> -};
> -
> -static int maybe_count_child(struct device *dev, void *c)
> -{
> -       struct count_children *ccp = c;
> -
> -       if (dev->bus == ccp->bus) {
> -               if (ccp->n)
> -                       return -EBUSY;
> -               ccp->n++;
> -       }
> -       return 0;
> -}
> -
>  static int mmc_spi_probe(struct spi_device *spi)
>  {
>        void                    *ones;
> @@ -1337,32 +1326,6 @@ static int mmc_spi_probe(struct spi_device *spi)
>                return status;
>        }
>
> -       /* We can use the bus safely iff nobody else will interfere with us.
> -        * Most commands consist of one SPI message to issue a command, then
> -        * several more to collect its response, then possibly more for data
> -        * transfer.  Clocking access to other devices during that period will
> -        * corrupt the command execution.
> -        *
> -        * Until we have software primitives which guarantee non-interference,
> -        * we'll aim for a hardware-level guarantee.
> -        *
> -        * REVISIT we can't guarantee another device won't be added later...
> -        */
> -       if (spi->master->num_chipselect > 1) {
> -               struct count_children cc;
> -
> -               cc.n = 0;
> -               cc.bus = spi->dev.bus;
> -               status = device_for_each_child(spi->dev.parent, &cc,
> -                               maybe_count_child);
> -               if (status < 0) {
> -                       dev_err(&spi->dev, "can't share SPI bus\n");
> -                       return status;
> -               }
> -
> -               dev_warn(&spi->dev, "ASSUMING SPI bus stays unshared!\n");
> -       }
> -
>        /* We need a supply of ones to transmit.  This is the only time
>         * the CPU touches these, so cache coherency isn't a concern.
>         *
>
>
>



-- 
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

Re: [PATCH 1/2] spi/mmc_spi: SPI bus locking API, using mutex

[Mike Frysinger]

On Wed, Feb 17, 2010 at 15:06, Grant Likely wrote:
> On Wed, Feb 17, 2010 at 12:03 PM, Mike Frysinger wrote:
>> On Wed, Feb 17, 2010 at 13:17, Ernst Schwab wrote:
>>> 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
>>
>> i dont think these new "_locked" versions are a good idea.  why cant
>> it be handled transparently to the caller in the core ?  the spi core
>> already requires the CS field of the spi device to be unique per bus.
>> re-use that to check ownership of the mutex.
>
> No.  the bus locking operation is completely orthogonal to the spi
> device being used for the transfer.  A driver could even obtain the
> lock, and then use multiple spi_devices to execute transfers before
> releasing it.
>
> The API addition is definitely required.  Callers locking the bus
> *absolutely* must understand what they are doing.
>
> If anything, I'd consent to a debug option that does a WARN_ON if the
> wrong function is called when the bus isn't locked.  ie. _locked
> version called when bus isn't locked.

the API provides no protection let alone detection of the code that
locked the bus is the one using it.  that would be the point of tying
a client to the locking step, but without it, there needs to be a
handle of some sort returned from the bus locking and subsequently
passed to the spi_{sync,async}_locked() and spi_bus_unlock().
-mike

------------------------------------------------------------------------------
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
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: [PATCH 1/2] spi/mmc_spi: SPI bus locking API, using mutex

[Grant Likely]

On Wed, Feb 17, 2010 at 1:19 PM, Mike Frysinger <vapier.adi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Feb 17, 2010 at 15:06, Grant Likely wrote:
>> On Wed, Feb 17, 2010 at 12:03 PM, Mike Frysinger wrote:
>>> On Wed, Feb 17, 2010 at 13:17, Ernst Schwab wrote:
>>>> 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
>>>
>>> i dont think these new "_locked" versions are a good idea.  why cant
>>> it be handled transparently to the caller in the core ?  the spi core
>>> already requires the CS field of the spi device to be unique per bus.
>>> re-use that to check ownership of the mutex.
>>
>> No.  the bus locking operation is completely orthogonal to the spi
>> device being used for the transfer.  A driver could even obtain the
>> lock, and then use multiple spi_devices to execute transfers before
>> releasing it.
>>
>> The API addition is definitely required.  Callers locking the bus
>> *absolutely* must understand what they are doing.
>>
>> If anything, I'd consent to a debug option that does a WARN_ON if the
>> wrong function is called when the bus isn't locked.  ie. _locked
>> version called when bus isn't locked.
>
> the API provides no protection let alone detection of the code that
> locked the bus is the one using it.  that would be the point of tying
> a client to the locking step, but without it, there needs to be a
> handle of some sort returned from the bus locking and subsequently
> passed to the spi_{sync,async}_locked() and spi_bus_unlock().
> -mike

...and yet spinlocks and mutexs are used to protect critical regions
all the time without any sort of handle or cookie.

No, I don't think a bus lock cookie is needed in any way.  If the new
API gets used without holding the bus lock, then we can add code to
debug that and do a WARN_ON().  If the existing API gets called when
the bus is locked, then the caller will just sleep.  The only danger
is if the new API gets called when *someone else* holds the lock.  And
considering that the use case for this functionality is pretty darn
small, and that any driver that gets it wrong in the absence of any
other bus locking device instance will simply not work (either trigger
the WARN, or sleep on the spinlock), I do not think that tieing the
cs# to the lock is a good idea.

If a cookie were to be used, using the cs# is the wrong thing.  If
anything it should be an anonymous value returned by spi_bus_lock().

Also, even if I agreed with the premise that a cookie is needed for
deciding who can use the bus when locked, it is still a good idea to
use a different API when working with a locked bus.  Locking issues
are subtle, and driver authors *must understand what they are doing*.
Using a different API for talking to a locked bus is one of the things
that makes absolute sense to make sure that drivers know which regions
have the bus locked, and which do not.

g.

------------------------------------------------------------------------------
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

Re: [PATCH 1/2] spi/mmc_spi: SPI bus locking API, using mutex

[Mike Frysinger]

On Wed, Feb 17, 2010 at 16:07, Grant Likely wrote:
> On Wed, Feb 17, 2010 at 1:19 PM, Mike Frysinger wrote:
>> On Wed, Feb 17, 2010 at 15:06, Grant Likely wrote:
>>> On Wed, Feb 17, 2010 at 12:03 PM, Mike Frysinger wrote:
>>>> On Wed, Feb 17, 2010 at 13:17, Ernst Schwab wrote:
>>>>> 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
>>>>
>>>> i dont think these new "_locked" versions are a good idea.  why cant
>>>> it be handled transparently to the caller in the core ?  the spi core
>>>> already requires the CS field of the spi device to be unique per bus.
>>>> re-use that to check ownership of the mutex.
>>>
>>> No.  the bus locking operation is completely orthogonal to the spi
>>> device being used for the transfer.  A driver could even obtain the
>>> lock, and then use multiple spi_devices to execute transfers before
>>> releasing it.
>>>
>>> The API addition is definitely required.  Callers locking the bus
>>> *absolutely* must understand what they are doing.
>>>
>>> If anything, I'd consent to a debug option that does a WARN_ON if the
>>> wrong function is called when the bus isn't locked.  ie. _locked
>>> version called when bus isn't locked.
>>
>> the API provides no protection let alone detection of the code that
>> locked the bus is the one using it.  that would be the point of tying
>> a client to the locking step, but without it, there needs to be a
>> handle of some sort returned from the bus locking and subsequently
>> passed to the spi_{sync,async}_locked() and spi_bus_unlock().
>
> ...and yet spinlocks and mutexs are used to protect critical regions
> all the time without any sort of handle or cookie.

because those critical regions tend to be small (by design).  that is
not what we have here -- the lock is acquired earlier and used in
pretty extensively nested code before being released.  ignoring the
code size, relatively speaking in SPI transactions, the MMC block is
going to hold it for a long time.

> No, I don't think a bus lock cookie is needed in any way.  If the new
> API gets used without holding the bus lock, then we can add code to
> debug that and do a WARN_ON().  If the existing API gets called when
> the bus is locked, then the caller will just sleep.  The only danger
> is if the new API gets called when *someone else* holds the lock.  And
> considering that the use case for this functionality is pretty darn
> small, and that any driver that gets it wrong in the absence of any
> other bus locking device instance will simply not work (either trigger
> the WARN, or sleep on the spinlock)

except for when asynchronous code paths happen to overlap in ways
people dont realize.  yes, *today* the use case is small, but it's
hard to add consumers of a framework when the functionality doesnt yet
exist.

> I do not think that tieing the cs# to the lock is a good idea.
>
> If a cookie were to be used, using the cs# is the wrong thing.  If
> anything it should be an anonymous value returned by spi_bus_lock().

i wasnt suggesting the cs# was the way to go.  read the statement
again -- "if it isnt locked to a client, then a cookie needs to be
returned _instead_".

> Also, even if I agreed with the premise that a cookie is needed for
> deciding who can use the bus when locked, it is still a good idea to
> use a different API when working with a locked bus.  Locking issues
> are subtle, and driver authors *must understand what they are doing*.
> Using a different API for talking to a locked bus is one of the things
> that makes absolute sense to make sure that drivers know which regions
> have the bus locked, and which do not.

these sort of statements are always made yet it doesnt seem to prevent
bugs.  of course people *should know what they're doing*, but
*reality* is that people make mistakes and bugs get merged all the
time.  a little proactive protection goes a long way.  i dont think
adding a cookie would increase overhead all that much.
-mike

------------------------------------------------------------------------------
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
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: [PATCH 1/2] spi/mmc_spi: SPI bus locking API, using mutex

[Grant Likely]

On Wed, Feb 17, 2010 at 2:41 PM, Mike Frysinger <vapier.adi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Feb 17, 2010 at 16:07, Grant Likely wrote:
>> Also, even if I agreed with the premise that a cookie is needed for
>> deciding who can use the bus when locked, it is still a good idea to
>> use a different API when working with a locked bus.  Locking issues
>> are subtle, and driver authors *must understand what they are doing*.
>> Using a different API for talking to a locked bus is one of the things
>> that makes absolute sense to make sure that drivers know which regions
>> have the bus locked, and which do not.
>
> these sort of statements are always made yet it doesnt seem to prevent
> bugs.  of course people *should know what they're doing*, but
> *reality* is that people make mistakes and bugs get merged all the
> time.  a little proactive protection goes a long way.  i dont think
> adding a cookie would increase overhead all that much.

Heh.  Alright.  Then I think I'd like to see a follow-on patch that
adds the cookie checking so that it can be reviewed independently of
all the locking additions.

Cheers,
g.

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev