On Wed, 24 Feb 2010 13:32:43 -0700 Grant Likely <grant.lik...@secretlab.ca>
wrote:
> On Thu, Feb 18, 2010 at 5:10 AM, Ernst Schwab <esch...@online.de> wrote:
[...]
> > I have concerns on the use of the spin lock around the
> > __spi_async() call, as Mike Frysinger already remarked.
[...]
> I think the spinlock should always be held when calling the bus
> drivers ->transfer() hook. That will also allow us to remove
> driver-specific locking from the bus drivers. Otherwise we've got an
> inconsistent locking scheme which is asking for trouble down the road.
Hm, I do not agree to that. I think it is a bad idea to have a spinlock
around a function call that may do a lot of things (we don't know about
it in the spi.c module) and may block multiple CPU cores or disable
interrupts for an indefinite time. The list_for_each_entry(xfer,...)
loop may process many elements, during that time, all other cores are
locked off without need.
The implementation present in the kernel (without patches) apparently
made the assumption, that master->transfer() itself can be called
from process and interrupt context concurrently and does its own locking.
This is true for the (few) drivers I looked it up.
The list_for_each_entry(xfer, &message->transfers, transfer_list) loop
seems to be safe (although I'm not absolutely sure about it) without
locking, since it only seems to access data stored in the message passed
as a function argument.
With our additional spinlocking around the whole stuff we add significant
additional locking time without a need.
I suggest to remove the spinlocking in spi_async (see patch below).
> > But: This all is true for the original design, there seems to be no
> > protection against accidental reentrant use of spi_async().
>
> spi_async() is intended to be callable from atomic context so that IRQ
> handers can trigger a new transfer without waiting around for it to
> complete. It is also totally valid for both process and IRQ contexts
> to call spi_async() at the same time, which is exactly why the
> spinlock should be required.
Seems the spinlock is already in master->transfer() (I missed that, because
I checked the wrong function...), so it is valid that multiple contexts
(process, IRQ) call master->transfer() concurrently.
> Hmmm. Are you concerned about a code path where an spi_async() call
> will call the bus driver, which will end up calling spi_async() on
> itself? I cannot think of a use case where that would be desirable.
No, I didn't think of this - this isn't desirable, I agree.
Regards
Ernst
=8<=====================================================
From: Ernst Schwab <esch...@online.de>
Followup-Patch: Remove spin lock around __spi_async call.
Signed-off-by: Ernst Schwab <esch...@online.de>
---
diff -uprN a/drivers/spi/spi.c b/drivers/spi/spi.c
--- a/drivers/spi/spi.c 2010-02-23 16:51:35.000000000 +0100
+++ b/drivers/spi/spi.c 2010-02-25 11:21:45.000000000 +0100
@@ -731,15 +731,11 @@ int spi_async(struct spi_device *spi, st
int ret;
unsigned long flags;
- spin_lock_irqsave(&master->bus_lock_spinlock, flags);
-
if (master->bus_lock_flag)
ret = -EBUSY;
else
ret = __spi_async(spi, message);
- spin_unlock_irqrestore(&master->bus_lock_spinlock, flags);
-
return ret;
}
EXPORT_SYMBOL_GPL(spi_async);
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
