[PATCH 0/5] spi/mmc_spi: SPI bus locking to use mmc_spi together with other SPI devices

[PATCH 0/5] spi/mmc_spi: SPI bus locking to use mmc_spi together with other SPI devices

[Ernst Schwab]

Abstract 
--------
* SPI bus locking required for mmc_spi driver sharing
  the bus with other SPI devices.
* Proposal for API functions for SPI bus locking:
  In addition to spi_lock_bus/spi_unlock_bus, there
  will be spi_register_lock_bus/spi_unregister_lock_bus.
* Patches for spi_mpc8xxx.c for SPI bus locking.

The problem
-----------
The mmc_spi driver requires an exclusive access to the
SPI bus that must not be interrupted by accessing any 
other SPI device on the same bus. 

The current situation
---------------------
In the current kernel, the mmc_spi driver is operable
only when no other SPI clients use the same bus. At 
initialization time, it checks if it is the only SPI 
client, and refuses to install itself with a 
"can't share SPI bus" message otherwise.
If it is the only SPI client, it tells its wishes 
with an "ASSUMING SPI bus stays unshared" message; 
future SPI client registrations will not be prohibited.

Previous attempts to solve the problem
--------------------------------------
There was a set of patches by Yi Li/Bryan Wu/Mike Frysinger 
to introduce two API functions spi_lock_bus() and 
spi_unlock_bus() to enable cooperative sharing of the
SPI bus between the mmc_spi driver and other SPI drivers. 
It requires code extension in the SPI master driver to 
handle the exclusive bus access, which was implemented in
the ucLinux Blackfin SPI driver spi_bfin5xx.c.
The extended SPI master driver transmits only SPI messages
for the SPI device that locked the bus, messages for all
other SPI devices remain queued until the bus is unlocked.
The code is available in the current uClinux Blackfin tree.

Proposal for a solution
-----------------------
In addition to the previously proposed spi_lock_bus() 
and spi_unlock_bus() functions, two functions 
spi_register_lock_bus() and spi_unregister_lock_bus() 
can be implemented that can be used by a driver 
that wants to call spi_lock_bus() and spi_unlock_bus. 

A transition from the existing situation 
to a convenient solution could be:

* Insertion of spi_(un)register_lock_bus 
  and spi_(un)lock_bus calls into mmc_spi.c
* Implementation of spi_register_(un)lock_bus() 
  in spi.c.
* Implementation of spi_(un)lock_bus() in spi.c.
* Integration of the changes to spi_bfin5xx.c 
  (uClinux Blackfin tree) and to the PowerQuicc 
  spi_mpc8xxx.c driver (new patch) implementing 
  the (un)lock_bus handlers.

Implementation of the API functions
-----------------------------------
int spi_register_lock_bus()
{
    // The implementation of spi_register_lock_bus()
    // allows bus locking only for one SPI driver,
    // i.e. the mmc_spi driver.
    if (bus locking rights already 
        granted to another caller) {
        return FAILURE;
    } 
    
    if (SPI master capable of bus locking) {
        return SUCCESS;
    } 

    if (caller is the only SPI device) {
        let future spi_add_device calls fail;
        return SUCCESS;
    } 
    

    return FAILURE;
}

void spi_lock_bus()
{
    // The implementation of spi_lock_bus has no need 
    // to sleep, and it cannot fail because nobody but
    // the caller itself (mmc_spi) can hold the lock.
  
    if (SPI master capable of bus locking) {
        call SPI master lock_bus();
    }
}

Possible future extensions
--------------------------
As soon as there will be a requirement for more than
one driver obtaining an SPI bus lock, the 
spi_(un)lock_bus() functions can be extended to 
put the caller to sleep until the bus lock 
becomes available. 
The spi_register_bus_lock() function then can allow
more than one driver to register for bus locking.
The mmc_spi driver and the already modified SPI
master drivers do not need to be reworked again.

Patches
-------
The successive mails will contain these patches:
1 - spi_lock_bus (previously released patch)
2 - spi_register_lock_bus
3 - added spi_lock_bus to mmc_spi (previously released patch)
4 - added spi_register_lock_bus to mmc_spi
5 - SPI bus locking for spi_mpc8xxx.c

------------------------------------------------------------------------------
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/5] spi: spi_lock_bus and spi_unlock_bus

[Ernst Schwab]

From: Yi Li <yi.li-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>

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-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bryan Wu <cooloney-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.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

  



------------------------------------------------------------------------------
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/5] spi: spi_lock_bus and spi_unlock_bus

[Grant Likely]

On Tue, Feb 16, 2010 at 12:57 PM, Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org> wrote:
> From: Yi Li <yi.li-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>
> 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-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Bryan Wu <cooloney-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.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);

This series seems to try and solve the problem the hard way, and by
creating a new locking scheme (and as history shows, new locking
schemes are *alwasy* broken).

Why is the locking getting pushed down to the bus driver level?  It
seems to me that the whole thing could be handled with common code and
a mutex in the spi_master structure.  spi_sync would be easy to handle
by putting a mutex around the spi_message submission.  spi_async would
be a little harder since it needs to be atomic, but that could also be
handled with a flag protected by a spinlock.

Basically, the idea is that existing drivers continue to use the API as-is

Drivers that want to lock the bus for exclusive access must call
spi_lock_bus() which should take the mutex and then sleep until all
in-flight spi_messages are processed.  After that, anyone calling
spi_async() will simply sleep until the locker unlocks the bus again.

To handle spi_sync() would probably require a flag protected by a
spinlock.  If the flag is set, then spi_sync() would simply fail.

Finally, the locking driver would need locked versions of spi_sync()
and spi_async() that sidestep the lock checks.  It would only be valid
to call these versions when holding the SPI bus lock.

There is no need to specify the spi_device in the lock request.  Since
the lock is exclusive, it is known that the only driver calling the
locked API version must already hold the lock.

Have I got this wrong?

g.

> +
> +/**
> + * 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
>
>
>
>
>



-- 
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/5] spi: spi_lock_bus and spi_unlock_bus

[Ned Forrester]

On 02/16/2010 03:43 PM, Grant Likely wrote:
> On Tue, Feb 16, 2010 at 12:57 PM, Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org> wrote:
>> From: Yi Li <yi.li-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>
>> 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.
> 
> This series seems to try and solve the problem the hard way, and by
> creating a new locking scheme (and as history shows, new locking
> schemes are *alwasy* broken).
> 
> Why is the locking getting pushed down to the bus driver level?  It
> seems to me that the whole thing could be handled with common code and
> a mutex in the spi_master structure.  spi_sync would be easy to handle
> by putting a mutex around the spi_message submission.  spi_async would
> be a little harder since it needs to be atomic, but that could also be
> handled with a flag protected by a spinlock.
> 
> Basically, the idea is that existing drivers continue to use the API as-is
> 
> Drivers that want to lock the bus for exclusive access must call
> spi_lock_bus() which should take the mutex and then sleep until all
> in-flight spi_messages are processed.  After that, anyone calling
> spi_async() will simply sleep until the locker unlocks the bus again.
> 
> To handle spi_sync() would probably require a flag protected by a
> spinlock.  If the flag is set, then spi_sync() would simply fail.
> 
> Finally, the locking driver would need locked versions of spi_sync()
> and spi_async() that sidestep the lock checks.  It would only be valid
> to call these versions when holding the SPI bus lock.
> 
> There is no need to specify the spi_device in the lock request.  Since
> the lock is exclusive, it is known that the only driver calling the
> locked API version must already hold the lock.
> 
> Have I got this wrong?

Not sure.  Does your proposal account for the case of a system with more
than one SPI bus?  In such a case, there should be a lock per bus.

-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


------------------------------------------------------------------------------
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/5] spi: spi_lock_bus and spi_unlock_bus

[Grant Likely]

On Tue, Feb 16, 2010 at 2:16 PM, Ned Forrester <nforrester-/d+BM93fTQY@public.gmane.org> wrote:
> On 02/16/2010 03:43 PM, Grant Likely wrote:
>> On Tue, Feb 16, 2010 at 12:57 PM, Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org> wrote:
>>> From: Yi Li <yi.li-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>>
>>> 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.
>>
>> This series seems to try and solve the problem the hard way, and by
>> creating a new locking scheme (and as history shows, new locking
>> schemes are *alwasy* broken).
>>
>> Why is the locking getting pushed down to the bus driver level?  It
>> seems to me that the whole thing could be handled with common code and
>> a mutex in the spi_master structure.  spi_sync would be easy to handle
>> by putting a mutex around the spi_message submission.  spi_async would
>> be a little harder since it needs to be atomic, but that could also be
>> handled with a flag protected by a spinlock.
>>
>> Basically, the idea is that existing drivers continue to use the API as-is
>>
>> Drivers that want to lock the bus for exclusive access must call
>> spi_lock_bus() which should take the mutex and then sleep until all
>> in-flight spi_messages are processed.  After that, anyone calling
>> spi_async() will simply sleep until the locker unlocks the bus again.
>>
>> To handle spi_sync() would probably require a flag protected by a
>> spinlock.  If the flag is set, then spi_sync() would simply fail.
>>
>> Finally, the locking driver would need locked versions of spi_sync()
>> and spi_async() that sidestep the lock checks.  It would only be valid
>> to call these versions when holding the SPI bus lock.
>>
>> There is no need to specify the spi_device in the lock request.  Since
>> the lock is exclusive, it is known that the only driver calling the
>> locked API version must already hold the lock.
>>
>> Have I got this wrong?
>
> Not sure.  Does your proposal account for the case of a system with more
> than one SPI bus?  In such a case, there should be a lock per bus.

Yes.  One lock per bus.  In the spi_master structure.

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/5] spi: spi_lock_bus and spi_unlock_bus

[Mike Frysinger]

On Tue, Feb 16, 2010 at 15:43, Grant Likely wrote:
> On Tue, Feb 16, 2010 at 12:57 PM, Ernst Schwab 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.
>> + *
>> + * 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);
>
> This series seems to try and solve the problem the hard way, and by
> creating a new locking scheme (and as history shows, new locking
> schemes are *alwasy* broken).
>
> Why is the locking getting pushed down to the bus driver level?  It
> seems to me that the whole thing could be handled with common code and
> a mutex in the spi_master structure.  spi_sync would be easy to handle
> by putting a mutex around the spi_message submission.  spi_async would
> be a little harder since it needs to be atomic, but that could also be
> handled with a flag protected by a spinlock.
>
> Basically, the idea is that existing drivers continue to use the API as-is
>
> Drivers that want to lock the bus for exclusive access must call
> spi_lock_bus() which should take the mutex and then sleep until all
> in-flight spi_messages are processed.  After that, anyone calling
> spi_async() will simply sleep until the locker unlocks the bus again.
>
> To handle spi_sync() would probably require a flag protected by a
> spinlock.  If the flag is set, then spi_sync() would simply fail.
>
> Finally, the locking driver would need locked versions of spi_sync()
> and spi_async() that sidestep the lock checks.  It would only be valid
> to call these versions when holding the SPI bus lock.
>
> There is no need to specify the spi_device in the lock request.  Since
> the lock is exclusive, it is known that the only driver calling the
> locked API version must already hold the lock.

this is what i proposed last time, but we havent gotten around to
implementing it:

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

------------------------------------------------------------------------------
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/5] spi: spi_lock_bus and spi_unlock_bus

[Ernst Schwab]

Mike Frysinger wrote:

> this is what i proposed last time, but we havent gotten around to
> implementing it:
>
>  - add a new spi_master flag to spi.h like SPI_MASTER_HALF_DUPLEX --
> SPI_MASTER_LOCK_BUS
>  - have the mmc_spi code check that bit in the master before falling
> back to its hack

This would limit the bus locking capability of the _API_ to a single
driver (the mmc_spi), or did I miss something?

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

Re: [PATCH 1/5] spi: spi_lock_bus and spi_unlock_bus

[Mike Frysinger]

On Tue, Feb 16, 2010 at 19:21, Ernst Schwab wrote:
> Mike Frysinger wrote:
>> this is what i proposed last time, but we havent gotten around to
>> implementing it:
>>
>>  - add a new spi_master flag to spi.h like SPI_MASTER_HALF_DUPLEX --
>> SPI_MASTER_LOCK_BUS
>>  - have the mmc_spi code check that bit in the master before falling
>> back to its hack
>
> This would limit the bus locking capability of the _API_ to a single
> driver (the mmc_spi), or did I miss something?

the mmc_spi code checks for exclusiveness at load time ... it needs to
have the lock/unlock steps pushed down to where it actually needs it
-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/5] spi: spi_lock_bus and spi_unlock_bus

[Grant Likely]

On Tue, Feb 16, 2010 at 5:07 PM, Mike Frysinger <vapier.adi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Feb 16, 2010 at 15:43, Grant Likely wrote:
>> On Tue, Feb 16, 2010 at 12:57 PM, Ernst Schwab wrote:
>>> From: Yi Li <yi.li-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>>
>>> 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-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>> Signed-off-by: Bryan Wu <cooloney-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.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);
>>
>> This series seems to try and solve the problem the hard way, and by
>> creating a new locking scheme (and as history shows, new locking
>> schemes are *alwasy* broken).
>>
>> Why is the locking getting pushed down to the bus driver level?  It
>> seems to me that the whole thing could be handled with common code and
>> a mutex in the spi_master structure.  spi_sync would be easy to handle
>> by putting a mutex around the spi_message submission.  spi_async would
>> be a little harder since it needs to be atomic, but that could also be
>> handled with a flag protected by a spinlock.
>>
>> Basically, the idea is that existing drivers continue to use the API as-is
>>
>> Drivers that want to lock the bus for exclusive access must call
>> spi_lock_bus() which should take the mutex and then sleep until all
>> in-flight spi_messages are processed.  After that, anyone calling
>> spi_async() will simply sleep until the locker unlocks the bus again.
>>
>> To handle spi_sync() would probably require a flag protected by a
>> spinlock.  If the flag is set, then spi_sync() would simply fail.
>>
>> Finally, the locking driver would need locked versions of spi_sync()
>> and spi_async() that sidestep the lock checks.  It would only be valid
>> to call these versions when holding the SPI bus lock.
>>
>> There is no need to specify the spi_device in the lock request.  Since
>> the lock is exclusive, it is known that the only driver calling the
>> locked API version must already hold the lock.
>
> this is what i proposed last time, but we havent gotten around to
> implementing it:
>
> 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

Am I missing something. I cannot find any lock related functions in
the spi code.  Is this stuff in mainline?

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/5] spi: spi_lock_bus and spi_unlock_bus

[Mike Frysinger]

On Tue, Feb 16, 2010 at 22:48, Grant Likely wrote:
> On Tue, Feb 16, 2010 at 5:07 PM, Mike Frysinger wrote:
>> On Tue, Feb 16, 2010 at 15:43, Grant Likely wrote:
>>> On Tue, Feb 16, 2010 at 12:57 PM, Ernst Schwab 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.
>>>> + *
>>>> + * 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);
>>>
>>> This series seems to try and solve the problem the hard way, and by
>>> creating a new locking scheme (and as history shows, new locking
>>> schemes are *alwasy* broken).
>>>
>>> Why is the locking getting pushed down to the bus driver level?  It
>>> seems to me that the whole thing could be handled with common code and
>>> a mutex in the spi_master structure.  spi_sync would be easy to handle
>>> by putting a mutex around the spi_message submission.  spi_async would
>>> be a little harder since it needs to be atomic, but that could also be
>>> handled with a flag protected by a spinlock.
>>>
>>> Basically, the idea is that existing drivers continue to use the API as-is
>>>
>>> Drivers that want to lock the bus for exclusive access must call
>>> spi_lock_bus() which should take the mutex and then sleep until all
>>> in-flight spi_messages are processed.  After that, anyone calling
>>> spi_async() will simply sleep until the locker unlocks the bus again.
>>>
>>> To handle spi_sync() would probably require a flag protected by a
>>> spinlock.  If the flag is set, then spi_sync() would simply fail.
>>>
>>> Finally, the locking driver would need locked versions of spi_sync()
>>> and spi_async() that sidestep the lock checks.  It would only be valid
>>> to call these versions when holding the SPI bus lock.
>>>
>>> There is no need to specify the spi_device in the lock request.  Since
>>> the lock is exclusive, it is known that the only driver calling the
>>> locked API version must already hold the lock.
>>
>> this is what i proposed last time, but we havent gotten around to
>> implementing it:
>>
>> 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
>
> Am I missing something. I cannot find any lock related functions in
> the spi code.  Is this stuff in mainline?

i'm proposing what should be done.  there is no locking logic anywhere atm.
-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/5] spi: spi_lock_bus and spi_unlock_bus

[Grant Likely]

On Tue, Feb 16, 2010 at 9:34 PM, Mike Frysinger <vapier.adi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Feb 16, 2010 at 22:48, Grant Likely wrote:
>>> 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
>>
>> Am I missing something. I cannot find any lock related functions in
>> the spi code.  Is this stuff in mainline?
>
> i'm proposing what should be done.  there is no locking logic anywhere atm.

Okay, but you're talking about removing functions (lock_bus,
unlock_bus) and config values (SPI_BFIN_LOCK) which I cannot find
anywhere in mainline.

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/5] spi: spi_lock_bus and spi_unlock_bus

[Mike Frysinger]

On Tue, Feb 16, 2010 at 23:47, Grant Likely wrote:
> On Tue, Feb 16, 2010 at 9:34 PM, Mike Frysinger wrote:
>> On Tue, Feb 16, 2010 at 22:48, Grant Likely wrote:
>>>> 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
>>>
>>> Am I missing something. I cannot find any lock related functions in
>>> the spi code.  Is this stuff in mainline?
>>
>> i'm proposing what should be done.  there is no locking logic anywhere atm.
>
> Okay, but you're talking about removing functions (lock_bus,
> unlock_bus) and config values (SPI_BFIN_LOCK) which I cannot find
> anywhere in mainline.

they were part of the patchsets that were under discussion
-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/5] spi: spi_lock_bus and spi_unlock_bus

[Grant Likely]

On Tue, Feb 16, 2010 at 10:04 PM, Mike Frysinger <vapier.adi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Feb 16, 2010 at 23:47, Grant Likely wrote:
>> On Tue, Feb 16, 2010 at 9:34 PM, Mike Frysinger wrote:
>>> On Tue, Feb 16, 2010 at 22:48, Grant Likely wrote:
>>>>> 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
>>>>
>>>> Am I missing something. I cannot find any lock related functions in
>>>> the spi code.  Is this stuff in mainline?
>>>
>>> i'm proposing what should be done.  there is no locking logic anywhere atm.
>>
>> Okay, but you're talking about removing functions (lock_bus,
>> unlock_bus) and config values (SPI_BFIN_LOCK) which I cannot find
>> anywhere in mainline.
>
> they were part of the patchsets that were under discussion

Ah, I misunderstood where you were coming from.

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/5] spi: spi_lock_bus and spi_unlock_bus

[Grant Likely]

On Tue, Feb 16, 2010 at 5:07 PM, Mike Frysinger <vapier.adi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Feb 16, 2010 at 15:43, Grant Likely wrote:
>> On Tue, Feb 16, 2010 at 12:57 PM, Ernst Schwab wrote:
>>> From: Yi Li <yi.li-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>>
>>> 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-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>> Signed-off-by: Bryan Wu <cooloney-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Signed-off-by: Mike Frysinger <vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.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);
>>
>> This series seems to try and solve the problem the hard way, and by
>> creating a new locking scheme (and as history shows, new locking
>> schemes are *alwasy* broken).
>>
>> Why is the locking getting pushed down to the bus driver level?  It
>> seems to me that the whole thing could be handled with common code and
>> a mutex in the spi_master structure.  spi_sync would be easy to handle
>> by putting a mutex around the spi_message submission.  spi_async would
>> be a little harder since it needs to be atomic, but that could also be
>> handled with a flag protected by a spinlock.
>>
>> Basically, the idea is that existing drivers continue to use the API as-is
>>
>> Drivers that want to lock the bus for exclusive access must call
>> spi_lock_bus() which should take the mutex and then sleep until all
>> in-flight spi_messages are processed.  After that, anyone calling
>> spi_async() will simply sleep until the locker unlocks the bus again.
>>
>> To handle spi_sync() would probably require a flag protected by a
>> spinlock.  If the flag is set, then spi_sync() would simply fail.
>>
>> Finally, the locking driver would need locked versions of spi_sync()
>> and spi_async() that sidestep the lock checks.  It would only be valid
>> to call these versions when holding the SPI bus lock.
>>
>> There is no need to specify the spi_device in the lock request.  Since
>> the lock is exclusive, it is known that the only driver calling the
>> locked API version must already hold the lock.
>
> this is what i proposed last time, but we havent gotten around to
> implementing it:
>
> 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

I think you're making it too complicated.  I think it can all be
implemented without touching the spi_master drivers at all.  And I
think the hack can be entirely removed from the mmc_spi code.  But I'm
coming into this problem cold, please feel free to point out my
mistaken assumptions and punch holes in my argument.  :-)

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/5] spi: spi_lock_bus and spi_unlock_bus

[Ernst Schwab]

Grant Likely wrote:

> This series seems to try and solve the problem the hard way, and by
> creating a new locking scheme (and as history shows, new locking
> schemes are *alwasy* broken).
> Why is the locking getting pushed down to the bus driver level?  

The proposal, as well as the proposal of the underlying patches
for the blackfin uClinux solution, wants to minimize the risk
introduced by changes to the SPI layer - it tries to be as
noninvasive as possible.
Thus, it allows the use of the mmc_spi driver under the same conditions
as before (only one SPI device present), and makes its use even
safer than before - in case an SPI device is registered after the mmc_spi
driver. 
The mechanism to have the atomic transmission of SPI sequences moved 
to the SPI master driver is existent in (now) two master drivers 
and working, and until now, we have only the choice between
the "ASSUMING SPI bus stays unshared!/can't share SPI bus" 
pestilence now present in the kernel and the "need to modify the 
SPI master" cholera. 
I think anything beyond this would be a new locking scheme, which...

> It
> seems to me that the whole thing could be handled with common code and
> a mutex in the spi_master structure.  spi_sync would be easy to handle
> by putting a mutex around the spi_message submission.  spi_async would
> be a little harder since it needs to be atomic, but that could also be
> handled with a flag protected by a spinlock.

Maybe, but who can implement that in a quality required to make it to
the kernel? Any volunteers ;-)? I will test it.

> There is no need to specify the spi_device in the lock request.  Since
> the lock is exclusive, it is known that the only driver calling the
> locked API version must already hold the lock.

On multiple SPI buses, there can be one lock per master, as Ned mentioned.

I want to summarize the properties of the suggested patches:

- with only one SPI device requiring the locking (the mmc_spi), 
  the patches should not cause any change in timing / locking / sleeping,
  neither with an unmodified SPI master nor with a modified.
- with multiple SPI devices not requiring the locking, the patches
  should not cause any change in timing / locking / sleeping,
  neither with an unmodified SPI master nor with a modified.
- if the mmc_spi driver is registered before other SPI devices, 
  used with an unmodified master, malfunction or data loss will now
  be prevented by disabling the SPI devices coming after the mmc_spi; 
  the current solution is not able to do so beside the "ASSUMING SPI 
  bus stays unshared!" message.
- the mmc_spi driver is working together with other SPI devices and a
  modified master - two modified masters exist.
- the spi_lock_bus calls added to the mmc_spi can be used for an advanced
  locking scheme (any volunteers?)

For me, there seems to be no disadvantage in having this or a similar
solution as an intermediate step to the perfect solution 
(any volunteers?). 

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

Re: [PATCH 1/5] spi: spi_lock_bus and spi_unlock_bus

[Grant Likely]

On Tue, Feb 16, 2010 at 5:16 PM, Ernst Schwab <eschwab-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org> wrote:
> Grant Likely wrote:
>
>> This series seems to try and solve the problem the hard way, and by
>> creating a new locking scheme (and as history shows, new locking
>> schemes are *alwasy* broken).
>> Why is the locking getting pushed down to the bus driver level?
>
> The proposal, as well as the proposal of the underlying patches
> for the blackfin uClinux solution, wants to minimize the risk
> introduced by changes to the SPI layer - it tries to be as
> noninvasive as possible.

It sounds like you're very worried about making changes to the core
code, and jumping through hopes to push as much as possible out to the
device drivers (working around instead of fixing a limitation of the
SPI bus infrastructure).  However, it ends up being ugly, complex, and
probably wrong.

Simplicity and correctness is far more important that trying to avoid
churn in the core code.  It is far better to change the core code
because then you'll get more testing and review, and you'll reduce the
burden on driver writers.

> Thus, it allows the use of the mmc_spi driver under the same conditions
> as before (only one SPI device present), and makes its use even
> safer than before - in case an SPI device is registered after the mmc_spi
> driver. The mechanism to have the atomic transmission of SPI sequences moved
> to the SPI master driver is existent in (now) two master drivers and
> working, and until now, we have only the choice between
> the "ASSUMING SPI bus stays unshared!/can't share SPI bus" pestilence now
> present in the kernel and the "need to modify the SPI master" cholera. I
> think anything beyond this would be a new locking scheme, which...
>
>> It
>> seems to me that the whole thing could be handled with common code and
>> a mutex in the spi_master structure.  spi_sync would be easy to handle
>> by putting a mutex around the spi_message submission.  spi_async would
>> be a little harder since it needs to be atomic, but that could also be
>> handled with a flag protected by a spinlock.
>
> Maybe, but who can implement that in a quality required to make it to
> the kernel? Any volunteers ;-)? I will test it.

You do it.  You write good code.  You're just approaching the problem
from the wrong angle.  You may not get it right on the first posting,
but there are lots of people who are here who will help you and
review.  As long as you are responsive to review you will do just
fine.

It will require 2 patches, plus another cleanup patch for each
spi_master driver.

1) add new locking API and implementation to core SPI code
- no drivers or mmc stuff has to change at this point

2) make mmc layer use the new api and remove the old hacks
- Clean cutover to new locking scheme.  Immediately stops using old
driver-specific hacks

3) remove the old hacks from spi_master driver
- because now the code isn't used anymore.  (But as far as I can tell,
none of this is actually mainlined, so this might not be necessary).
- However, by moving locking into the spi core, it may be that
spi_master drivers can become simpler and remove a lot of driver-local
locking.
- Regardless, neither patch 1 nor 2 force changes to the spi_master drivers.

In fact, now that I've had an evening to think about it, the solution
is even simpler than what I said earlier.  It requires 3 things to be
added to struct spi_master.
- 1 Mutex
- 1 spin lock
- 1 flag.

The mutex protects spi_async, and provides sleeping "for free"
The spinlock protects the atomic spi_sync call.
The flag is set when the lock is obtained, and checked while holding
the spinlock in spi_sync().  If the flag is checked, then spi_sync()
must fail immediately.

The current runtime API looks like this:
spi_sync(struct spi_device*, struct spi_message*);
spi_async(struct spi_device*, struct spi_message*);

The API needs to be extended to this:
spi_sync(struct spi_device*, struct spi_message*)
spi_async(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_sync_locked(struct spi_device*, struct spi_message*)
spi_async_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_sync() and spi_sync_locked() is that the
locked version bypasses the check of the lock flag.  Both versions
need to obtain the spinlock.

The difference between spi_async() and spi_async_locked() is that
spi_async() must hold the mutex while enqueuing a new transfer.
spi_async_locked() doesn't because the mutex is already held.  Note
however that spi_async 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_sync() and
spi_async() can probably be renamed to __spi_sync() and __spI_async()
so that spi_sync(), spi_async(), spi_sync_locked() and
spi_async_locked() can just become wrappers around the common code.

That's it.  Easy to implement, low risk, simple and above all passes
the correctness test.

> For me, there seems to be no disadvantage in having this or a similar
> solution as an intermediate step to the perfect solution (any volunteers?).

The intermediate step is worse that the current situation.  It reduces
the pain for some users (which reduces motivation) while complicating
the code and inventing a new (and guaranteed to be subtly broken)
locking scheme.

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/5] spi: spi_lock_bus and spi_unlock_bus

[Ernst Schwab]

Grant Likely wrote:

> It sounds like you're very worried about making changes to the core
> code, 

Maybe...

> In fact, now that I've had an evening to think about it, the solution
> is even simpler than what I said earlier.  It requires 3 things to be
> added to struct spi_master.
> - 1 Mutex
> - 1 spin lock
> - 1 flag.

> The mutex protects spi_async, and provides sleeping "for free"
> The spinlock protects the atomic spi_sync call.

Sound viable and good, but, in Documentation/spi/spi-summary we have:

"
The basic I/O primitive is spi_async().  Async requests may be
issued in any context (irq handler, task, etc) and completion
is reported using a callback provided with the message.
After any detected error, the chip is deselected and processing
of that spi_message is aborted.
"

Is this compatible with a Mutex protecting spi_async?
It seems to me, that spi_async must queue the SPI message
immediately and return. 
The queue is implemented in the SPI master driver. 
I suppose that's why the blackfin bus locking pioneers 
chose to make the change in the SPI master driver.

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

Re: [PATCH 1/5] spi: spi_lock_bus and spi_unlock_bus

[Grant Likely]

On Wed, Feb 17, 2010 at 12:35 AM, Ernst Schwab <mail-3FdAPXqljGQisWB6T/0fQA@public.gmane.org> wrote:
> Grant Likely wrote:
>
>> It sounds like you're very worried about making changes to the core
>> code,
>
> Maybe...
>
>> In fact, now that I've had an evening to think about it, the solution
>> is even simpler than what I said earlier.  It requires 3 things to be
>> added to struct spi_master.
>> - 1 Mutex
>> - 1 spin lock
>> - 1 flag.
>
>> The mutex protects spi_async, and provides sleeping "for free"
>> The spinlock protects the atomic spi_sync call.
>
> Sound viable and good, but, in Documentation/spi/spi-summary we have:
>
> "
> The basic I/O primitive is spi_async().  Async requests may be
> issued in any context (irq handler, task, etc) and completion
> is reported using a callback provided with the message.
> After any detected error, the chip is deselected and processing
> of that spi_message is aborted.
> "
>
> Is this compatible with a Mutex protecting spi_async?
> It seems to me, that spi_async must queue the SPI message
> immediately and return. The queue is implemented in the SPI master driver. I
> suppose that's why the blackfin bus locking pioneers chose to make the
> change in the SPI master driver.

No, sorry, I made a mistake and got the functions backwards when I was
writing up my description.  What I meant to say is:

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

Cheers,
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/5] spi: spi_lock_bus and spi_unlock_bus

[Grant Likely]

On Wed, Feb 17, 2010 at 6:30 AM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> On Wed, Feb 17, 2010 at 12:35 AM, Ernst Schwab <mail-3FdAPXqljGQisWB6T/0fQA@public.gmane.org> wrote:
>> Grant Likely wrote:
>>
>>> It sounds like you're very worried about making changes to the core
>>> code,
>>
>> Maybe...
>>
>>> In fact, now that I've had an evening to think about it, the solution
>>> is even simpler than what I said earlier.  It requires 3 things to be
>>> added to struct spi_master.
>>> - 1 Mutex
>>> - 1 spin lock
>>> - 1 flag.
>>
>>> The mutex protects spi_async, and provides sleeping "for free"
>>> The spinlock protects the atomic spi_sync call.
>>
>> Sound viable and good, but, in Documentation/spi/spi-summary we have:
>>
>> "
>> The basic I/O primitive is spi_async().  Async requests may be
>> issued in any context (irq handler, task, etc) and completion
>> is reported using a callback provided with the message.
>> After any detected error, the chip is deselected and processing
>> of that spi_message is aborted.
>> "
>>
>> Is this compatible with a Mutex protecting spi_async?
>> It seems to me, that spi_async must queue the SPI message
>> immediately and return. The queue is implemented in the SPI master driver. I
>> suppose that's why the blackfin bus locking pioneers chose to make the
>> change in the SPI master driver.
>
> No, sorry, I made a mistake and got the functions backwards when I was
> writing up my description.  What I meant to say is:
>
> 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

On that note, apparently I find sync/async easy to mix up.  I wonder
if other people find the same and if it would be a good idea to
migrate to clearer names.  Perhaps spi_submit() and
spi_submit_atomic().

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