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

[Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>]

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