Message-ID: <20150211123656.GG9154@leverpostej>
Date:	Wed, 11 Feb 2015 12:36:56 +0000
From:	Mark Rutland <mark.rutland@....com>
To:	Boris Brezillon <boris.brezillon@...e-electrons.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Jason Cooper <jason@...edaemon.net>,
	Nicolas Ferre <nicolas.ferre@...el.com>,
	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <Pawel.Moll@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq
 demuxer chip

On Wed, Feb 11, 2015 at 12:24:37PM +0000, Boris Brezillon wrote:
> Hi Mark,
> 
> On Wed, 11 Feb 2015 11:11:06 +0000
> Mark Rutland <mark.rutland@....com> wrote:
> 
> > On Wed, Feb 11, 2015 at 08:53:39AM +0000, Boris Brezillon wrote:
> > > Hi Mark,
> > > 
> > > On Tue, 10 Feb 2015 20:48:36 +0000
> > > Mark Rutland <mark.rutland@....com> wrote:
> > > 
> > > > On Tue, Feb 10, 2015 at 03:52:01PM +0000, Boris Brezillon wrote:
> > > > > Hi Mark,
> > > > > 
> > > > > On Tue, 10 Feb 2015 15:36:28 +0000
> > > > > Mark Rutland <mark.rutland@....com> wrote:
> > > > > 
> > > > > > Hi Boris,
> > > > > > 
> > > > > > On Thu, Jan 29, 2015 at 10:33:38AM +0000, Boris Brezillon wrote:
> > > > > > > Add documentation for the virtual irq demuxer.
> > > > > > > 
> > > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@...e-electrons.com>
> > > > > > > Acked-by: Nicolas Ferre <nicolas.ferre@...el.com>
> > > > > > > ---
> > > > > > >  .../bindings/interrupt-controller/dumb-demux.txt   | 41 ++++++++++++++++++++++
> > > > > > >  1 file changed, 41 insertions(+)
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > > > 
> > > > > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > > > new file mode 100644
> > > > > > > index 0000000..b9a7830
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > > > @@ -0,0 +1,41 @@
> > > > > > > +* Virtual Interrupt Demultiplexer
> > > > > > > +
> > > > > > > +This virtual demultiplexer simply forward all incoming interrupts to its
> > > > > > > +enabled/unmasked children.
> > > > > > > +It is only intended to be used by hardware that do not provide a proper way
> > > > > > > +to demultiplex a source interrupt, and thus have to wake all their children
> > > > > > > +up so that they can possibly handle the interrupt (if needed).
> > > > > > > +This can be seen as an alternative to shared interrupts when at least one
> > > > > > > +of the interrupt children is a timer (and require the irq to stay enabled
> > > > > > > +on suspend) while others are not. This will prevent calling irq handlers of
> > > > > > > +non timer devices while they are suspended.
> > > > > > 
> > > > > > This sounds like a DT-workaround for a Linux implementation problem, and
> > > > > > I don't think this the right way to solve your problem.
> > > > > 
> > > > > I understand your concern, but why are you answering while I asked for
> > > > > DT maintainers reviews for several days (if not several weeks).
> > > > > 
> > > > > > 
> > > > > > Why does this have to be in DT at all? Why can we not fix the core to
> > > > > > handle these details?
> > > > > 
> > > > > We already discussed that with Rob and Thomas, and hiding such a
> > > > > demuxer chip is not an easy task.
> > > > > I'm open to any suggestion to do that, though I'd like you (I mean DT
> > > > > guys) to provide a working implementation (or at least a viable concept)
> > > > > that would silently demultiplex an irq.
> > > > > 
> > > > > > 
> > > > > > I am very much not keen on this binding.
> > > > > 
> > > > > Yes, but do you have anything else to propose.
> > > > > We're experiencing this warning for 2 releases now, and this is time to
> > > > > find a solution (even if it's not a perfect one).
> > > > 
> > > > Thoughts on the patch below?
> > > 
> > > That's pretty much what I proposed in my first attempt to solve this
> > > problem [1] (except for a few things commented below).
> > > Anyway, Thomas suggested to go for the "dumb/virt irq demultiplexer"
> > > approach instead.
> > 
> > There is one fundamental difference in that this patch does not require
> > drivers to be updated (the new flag is only used internally). Which
> > avoids having to patch every single driver that could possibly be used
> > in combination with one wanting interrupts during suspend.
> 
> Actually, that was one of the requirements expressed by Thomas (Thomas,
> correct me if I'm wrong).
> The point was to force shared irq users to explicitly specify that they
> are mixing !IRQF_NO_SUSPEND and IRQF_NO_SUSPEND because they have no
> other choice.
> 
> With your patch, there's no way to inform users that they are
> erroneously setting the IRQF_NO_SUSPEND flag on one of their shared
> interrupt.

Sure, but even with the demux that's still the case (because it pretends
that this mismatch is a HW property rather than a property of the set of
drivers sharing the interrupt).

Whether there's a demux node in the DTB is entirely separate from
whether the drivers can actually handle the situation.

So if we need a warning in the presence of mismatch and action masking,
we need the exact same warning with the demux.

> > Any used which did not explicitly request with IRQF_NO_SUSPEND will not
> > receive interrupts during suspend.
> > 
> > [...]
> > 
> > > > +static irqreturn_t __handle_irq_event_percpu(unsigned int irq, struct irqaction *action)
> > > > +{
> > > > +	/*
> > > > +	 * During suspend we must not call potentially unsafe irq handlers.
> > > > +	 * See suspend_suspendable_actions.
> > > > +	 */
> > > > +	if (unlikely(action->flags & IRQF_NO_ACTION))
> > > > +		return IRQ_NONE;
> > > 
> > > Thomas was trying to avoid any new conditional code in the interrupt
> > > handling path, that's why I added a suspended_action list in my
> > > proposal.
> > > Even if your 'unlikely' statement make things better I'm pretty sure it
> > > adds some latency.
> > 
> > I can see that we don't want to add more code here to keep things
> > clean/pure, but I find it hard to believe that a single bit test and
> > branch (for data that should be hot in the cache) are going to add
> > measurable latency to a path that does pointer chasing to get to the
> > irqaction in the first place. I could be wrong though, and I'm happy to
> > benchmark.
> 
> Again, I don't have enough experience to say this is (or isn't)
> impacting irq handling latency, I'm just reporting what Thomas told me.

Sure.

I can certainly see the point about watning to keep this clean,
regardless.

> > It would be possible to go for your list shuffling approach here while
> > still keeping the flag internal and all the logic hidden away in
> > kernel/irq/pm.c. I wasn't sure how actions could be manipulated during
> > suspend, which made me wary of moving them to a separate list.
> 
> Moving them to a temporary list on suspend and restoring them on
> resume should not be a problem.
> The only drawback I see is that actions might be reordered after the
> first resume (anyway, relying on shared irq action order is dangerous
> IMHO).

Ok. I am happy to rework to that effect.

> > > > +	return action->handler(irq, action->dev_id);
> > > > +}
> > > > +
> > > >  irqreturn_t
> > > >  handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
> > > >  {
> > > > @@ -140,7 +151,7 @@ handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
> > > >  		irqreturn_t res;
> > > >  
> > > >  		trace_irq_handler_entry(irq, action);
> > > > -		res = action->handler(irq, action->dev_id);
> > > > +		res = __handle_irq_event_percpu(irq, action);
> > > >  		trace_irq_handler_exit(irq, action, res);
> > > >  
> > > >  		if (WARN_ONCE(!irqs_disabled(),"irq %u handler %pF enabled interrupts\n",
> > > > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > > > index 3ca5325..9d8a71f 100644
> > > > --- a/kernel/irq/pm.c
> > > > +++ b/kernel/irq/pm.c
> > > > @@ -43,9 +43,6 @@ void irq_pm_install_action(struct irq_desc *desc, struct irqaction *action)
> > > >  
> > > >  	if (action->flags & IRQF_NO_SUSPEND)
> > > >  		desc->no_suspend_depth++;
> > > > -
> > > > -	WARN_ON_ONCE(desc->no_suspend_depth &&
> > > > -		     desc->no_suspend_depth != desc->nr_actions);
> > > 
> > > Hm, actually this WARN_ON was here to detect offending drivers
> > > (those mixing handler with and without IRQF_NO_SUSPEND on a shared irq).
> > > IMO, removing it is not such a good idea.
> > 
> > I'm not sure I follow. This patch remove the reason we care, no?
> 
> Yep, but as explained above, the whole point of this warning is to make
> people think twice before passing IRQF_NO_SUSPEND when registering a
> shared irq, if you silently handle this case, then nobody can notice
> they might be doing something wrong.

People are still going to hack around that with a demux, regardless of
whether the drivers can actually handle the situation. I'm happy to keep
a pr_warn regardless, though with the masking the situation doesn't
necessarily require a backtrace.

> > When going for suspend the patch uses the mismatch to detect whether it
> > needs to mask out any actions for the desc.
> > 
> > If unexpected interrupts can be generated from devices we're masking the
> > actions for things could go wrong, but that's also the case with the
> > demux. So if we need the warning here we should also have one in the
> > demux to that effect...
> 
> Except they are explicitly defining a virtual irq demuxer.
> This should imply taking the appropriate action to prevent any
> interrupt coming from the devices when entering suspend, but maybe
> that's not clear enough in the virtual demux description.

The presence of a demux implies the DTB author believes they have solved
the problem with the demux, not necessarily that they have considered
the situation and updated drivers appropriately. Relying on the demux to
imply that everything is fine only gives us the illusion that everything
is fine.

Thanks,
Mark.
--
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