Date:	Sun, 21 Sep 2014 02:48:30 +0200
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Dmitry Torokhov <dtor@...gle.com>,
	Eric Caruso <ejcaruso@...gle.com>,
	linux-kernel@...r.kernel.org, Benson Leung <bleung@...gle.com>
Subject: Re: irq mask swapping during suspend/resume

On Thursday, September 18, 2014 01:32:06 PM Thomas Gleixner wrote:
> On Wed, 17 Sep 2014, Dmitry Torokhov wrote:
> > Hi Thomas,
> > 
> > On Wednesday, September 17, 2014 12:05:42 PM Thomas Gleixner wrote:
> > > On Tue, 16 Sep 2014, Eric Caruso wrote:
> > > > We would like to be able to set different irq masks for triggers during
> > > > normal operation and for waking up the system. For example, while a laptop
> > > > is awake, closing the lid and opening the lid should both fire an
> > > > interrupt, but when the system is asleep, we would like to stay asleep
> > > > when
> > > > closing the lid.
> > > > 
> > > > We are thinking about stashing the irq mask used specifically for waking
> > > > the system up in the irq_desc struct, and then swapping it during
> > > > enable_irq_wake and disable_irq_wake calls. Devices that do not specify a
> > > > different wake mask will use their normal trigger mask for both
> > > > situations.
> > > > 
> > > > Is this acceptable?
> > > 
> > > Not really. Why should irq_desc provide storage for random
> > > configurations and bind them to some random system state?
> > > 
> > > What's wrong with calling
> > > 
> > >        irq_set_type(irq, B);
> > >        enable_irq_wake(irq);
> > > 
> > >        disable_irq_wake(irq);
> > >        irq_set_type(irq, A);
> > 
> > The desire is to avoid doing it in [every] driver but rather have it done
> > centrally by device/PM core. It does not have to be irq_desc though,
> > maybe you can suggest a better place for it (aside of the individual driver
> > code that is)?
> 
> Well, if it should be done by the device/pm core then you want to
> store that information in the device related data structure. 
> 
> struct dev_pm_info might be the right place for it, but that's up to
> Rafael.
> 
> So driver would set
> 
>    dev->power.update_wakeirq_type = true;
>    dev->power.irq_type_normal = IRQ_TYPE_EDGE_BOTH;
>    dev->power.irq_type_sleep = IRQ_TYPE_EDGE_LOW;
> 
> And the dev/PM core can issue the calls on suspend/resume.

So I'd rather put that into the struct wakeup_source pointed to by
the wakeup pointer in struct dev_pm_info.  That would give us a mapping
between wakeup source objects and wakeup interrupts and which would make
a fair amount of sense in my view.

Then, we could simply walk the list of wakeup source objects before
suspend_device_irqs() and call enable_irq_wake() etc. for all of the
interrupts in question without drivers having to worry about that.
We also could save the current IRQ type for them at that point and
restore it during resume.

Of course, that would require some changes to wakeup_source_create()
and friends, but is probably worth doing.

Still, before we start making those changes, here's a bunch of questions
to answer:

(1) Say a wakeup interrupt is shared between two drivers and one of them
    asks for a different "IRQ type for sleep" than the other one.  How are
    we going to resolve such conflicts?

(2) Can platforms place restrictions on the IRQ type to be used with a given
    line?  If so, how do we handle situations in which the requested
    "IRQ type for sleep" is different from what the given line can use?
    Do we need to resolve that at the struct wakeup_source creation time or
    can we do that later (during suspend?) and how?

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists