Hey Ernst. Good patch, thank you. The merge window is going to open any moment now, and this patch hasn't had any linux-next exposure, so I'm not going to pick it up for 2.6.34. Instead, I'll wait until after the merge window closes and add it to my next branch so that it gets lots of exposure before the 2.6.35 merge window. Since this is a pretty core change, I'd like to have as much feedback and testing on it as possible before I commit to pushing it into mainline.
Anyone who is following along and is interested, please test these two patches and let me know if they work for you. Ping me after Linus tags v2.6.34-rc1 to make sure I don't forget to put it in my -next branch. Some comments below... On Thu, Feb 18, 2010 at 5:10 AM, Ernst Schwab <[email protected]> wrote: > I integrated your changes in V2, I hope I did not miss anything. > > On Wed, 17 Feb 2010 13:03:04 -0700 > Grant Likely <[email protected]> wrote: >> > extern int spi_setup(struct spi_device *spi); >> > extern int spi_async(struct spi_device *spi, struct spi_message *message); >> > +extern int >> > +spi_async_locked(struct spi_device *spi, struct spi_message *message); >> >> Please keep return parameters and annotations on the same line as the >> function name (makes grep results more useful). You can break lines >> between parameters. > > Hm, the prototype for spi_alloc_master in the same file let me assume, > that this should be the style used in this header file. Normally, I > wouldn't do a linebreak there, ever... Yeah, things are inconsistent, but in this case both styles are present in the file, and when grepping I find it more important to have the annotations than the parameters immediately visable. > > Something more important: > > I have concerns on the use of the spin lock around the > __spi_async() call, as Mike Frysinger already remarked. > > Why do we 'need' this spin lock: > - by definition, spi_async can be called from any context > " > * Context: any (irqs may be blocked, etc) > * > * This call may be used in_irq and other contexts which can't sleep, > * as well as from task contexts which can sleep. > " > - we are not sure that spi_async isn't used reentrant (e.g. interrupt > service routine and normal context > - __spi_async calls the master->transfer() function which is not required > to be reentrant (or is it?) Um, I'm not sure I follow the argument. spin locks are safe to obtain in any context. Mutexes are the ones that cannot be held at atomic context. The spin lock does more than just protect the bus_is_locked flag. It also prevent overlap between process and IRQ context running in the relevant code. Basically the spin_lock_irqsave() either defers all IRQ processing in uniprocessor, or has other processors spin in SMP. 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. > > 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. 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. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ------------------------------------------------------------------------------ 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 [email protected] https://lists.sourceforge.net/lists/listinfo/spi-devel-general
