On Thu, Feb 25, 2010 at 10:25 AM, Ernst Schwab <esch...@online.de> wrote: > On Thu, 25 Feb 2010 09:38:48 -0700 Grant Likely <grant.lik...@secretlab.ca> > wrote: > >> I looked at them all this morning. Without exception, every single >> driver follows this pattern: >> - [maybe check some stuff] >> - spin_lock_irqsave() >> - list_add_tail() >> - <trigger workqueue or other way to kick off transfer> >> - spin_unlock_irqrestore() >> >> Yet every driver implements its own transfer list and spin lock in the >> private data structure. That says to me that the spi layer is >> abstracting at the wrong level. I'm cautious about taking away the >> ability for drivers to make their own decisions about how to manage >> transfers, but it seems like the transfer queue and spin lock should >> be provided by the common code. The trick is how to do it while >> providing a migration path for drivers. >> >> One option is to deprecate the ->transfer hook and allow drivers to >> provide a .request() hook to signal the driver that new transfers have >> been added to the list. Something like how the block layer works. >> That would keep the critical region in spi_async() small. Another >> option is to let drivers provide a .prepare() hook that validates and >> prepares a message before entering the critical region. The >> .prepare() option is probably the simplest to implement and has the >> lowest impact on existing device drivers to use the new functionality, >> but the .request() option might be better in the long run. > > I'd vote for the .request() hook.
It may even be advantageous to provide both .prepare and .request hooks. .prepare would allow messages to be checked out of line before touching any critical regions at all. The two hooks are pretty much mutually independent. >> The spinlock is absolutely required. Otherwise the test of >> bus_lock_flag is racy. > [...] >> See? Without the spinlock there is nothing preventing B3 from getting >> called in the midst of A's bus lock. > > Ok, now, I see; an interrupt being executed on a different core than > the process context will manage it to malfunction if the spinlock is > missing. It is even broken on a single processor. If an IRQ occurs between B2 and B3, and the IRQ goes down an spi bus lock path, then things still get out of order. >> > A possible solution would be to add an extra queue, handled in the >> > spi.c module, stored in the master's data structure, that stores all >> > messages coming in while the bus lock is active. The queue can be >> > flushed by calling __spi_async() for each message as soon as the bus >> > lock is removed. >> >> That also works. In fact, this would work well with the .request() >> hook suggestion I made above because when the lock is removed, the >> delayed list could just be appended to the normal request list. A >> .prepare() hook would also be useful because the message can still be >> pre-checked for validity outside of the critical region before being >> added to the delay list. > > Maybe all of the list/queue handling should be done in spi.c, and (inline) > functions in spi.h; then, the driver could call a 'spi_get_next_message()' > function and will get a message out of one of the two queues: Try not to get too exotic here. It should be sufficient for the driver to only look at the 'direct' queue and let the core code decide when to move stuff from delayed to direct. In fact, the existing list_del_init() callsites will probably work as-is with just changing the pointer to the list head. > spi_sync_locked() and spi_async_locked() > will add to 'direct' queue, and call .request() > spi_sync() and spi_async() > will add to 'delayed' queue if spi bus is locked > will add to 'direct' queue if unlocked, and call .request() > > spi_bus_unlock() > will append entire 'delayed' to 'direct' queue, and call .request() Sounds appropriate. However, I think you'll want to avoid having to change every bus driver all at once. I think this can be done without breaking drivers. If you leave the .transfer() hook in place and add the .request() hook, then drivers can provide one or the other. If a .transfer() hook is provided, then everything works fine. spi_async*() calls the transfer hook for each message received. If the bus is locked, then spi_async() adds them to the delayed queue without calling .transfer(). If transfer is NULL, then the setup code should set .transfer to a 'stock' transfer function that only adds the message to the direct queue. Drivers that migrate to this mode then take advantage of the nice short critical region provided by the stock .transfer() hook. In either case, spi_bus_unlock() will walk the delayed queue and call .transfer() on each message. If .request is set, then it should be called after each .transfer() call and also at the end of spi_bus_unlock(). Once all the drivers are migrated to the .request() API, then the stock transfer hook can be folded directly into __spi_async() , the hook removed from struct spi_master, and the spi_bus_unlock() path can be simplified to append the delayed queue onto the direct queue. >> If you want to attempt this, maintain it as a separate patch on top of >> the core bus locking patch. > > I will look at it if I find the time... Thanks. g. ------------------------------------------------------------------------------ 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
