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

[Mike Frysinger <vapier.adi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Wed, Feb 17, 2010 at 15:06, Grant Likely wrote:
> On Wed, Feb 17, 2010 at 12:03 PM, Mike Frysinger wrote:
>> On Wed, Feb 17, 2010 at 13:17, Ernst Schwab wrote:
>>> The current runtime API looks like this:
>>> spi_async(struct spi_device*, struct spi_message*);
>>> spi_sync(struct spi_device*, struct spi_message*);
>>>
>>> The API needs to be extended to this:
>>> spi_async(struct spi_device*, struct spi_message*)
>>> spi_sync(struct spi_device*, struct spi_message*)
>>> spi_bus_lock(struct spi_master*)  /* although struct spi_device* might
>>> be easier */
>>> spi_bus_unlock(struct spi_master*)
>>> spi_async_locked(struct spi_device*, struct spi_message*)
>>> spi_sync_locked(struct spi_device*, struct spi_message*)
>>>
>>> Drivers can only call the last two if they already hold the spi_master_lock().
>>>
>>> spi_bus_lock() obtains the mutex, obtains the spin lock, sets the
>>> flag, and releases the spin lock before returning.  It doesn't even
>>> need to sleep while waiting for "in-flight" spi_transactions to
>>> complete because its purpose is to guarantee no additional
>>> transactions are added.  It does not guarantee that the bus is idle.
>>>
>>> spi_bus_unlock() clears the flag and releases the mutex, which will
>>> wake up any waiters.
>>>
>>> The difference between spi_async() and spi_async_locked() is that the
>>> locked version bypasses the check of the lock flag.  Both versions
>>> need to obtain the spinlock.
>>>
>>> The difference between spi_sync() and spi_sync_locked() is that
>>> spi_sync() must hold the mutex while enqueuing a new transfer.
>>> spi_sync_locked() doesn't because the mutex is already held.  Note
>>> however that spi_sync must *not* continue to hold the mutex while
>>> waiting for the transfer to complete, otherwise only one transfer
>>> could be queued up at a time!
>>>
>>> Almost no code needs to be written.  The current spi_async() and
>>> spi_sync() can probably be renamed to __spi_async() and __spi_sync()
>>> so that spi_async(), spi_sync(), spi_async_locked() and
>>> spi_sync_locked() can just become wrappers around the common code.
>>>
>>> spi_sync() is protected by a mutex because it can sleep
>>> spi_async() needs to be protected with a flag and a spinlock because
>>> it can be called atomically and must not sleep
>>
>> i dont think these new "_locked" versions are a good idea.  why cant
>> it be handled transparently to the caller in the core ?  the spi core
>> already requires the CS field of the spi device to be unique per bus.
>> re-use that to check ownership of the mutex.
>
> No.  the bus locking operation is completely orthogonal to the spi
> device being used for the transfer.  A driver could even obtain the
> lock, and then use multiple spi_devices to execute transfers before
> releasing it.
>
> The API addition is definitely required.  Callers locking the bus
> *absolutely* must understand what they are doing.
>
> If anything, I'd consent to a debug option that does a WARN_ON if the
> wrong function is called when the bus isn't locked.  ie. _locked
> version called when bus isn't locked.

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

------------------------------------------------------------------------------
SOLARIS 10 is the OS for Data Centers - provides features such as DTrace,
Predictive Self Healing and Award Winning ZFS. Get Solaris 10 NOW
http://p.sf.net/sfu/solaris-dev2dev
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general