[PATCH 1/2] spi: new SPI bus lock/unlock functions

[PATCH 1/2] spi: new SPI bus lock/unlock functions

[Mike Frysinger]

From: Yi Li <yi.li@analog.com>

For some MMC cards over SPI bus, it needs to lock the SPI bus for its own
use.  The SPI transfer must not be interrupted by other SPI devices that
share the SPI bus with SPI MMC card.

This patch introduces 2 APIs for SPI bus locking operation.

Signed-off-by: Yi Li <yi.li@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
Andrew: we've posted these in the past with no response.  could you pick
        them up please ?

 drivers/spi/spi.c       |   48 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi.h |    7 ++++++
 2 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 70845cc..b82b8ad 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -653,6 +653,54 @@ static void spi_complete(void *arg)
 }
 
 /**
+ * spi_lock_bus - lock SPI bus for exclusive access
+ * @spi: device which want to lock the bus
+ * Context: any
+ *
+ * Once the caller owns exclusive access to the SPI bus,
+ * only messages for this device will be transferred.
+ * Messages for other devices are queued but not transferred until
+ * the bus owner unlock the bus.
+ *
+ * The caller may call spi_lock_bus() before spi_sync() or spi_async().
+ * So this call may be used in irq and other contexts which can't sleep,
+ * as well as from task contexts which can sleep.
+ *
+ * It returns zero on success, else a negative error code.
+ */
+int spi_lock_bus(struct spi_device *spi)
+{
+	if (spi->master->lock_bus)
+		return spi->master->lock_bus(spi);
+	else
+		return 0;
+}
+EXPORT_SYMBOL_GPL(spi_lock_bus);
+
+/**
+ * spi_unlock_bus - unlock SPI bus
+ * @spi: device which want to unlock the bus
+ * Context: any
+ *
+ * The caller has called spi_lock_bus() to lock the bus. It calls
+ * spi_unlock_bus() to release the bus so messages for other devices
+ * can be transferred.
+ *
+ * If the caller did not call spi_lock_bus() before, spi_unlock_bus()
+ * should have no effect.
+ *
+ * It returns zero on success, else a negative error code.
+ */
+int spi_unlock_bus(struct spi_device *spi)
+{
+	if (spi->master->unlock_bus)
+		return spi->master->unlock_bus(spi);
+	else
+		return 0;
+}
+EXPORT_SYMBOL_GPL(spi_unlock_bus);
+
+/**
  * spi_sync - blocking/synchronous SPI data transfers
  * @spi: device with which data will be exchanged
  * @message: describes the data transfers
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index c47c4b4..c53292c 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -214,6 +214,8 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  *	the device whose settings are being modified.
  * @transfer: adds a message to the controller's transfer queue.
  * @cleanup: frees controller-specific state
+ * @lock_bus: lock SPI bus for exclusive access
+ * @unlock_bus: unlock SPI bus so other devices can access
  *
  * Each SPI master controller can communicate with one or more @spi_device
  * children.  These make a small bus, sharing MOSI, MISO and SCK signals
@@ -286,6 +288,9 @@ struct spi_master {
 
 	/* called on release() to free memory provided by spi_master */
 	void			(*cleanup)(struct spi_device *spi);
+
+	int			(*lock_bus)(struct spi_device *spi);
+	int			(*unlock_bus)(struct spi_device *spi);
 };
 
 static inline void *spi_master_get_devdata(struct spi_master *master)
@@ -578,6 +583,8 @@ 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_lock_bus(struct spi_device *spi);
+extern int spi_unlock_bus(struct spi_device *spi);
 
 /**
  * spi_write - SPI synchronous write
-- 
1.6.5.rc1

[PATCH 2/2] mmc_spi: lock the SPI bus when accessing the card

[Mike Frysinger]

From: Yi Li <yi.li@analog.com>

The MMC/SPI spec does not play well with typical SPI design -- it often
needs to send out a command in one message, read a response, then do some
other arbitrary step.  Since we can't let another SPI client use the bus
during this time, use the new SPI lock/unlock functions to provide the
required exclusivity.

Signed-off-by: Yi Li <yi.li@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/mmc/host/mmc_spi.c |   29 ++---------------------------
 1 files changed, 2 insertions(+), 27 deletions(-)

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index a461017..a96e058 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -1084,6 +1084,7 @@ static void mmc_spi_request(struct mmc_host *mmc, struct mmc_request *mrq)
 #endif
 
 	/* issue command; then optionally data and stop */
+	spi_lock_bus(host->spi);
 	status = mmc_spi_command_send(host, mrq, mrq->cmd, mrq->data != NULL);
 	if (status == 0 && mrq->data) {
 		mmc_spi_data_do(host, mrq->cmd, mrq->data, mrq->data->blksz);
@@ -1092,7 +1093,7 @@ static void mmc_spi_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		else
 			mmc_cs_off(host);
 	}
-
+	spi_unlock_bus(host->spi);
 	mmc_request_done(host->mmc, mrq);
 }
 
@@ -1337,32 +1338,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.
 	 *
-- 
1.6.5.rc1

RE: [spi-devel-general] [PATCH 2/2] mmc_spi: lock the SPI bus whenaccessing the card

[H Hartley Sweeten]

On Thursday, September 17, 2009 3:03 PM, Mike Frysinger wrote:
> From: Yi Li <yi.li@analog.com>
> 
> The MMC/SPI spec does not play well with typical SPI design -- it often
> needs to send out a command in one message, read a response, then do some
> other arbitrary step.  Since we can't let another SPI client use the bus
> during this time, use the new SPI lock/unlock functions to provide the
> required exclusivity.
> 
> Signed-off-by: Yi Li <yi.li@analog.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
>  drivers/mmc/host/mmc_spi.c |   29 ++---------------------------
>  1 files changed, 2 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> index a461017..a96e058 100644
> --- a/drivers/mmc/host/mmc_spi.c
> +++ b/drivers/mmc/host/mmc_spi.c
> @@ -1084,6 +1084,7 @@ static void mmc_spi_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  #endif
>  
>  	/* issue command; then optionally data and stop */
> +	spi_lock_bus(host->spi);
>  	status = mmc_spi_command_send(host, mrq, mrq->cmd, mrq->data != NULL);
>  	if (status == 0 && mrq->data) {
>  		mmc_spi_data_do(host, mrq->cmd, mrq->data, mrq->data->blksz);
> @@ -1092,7 +1093,7 @@ static void mmc_spi_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  		else
>  			mmc_cs_off(host);
>  	}
> -
> +	spi_unlock_bus(host->spi);
>  	mmc_request_done(host->mmc, mrq);
>  }
>  
> @@ -1337,32 +1338,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.
>  	 *

Removing the above block of code causes:

drivers/mmc/host/mmc_spi.c:1299: warning: 'maybe_count_child' defined but not used

That function should also be removed along with the "struct count_children"
definition.

Regards,
Hartley

[PATCH 2/2 v2] mmc_spi: lock the SPI bus when accessing the card

[Mike Frysinger]

From: Yi Li <yi.li@analog.com>

The MMC/SPI spec does not play well with typical SPI design -- it often
needs to send out a command in one message, read a response, then do some
other arbitrary step.  Since we can't let another SPI client use the bus
during this time, use the new SPI lock/unlock functions to provide the
required exclusivity.

Signed-off-by: Yi Li <yi.li@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
v2
	- drop now unused maybe_count_child as pointed out by H Hartley Sweeten

 drivers/mmc/host/mmc_spi.c |   41 ++---------------------------------------
 1 files changed, 2 insertions(+), 39 deletions(-)

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index a461017..c3563a7 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -1084,6 +1084,7 @@ static void mmc_spi_request(struct mmc_host *mmc, struct mmc_request *mrq)
 #endif
 
 	/* issue command; then optionally data and stop */
+	spi_lock_bus(host->spi);
 	status = mmc_spi_command_send(host, mrq, mrq->cmd, mrq->data != NULL);
 	if (status == 0 && mrq->data) {
 		mmc_spi_data_do(host, mrq->cmd, mrq->data, mrq->data->blksz);
@@ -1092,7 +1093,7 @@ static void mmc_spi_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		else
 			mmc_cs_off(host);
 	}
-
+	spi_unlock_bus(host->spi);
 	mmc_request_done(host->mmc, mrq);
 }
 
@@ -1294,18 +1295,6 @@ struct count_children {
 	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.
 	 *
-- 
1.6.5.rc1

Re: [PATCH 2/2 v2] mmc_spi: lock the SPI bus when accessing the card

[Andrew Morton]

On Thu, 17 Sep 2009 20:52:14 -0400
Mike Frysinger <vapier@gentoo.org> wrote:

> From: Yi Li <yi.li@analog.com>
> 
> The MMC/SPI spec does not play well with typical SPI design -- it often
> needs to send out a command in one message, read a response, then do some
> other arbitrary step.  Since we can't let another SPI client use the bus
> during this time, use the new SPI lock/unlock functions to provide the
> required exclusivity.
> 
> Signed-off-by: Yi Li <yi.li@analog.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> v2
> 	- drop now unused maybe_count_child as pointed out by H Hartley Sweeten
> 
>  drivers/mmc/host/mmc_spi.c |   41 ++---------------------------------------
>  1 files changed, 2 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> index a461017..c3563a7 100644
> --- a/drivers/mmc/host/mmc_spi.c
> +++ b/drivers/mmc/host/mmc_spi.c
> @@ -1084,6 +1084,7 @@ static void mmc_spi_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  #endif
>  
>  	/* issue command; then optionally data and stop */
> +	spi_lock_bus(host->spi);
>  	status = mmc_spi_command_send(host, mrq, mrq->cmd, mrq->data != NULL);
>  	if (status == 0 && mrq->data) {
>  		mmc_spi_data_do(host, mrq->cmd, mrq->data, mrq->data->blksz);
> @@ -1092,7 +1093,7 @@ static void mmc_spi_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  		else
>  			mmc_cs_off(host);
>  	}
> -
> +	spi_unlock_bus(host->spi);
>  	mmc_request_done(host->mmc, mrq);

I can't find any code anywhere which puts a non-zero value into
spi_master.[un]lock_bus so in my tree at least, neither of these
patches do anything.

This makes it all a bit hard to understand and review.

RE: [spi-devel-general] [PATCH 1/2] spi: new SPI bus lock/unlockfunctions

[H Hartley Sweeten]

On Thursday, September 17, 2009 3:03 PM, Mike Frysinger wrote:
> From: Yi Li <yi.li@analog.com>
> 
> For some MMC cards over SPI bus, it needs to lock the SPI bus for its own
> use.  The SPI transfer must not be interrupted by other SPI devices that
> share the SPI bus with SPI MMC card.
> 
> This patch introduces 2 APIs for SPI bus locking operation.
> 
> Signed-off-by: Yi Li <yi.li@analog.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> Andrew: we've posted these in the past with no response.  could you pick
>         them up please ?

Hello Mike,

This is the first time I have seen this patch.  I might have missed it
previously.

I would like to test it on my ep93xx system but have some question below.
 
>  drivers/spi/spi.c       |   48 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi.h |    7 ++++++
>  2 files changed, 55 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 70845cc..b82b8ad 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -653,6 +653,54 @@ static void spi_complete(void *arg)
>  }
>  
>  /**
> + * spi_lock_bus - lock SPI bus for exclusive access
> + * @spi: device which want to lock the bus
> + * Context: any
> + *
> + * Once the caller owns exclusive access to the SPI bus,
> + * only messages for this device will be transferred.
> + * Messages for other devices are queued but not transferred until
> + * the bus owner unlock the bus.
> + *
> + * The caller may call spi_lock_bus() before spi_sync() or spi_async().
> + * So this call may be used in irq and other contexts which can't sleep,
> + * as well as from task contexts which can sleep.
> + *
> + * It returns zero on success, else a negative error code.
> + */
> +int spi_lock_bus(struct spi_device *spi)
> +{
> +	if (spi->master->lock_bus)
> +		return spi->master->lock_bus(spi);
> +	else
> +		return 0;
> +}
> +EXPORT_SYMBOL_GPL(spi_lock_bus);
> +
> +/**
> + * spi_unlock_bus - unlock SPI bus
> + * @spi: device which want to unlock the bus
> + * Context: any
> + *
> + * The caller has called spi_lock_bus() to lock the bus. It calls
> + * spi_unlock_bus() to release the bus so messages for other devices
> + * can be transferred.
> + *
> + * If the caller did not call spi_lock_bus() before, spi_unlock_bus()
> + * should have no effect.
> + *
> + * It returns zero on success, else a negative error code.
> + */
> +int spi_unlock_bus(struct spi_device *spi)
> +{
> +	if (spi->master->unlock_bus)
> +		return spi->master->unlock_bus(spi);
> +	else
> +		return 0;
> +}
> +EXPORT_SYMBOL_GPL(spi_unlock_bus);
> +
> +/**

I assume the spi master driver must supply the {lock/unlock}_bus methods
to properly support the locking.  But, by returning 0 when the methods
are not supplied you are basically saying all the current master drivers
in mainline support bus locking.  I think this is really only "true" if
spi->master->num_chipselect == 1.

Also, do you have a master driver that does have the {lock/unlock}_bus
methods?  I would like to see how you handled it.

Regards,
Hartley

Re: [spi-devel-general] [PATCH 1/2] spi: new SPI bus lock/unlockfunctions

[Mike Frysinger]

On Thu, Sep 17, 2009 at 18:45, H Hartley Sweeten wrote:
> On Thursday, September 17, 2009 3:03 PM, Mike Frysinger wrote:
>> From: Yi Li <yi.li@analog.com>
>>
>> For some MMC cards over SPI bus, it needs to lock the SPI bus for its own
>> use.  The SPI transfer must not be interrupted by other SPI devices that
>> share the SPI bus with SPI MMC card.
>>
>> This patch introduces 2 APIs for SPI bus locking operation.
>>
>>  /**
>> + * spi_lock_bus - lock SPI bus for exclusive access
>> + * @spi: device which want to lock the bus
>> + * Context: any
>> + *
>> + * Once the caller owns exclusive access to the SPI bus,
>> + * only messages for this device will be transferred.
>> + * Messages for other devices are queued but not transferred until
>> + * the bus owner unlock the bus.
>> + *
>> + * The caller may call spi_lock_bus() before spi_sync() or spi_async().
>> + * So this call may be used in irq and other contexts which can't sleep,
>> + * as well as from task contexts which can sleep.
>> + *
>> + * It returns zero on success, else a negative error code.
>> + */
>> +int spi_lock_bus(struct spi_device *spi)
>> +{
>> +     if (spi->master->lock_bus)
>> +             return spi->master->lock_bus(spi);
>> +     else
>> +             return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(spi_lock_bus);
>> +
>> +/**
>> + * spi_unlock_bus - unlock SPI bus
>> + * @spi: device which want to unlock the bus
>> + * Context: any
>> + *
>> + * The caller has called spi_lock_bus() to lock the bus. It calls
>> + * spi_unlock_bus() to release the bus so messages for other devices
>> + * can be transferred.
>> + *
>> + * If the caller did not call spi_lock_bus() before, spi_unlock_bus()
>> + * should have no effect.
>> + *
>> + * It returns zero on success, else a negative error code.
>> + */
>> +int spi_unlock_bus(struct spi_device *spi)
>> +{
>> +     if (spi->master->unlock_bus)
>> +             return spi->master->unlock_bus(spi);
>> +     else
>> +             return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(spi_unlock_bus);
>> +
>> +/**
>
> I assume the spi master driver must supply the {lock/unlock}_bus methods
> to properly support the locking.

currently, yes.  a common solution would be nice.  ideas/patches welcome ;).

> But, by returning 0 when the methods
> are not supplied you are basically saying all the current master drivers
> in mainline support bus locking.  I think this is really only "true" if
> spi->master->num_chipselect == 1.

right, but that is no different from what we have today.  there is no
way for a client to guarantee exclusive access, so you cant write code
assuming it in the first place.  the only consumer thus far (mmc_spi)
actually errors out if such conditions exist.

in other words, we arent breaking anything.

> Also, do you have a master driver that does have the {lock/unlock}_bus
> methods?  I would like to see how you handled it.

http://git.kernel.org/?p=linux/kernel/git/vapier/blackfin.git;a=commitdiff;h=cc54fa8ed63e11a000031bc650d41d57b441803d
-mike

RE: [spi-devel-general] [PATCH 1/2] spi: new SPI bus lock/unlockfunctions

[H Hartley Sweeten]

On Thursday, September 17, 2009 3:54 PM, Mike Frysinger wrote:
>> I assume the spi master driver must supply the {lock/unlock}_bus methods
>> to properly support the locking.
>
> currently, yes.  a common solution would be nice.  ideas/patches welcome ;).
>
>> But, by returning 0 when the methods
>> are not supplied you are basically saying all the current master drivers
>> in mainline support bus locking.  I think this is really only "true" if
>> spi->master->num_chipselect == 1.
>
> right, but that is no different from what we have today.  there is no
> way for a client to guarantee exclusive access, so you cant write code
> assuming it in the first place.  the only consumer thus far (mmc_spi)
> actually errors out if such conditions exist.
>
> in other words, we arent breaking anything.

Actually you are breaking the mmc_spi driver.

By returning 0 when the methods are not supplied you are saying that the
master driver supports and locked the bus.  At a minimum, I think spi_lock_bus()
should return an error code if locking is not supported.

Also, as Andrew Morton pointed out, calling spi_unlock_bus() without having
a valid lock by calling spi_lock_bus() is a bug.

In addition your patch to mmc_spi should check the return code from
spi_lock_bus().  If the driver "requires" that the bus be locked it should
trigger an error path if it cannot be locked.

>> Also, do you have a master driver that does have the {lock/unlock}_bus
>> methods?  I would like to see how you handled it.
>
> http://git.kernel.org/?p=linux/kernel/git/vapier/blackfin.git;a=commitdiff;h=cc54fa8ed63e11a000031bc650d41d57b441803d

Oiy... The lock/unlock functions are simple enough but the change needed
to bfin_spi_pump_messages() is a bit complicated.

What happens to next_msg if it is for other devices on the bus?

Regards,
Hartley

Re: [spi-devel-general] [PATCH 1/2] spi: new SPI bus lock/unlockfunctions

[Mike Frysinger]

On Fri, Sep 18, 2009 at 19:00, H Hartley Sweeten wrote:
> On Thursday, September 17, 2009 3:54 PM, Mike Frysinger wrote:
>>> I assume the spi master driver must supply the {lock/unlock}_bus methods
>>> to properly support the locking.
>>
>> currently, yes.  a common solution would be nice.  ideas/patches welcome ;).
>>
>>> But, by returning 0 when the methods
>>> are not supplied you are basically saying all the current master drivers
>>> in mainline support bus locking.  I think this is really only "true" if
>>> spi->master->num_chipselect == 1.
>>
>> right, but that is no different from what we have today.  there is no
>> way for a client to guarantee exclusive access, so you cant write code
>> assuming it in the first place.  the only consumer thus far (mmc_spi)
>> actually errors out if such conditions exist.
>>
>> in other words, we arent breaking anything.
>
> Actually you are breaking the mmc_spi driver.

no we arent.  the current code does some sloppy checking for possible
concurrent access and then aborts.  the new code drops that sloppy
checking in place of the bus master doing it right.  any setup that is
working today will continue to work fine.  and setup *that is already
broken* will continue to be broken.

> By returning 0 when the methods are not supplied you are saying that the
> master driver supports and locked the bus.  At a minimum, I think spi_lock_bus()
> should return an error code if locking is not supported.
>
> Also, as Andrew Morton pointed out, calling spi_unlock_bus() without having
> a valid lock by calling spi_lock_bus() is a bug.
>
> In addition your patch to mmc_spi should check the return code from
> spi_lock_bus().  If the driver "requires" that the bus be locked it should
> trigger an error path if it cannot be locked.
>
>>> Also, do you have a master driver that does have the {lock/unlock}_bus
>>> methods?  I would like to see how you handled it.
>>
>> http://git.kernel.org/?p=linux/kernel/git/vapier/blackfin.git;a=commitdiff;h=cc54fa8ed63e11a000031bc650d41d57b441803d
>
> Oiy... The lock/unlock functions are simple enough but the change needed
> to bfin_spi_pump_messages() is a bit complicated.
>
> What happens to next_msg if it is for other devices on the bus?

Yi can take a stab at addressing these issues since he is the guy who
has been putting together in the first place.
-mike

RE: [spi-devel-general] [PATCH 1/2] spi: new SPI bus lock/unlockfunctions

[Li Yi]

On Fri, 2009-09-18 at 19:00 -0400, H Hartley Sweeten wrote:
> On Thursday, September 17, 2009 3:54 PM, Mike Frysinger wrote:
> >> I assume the spi master driver must supply the {lock/unlock}_bus methods
> >> to properly support the locking.
> >
> > currently, yes.  a common solution would be nice.  ideas/patches welcome ;).
> >
> >> But, by returning 0 when the methods
> >> are not supplied you are basically saying all the current master drivers
> >> in mainline support bus locking.  I think this is really only "true" if
> >> spi->master->num_chipselect == 1.
> >
> > right, but that is no different from what we have today.  there is no
> > way for a client to guarantee exclusive access, so you cant write code
> > assuming it in the first place.  the only consumer thus far (mmc_spi)
> > actually errors out if such conditions exist.
> >
> > in other words, we arent breaking anything.
> 
> Actually you are breaking the mmc_spi driver.
> 
> By returning 0 when the methods are not supplied you are saying that the
> master driver supports and locked the bus.  At a minimum, I think spi_lock_bus()
> should return an error code if locking is not supported.
> 
Thanks for the feedback.

The original thought of returning '0' was trying not to break mmc_spi
driver on system without spi_lock_bus() implementation.
What do you think of a check in mmc_spi_probe() like this?

if (spi->master->num_chipselect > 1 && !spi->master->lock_bus) {
	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");
}

And spi_lock_bus() still returns 0 if not implemented.

> Also, as Andrew Morton pointed out, calling spi_unlock_bus() without having
> a valid lock by calling spi_lock_bus() is a bug.
> 
Will be fixed.

> In addition your patch to mmc_spi should check the return code from
> spi_lock_bus().  If the driver "requires" that the bus be locked it should
> trigger an error path if it cannot be locked.
> 
Will add code to check the return code. 
> >> Also, do you have a master driver that does have the {lock/unlock}_bus
> >> methods?  I would like to see how you handled it.
> >
> > http://git.kernel.org/?p=linux/kernel/git/vapier/blackfin.git;a=commitdiff;h=cc54fa8ed63e11a000031bc650d41d57b441803d
> 
> Oiy... The lock/unlock functions are simple enough but the change needed
> to bfin_spi_pump_messages() is a bit complicated.
> 
In spi_bf5xx.c, if a spi device (e.g, whose chip select number is "3")
locks the bus, the global status "locked" is set to "3" - indicates the
spi bus is locked.

> What happens to next_msg if it is for other devices on the bus?
> 
This spi message will be left in the queue until the spi bus is
unlocked. The messages for the spi device holding the lock will be
selected and transferred firstly. 

Regards,
-Yi

Re: [PATCH 1/2] spi: new SPI bus lock/unlock functions

[Andrew Morton]

On Thu, 17 Sep 2009 18:03:16 -0400
Mike Frysinger <vapier@gentoo.org> wrote:

> From: Yi Li <yi.li@analog.com>
> 
> For some MMC cards over SPI bus, it needs to lock the SPI bus for its own
> use.  The SPI transfer must not be interrupted by other SPI devices that
> share the SPI bus with SPI MMC card.
> 
> This patch introduces 2 APIs for SPI bus locking operation.
> 
> Signed-off-by: Yi Li <yi.li@analog.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> Andrew: we've posted these in the past with no response.  could you pick
>         them up please ?
> 
>  drivers/spi/spi.c       |   48 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/spi.h |    7 ++++++
>  2 files changed, 55 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 70845cc..b82b8ad 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -653,6 +653,54 @@ static void spi_complete(void *arg)
>  }
>  
>  /**
> + * spi_lock_bus - lock SPI bus for exclusive access
> + * @spi: device which want to lock the bus
> + * Context: any
> + *
> + * Once the caller owns exclusive access to the SPI bus,
> + * only messages for this device will be transferred.
> + * Messages for other devices are queued but not transferred until
> + * the bus owner unlock the bus.
> + *
> + * The caller may call spi_lock_bus() before spi_sync() or spi_async().
> + * So this call may be used in irq and other contexts which can't sleep,
> + * as well as from task contexts which can sleep.

Hence spi_lock_bus() basically has to use a spinning lock?

So code which has called spi_lock_bus() cannot sleep until it calls
spi_unlock_bus()?

That's worth mentioning in the description.

> + * It returns zero on success, else a negative error code.
> + */
> +int spi_lock_bus(struct spi_device *spi)
> +{
> +	if (spi->master->lock_bus)
> +		return spi->master->lock_bus(spi);
> +	else
> +		return 0;
> +}
> +EXPORT_SYMBOL_GPL(spi_lock_bus);
> +
> +/**
> + * spi_unlock_bus - unlock SPI bus
> + * @spi: device which want to unlock the bus
> + * Context: any
> + *
> + * The caller has called spi_lock_bus() to lock the bus. It calls
> + * spi_unlock_bus() to release the bus so messages for other devices
> + * can be transferred.
> + *
> + * If the caller did not call spi_lock_bus() before, spi_unlock_bus()
> + * should have no effect.

That's crazy.

Calling spi_unlock_bus() without having earlier called spi_lock_bus()
is a bug, and the kernel's response should be to go BUG(), not to
silently ignore the bug.  Especially as the implementation will need to
add extra code to silently ignore the bug!

Perhaps the comment wasn't well thought-out.  I cannot tell because I
cannot see any implementations of ->lock_bus() anywhere.

> + * It returns zero on success, else a negative error code.
> + */
> +int spi_unlock_bus(struct spi_device *spi)
> +{
> +	if (spi->master->unlock_bus)
> +		return spi->master->unlock_bus(spi);
> +	else
> +		return 0;
> +}
> +EXPORT_SYMBOL_GPL(spi_unlock_bus);

Re: [PATCH 1/2] spi: new SPI bus lock/unlock functions

[Li Yi]

On Fri, 2009-09-18 at 14:29 -0700, Andrew Morton wrote:
> On Thu, 17 Sep 2009 18:03:16 -0400
> Mike Frysinger <vapier@gentoo.org> wrote:
> 
> > From: Yi Li <yi.li@analog.com>
> > 
> > For some MMC cards over SPI bus, it needs to lock the SPI bus for its own
> > use.  The SPI transfer must not be interrupted by other SPI devices that
> > share the SPI bus with SPI MMC card.
> > 
> > This patch introduces 2 APIs for SPI bus locking operation.
> > 
> > Signed-off-by: Yi Li <yi.li@analog.com>
> > Signed-off-by: Bryan Wu <cooloney@kernel.org>
> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> > ---
> > Andrew: we've posted these in the past with no response.  could you pick
> >         them up please ?
> > 
> >  drivers/spi/spi.c       |   48 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/spi/spi.h |    7 ++++++
> >  2 files changed, 55 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index 70845cc..b82b8ad 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -653,6 +653,54 @@ static void spi_complete(void *arg)
> >  }
> >  
> >  /**
> > + * spi_lock_bus - lock SPI bus for exclusive access
> > + * @spi: device which want to lock the bus
> > + * Context: any
> > + *
> > + * Once the caller owns exclusive access to the SPI bus,
> > + * only messages for this device will be transferred.
> > + * Messages for other devices are queued but not transferred until
> > + * the bus owner unlock the bus.
> > + *
> > + * The caller may call spi_lock_bus() before spi_sync() or spi_async().
> > + * So this call may be used in irq and other contexts which can't sleep,
> > + * as well as from task contexts which can sleep.
> 
> Hence spi_lock_bus() basically has to use a spinning lock?
> 
> So code which has called spi_lock_bus() cannot sleep until it calls
> spi_unlock_bus()?
> 
> That's worth mentioning in the description.
> 
Code called spi_lock_bus() _can_ sleep. This is mentioned in the
comments.

> > + * It returns zero on success, else a negative error code.
> > + */
> > +int spi_lock_bus(struct spi_device *spi)
> > +{
> > +	if (spi->master->lock_bus)
> > +		return spi->master->lock_bus(spi);
> > +	else
> > +		return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(spi_lock_bus);
> > +
> > +/**
> > + * spi_unlock_bus - unlock SPI bus
> > + * @spi: device which want to unlock the bus
> > + * Context: any
> > + *
> > + * The caller has called spi_lock_bus() to lock the bus. It calls
> > + * spi_unlock_bus() to release the bus so messages for other devices
> > + * can be transferred.
> > + *
> > + * If the caller did not call spi_lock_bus() before, spi_unlock_bus()
> > + * should have no effect.
> 
> That's crazy.
> 
> Calling spi_unlock_bus() without having earlier called spi_lock_bus()
> is a bug, and the kernel's response should be to go BUG(), not to
> silently ignore the bug.  Especially as the implementation will need to
> add extra code to silently ignore the bug!
> 
I will change the patch to fix this. Thanks.

> Perhaps the comment wasn't well thought-out.  I cannot tell because I
> cannot see any implementations of ->lock_bus() anywhere.
> 
The implementation for blackfin spi master is in
drivers/spi/spi_bfin5xx.c:
http://git.kernel.org/?p=linux/kernel/git/vapier/blackfin.git;a=commitdiff;h=cc54fa8ed63e11a000031bc650d41d57b441803d

As you mentioned, if a spi device calls spi_unlock_bus() without calling
spi_lock_bus() before, it should be returned an error. This will be
fixed in next version of the patch.

-Yi

Re: [PATCH 1/2] spi: new SPI bus lock/unlock functions

[Mike Frysinger]

On Mon, Sep 21, 2009 at 02:33, Li Yi wrote:
> On Fri, 2009-09-18 at 14:29 -0700, Andrew Morton wrote:
>> On Thu, 17 Sep 2009 18:03:16 -0400
>> Mike Frysinger <vapier@gentoo.org> wrote:
>>
>> > From: Yi Li <yi.li@analog.com>
>> >
>> > For some MMC cards over SPI bus, it needs to lock the SPI bus for its own
>> > use.  The SPI transfer must not be interrupted by other SPI devices that
>> > share the SPI bus with SPI MMC card.
>> >
>> > This patch introduces 2 APIs for SPI bus locking operation.
>> >
>> > Signed-off-by: Yi Li <yi.li@analog.com>
>> > Signed-off-by: Bryan Wu <cooloney@kernel.org>
>> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>> > ---
>> > Andrew: we've posted these in the past with no response.  could you pick
>> >         them up please ?
>> >
>> >  drivers/spi/spi.c       |   48 +++++++++++++++++++++++++++++++++++++++++++++++
>> >  include/linux/spi/spi.h |    7 ++++++
>> >  2 files changed, 55 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> > index 70845cc..b82b8ad 100644
>> > --- a/drivers/spi/spi.c
>> > +++ b/drivers/spi/spi.c
>> > @@ -653,6 +653,54 @@ static void spi_complete(void *arg)
>> >  }
>> >
>> >  /**
>> > + * spi_lock_bus - lock SPI bus for exclusive access
>> > + * @spi: device which want to lock the bus
>> > + * Context: any
>> > + *
>> > + * Once the caller owns exclusive access to the SPI bus,
>> > + * only messages for this device will be transferred.
>> > + * Messages for other devices are queued but not transferred until
>> > + * the bus owner unlock the bus.
>> > + *
>> > + * The caller may call spi_lock_bus() before spi_sync() or spi_async().
>> > + * So this call may be used in irq and other contexts which can't sleep,
>> > + * as well as from task contexts which can sleep.
>>
>> Hence spi_lock_bus() basically has to use a spinning lock?
>>
>> So code which has called spi_lock_bus() cannot sleep until it calls
>> spi_unlock_bus()?
>>
>> That's worth mentioning in the description.
>>
> Code called spi_lock_bus() _can_ sleep. This is mentioned in the
> comments.

should add might_sleep() to the common spi_lock_bus() function then.
-mike

Re: [PATCH 1/2] spi: new SPI bus lock/unlock functions

[Mike Frysinger]

On Thu, Sep 17, 2009 at 18:03, Mike Frysinger wrote:
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -653,6 +653,54 @@ static void spi_complete(void *arg)
> +int spi_lock_bus(struct spi_device *spi)
> +{
> +       if (spi->master->lock_bus)
> +               return spi->master->lock_bus(spi);
> +       else
> +               return 0;
> +}
> +EXPORT_SYMBOL_GPL(spi_lock_bus);
> +
> +int spi_unlock_bus(struct spi_device *spi)
> +{
> +       if (spi->master->unlock_bus)
> +               return spi->master->unlock_bus(spi);
> +       else
> +               return 0;
> +}
> +EXPORT_SYMBOL_GPL(spi_unlock_bus);

there's nothing Blackfin-specific in the implementation of these
functions.  i think the way we should be handling these is by doing:
 - remove {lock,unlock}_bus functions from spi_master
 - move the {lock,unlock}_bus code from spi_bfin5xx.c to spi.c
 - drop the SPI_BFIN_LOCK Kconfig
 - add a new spi_master flag to spi.h like SPI_MASTER_HALF_DUPLEX --
SPI_MASTER_LOCK_BUS
 - have spi_bfin5xx.c/bfin_sport_spi.c add that flag to its master setup
 - have the common spi code key off of that flag to return ENOSYS
 - have the mmc_spi code check that bit in the master before falling
back to its hack
-mike

RE: [spi-devel-general] [PATCH 1/2] spi: new SPI bus lock/unlockfunctions

[H Hartley Sweeten]

On Thursday, September 17, 2009 3:03 PM, Mike Frysinger wrote:
>
> From: Yi Li <yi.li@analog.com>
>
> For some MMC cards over SPI bus, it needs to lock the SPI bus for its own
> use.  The SPI transfer must not be interrupted by other SPI devices that
> share the SPI bus with SPI MMC card.
> 
> This patch introduces 2 APIs for SPI bus locking operation.
> 
> Signed-off-by: Yi Li <yi.li@analog.com>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

Has any additional work be done on this patchset?

It would be nice to finally have a solution that lets the mmc_spi driver
work on a shared spi bus.

Regards,
Hartley

Re: [spi-devel-general] [PATCH 1/2] spi: new SPI bus lock/unlockfunctions

[Mike Frysinger]

On Wed, Dec 16, 2009 at 17:49, H Hartley Sweeten wrote:
> On Thursday, September 17, 2009 3:03 PM, Mike Frysinger wrote:
>>
>> From: Yi Li <yi.li@analog.com>
>>
>> For some MMC cards over SPI bus, it needs to lock the SPI bus for its own
>> use.  The SPI transfer must not be interrupted by other SPI devices that
>> share the SPI bus with SPI MMC card.
>>
>> This patch introduces 2 APIs for SPI bus locking operation.
>
> Has any additional work be done on this patchset?

nope

> It would be nice to finally have a solution that lets the mmc_spi driver
> work on a shared spi bus.

the latest patch is on the list for _anyone_ to contribute their fixes
to address the posted issues
-mike