Message-ID: <1968084.K4YyaeZfBq@vostro.rjw.lan>
Date:	Fri, 27 Feb 2015 23:13:57 +0100
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Boris Brezillon <boris.brezillon@...e-electrons.com>,
	Mark Rutland <mark.rutland@....com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Jason Cooper <jason@...edaemon.net>,
	linux-kernel@...r.kernel.org,
	Nicolas Ferre <nicolas.ferre@...el.com>,
	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
	linux-arm-kernel@...ts.infradead.org,
	Linux PM list <linux-pm@...r.kernel.org>
Subject: Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines

On Friday, February 27, 2015 09:38:59 AM Peter Zijlstra wrote:
> On Fri, Feb 27, 2015 at 12:07:55AM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > 
> > It currently is required that all users of NO_SUSPEND interrupt
> > lines pass the IRQF_NO_SUSPEND flag when requesting the IRQ or the
> > WARN_ON_ONCE() in irq_pm_install_action() will trigger.  That is
> > done to warn about situations in which unprepared interrupt handlers
> > may be run unnecessarily for suspended devices and may attempt to
> > access those devices by mistake.  However, it may cause drivers
> > that have no technical reasons for using IRQF_NO_SUSPEND to set
> > that flag just because they happen to share the interrupt line
> > with something like a timer.
> > 
> > Moreover, the generic handling of wakeup interrupts introduced by
> > commit 9ce7a25849e8 (genirq: Simplify wakeup mechanism) only works
> > for IRQs without any NO_SUSPEND users, so the drivers of wakeup
> > devices needing to use shared NO_SUSPEND interrupt lines for
> > signaling system wakeup generally have to detect wakeup in their
> > interrupt handlers.  Thus if they happen to share an interrupt line
> > with a NO_SUSPEND user, they also need to request that their
> > interrupt handlers be run after suspend_device_irqs().
> > 
> > In both cases the reason for using IRQF_NO_SUSPEND is not because
> > the driver in question has a genuine need to run its interrupt
> > handler after suspend_device_irqs(), but because it happens to
> > share the line with some other NO_SUSPEND user.  Otherwise, the
> > driver would do without IRQF_NO_SUSPEND just fine.
> > 
> > To make it possible to specify that condition explicitly, introduce
> > a new IRQ action handler flag for shared IRQs, IRQF_COND_SUSPEND,
> > that, when set, will indicate to the IRQ core that the interrupt
> > user is generally fine with suspending the IRQ, but it also can
> > tolerate handler invocations after suspend_device_irqs() and, in
> > particular, it is capable of detecting system wakeup and triggering
> > it as appropriate from its interrupt handler.
> > 
> > That will allow us to work around a problem with a shared timer
> > interrupt line on at91 platforms.
> > 
> > Link: http://marc.info/?l=linux-kernel&m=142252777602084&w=2
> > Link: http://marc.info/?t=142252775300011&r=1&w=2
> > Linx: https://lkml.org/lkml/2014/12/15/552
> > Reported-by: Boris Brezillon <boris.brezillon@...e-electrons.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> 
> Seems good to me. Should I take this through tip/irq ?

I can apply it along with the previous IRQF_NO_SUSPEND documentation patch
from Mark Rutland if you ACK it for me. :-)

> Also, should we warn if people use enable_irq_wake() where there is only
> a single descriptor with NO_SUSPEND?

We probably should do that, but that would be a separate patch IMO?

--
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