From: Boris Brezillon <boris.brezillon@free-electrons.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>,
Thomas Gleixner <tglx@linutronix.de>,
Jason Cooper <jason@lakedaemon.net>,
Nicolas Ferre <nicolas.ferre@atmel.com>,
Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>,
Alexandre Belloni <alexandre.belloni@free-electrons.com>,
Rob Herring <robh+dt@kernel.org>, Pawel Moll <Pawel.Moll@arm.com>,
Ian Campbell <ijc+devicetree@hellion.org.uk>,
Kumar Gala <galak@codeaurora.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
Date: Thu, 12 Feb 2015 12:09:17 +0100 [thread overview]
Message-ID: <20150212120917.44e58bf6@bbrezillon> (raw)
In-Reply-To: <20150212105215.GA1522@leverpostej>
On Thu, 12 Feb 2015 10:52:15 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
> [...]
>
> > > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> > > index d9b05b5..2b8ff50 100644
> > > --- a/include/linux/interrupt.h
> > > +++ b/include/linux/interrupt.h
> > > @@ -57,6 +57,9 @@
> > > * IRQF_NO_THREAD - Interrupt cannot be threaded
> > > * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device
> > > * resume time.
> > > + * IRQF_SHARED_TIMER_OK - Interrupt is safe to be shared with a timer. The
> > > + * handler may be called spuriously during suspend
> > > + * without issue.
> > > */
> > > #define IRQF_DISABLED 0x00000020
> > > #define IRQF_SHARED 0x00000080
> > > @@ -70,8 +73,10 @@
> > > #define IRQF_FORCE_RESUME 0x00008000
> > > #define IRQF_NO_THREAD 0x00010000
> > > #define IRQF_EARLY_RESUME 0x00020000
> > > +#define __IRQF_TIMER_SIBLING_OK 0x00040000
> > >
> > > #define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
> > > +#define IRQF_SHARED_TIMER_OK (IRQF_SHARED | __IRQF_TIMER_SIBLING_OK)
> > >
> > > /*
> > > * These values can be returned by request_any_context_irq() and
> > > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > > index 3ca5325..e4ec91a 100644
> > > --- a/kernel/irq/pm.c
> > > +++ b/kernel/irq/pm.c
> > > @@ -28,6 +28,47 @@ bool irq_pm_check_wakeup(struct irq_desc *desc)
> > > }
> > >
> > > /*
> > > + * Check whether an interrupt is safe to occur during suspend.
> > > + *
> > > + * Physical IRQ lines may be shared between devices which may be expected to
> > > + * raise interrupts during suspend (e.g. timers) and those which may not (e.g.
> > > + * anything we cut the power to). Not all handlers will be safe to call during
> > > + * suspend, so we need to scream if there's the possibility an unsafe handler
> > > + * will be called.
> > > + *
> > > + * A small number of handlers are safe to be shared with timer interrupts, and
> > > + * we don't want to warn erroneously for these. Such handlers will not poke
> > > + * hardware that's not powered or call into kernel infrastructure not available
> > > + * during suspend. These are marked with __IRQF_TIMER_SIBLING_OK.
> > > + */
> > > +bool irq_safe_during_suspend(struct irq_desc * desc, struct irqaction *action)
> > > +{
> > > + const unsigned int safe_flags =
> > > + __IRQF_TIMER_SIBLING_OK | IRQF_NO_SUSPEND;
> > > +
> > > + /*
> > > + * If no-one wants to be called during suspend, or if everyone does,
> > > + * then there's no potential conflict.
> > > + */
> > > + if (!desc->no_suspend_depth)
> > > + return true;
> > > + if (desc->no_suspend_depth == desc->nr_actions)
> > > + return true;
Just another nit, can't we also return early when
desc->nr_actions == 1 (I mean, the handler cannot conflict with anything
since it is the only one registered) ?
> > > +
> > > + /*
> > > + * If any action hasn't asked to be called during suspend or is not
> > > + * happy to be called during suspend, we have a potential problem.
> > > + */
> > > + if (!(action->flags & safe_flags))
> > > + return false;
> > else if (!(action->flags & IRQF_NO_SUSPEND) ||
> > desc->no_suspend_depth > 1)
> > return true;
> >
> > Am I missing something or is the following loop only required if
> > we're adding an action with the IRQF_NO_SUSPEND flag set for the
> > first time ?
>
> With the check above we could return true incorrectly after the first
> time we return true. Consider adding the following in order to an empty
> desc:
>
> flags = IRQF_SHARED // safe, returns true
> flags = IRQF_NO_SUSPEND // unsafe, returns false
> flags = IRQF_NO_SUSPEND // unsafe, but returns true
Yep, you're right.
>
> Currently it shouldn't matter as the only caller is a WARN_ON_ONCE(),
> but it seems unfortunate to allow this.
Absolutely, forget about that, I guess we don't have to optimize that
test anyway.
>
> We'd also run the loop until we had at least two IRQF_NO_SUSPEND
> irqactions:
>
> flags = IRQF_SHARED_TIMER_OK // early return
> flags = IRQF_NO_SUSPEND // run loop
> flags = IRQF_SHARED_TIMER_OK // run loop
Hm, no, this one would return directly (it's an '||' operator not an
'&&' one), because we're not adding an IRQF_NO_SUSPEND handler here, and
adding IRQF_SHARED_TIMER_OK is always safe, isn't it ?
> flags = IRQF_SHARED_TIMER_OK // run loop
> flags = IRQF_SHARED_TIMER_OK // run loop
> flags = IRQF_NO_SUSPEND // don't run loop.
> flags = IRQF_SHARED_TIMER_OK // don't run loop
>
> I assume that we only have one IRQF_NO_SUSPEND action sharing the line
> anyway in your case?
Yep.
>
> Given that we'll only bother to run the test if there's a mismatch
> between desc->no_suspend_depth and desc->nr_actions, I don't think we
> win much. These cases should be rare in practice, the tests only
> performed when we request the irq, and there shouldn't be that many
> actions to loop over.
Sure, never mind, as I said, I'm not sure extra optimization is needed
here.
Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
next prev parent reply other threads:[~2015-02-12 11:09 UTC|newest]
Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-01-29 10:33 [PATCH v4 0/5] ARM: at91: fix irq_pm_install_action WARNING Boris Brezillon
2015-01-29 10:33 ` [PATCH v4 1/5] genirq: Authorize chained handlers to remain disabled when initialized Boris Brezillon
2015-01-29 10:33 ` [PATCH v4 2/5] irqchip: add virtual demultiplexer implementation Boris Brezillon
2015-02-10 15:00 ` Peter Zijlstra
2015-02-10 15:20 ` Boris Brezillon
2015-02-10 15:43 ` [PATCH] genirq: fix virtual irq demuxer related comments Boris Brezillon
2015-02-10 16:14 ` Peter Zijlstra
2015-02-20 16:12 ` Mark Rutland
2015-02-20 16:17 ` Peter Zijlstra
2015-02-10 15:48 ` [PATCH v4 2/5] irqchip: add virtual demultiplexer implementation Mark Rutland
2015-01-29 10:33 ` [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip Boris Brezillon
2015-02-10 15:36 ` Mark Rutland
2015-02-10 15:52 ` Boris Brezillon
2015-02-10 16:06 ` Boris Brezillon
2015-02-10 16:16 ` Mark Rutland
2015-02-10 16:20 ` Boris Brezillon
2015-02-10 20:48 ` Mark Rutland
2015-02-11 8:53 ` Boris Brezillon
2015-02-11 11:11 ` Mark Rutland
2015-02-11 12:24 ` Boris Brezillon
2015-02-11 12:36 ` Mark Rutland
2015-02-11 13:38 ` Alexandre Belloni
2015-02-11 13:48 ` Mark Rutland
2015-02-11 14:55 ` Rafael J. Wysocki
2015-02-11 14:43 ` Mark Rutland
2015-02-11 15:17 ` Rafael J. Wysocki
2015-02-11 15:03 ` Boris Brezillon
2015-02-11 15:39 ` Rafael J. Wysocki
2015-02-11 15:23 ` Mark Rutland
2015-02-11 15:12 ` Mark Rutland
2015-02-11 15:51 ` Rafael J. Wysocki
2015-02-11 15:57 ` Mark Rutland
2015-02-11 16:15 ` Boris Brezillon
2015-02-11 16:32 ` Mark Rutland
2015-02-11 16:38 ` Boris Brezillon
2015-02-11 17:17 ` Mark Rutland
2015-02-20 14:22 ` Mark Rutland
2015-02-20 14:53 ` Boris Brezillon
2015-02-20 15:16 ` Mark Rutland
2015-02-23 17:00 ` Boris Brezillon
2015-02-23 18:14 ` Mark Rutland
2015-02-23 20:16 ` Boris Brezillon
2015-02-11 16:42 ` Rafael J. Wysocki
2015-02-11 16:28 ` Boris Brezillon
2015-02-11 17:13 ` Mark Rutland
2015-02-11 17:29 ` Boris Brezillon
2015-02-12 10:52 ` Mark Rutland
2015-02-12 11:09 ` Boris Brezillon [this message]
2015-02-12 11:23 ` Mark Rutland
2015-02-16 9:49 ` Peter Zijlstra
2015-02-16 9:28 ` Peter Zijlstra
2015-02-16 12:23 ` Mark Rutland
2015-02-19 1:16 ` Rafael J. Wysocki
2015-02-19 11:23 ` Mark Rutland
2015-02-19 22:35 ` Rafael J. Wysocki
2015-02-20 10:31 ` Mark Rutland
2015-02-24 1:02 ` Rafael J. Wysocki
2015-02-24 8:42 ` Boris Brezillon
2015-02-11 14:45 ` Boris Brezillon
2015-02-11 14:39 ` Rafael J. Wysocki
2015-02-11 9:11 ` Peter Zijlstra
2015-02-11 11:15 ` Mark Rutland
2015-02-11 14:31 ` Rafael J. Wysocki
2015-02-11 14:14 ` Mark Rutland
2015-02-11 15:07 ` Rafael J. Wysocki
2015-02-11 15:03 ` Mark Rutland
2015-02-11 14:34 ` Rafael J. Wysocki
2015-01-29 10:33 ` [PATCH v4 4/5] ARM: at91/dt: select VIRT_IRQ_DEMUX for all at91 SoCs Boris Brezillon
2015-01-29 10:33 ` [PATCH v4 5/5] ARM: at91/dt: define a virtual irq demultiplexer chip connected on irq1 Boris Brezillon
2015-02-09 15:47 ` [PATCH v4 0/5] ARM: at91: fix irq_pm_install_action WARNING Boris Brezillon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150212120917.44e58bf6@bbrezillon \
--to=boris.brezillon@free-electrons.com \
--cc=Pawel.Moll@arm.com \
--cc=alexandre.belloni@free-electrons.com \
--cc=devicetree@vger.kernel.org \
--cc=galak@codeaurora.org \
--cc=ijc+devicetree@hellion.org.uk \
--cc=jason@lakedaemon.net \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=nicolas.ferre@atmel.com \
--cc=plagnioj@jcrosoft.com \
--cc=rafael.j.wysocki@intel.com \
--cc=rjw@rjwysocki.net \
--cc=robh+dt@kernel.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).