Messages in this thread | | | Date | Sat, 15 Aug 2009 16:29:25 +0200 | From | Ingo Molnar <> | Subject | Re: [RFC patch 2/3] genirq: Add buslock support for irq chips on slow busses |
| |
* Thomas Gleixner <tglx@linutronix.de> wrote:
> On Sat, 15 Aug 2009, Ingo Molnar wrote: > > * Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > > > > > On Fri, Aug 14, 2009 at 12:20:49PM +0200, Thomas Gleixner wrote: > > > > On Fri, 14 Aug 2009, Pavel Machek wrote: > > > > > > > > AFAICT this means that driver would need to know what kind of IRQ it > > > > > is hooked to, right? That will lead to some ugly code in drivers that > > > > > can handle both normal and slowbus irqs, right? > > > > > > > Are there such drivers in reality ? > > > > > > Yes. The GPIO based stuff is the prime example but there's other > > > examples - one is the WM831x touchscreen (no driver in mainline > > > yet) which can use interrupts via the main interrupt controller on > > > the CPU but also has the option of bringing the interrupt signals > > > out to dedicated pins on the chip for direct connection to the CPU > > > precisely to avoid the overheads of these slow interrupt > > > controllers. > > > > This would call for Thomas's first version of the patch, that is > > transparent to drivers - the IRQ subsystem will know how to lock > > access to the line. > > > > How about implementing that first patch in a cleaner way - can we > > somehow express the slow-bus property purely via the irqchip? Or is > > that too lowlevel? > > The problem here is that the management functions serialize via > irqdesc->lock and call the chip level function with the lock held > (preemption and interrupts disabled). So we can not access the > slow bus chips from these low level functions. > > If the low level functions just store the information and schedule > it for bus access then there is no serialization anymore. Lets > look at disable_irq(): > > spin_lock_irqsave(&desc->lock); > .... > desc->chip->mask(); > schedule bus access; > ... > spin_lock_irqsave(&desc->lock); > > So now we return, but the mask has not reached the chip. > > The idea of bus_lock/bus_sync_unlock() was to provide well defined > synchronization points for the chip level implementation to do the bus > update and have this serialized against other management functions. > > desc->chip->bus_lock(); > Take the chip->bus mutex > > spin_lock_irqsave(&desc->lock); > .... > desc->chip->mask(); > store mask information > ... > spin_lock_irqsave(&desc->lock); > > desc->chip->bus_sync_unlock(); > Update the mask via the slow bus > Release chip->bus mutex > > That way we have made sure that the change to the chip actually > hits the hardware before we allow further management operations > and it simplifies the code for the chip implementation as the bus > access can be done in the context of the caller w/o the need of an > extra thread/workqueue ...
Given the alternatives i'd prefer this one now ...
Ingo
| |