Re: Seeking clarity on IRQCHIP_MASK_ON_SUSPEND

[NeilBrown]

On Mon, 10 Sep 2012 12:28:35 +0200 (CEST) Thomas Gleixner
<tglx@xxxxxxxxxxxxx> wrote:

> On Mon, 10 Sep 2012, NeilBrown wrote:
> > 
> > The IRQCHIP_MASK_ON_SUSPEND flag seems to be hard to use correctly, so either
> > I'm understanding it wrongly, or it could be made easier to use.
> > If the first case, I'm hoping that some improvement to documentation might
> > result.  If the second, then maybe we can fix the code.
> ... 
> > Is anyone able to give a definitive answer on this?  Should
> > IRQCHIP_MASK_ON_SUSPEND be removed?
> 
> The whole point of IRQCHIP_MASK_ON_SUSPEND is to deal with hardware
> designed by geniuses.
> 
> Most SoCs have a way to mark the interrupts which serve as a wake up
> source as such. All other interrupts are magically "masked" on entry
> to suspend.
> 
> Now there is hardware which is missing such a control, so we need to
> mask the non wakeup interrupts right before going into suspend.

Even allowing for the obvious sarcasm, I'm having a lot of trouble
understanding the nature of this 'bad' hardware.

My understand of the SoC world is that the goal is for 'suspend' to be no
different from deep runtime PM (from a hardware perspective).  So where there
is a way to mark an interrupt as a wakeup source, this is used both to support
wakeup from suspend, and to support wakeup from deep runtime-PM

When it is used to wake-from-runtime-PM, the lazy interrupt masking is
sufficient to mask it when needed - the extra wakeup isn't a big cost.
When it is used to wake-from-suspend - lazy interrupt masking can cause an
unwanted wake from suspend which is much more expensive.

So even with sane hardware that supports marking interrupts as wakeup
sources, we still need do extra explicit masking for the suspend case, and
IRQCHIP_MASK_ON_SUSPEND looks like the tool to use ... except that as
described previously it doesn't work.

> 
> That's what IRQCHIP_MASK_ON_SUSPEND does. Not more, not less. See
> commit d209a699a0b for more ugly details.

One detail that I get from that commit is that it was 'Reviewed-by' someone
from 'codeaurora', and that name appears in the pm8xxx-irq.c driver which is
one of the two users.  However that driver can only be used if
CONFIG_MSM_SSBI, and nothing ever devices that so it all seems completely
theoretical.

> 
> You might be looking for a different functionality. Can you explain
> what you need?

I want as particular GPIO interrupt to be masked before entering suspend.
I produced code to get the ->suspend() callback to perform this masking.
Another developer (Santosh) felt that IRQCHIP_MASK_ON_SUSPEND was the
preferred way to do this and on the surface this looks like it should be
correct.  However it doesn't work as explained previously.
I want a resolution to this difference of opinion that doesn't just sweep the
issue under that carpet but provides a clear answer to this sort of issue.

My current opinion is that IRQCHIP_MASK_ON_SUSPEND should be discarded.  The
patch which introduced it says:

    Rather than working around this in the affected interrupt chip
    implementations we can solve this elegant in the core code itself.
    
It appears that the solution in core code, while elegant, is wrong.  It
happens too late to be generally usable.  It is easy enough to handle this
issue in the interrupt chip drivers so maybe that is the best place to handle
it.

The the very least I think we need a big comment saying the
IRQCHIP_MASK_ON_SUSPEND can only be used for irqchips which can always be
programmed, even when they are suspended from an runtime-PM perspective,
and that those chips must handle masking in their 'suspend' callback.

Thank,
NeilBrown

Attachment: signature.asc
Description: PGP signature