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