Re: [PATCH] powerpc/mpic: supply a .disable callback

[Scott Wood <scottwood@freescale.com>]

On Tue, 2014-01-07 at 04:18 -0600, Wang Dongsheng-B40534 wrote:
> 
> > -----Original Message-----
> > From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]
> > Sent: Tuesday, January 07, 2014 1:50 PM
> > To: Wang Dongsheng-B40534
> > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org
> > Subject: Re: [PATCH] powerpc/mpic: supply a .disable callback
> > 
> > On Tue, 2014-01-07 at 13:38 +0800, Dongsheng Wang wrote:
> > > From: Wang Dongsheng <dongsheng.wang@freescale.com>
> > >
> > > Currently MPIC provides .mask, but not .disable.  This means that
> > > effectively disable_irq() soft-disables the interrupt, and you get
> > > a .mask call if an interrupt actually occurs.
> > >
> > > I'm not sure if this was intended as a performance benefit (it seems common
> > > to omit .disable on powerpc interrupt controllers, but nowhere else), but it
> > > interacts badly with threaded/workqueue interrupts (including KVM
> > > reflection).  In such cases, where the real interrupt handler does a
> > > disable_irq_nosync(), schedules defered handling, and returns, we get two
> > > interrupts for every real interrupt.  The second interrupt does nothing
> > > but see that IRQ_DISABLED is set, and decide that it would be a good
> > > idea to actually call .mask.
> > 
> > We probably don't want to do that for edge, only level interrupts.
> > 
> Sorry Ben, I am not understand your comments.
> 
> This issue is the kernel api irq_disable() only use chip->irq_disable(), but mpic
> not have this interface so we don't real disable the interrupt.

I think he means that the "two interrupts for every real interrupt"
effect will only happen with level triggered interrupts, and he'd like
to keep the potential performance benefit of lazy disabling for edge
interrupts.

To implement this for "ordinary" edge interrupts (not IPI, timer, etc)
we'd need to add a new .irq_disable() function that checks whether it's
level/edge and only calls .irq_mask() if level -- or, introduce a
separate struct irq_chip for edge versus level.

-Scott