| Message ID | 1422527620-8308-4-git-send-email-boris.brezillon@free-electrons.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
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@free-electrons.com> > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.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. Why does this have to be in DT at all? Why can we not fix the core to handle these details? I am very much not keen on this binding. > + > +Required properties: > +- compatible: Should be "virtual,irq-demux". > +- interrupt-controller: Identifies the node as an interrupt controller. > +- interrupts-extended or interrupt-parent and interrupts: Reference the source > + interrupt connected to this dumb demuxer. > +- #interrupt-cells: The number of cells to define the interrupts (should be 1). > + The only cell is the IRQ number. > +- irqs: u32 bitfield specifying the interrupts provided by the demuxer. Arbitrary limitation? Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On Tue, 10 Feb 2015 15:36:28 +0000 Mark Rutland <mark.rutland@arm.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@free-electrons.com> > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.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). > > > + > > +Required properties: > > +- compatible: Should be "virtual,irq-demux". > > +- interrupt-controller: Identifies the node as an interrupt controller. > > +- interrupts-extended or interrupt-parent and interrupts: Reference the source > > + interrupt connected to this dumb demuxer. > > +- #interrupt-cells: The number of cells to define the interrupts (should be 1). > > + The only cell is the IRQ number. > > +- irqs: u32 bitfield specifying the interrupts provided by the demuxer. > > Arbitrary limitation? I first proposed to make this field unlimited, but Thomas suggested to keep it limited to 32 bits (and I didn't complain since my HW needs far less than 32 interrupts). Best Regards, Boris
I'm fixing my own answer :-) On Tue, 10 Feb 2015 16:52:01 +0100 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Hi Mark, > > On Tue, 10 Feb 2015 15:36:28 +0000 > Mark Rutland <mark.rutland@arm.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@free-electrons.com> > > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.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 but why are you answering now, while .... > 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). > > > > > > + > > > +Required properties: > > > +- compatible: Should be "virtual,irq-demux". > > > +- interrupt-controller: Identifies the node as an interrupt controller. > > > +- interrupts-extended or interrupt-parent and interrupts: Reference the source > > > + interrupt connected to this dumb demuxer. > > > +- #interrupt-cells: The number of cells to define the interrupts (should be 1). > > > + The only cell is the IRQ number. > > > +- irqs: u32 bitfield specifying the interrupts provided by the demuxer. > > > > Arbitrary limitation? > > I first proposed to make this field unlimited, but Thomas suggested to > keep it limited to 32 bits (and I didn't complain since my HW needs > far less than 32 interrupts). Here is the first implementation I proposed [1], where the 'irqs' property was an array of u32, each entry containing an irq id. [1]https://lkml.org/lkml/2015/1/8/233
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@arm.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@free-electrons.com> > > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.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). I am sorry that I did not spot those, and I am very sorry that this means I am only now able to air my concerns. > > 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. Is it truly necessary to drop a emux in the middle? As far as I can see, all that we're attempting to do here is hide the IRQF_NO_SUSPEND mismatch from the core IRQ code, though I've only just started digging and haven't yet figured out where/why the core code cares. Any hints? > > 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). I appreciate this, and I am really sorry that I have come to this so late. > > > +Required properties: > > > +- compatible: Should be "virtual,irq-demux". > > > +- interrupt-controller: Identifies the node as an interrupt controller. > > > +- interrupts-extended or interrupt-parent and interrupts: Reference the source > > > + interrupt connected to this dumb demuxer. > > > +- #interrupt-cells: The number of cells to define the interrupts (should be 1). > > > + The only cell is the IRQ number. > > > +- irqs: u32 bitfield specifying the interrupts provided by the demuxer. > > > > Arbitrary limitation? > > I first proposed to make this field unlimited, but Thomas suggested to > keep it limited to 32 bits (and I didn't complain since my HW needs > far less than 32 interrupts). Ok. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 10 Feb 2015 16:16:00 +0000 Mark Rutland <mark.rutland@arm.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@arm.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@free-electrons.com> > > > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.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). > > I am sorry that I did not spot those, and I am very sorry that this > means I am only now able to air my concerns. > > > > 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. > > Is it truly necessary to drop a emux in the middle? > > As far as I can see, all that we're attempting to do here is hide the > IRQF_NO_SUSPEND mismatch from the core IRQ code, though I've only just > started digging and haven't yet figured out where/why the core code > cares. Any hints? You should have a look at this thread [1] ;-) [1]https://lkml.org/lkml/2014/12/15/552
Hi Mark, On Tue, 10 Feb 2015 20:48:36 +0000 Mark Rutland <mark.rutland@arm.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@arm.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@free-electrons.com> > > > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.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. > > Rather than handling this at the desc level it adds an extra flag to the > irqaction which can be set/unset during suspend for those irqs we don't > want to handle. That way we don't need to tell the core about the > mismatch explicitly in DT (or ACPI/board files/whatever). > > If we can request/free interrupts during suspend then there's some logic > missing, but it shows the basic idea. > > I didn't have a system to hand with shared mismatched IRQF_NO_SUSPEND > interrupts, so I had to fake that up in code for testing. > > Thanks, > Mark. > > ---->8---- > From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 2001 > From: Mark Rutland <mark.rutland@arm.com> > Date: Tue, 10 Feb 2015 20:14:33 +0000 > Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests > > In some cases a physical IRQ line may be shared between devices from > which we expect interrupts during suspend (e.g. timers) and those we do > not (e.g. anything we cut the power to). Where a driver did not request > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > being called during suspend, and it may bring down the system. > > This patch adds logic to automatically mark the irqactions for these > potentially unsafe handlers as disabled during suspend, leaving actions > with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared line > during suspend, only the handlers requested with IRQF_NO_SUSPEND will be > called. The handlers requested without IRQF_NO_SUSPEND will be skipped > as if they had immediately returned IRQF_NONE. > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > Cc: Jason Cooper <jason@lakedaemon.net> > Cc: Nicolas Ferre <nicolas.ferre@atmel.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > --- > include/linux/interrupt.h | 4 ++++ > kernel/irq/handle.c | 13 +++++++++++- > kernel/irq/pm.c | 50 +++++++++++++++++++++++++++++++++++++++++++---- > 3 files changed, 62 insertions(+), 5 deletions(-) > > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h > index d9b05b5..49dcb35 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_NO_ACTION - This irqaction should not be triggered. > + * Used during suspend for !IRQF_NO_SUSPEND irqactions which > + * share lines with IRQF_NO_SUSPEND irqactions. > */ > #define IRQF_DISABLED 0x00000020 > #define IRQF_SHARED 0x00000080 > @@ -70,6 +73,7 @@ > #define IRQF_FORCE_RESUME 0x00008000 > #define IRQF_NO_THREAD 0x00010000 > #define IRQF_EARLY_RESUME 0x00020000 > +#define IRQF_NO_ACTION 0x00040000 > > #define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD) > > diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c > index 6354802..44c8662 100644 > --- a/kernel/irq/handle.c > +++ b/kernel/irq/handle.c > @@ -130,6 +130,17 @@ void __irq_wake_thread(struct irq_desc *desc, struct irqaction *action) > wake_up_process(action->thread); > } > > +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. > + 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. Best Regards, Boris [1]https://lkml.org/lkml/2014/12/15/551
On Tue, Feb 10, 2015 at 08:48:36PM +0000, Mark Rutland wrote: > From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 2001 > From: Mark Rutland <mark.rutland@arm.com> > Date: Tue, 10 Feb 2015 20:14:33 +0000 > Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests > > In some cases a physical IRQ line may be shared between devices from > which we expect interrupts during suspend (e.g. timers) and those we do > not (e.g. anything we cut the power to). Where a driver did not request > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > being called during suspend, and it may bring down the system. > > This patch adds logic to automatically mark the irqactions for these > potentially unsafe handlers as disabled during suspend, leaving actions > with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared line > during suspend, only the handlers requested with IRQF_NO_SUSPEND will be > called. The handlers requested without IRQF_NO_SUSPEND will be skipped > as if they had immediately returned IRQF_NONE. > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > Cc: Jason Cooper <jason@lakedaemon.net> > Cc: Nicolas Ferre <nicolas.ferre@atmel.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Mark Rutland <mark.rutland@arm.com> Aw gawd.. not that again. So Rafael and tglx went over this a few months ago I think: lkml.kernel.org/r/26580319.OZP7jvJnA9@vostro.rjw.lan is the last series I could find. Maybe Rafael can summarize? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@arm.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@arm.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@free-electrons.com> > > > > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.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. 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. 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. > > + 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? 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... Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 11, 2015 at 09:11:59AM +0000, Peter Zijlstra wrote: > On Tue, Feb 10, 2015 at 08:48:36PM +0000, Mark Rutland wrote: > > From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 2001 > > From: Mark Rutland <mark.rutland@arm.com> > > Date: Tue, 10 Feb 2015 20:14:33 +0000 > > Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests > > > > In some cases a physical IRQ line may be shared between devices from > > which we expect interrupts during suspend (e.g. timers) and those we do > > not (e.g. anything we cut the power to). Where a driver did not request > > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > > being called during suspend, and it may bring down the system. > > > > This patch adds logic to automatically mark the irqactions for these > > potentially unsafe handlers as disabled during suspend, leaving actions > > with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared line > > during suspend, only the handlers requested with IRQF_NO_SUSPEND will be > > called. The handlers requested without IRQF_NO_SUSPEND will be skipped > > as if they had immediately returned IRQF_NONE. > > > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > > Cc: Jason Cooper <jason@lakedaemon.net> > > Cc: Nicolas Ferre <nicolas.ferre@atmel.com> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Aw gawd.. not that again. I agree this isn't pretty, but at least it doesn't require the HW description to know about Linux internals, and it can work for !DT systems. I'm really not happy with placing Linux implementation details into DTBs. > So Rafael and tglx went over this a few months ago I think: > > lkml.kernel.org/r/26580319.OZP7jvJnA9@vostro.rjw.lan > > is the last series I could find. Maybe Rafael can summarize? I can't get at any commentary from that link, unfortunately. Rafael? Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On Wed, 11 Feb 2015 11:11:06 +0000 Mark Rutland <mark.rutland@arm.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@arm.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@arm.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@free-electrons.com> > > > > > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.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. > > 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. > > 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). > > > > + 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. > > 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. Regards, Boris
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@arm.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@arm.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@arm.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@free-electrons.com> > > > > > > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/02/2015 at 12:36:56 +0000, Mark Rutland wrote : > > 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. > Actually, we only care about removing the warning. It is effectively the HW that forces us to do so. So we would be completely happy with a new flag to silence the warning as we know what we are doing (I think that has already been suggested). > 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. > Whatever the solution, it could be used as a workaround the warning as this is exactly what we need for our platform.
On Wed, Feb 11, 2015 at 01:38:59PM +0000, Alexandre Belloni wrote: > On 11/02/2015 at 12:36:56 +0000, Mark Rutland wrote : > > > 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. > > > > Actually, we only care about removing the warning. It is effectively the > HW that forces us to do so. So we would be completely happy with a new > flag to silence the warning as we know what we are doing (I think that > has already been suggested). Sure. Given that the warning is there to tell you that the _drivers_ aren't using the interrupt in a consistent manner, I don't see how introducing a new fake device in the DT solves that. It simply masks some irqactions at a different level of the SW stack from my patch, completely independently of the drivers. So whether or not the warning is necessary is orthogonal to whether or not this patch is reasonable. If it's not necessary, neither patch needs it. If it is necessary, it's also necessary for the demux. > > 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. > > > > Whatever the solution, it could be used as a workaround the warning as > this is exactly what we need for our platform. Let's hope we can get this patch into shape quickly then :) Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 11, 2015 at 02:31:18PM +0000, Rafael J. Wysocki wrote: > On Wednesday, February 11, 2015 11:15:17 AM Mark Rutland wrote: > > On Wed, Feb 11, 2015 at 09:11:59AM +0000, Peter Zijlstra wrote: > > > On Tue, Feb 10, 2015 at 08:48:36PM +0000, Mark Rutland wrote: > > > > From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 2001 > > > > From: Mark Rutland <mark.rutland@arm.com> > > > > Date: Tue, 10 Feb 2015 20:14:33 +0000 > > > > Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests > > > > > > > > In some cases a physical IRQ line may be shared between devices from > > > > which we expect interrupts during suspend (e.g. timers) and those we do > > > > not (e.g. anything we cut the power to). Where a driver did not request > > > > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > > > > being called during suspend, and it may bring down the system. > > > > > > > > This patch adds logic to automatically mark the irqactions for these > > > > potentially unsafe handlers as disabled during suspend, leaving actions > > > > with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared line > > > > during suspend, only the handlers requested with IRQF_NO_SUSPEND will be > > > > called. The handlers requested without IRQF_NO_SUSPEND will be skipped > > > > as if they had immediately returned IRQF_NONE. > > > > > > > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > > > > Cc: Jason Cooper <jason@lakedaemon.net> > > > > Cc: Nicolas Ferre <nicolas.ferre@atmel.com> > > > > Cc: Peter Zijlstra <peterz@infradead.org> > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Cc: Thomas Gleixner <tglx@linutronix.de> > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > > > > > Aw gawd.. not that again. > > > > I agree this isn't pretty, but at least it doesn't require the HW > > description to know about Linux internals, and it can work for !DT > > systems. > > > > I'm really not happy with placing Linux implementation details into > > DTBs. > > > > > So Rafael and tglx went over this a few months ago I think: > > > > > > lkml.kernel.org/r/26580319.OZP7jvJnA9@vostro.rjw.lan > > > > > > is the last series I could find. Maybe Rafael can summarize? > > > > I can't get at any commentary from that link, unfortunately. > > > > Rafael? > > Well, the commentary is not there, because both I and Thomas implicitly agreed > on one thing: We cannot add any suspend-related checks to the interrupt handling > hot path, because that will affect everyone including people who don't use > suspend at all and who *really* care about interrupt handling performance. That's fair enough, and I'm happy to avoid that by other means. My fundamental objection(s) to the current approach is that we create a binding for a non-existent device that people will abuse without considering the consequences. All we will end up with is more DTBs containing the mux regardless of wether the drivers (or hardware) are actually safe with a shared line. So with the changes moves out of the hot-path (e.g. with shuffling to/from a suspended_actions list in the pm code), is there some issue that I have not considered? Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, February 11, 2015 11:15:17 AM Mark Rutland wrote: > On Wed, Feb 11, 2015 at 09:11:59AM +0000, Peter Zijlstra wrote: > > On Tue, Feb 10, 2015 at 08:48:36PM +0000, Mark Rutland wrote: > > > From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 2001 > > > From: Mark Rutland <mark.rutland@arm.com> > > > Date: Tue, 10 Feb 2015 20:14:33 +0000 > > > Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests > > > > > > In some cases a physical IRQ line may be shared between devices from > > > which we expect interrupts during suspend (e.g. timers) and those we do > > > not (e.g. anything we cut the power to). Where a driver did not request > > > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > > > being called during suspend, and it may bring down the system. > > > > > > This patch adds logic to automatically mark the irqactions for these > > > potentially unsafe handlers as disabled during suspend, leaving actions > > > with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared line > > > during suspend, only the handlers requested with IRQF_NO_SUSPEND will be > > > called. The handlers requested without IRQF_NO_SUSPEND will be skipped > > > as if they had immediately returned IRQF_NONE. > > > > > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > > > Cc: Jason Cooper <jason@lakedaemon.net> > > > Cc: Nicolas Ferre <nicolas.ferre@atmel.com> > > > Cc: Peter Zijlstra <peterz@infradead.org> > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > Cc: Thomas Gleixner <tglx@linutronix.de> > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > > > Aw gawd.. not that again. > > I agree this isn't pretty, but at least it doesn't require the HW > description to know about Linux internals, and it can work for !DT > systems. > > I'm really not happy with placing Linux implementation details into > DTBs. > > > So Rafael and tglx went over this a few months ago I think: > > > > lkml.kernel.org/r/26580319.OZP7jvJnA9@vostro.rjw.lan > > > > is the last series I could find. Maybe Rafael can summarize? > > I can't get at any commentary from that link, unfortunately. > > Rafael? Well, the commentary is not there, because both I and Thomas implicitly agreed on one thing: We cannot add any suspend-related checks to the interrupt handling hot path, because that will affect everyone including people who don't use suspend at all and who *really* care about interrupt handling performance. Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, February 10, 2015 08:48:36 PM Mark Rutland 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@arm.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@free-electrons.com> > > > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.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? > > Rather than handling this at the desc level it adds an extra flag to the > irqaction which can be set/unset during suspend for those irqs we don't > want to handle. That way we don't need to tell the core about the > mismatch explicitly in DT (or ACPI/board files/whatever). > > If we can request/free interrupts during suspend then there's some logic > missing, but it shows the basic idea. > > I didn't have a system to hand with shared mismatched IRQF_NO_SUSPEND > interrupts, so I had to fake that up in code for testing. > > Thanks, > Mark. > > ---->8---- > From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 2001 > From: Mark Rutland <mark.rutland@arm.com> > Date: Tue, 10 Feb 2015 20:14:33 +0000 > Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests > > In some cases a physical IRQ line may be shared between devices from > which we expect interrupts during suspend (e.g. timers) and those we do > not (e.g. anything we cut the power to). Where a driver did not request > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > being called during suspend, and it may bring down the system. > > This patch adds logic to automatically mark the irqactions for these > potentially unsafe handlers as disabled during suspend, leaving actions > with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared line > during suspend, only the handlers requested with IRQF_NO_SUSPEND will be > called. The handlers requested without IRQF_NO_SUSPEND will be skipped > as if they had immediately returned IRQF_NONE. > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > Cc: Jason Cooper <jason@lakedaemon.net> > Cc: Nicolas Ferre <nicolas.ferre@atmel.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > --- > include/linux/interrupt.h | 4 ++++ > kernel/irq/handle.c | 13 +++++++++++- > kernel/irq/pm.c | 50 +++++++++++++++++++++++++++++++++++++++++++---- > 3 files changed, 62 insertions(+), 5 deletions(-) > > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h > index d9b05b5..49dcb35 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_NO_ACTION - This irqaction should not be triggered. > + * Used during suspend for !IRQF_NO_SUSPEND irqactions which > + * share lines with IRQF_NO_SUSPEND irqactions. > */ > #define IRQF_DISABLED 0x00000020 > #define IRQF_SHARED 0x00000080 > @@ -70,6 +73,7 @@ > #define IRQF_FORCE_RESUME 0x00008000 > #define IRQF_NO_THREAD 0x00010000 > #define IRQF_EARLY_RESUME 0x00020000 > +#define IRQF_NO_ACTION 0x00040000 > > #define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD) > > diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c > index 6354802..44c8662 100644 > --- a/kernel/irq/handle.c > +++ b/kernel/irq/handle.c > @@ -130,6 +130,17 @@ void __irq_wake_thread(struct irq_desc *desc, struct irqaction *action) > wake_up_process(action->thread); > } > > +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; Nope. We can't add this check here, as that would affect interrupt handling performance for everyone including people who never use system suspend and never will. > + return action->handler(irq, action->dev_id); > +} Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, February 11, 2015 11:11:06 AM Mark Rutland 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@arm.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@arm.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@free-electrons.com> > > > > > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.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. > > 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. You don't have to. There are people who care about every single branch in the interrupt handling hot path. Really, we can't add any suspend-related checks here (and the majority of people who care about those latencies don't give a damn about system suspend). Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] > > > > > +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. > > > > > > > > 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). > > We considered doing that too and saw some drawbacks (in addition to the > reordering of actions you've mentioned). It added just too much complexity > to the IRQ suspend-resume code. > > I, personally, would be fine with adding an IRQ flag to silence the > warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED that would > be set automatically if someone requested IRQF_TIMER | IRQF_SHARED. > > Thoughts? Even if the timer driver does that, we still require the other handlers sharing the line to do the right thing across suspend, no? So either their actions need to be masked at suspend time, or the handlers need to detect when they're called during suspend and return early. So for the flag at request time approach to work, all the drivers using the interrupt would have to flag they're safe in that context. I'm not averse to having that (only a few drivers shuold be affected and we can sanity check them now). Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 11 Feb 2015 15:55:47 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > On Wednesday, February 11, 2015 01:24:37 PM Boris Brezillon wrote: > > Hi Mark, > > > > On Wed, 11 Feb 2015 11:11:06 +0000 > > Mark Rutland <mark.rutland@arm.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@arm.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@arm.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@free-electrons.com> > > > > > > > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.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. > > > > > > > > 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. > > > > > > > > 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). > > We considered doing that too and saw some drawbacks (in addition to the > reordering of actions you've mentioned). It added just too much complexity > to the IRQ suspend-resume code. > > I, personally, would be fine with adding an IRQ flag to silence the > warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED that would > be set automatically if someone requested IRQF_TIMER | IRQF_SHARED. Yep, but that won't prevent irq handler from being called (even when they are suspended), and IIRC, that was one of Thomas' concerns. This shouldn't be a problem for the at91 platform though (actually, this is the current behavior).
On Wednesday, February 11, 2015 01:24:37 PM Boris Brezillon wrote: > Hi Mark, > > On Wed, 11 Feb 2015 11:11:06 +0000 > Mark Rutland <mark.rutland@arm.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@arm.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@arm.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@free-electrons.com> > > > > > > > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.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. > > > > > 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. > > > > > 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). We considered doing that too and saw some drawbacks (in addition to the reordering of actions you've mentioned). It added just too much complexity to the IRQ suspend-resume code. I, personally, would be fine with adding an IRQ flag to silence the warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED that would be set automatically if someone requested IRQF_TIMER | IRQF_SHARED. Thoughts? Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 11 Feb 2015 16:17:20 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > On Wednesday, February 11, 2015 02:43:45 PM Mark Rutland wrote: > > [...] > > > > > > > > > +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. > > > > > > > > > > > > > > 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). > > > > > > We considered doing that too and saw some drawbacks (in addition to the > > > reordering of actions you've mentioned). It added just too much complexity > > > to the IRQ suspend-resume code. > > > > > > I, personally, would be fine with adding an IRQ flag to silence the > > > warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED that would > > > be set automatically if someone requested IRQF_TIMER | IRQF_SHARED. > > > > > > Thoughts? > > > > Even if the timer driver does that, we still require the other handlers > > sharing the line to do the right thing across suspend, no? So either > > their actions need to be masked at suspend time, or the handlers need to > > detect when they're called during suspend and return early. > > Well, the issue at hand is about things that share an IRQ with a timer AFAICS. > > That is odd enough already and I'd say everyone in that situation needs to > be prepared to take the pain (including having to check if the device is not > suspended in their interrupt handlers). > > And quite frankly they need to do that already, because we've never suspended > timer IRQs. > > > So for the flag at request time approach to work, all the drivers using > > the interrupt would have to flag they're safe in that context. > > Something like IRQF_"I can share the line with a timer" I guess? That wouldn't > hurt and can be checked at request time even. > > > I'm not averse to having that (only a few drivers shuold be affected and > > we can sanity check them now). > > Right. Okay, if everyone agrees on this solution, then I'm fine with that too (even if I'm a bit disappointed to have spent so much time on this problem to eventually end-up with a simple IRQF_SHARED_TIMER flag :-().
On Wed, Feb 11, 2015 at 03:07:48PM +0000, Rafael J. Wysocki wrote: > On Wednesday, February 11, 2015 02:14:37 PM Mark Rutland wrote: > > On Wed, Feb 11, 2015 at 02:31:18PM +0000, Rafael J. Wysocki wrote: > > > On Wednesday, February 11, 2015 11:15:17 AM Mark Rutland wrote: > > > > On Wed, Feb 11, 2015 at 09:11:59AM +0000, Peter Zijlstra wrote: > > > > > On Tue, Feb 10, 2015 at 08:48:36PM +0000, Mark Rutland wrote: > > > > > > From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 2001 > > > > > > From: Mark Rutland <mark.rutland@arm.com> > > > > > > Date: Tue, 10 Feb 2015 20:14:33 +0000 > > > > > > Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests > > > > > > > > > > > > In some cases a physical IRQ line may be shared between devices from > > > > > > which we expect interrupts during suspend (e.g. timers) and those we do > > > > > > not (e.g. anything we cut the power to). Where a driver did not request > > > > > > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > > > > > > being called during suspend, and it may bring down the system. > > > > > > > > > > > > This patch adds logic to automatically mark the irqactions for these > > > > > > potentially unsafe handlers as disabled during suspend, leaving actions > > > > > > with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared line > > > > > > during suspend, only the handlers requested with IRQF_NO_SUSPEND will be > > > > > > called. The handlers requested without IRQF_NO_SUSPEND will be skipped > > > > > > as if they had immediately returned IRQF_NONE. > > > > > > > > > > > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > > > > > > Cc: Jason Cooper <jason@lakedaemon.net> > > > > > > Cc: Nicolas Ferre <nicolas.ferre@atmel.com> > > > > > > Cc: Peter Zijlstra <peterz@infradead.org> > > > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Cc: Thomas Gleixner <tglx@linutronix.de> > > > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > > > > > > > > > Aw gawd.. not that again. > > > > > > > > I agree this isn't pretty, but at least it doesn't require the HW > > > > description to know about Linux internals, and it can work for !DT > > > > systems. > > > > > > > > I'm really not happy with placing Linux implementation details into > > > > DTBs. > > > > > > > > > So Rafael and tglx went over this a few months ago I think: > > > > > > > > > > lkml.kernel.org/r/26580319.OZP7jvJnA9@vostro.rjw.lan > > > > > > > > > > is the last series I could find. Maybe Rafael can summarize? > > > > > > > > I can't get at any commentary from that link, unfortunately. > > > > > > > > Rafael? > > > > > > Well, the commentary is not there, because both I and Thomas implicitly agreed > > > on one thing: We cannot add any suspend-related checks to the interrupt handling > > > hot path, because that will affect everyone including people who don't use > > > suspend at all and who *really* care about interrupt handling performance. > > > > That's fair enough, and I'm happy to avoid that by other means. > > > > My fundamental objection(s) to the current approach is that we create a > > binding for a non-existent device that people will abuse without > > considering the consequences. All we will end up with is more DTBs > > containing the mux regardless of wether the drivers (or hardware) are > > actually safe with a shared line. > > That is a valid concern in my view. > > > So with the changes moves out of the hot-path (e.g. with shuffling > > to/from a suspended_actions list in the pm code), is there some issue > > that I have not considered? > > When we were reworking the handling of wakeup interrupts during the 3.18 > cycle, one of my proposals was to move the "suspended" irqactions to an > "inactive" list during suspend_device_irqs() and back during > resume_device_irqs(), but Thomas didn't like that approach. His main > argument was that it made the code in question overly complex which > was fair enough to me. I've just looked into that, and have a trivial implementation that's contained within kernel/irq/pm.c, but it assumes that during suspend nothing needs to modify actions. > What about adding a new flag like I said? That works for me. I'll respond in the other mail. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, February 11, 2015 02:14:37 PM Mark Rutland wrote: > On Wed, Feb 11, 2015 at 02:31:18PM +0000, Rafael J. Wysocki wrote: > > On Wednesday, February 11, 2015 11:15:17 AM Mark Rutland wrote: > > > On Wed, Feb 11, 2015 at 09:11:59AM +0000, Peter Zijlstra wrote: > > > > On Tue, Feb 10, 2015 at 08:48:36PM +0000, Mark Rutland wrote: > > > > > From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 2001 > > > > > From: Mark Rutland <mark.rutland@arm.com> > > > > > Date: Tue, 10 Feb 2015 20:14:33 +0000 > > > > > Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests > > > > > > > > > > In some cases a physical IRQ line may be shared between devices from > > > > > which we expect interrupts during suspend (e.g. timers) and those we do > > > > > not (e.g. anything we cut the power to). Where a driver did not request > > > > > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > > > > > being called during suspend, and it may bring down the system. > > > > > > > > > > This patch adds logic to automatically mark the irqactions for these > > > > > potentially unsafe handlers as disabled during suspend, leaving actions > > > > > with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared line > > > > > during suspend, only the handlers requested with IRQF_NO_SUSPEND will be > > > > > called. The handlers requested without IRQF_NO_SUSPEND will be skipped > > > > > as if they had immediately returned IRQF_NONE. > > > > > > > > > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > > > > > Cc: Jason Cooper <jason@lakedaemon.net> > > > > > Cc: Nicolas Ferre <nicolas.ferre@atmel.com> > > > > > Cc: Peter Zijlstra <peterz@infradead.org> > > > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > Cc: Thomas Gleixner <tglx@linutronix.de> > > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > > > > > > > Aw gawd.. not that again. > > > > > > I agree this isn't pretty, but at least it doesn't require the HW > > > description to know about Linux internals, and it can work for !DT > > > systems. > > > > > > I'm really not happy with placing Linux implementation details into > > > DTBs. > > > > > > > So Rafael and tglx went over this a few months ago I think: > > > > > > > > lkml.kernel.org/r/26580319.OZP7jvJnA9@vostro.rjw.lan > > > > > > > > is the last series I could find. Maybe Rafael can summarize? > > > > > > I can't get at any commentary from that link, unfortunately. > > > > > > Rafael? > > > > Well, the commentary is not there, because both I and Thomas implicitly agreed > > on one thing: We cannot add any suspend-related checks to the interrupt handling > > hot path, because that will affect everyone including people who don't use > > suspend at all and who *really* care about interrupt handling performance. > > That's fair enough, and I'm happy to avoid that by other means. > > My fundamental objection(s) to the current approach is that we create a > binding for a non-existent device that people will abuse without > considering the consequences. All we will end up with is more DTBs > containing the mux regardless of wether the drivers (or hardware) are > actually safe with a shared line. That is a valid concern in my view. > So with the changes moves out of the hot-path (e.g. with shuffling > to/from a suspended_actions list in the pm code), is there some issue > that I have not considered? When we were reworking the handling of wakeup interrupts during the 3.18 cycle, one of my proposals was to move the "suspended" irqactions to an "inactive" list during suspend_device_irqs() and back during resume_device_irqs(), but Thomas didn't like that approach. His main argument was that it made the code in question overly complex which was fair enough to me. What about adding a new flag like I said? Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 11, 2015 at 03:17:20PM +0000, Rafael J. Wysocki wrote: > On Wednesday, February 11, 2015 02:43:45 PM Mark Rutland wrote: > > [...] > > > > > > > > > +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. > > > > > > > > > > > > > > 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). > > > > > > We considered doing that too and saw some drawbacks (in addition to the > > > reordering of actions you've mentioned). It added just too much complexity > > > to the IRQ suspend-resume code. > > > > > > I, personally, would be fine with adding an IRQ flag to silence the > > > warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED that would > > > be set automatically if someone requested IRQF_TIMER | IRQF_SHARED. > > > > > > Thoughts? > > > > Even if the timer driver does that, we still require the other handlers > > sharing the line to do the right thing across suspend, no? So either > > their actions need to be masked at suspend time, or the handlers need to > > detect when they're called during suspend and return early. > > Well, the issue at hand is about things that share an IRQ with a timer AFAICS. > > That is odd enough already and I'd say everyone in that situation needs to > be prepared to take the pain (including having to check if the device is not > suspended in their interrupt handlers). IMO if the line is shared it would be ideal for the core to mask the action (as that's essentially the behaviour when the line isn't shared with an IRQF_NO_SUSPEND action), but that's not esseential if a flag is OK for now. > And quite frankly they need to do that already, because we've never suspended > timer IRQs. This is a very good point. > > So for the flag at request time approach to work, all the drivers using > > the interrupt would have to flag they're safe in that context. > > Something like IRQF_"I can share the line with a timer" I guess? That wouldn't > hurt and can be checked at request time even. I guess that would have to imply IRQF_SHARED, so we'd have something like: IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during suspend in the case the line is shared. The handler will not access unavailable hardware or kernel infrastructure during this period. #define __IRQF_SUSPEND_SPURIOUS 0x00040000 #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | __IRQF_SUSPEND_SPURIOUS) Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, February 11, 2015 02:43:45 PM Mark Rutland wrote: > [...] > > > > > > > +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. > > > > > > > > > > > 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). > > > > We considered doing that too and saw some drawbacks (in addition to the > > reordering of actions you've mentioned). It added just too much complexity > > to the IRQ suspend-resume code. > > > > I, personally, would be fine with adding an IRQ flag to silence the > > warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED that would > > be set automatically if someone requested IRQF_TIMER | IRQF_SHARED. > > > > Thoughts? > > Even if the timer driver does that, we still require the other handlers > sharing the line to do the right thing across suspend, no? So either > their actions need to be masked at suspend time, or the handlers need to > detect when they're called during suspend and return early. Well, the issue at hand is about things that share an IRQ with a timer AFAICS. That is odd enough already and I'd say everyone in that situation needs to be prepared to take the pain (including having to check if the device is not suspended in their interrupt handlers). And quite frankly they need to do that already, because we've never suspended timer IRQs. > So for the flag at request time approach to work, all the drivers using > the interrupt would have to flag they're safe in that context. Something like IRQF_"I can share the line with a timer" I guess? That wouldn't hurt and can be checked at request time even. > I'm not averse to having that (only a few drivers shuold be affected and > we can sanity check them now). Right. Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 11, 2015 at 03:39:48PM +0000, Rafael J. Wysocki wrote: > On Wednesday, February 11, 2015 04:03:17 PM Boris Brezillon wrote: > > On Wed, 11 Feb 2015 16:17:20 +0100 > > "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > > > > > On Wednesday, February 11, 2015 02:43:45 PM Mark Rutland wrote: > > > > [...] > > > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > > > > 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). > > > > > > > > > > We considered doing that too and saw some drawbacks (in addition to the > > > > > reordering of actions you've mentioned). It added just too much complexity > > > > > to the IRQ suspend-resume code. > > > > > > > > > > I, personally, would be fine with adding an IRQ flag to silence the > > > > > warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED that would > > > > > be set automatically if someone requested IRQF_TIMER | IRQF_SHARED. > > > > > > > > > > Thoughts? > > > > > > > > Even if the timer driver does that, we still require the other handlers > > > > sharing the line to do the right thing across suspend, no? So either > > > > their actions need to be masked at suspend time, or the handlers need to > > > > detect when they're called during suspend and return early. > > > > > > Well, the issue at hand is about things that share an IRQ with a timer AFAICS. > > > > > > That is odd enough already and I'd say everyone in that situation needs to > > > be prepared to take the pain (including having to check if the device is not > > > suspended in their interrupt handlers). > > > > > > And quite frankly they need to do that already, because we've never suspended > > > timer IRQs. > > > > > > > So for the flag at request time approach to work, all the drivers using > > > > the interrupt would have to flag they're safe in that context. > > > > > > Something like IRQF_"I can share the line with a timer" I guess? That wouldn't > > > hurt and can be checked at request time even. > > > > > > > I'm not averse to having that (only a few drivers shuold be affected and > > > > we can sanity check them now). > > > > > > Right. > > > > Okay, if everyone agrees on this solution, then I'm fine with that too > > (even if I'm a bit disappointed to have spent so much time on this > > problem to eventually end-up with a simple IRQF_SHARED_TIMER flag :-(). > > IRQD_SHARED_TIMER (that needs to be an IRQ flag, not an irqaction one). Surely for the drivers to be able to announce that they can handle suspend safely this has to be an irqaction flag? For IRQD_SHARED_TIMER to work the irqchip would need to set this, and it doesn't know anything about the drivers (or potentially what any of the interrupts are if the block is reused). > And spending time on things that never go in happens a lot in the core > development. I've sent several versions of the wakeup interrupts handling > rework and then Thomas wrote the final one himself. :-) > > The goal is to find a way that will address everyone's needs and that's > not always starightforward. Indeed! Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, February 11, 2015 04:03:17 PM Boris Brezillon wrote: > On Wed, 11 Feb 2015 16:17:20 +0100 > "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > > > On Wednesday, February 11, 2015 02:43:45 PM Mark Rutland wrote: > > > [...] > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > 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). > > > > > > > > We considered doing that too and saw some drawbacks (in addition to the > > > > reordering of actions you've mentioned). It added just too much complexity > > > > to the IRQ suspend-resume code. > > > > > > > > I, personally, would be fine with adding an IRQ flag to silence the > > > > warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED that would > > > > be set automatically if someone requested IRQF_TIMER | IRQF_SHARED. > > > > > > > > Thoughts? > > > > > > Even if the timer driver does that, we still require the other handlers > > > sharing the line to do the right thing across suspend, no? So either > > > their actions need to be masked at suspend time, or the handlers need to > > > detect when they're called during suspend and return early. > > > > Well, the issue at hand is about things that share an IRQ with a timer AFAICS. > > > > That is odd enough already and I'd say everyone in that situation needs to > > be prepared to take the pain (including having to check if the device is not > > suspended in their interrupt handlers). > > > > And quite frankly they need to do that already, because we've never suspended > > timer IRQs. > > > > > So for the flag at request time approach to work, all the drivers using > > > the interrupt would have to flag they're safe in that context. > > > > Something like IRQF_"I can share the line with a timer" I guess? That wouldn't > > hurt and can be checked at request time even. > > > > > I'm not averse to having that (only a few drivers shuold be affected and > > > we can sanity check them now). > > > > Right. > > Okay, if everyone agrees on this solution, then I'm fine with that too > (even if I'm a bit disappointed to have spent so much time on this > problem to eventually end-up with a simple IRQF_SHARED_TIMER flag :-(). IRQD_SHARED_TIMER (that needs to be an IRQ flag, not an irqaction one). And spending time on things that never go in happens a lot in the core development. I've sent several versions of the wakeup interrupts handling rework and then Thomas wrote the final one himself. :-) The goal is to find a way that will address everyone's needs and that's not always starightforward. Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, February 11, 2015 03:12:38 PM Mark Rutland wrote: > On Wed, Feb 11, 2015 at 03:17:20PM +0000, Rafael J. Wysocki wrote: > > On Wednesday, February 11, 2015 02:43:45 PM Mark Rutland wrote: > > > [...] > > > > > > > > > > > +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. > > > > > > > > > > > > > > > > > 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). > > > > > > > > We considered doing that too and saw some drawbacks (in addition to the > > > > reordering of actions you've mentioned). It added just too much complexity > > > > to the IRQ suspend-resume code. > > > > > > > > I, personally, would be fine with adding an IRQ flag to silence the > > > > warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED that would > > > > be set automatically if someone requested IRQF_TIMER | IRQF_SHARED. > > > > > > > > Thoughts? > > > > > > Even if the timer driver does that, we still require the other handlers > > > sharing the line to do the right thing across suspend, no? So either > > > their actions need to be masked at suspend time, or the handlers need to > > > detect when they're called during suspend and return early. > > > > Well, the issue at hand is about things that share an IRQ with a timer AFAICS. > > > > That is odd enough already and I'd say everyone in that situation needs to > > be prepared to take the pain (including having to check if the device is not > > suspended in their interrupt handlers). > > IMO if the line is shared it would be ideal for the core to mask the > action (as that's essentially the behaviour when the line isn't shared > with an IRQF_NO_SUSPEND action), but that's not esseential if a flag is > OK for now. > > > And quite frankly they need to do that already, because we've never suspended > > timer IRQs. > > This is a very good point. > > > > So for the flag at request time approach to work, all the drivers using > > > the interrupt would have to flag they're safe in that context. > > > > Something like IRQF_"I can share the line with a timer" I guess? That wouldn't > > hurt and can be checked at request time even. > > I guess that would have to imply IRQF_SHARED, so we'd have something > like: > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during > suspend in the case the line is shared. The > handler will not access unavailable hardware > or kernel infrastructure during this period. > > #define __IRQF_SUSPEND_SPURIOUS 0x00040000 > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | __IRQF_SUSPEND_SPURIOUS) What about #define __IRQF_TIMER_SIBLING_OK 0x00040000 #define IRQF_SHARED_TIMER_OK (IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) The "suspend" part is kind of a distraction to me here, because that really only is about sharing an IRQ with a timer and the "your interrupt handler may be called when the device is suspended" part is just a consequence of that. So IMO it's better to have "TIMER" in the names to avoid encouraging people to abuse this for other purposes not related to timers. Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] > > > > So for the flag at request time approach to work, all the drivers using > > > > the interrupt would have to flag they're safe in that context. > > > > > > Something like IRQF_"I can share the line with a timer" I guess? That wouldn't > > > hurt and can be checked at request time even. > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > like: > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during > > suspend in the case the line is shared. The > > handler will not access unavailable hardware > > or kernel infrastructure during this period. > > > > #define __IRQF_SUSPEND_SPURIOUS 0x00040000 > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | __IRQF_SUSPEND_SPURIOUS) > > What about > > #define __IRQF_TIMER_SIBLING_OK 0x00040000 > #define IRQF_SHARED_TIMER_OK (IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > The "suspend" part is kind of a distraction to me here, because that really > only is about sharing an IRQ with a timer and the "your interrupt handler > may be called when the device is suspended" part is just a consequence of that. My rationale was that you didn't really care who else was using the IRQ (e.g. the timer); you're just stating that you can survive being called during suspend (which is what the driver may need to check for in the handler if the device happens to be powered down or whatever). So I guess I see it the other way around. This is essentially claiming we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > So IMO it's better to have "TIMER" in the names to avoid encouraging people to > abuse this for other purposes not related to timers. In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is better I shan't complain. The fundamental issue I'm concerned with is addressed by this approach. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 11 Feb 2015 15:57:20 +0000 Mark Rutland <mark.rutland@arm.com> wrote: > [...] > > > > > > So for the flag at request time approach to work, all the drivers using > > > > > the interrupt would have to flag they're safe in that context. > > > > > > > > Something like IRQF_"I can share the line with a timer" I guess? That wouldn't > > > > hurt and can be checked at request time even. > > > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > > like: > > > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during > > > suspend in the case the line is shared. The > > > handler will not access unavailable hardware > > > or kernel infrastructure during this period. > > > > > > #define __IRQF_SUSPEND_SPURIOUS 0x00040000 > > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | __IRQF_SUSPEND_SPURIOUS) > > > > What about > > > > #define __IRQF_TIMER_SIBLING_OK 0x00040000 > > #define IRQF_SHARED_TIMER_OK (IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > > > The "suspend" part is kind of a distraction to me here, because that really > > only is about sharing an IRQ with a timer and the "your interrupt handler > > may be called when the device is suspended" part is just a consequence of that. > > My rationale was that you didn't really care who else was using the IRQ > (e.g. the timer); you're just stating that you can survive being called > during suspend (which is what the driver may need to check for in the > handler if the device happens to be powered down or whatever). > > So I guess I see it the other way around. This is essentially claiming > we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > > > So IMO it's better to have "TIMER" in the names to avoid encouraging people to > > abuse this for other purposes not related to timers. > > In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is > better I shan't complain. > > The fundamental issue I'm concerned with is addressed by this approach. Okay then, is anyone taking care of submitting such a patch (Mark ?) ?
On Wed, 11 Feb 2015 17:42:22 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > On Wednesday, February 11, 2015 05:15:15 PM Boris Brezillon wrote: > > On Wed, 11 Feb 2015 15:57:20 +0000 > > Mark Rutland <mark.rutland@arm.com> wrote: > > > > > [...] > > > > > > > > > > So for the flag at request time approach to work, all the drivers using > > > > > > > the interrupt would have to flag they're safe in that context. > > > > > > > > > > > > Something like IRQF_"I can share the line with a timer" I guess? That wouldn't > > > > > > hurt and can be checked at request time even. > > > > > > > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > > > > like: > > > > > > > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during > > > > > suspend in the case the line is shared. The > > > > > handler will not access unavailable hardware > > > > > or kernel infrastructure during this period. > > > > > > > > > > #define __IRQF_SUSPEND_SPURIOUS 0x00040000 > > > > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | __IRQF_SUSPEND_SPURIOUS) > > > > > > > > What about > > > > > > > > #define __IRQF_TIMER_SIBLING_OK 0x00040000 > > > > #define IRQF_SHARED_TIMER_OK (IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > > > > > > > The "suspend" part is kind of a distraction to me here, because that really > > > > only is about sharing an IRQ with a timer and the "your interrupt handler > > > > may be called when the device is suspended" part is just a consequence of that. > > > > > > My rationale was that you didn't really care who else was using the IRQ > > > (e.g. the timer); you're just stating that you can survive being called > > > during suspend (which is what the driver may need to check for in the > > > handler if the device happens to be powered down or whatever). > > > > > > So I guess I see it the other way around. This is essentially claiming > > > we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > > > > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging people to > > > > abuse this for other purposes not related to timers. > > > > > > In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is > > > better I shan't complain. > > > > > > The fundamental issue I'm concerned with is addressed by this approach. > > > > Okay then, is anyone taking care of submitting such a patch (Mark ?) ? > > Well, I guess I should take the responsibility for that. :-) > > I'll try to cut one later today or tomorrow unless someone else beats me to that. I won't (I'm done with these irq stuff for now ;-)). Peter, if this patch is accepted, I guess you'll have to drop (or revert my patches). Thanks, Boris
On Wed, Feb 11, 2015 at 04:15:15PM +0000, Boris Brezillon wrote: > On Wed, 11 Feb 2015 15:57:20 +0000 > Mark Rutland <mark.rutland@arm.com> wrote: > > > [...] > > > > > > > > So for the flag at request time approach to work, all the drivers using > > > > > > the interrupt would have to flag they're safe in that context. > > > > > > > > > > Something like IRQF_"I can share the line with a timer" I guess? That wouldn't > > > > > hurt and can be checked at request time even. > > > > > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > > > like: > > > > > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during > > > > suspend in the case the line is shared. The > > > > handler will not access unavailable hardware > > > > or kernel infrastructure during this period. > > > > > > > > #define __IRQF_SUSPEND_SPURIOUS 0x00040000 > > > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | __IRQF_SUSPEND_SPURIOUS) > > > > > > What about > > > > > > #define __IRQF_TIMER_SIBLING_OK 0x00040000 > > > #define IRQF_SHARED_TIMER_OK (IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > > > > > The "suspend" part is kind of a distraction to me here, because that really > > > only is about sharing an IRQ with a timer and the "your interrupt handler > > > may be called when the device is suspended" part is just a consequence of that. > > > > My rationale was that you didn't really care who else was using the IRQ > > (e.g. the timer); you're just stating that you can survive being called > > during suspend (which is what the driver may need to check for in the > > handler if the device happens to be powered down or whatever). > > > > So I guess I see it the other way around. This is essentially claiming > > we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging people to > > > abuse this for other purposes not related to timers. > > > > In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is > > better I shan't complain. > > > > The fundamental issue I'm concerned with is addressed by this approach. > > Okay then, is anyone taking care of submitting such a patch (Mark ?) ? I'll have the core patch shortly. I'll need to ask for your help tagging the relevant drivers and testing. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 11 Feb 2015 16:32:31 +0000 Mark Rutland <mark.rutland@arm.com> wrote: > On Wed, Feb 11, 2015 at 04:15:15PM +0000, Boris Brezillon wrote: > > On Wed, 11 Feb 2015 15:57:20 +0000 > > Mark Rutland <mark.rutland@arm.com> wrote: > > > > > [...] > > > > > > > > > > So for the flag at request time approach to work, all the drivers using > > > > > > > the interrupt would have to flag they're safe in that context. > > > > > > > > > > > > Something like IRQF_"I can share the line with a timer" I guess? That wouldn't > > > > > > hurt and can be checked at request time even. > > > > > > > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > > > > like: > > > > > > > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during > > > > > suspend in the case the line is shared. The > > > > > handler will not access unavailable hardware > > > > > or kernel infrastructure during this period. > > > > > > > > > > #define __IRQF_SUSPEND_SPURIOUS 0x00040000 > > > > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | __IRQF_SUSPEND_SPURIOUS) > > > > > > > > What about > > > > > > > > #define __IRQF_TIMER_SIBLING_OK 0x00040000 > > > > #define IRQF_SHARED_TIMER_OK (IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > > > > > > > The "suspend" part is kind of a distraction to me here, because that really > > > > only is about sharing an IRQ with a timer and the "your interrupt handler > > > > may be called when the device is suspended" part is just a consequence of that. > > > > > > My rationale was that you didn't really care who else was using the IRQ > > > (e.g. the timer); you're just stating that you can survive being called > > > during suspend (which is what the driver may need to check for in the > > > handler if the device happens to be powered down or whatever). > > > > > > So I guess I see it the other way around. This is essentially claiming > > > we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > > > > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging people to > > > > abuse this for other purposes not related to timers. > > > > > > In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is > > > better I shan't complain. > > > > > > The fundamental issue I'm concerned with is addressed by this approach. > > > > Okay then, is anyone taking care of submitting such a patch (Mark ?) ? > > I'll have the core patch shortly. > > I'll need to ask for your help tagging the relevant drivers and testing. For the list of impacted drivers, you can have a look at this series [1] (patches 2 to 5), and I'll take care of the testing part once every one has agreed on the solution ;-). [1]https://lkml.org/lkml/2014/12/15/552
On Wednesday, February 11, 2015 05:15:15 PM Boris Brezillon wrote: > On Wed, 11 Feb 2015 15:57:20 +0000 > Mark Rutland <mark.rutland@arm.com> wrote: > > > [...] > > > > > > > > So for the flag at request time approach to work, all the drivers using > > > > > > the interrupt would have to flag they're safe in that context. > > > > > > > > > > Something like IRQF_"I can share the line with a timer" I guess? That wouldn't > > > > > hurt and can be checked at request time even. > > > > > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > > > like: > > > > > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during > > > > suspend in the case the line is shared. The > > > > handler will not access unavailable hardware > > > > or kernel infrastructure during this period. > > > > > > > > #define __IRQF_SUSPEND_SPURIOUS 0x00040000 > > > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | __IRQF_SUSPEND_SPURIOUS) > > > > > > What about > > > > > > #define __IRQF_TIMER_SIBLING_OK 0x00040000 > > > #define IRQF_SHARED_TIMER_OK (IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > > > > > The "suspend" part is kind of a distraction to me here, because that really > > > only is about sharing an IRQ with a timer and the "your interrupt handler > > > may be called when the device is suspended" part is just a consequence of that. > > > > My rationale was that you didn't really care who else was using the IRQ > > (e.g. the timer); you're just stating that you can survive being called > > during suspend (which is what the driver may need to check for in the > > handler if the device happens to be powered down or whatever). > > > > So I guess I see it the other way around. This is essentially claiming > > we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging people to > > > abuse this for other purposes not related to timers. > > > > In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is > > better I shan't complain. > > > > The fundamental issue I'm concerned with is addressed by this approach. > > Okay then, is anyone taking care of submitting such a patch (Mark ?) ? Well, I guess I should take the responsibility for that. :-) I'll try to cut one later today or tomorrow unless someone else beats me to that. Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 11, 2015 at 04:38:23PM +0000, Boris Brezillon wrote: > On Wed, 11 Feb 2015 16:32:31 +0000 > Mark Rutland <mark.rutland@arm.com> wrote: > > > On Wed, Feb 11, 2015 at 04:15:15PM +0000, Boris Brezillon wrote: > > > On Wed, 11 Feb 2015 15:57:20 +0000 > > > Mark Rutland <mark.rutland@arm.com> wrote: > > > > > > > [...] > > > > > > > > > > > > So for the flag at request time approach to work, all the drivers using > > > > > > > > the interrupt would have to flag they're safe in that context. > > > > > > > > > > > > > > Something like IRQF_"I can share the line with a timer" I guess? That wouldn't > > > > > > > hurt and can be checked at request time even. > > > > > > > > > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > > > > > like: > > > > > > > > > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during > > > > > > suspend in the case the line is shared. The > > > > > > handler will not access unavailable hardware > > > > > > or kernel infrastructure during this period. > > > > > > > > > > > > #define __IRQF_SUSPEND_SPURIOUS 0x00040000 > > > > > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | __IRQF_SUSPEND_SPURIOUS) > > > > > > > > > > What about > > > > > > > > > > #define __IRQF_TIMER_SIBLING_OK 0x00040000 > > > > > #define IRQF_SHARED_TIMER_OK (IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > > > > > > > > > The "suspend" part is kind of a distraction to me here, because that really > > > > > only is about sharing an IRQ with a timer and the "your interrupt handler > > > > > may be called when the device is suspended" part is just a consequence of that. > > > > > > > > My rationale was that you didn't really care who else was using the IRQ > > > > (e.g. the timer); you're just stating that you can survive being called > > > > during suspend (which is what the driver may need to check for in the > > > > handler if the device happens to be powered down or whatever). > > > > > > > > So I guess I see it the other way around. This is essentially claiming > > > > we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > > > > > > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging people to > > > > > abuse this for other purposes not related to timers. > > > > > > > > In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is > > > > better I shan't complain. > > > > > > > > The fundamental issue I'm concerned with is addressed by this approach. > > > > > > Okay then, is anyone taking care of submitting such a patch (Mark ?) ? > > > > I'll have the core patch shortly. > > > > I'll need to ask for your help tagging the relevant drivers and testing. > > For the list of impacted drivers, you can have a look at this series [1] > (patches 2 to 5), and I'll take care of the testing part once every one > has agreed on the solution ;-). > > [1]https://lkml.org/lkml/2014/12/15/552 Thanks for the link. I'll take a look at this once Rafael's given the core patch a once-over. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 11 Feb 2015 17:13:13 +0000 Mark Rutland <mark.rutland@arm.com> wrote: > On Wed, Feb 11, 2015 at 04:42:22PM +0000, Rafael J. Wysocki wrote: > > On Wednesday, February 11, 2015 05:15:15 PM Boris Brezillon wrote: > > > On Wed, 11 Feb 2015 15:57:20 +0000 > > > Mark Rutland <mark.rutland@arm.com> wrote: > > > > > > > [...] > > > > > > > > > > > > So for the flag at request time approach to work, all the drivers using > > > > > > > > the interrupt would have to flag they're safe in that context. > > > > > > > > > > > > > > Something like IRQF_"I can share the line with a timer" I guess? That wouldn't > > > > > > > hurt and can be checked at request time even. > > > > > > > > > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > > > > > like: > > > > > > > > > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during > > > > > > suspend in the case the line is shared. The > > > > > > handler will not access unavailable hardware > > > > > > or kernel infrastructure during this period. > > > > > > > > > > > > #define __IRQF_SUSPEND_SPURIOUS 0x00040000 > > > > > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | __IRQF_SUSPEND_SPURIOUS) > > > > > > > > > > What about > > > > > > > > > > #define __IRQF_TIMER_SIBLING_OK 0x00040000 > > > > > #define IRQF_SHARED_TIMER_OK (IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > > > > > > > > > The "suspend" part is kind of a distraction to me here, because that really > > > > > only is about sharing an IRQ with a timer and the "your interrupt handler > > > > > may be called when the device is suspended" part is just a consequence of that. > > > > > > > > My rationale was that you didn't really care who else was using the IRQ > > > > (e.g. the timer); you're just stating that you can survive being called > > > > during suspend (which is what the driver may need to check for in the > > > > handler if the device happens to be powered down or whatever). > > > > > > > > So I guess I see it the other way around. This is essentially claiming > > > > we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > > > > > > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging people to > > > > > abuse this for other purposes not related to timers. > > > > > > > > In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is > > > > better I shan't complain. > > > > > > > > The fundamental issue I'm concerned with is addressed by this approach. > > > > > > Okay then, is anyone taking care of submitting such a patch (Mark ?) ? > > > > Well, I guess I should take the responsibility for that. :-) > > > > I'll try to cut one later today or tomorrow unless someone else beats me to that. > > I had a go at the core part below. Does it look like what you had in > mind? > > I've given it a go on a hacked-up platform, but I don't have any at91 > stuff to test with, so I haven't bothered with the driver portions just > yet. > > Thanks, > Mark. > > ---->8---- > From 2d9013517637bb567fbcde3e20797cb2fab1c4c5 Mon Sep 17 00:00:00 2001 > From: Mark Rutland <mark.rutland@arm.com> > Date: Wed, 11 Feb 2015 16:44:06 +0000 > Subject: [PATCH] genirq: allow safe sharing of irqs during suspend > > In some cases a physical IRQ line may be shared between devices from > which we expect interrupts during suspend (e.g. timers) and those we do > not (e.g. anything we cut the power to). Where a driver did not request > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > being called during suspend, and where the IRQ PM code detects a > mismatch it produces a loud warning (via WARN_ON_ONCE). > > In a small set of cases the handlers for the devices other than timers > can tolerate being called during suspend time. In these cases the > warning is spurious, and masks other potentially unsafe mismatches as it > is only printed for the first mismatch detected. As the behaviour of the > handlers is an implementation detail, we cannot rely on external data to > decide when it is safe for a given interrupt line to be shared in this > manner. > > This patch adds a new flag, IRQF_SHARED_TIMER_OK, which drivers can use > when requesting an IRQ to state that they can cope if the interrupt is > shared with a timer driver (and hence may be raised during suspend). The > PM code is updated to only warn when a mismatch occurs and at least one > irqaction has neither asked to be called during suspend or has stated it > is safe to be called during suspend. > > This reduces the set of warnings to those cases where there is a real > problem. While it is possible that this flag may be abused, any such > abuses will be explicit in the kernel source and can be detected. > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > Cc: Jason Cooper <jason@lakedaemon.net> > Cc: Nicolas Ferre <nicolas.ferre@atmel.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > --- > include/linux/interrupt.h | 5 +++++ > kernel/irq/pm.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 47 insertions(+), 2 deletions(-) > > 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; > + > + /* > + * 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 ? > + for (action = desc->action; action; action = action->next) > + if (!(action->flags & safe_flags)) > + return false; > + > + return true; > +} > + > +/* > * Called from __setup_irq() with desc->lock held after @action has > * been installed in the action chain. > */ > @@ -44,8 +85,7 @@ 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); > + WARN_ON_ONCE(!irq_safe_during_suspend(desc, action)); > } > > /*
[...] > > 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; > > + > > + /* > > + * 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 Currently it shouldn't matter as the only caller is a WARN_ON_ONCE(), but it seems unfortunate to allow this. 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 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? 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. Thanks, Mark. > > > + for (action = desc->action; action; action = action->next) > > + if (!(action->flags & safe_flags)) > > + return false; > > + > > + return true; > > +} > > + > > +/* > > * Called from __setup_irq() with desc->lock held after @action has > > * been installed in the action chain. > > */ > > @@ -44,8 +85,7 @@ 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); > > + WARN_ON_ONCE(!irq_safe_during_suspend(desc, action)); > > } > > > > /* > > > > -- > Boris Brezillon, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
On Thu, Feb 12, 2015 at 11:09:17AM +0000, Boris Brezillon wrote: > 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) ? I guess we can, but that case is already covered by the above tests. If the single action was not IRQF_NO_SUSPEND, then desc->no_suspend_depth == 0, and we return early. If the single action was IRQF_NO_SUSPEND, then desc->no_suspend_depth == desc->nr_actions, and we return early. We could change the second test to: if (desc->nr_actions == 1 || desc->nr_actions == desc->no_suspend_depth) ...but I don't see that we gain much by doing so. > > > > + > > > > + /* > > > > + * 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 ? Sorry, you are correct. > > 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. To keep things easy to reason about, let's leave this as-is for now. If we encounter a performance problem we can see about optimizing. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Guys, trim your emails, please! On Wed, Feb 11, 2015 at 04:51:36PM +0100, Rafael J. Wysocki wrote: > On Wednesday, February 11, 2015 03:12:38 PM Mark Rutland wrote: > > I guess that would have to imply IRQF_SHARED, so we'd have something > > like: > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during > > suspend in the case the line is shared. The > > handler will not access unavailable hardware > > or kernel infrastructure during this period. > > > > #define __IRQF_SUSPEND_SPURIOUS 0x00040000 > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | __IRQF_SUSPEND_SPURIOUS) > > What about > > #define __IRQF_TIMER_SIBLING_OK 0x00040000 > #define IRQF_SHARED_TIMER_OK (IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > The "suspend" part is kind of a distraction to me here, because that really > only is about sharing an IRQ with a timer and the "your interrupt handler > may be called when the device is suspended" part is just a consequence of that. > > So IMO it's better to have "TIMER" in the names to avoid encouraging people to > abuse this for other purposes not related to timers. Sorry to be late to the bike-shed party, but what about: Documentation/power/suspend-and-interrupts.txt:the IRQ's users. For this reason, using IRQF_NO_SUSPEND and IRQF_SHARED at the arch/arm/mach-omap2/mux.c: omap_hwmod_mux_handle_irq, IRQF_SHARED | IRQF_NO_SUSPEND, arch/arm/mach-omap2/pm34xx.c: _prcm_int_handle_io, IRQF_SHARED | IRQF_NO_SUSPEND, "pm_io", drivers/mfd/ab8500-debugfs.c: IRQF_SHARED | IRQF_NO_SUSPEND, drivers/mfd/ab8500-gpadc.c: IRQF_NO_SUSPEND | IRQF_SHARED, "ab8500-gpadc-sw", drivers/mfd/ab8500-gpadc.c: IRQF_NO_SUSPEND | IRQF_SHARED, "ab8500-gpadc-hw", drivers/pinctrl/pinctrl-single.c: IRQF_SHARED | IRQF_NO_SUSPEND, drivers/power/ab8500_btemp.c: IRQF_SHARED | IRQF_NO_SUSPEND, drivers/power/ab8500_charger.c: IRQF_SHARED | IRQF_NO_SUSPEND, drivers/power/ab8500_fg.c: IRQF_SHARED | IRQF_NO_SUSPEND, drivers/rtc/rtc-pl031.c: .irqflags = IRQF_SHARED | IRQF_NO_SUSPEND, drivers/usb/phy/phy-ab8500-usb.c: IRQF_NO_SUSPEND | IRQF_SHARED, drivers/usb/phy/phy-ab8500-usb.c: IRQF_NO_SUSPEND | IRQF_SHARED, drivers/usb/phy/phy-ab8500-usb.c: IRQF_NO_SUSPEND | IRQF_SHARED, drivers/watchdog/intel-mid_wdt.c: IRQF_SHARED | IRQF_NO_SUSPEND, "watchdog", Is there a single legitimate user in that list? If so, the TIMER name might be misleading. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Please change the Subject to start with [PATCH] again when including patches, otherwise its too easy for them to get lost. Esp. with excessive quoting on top. I nearly missed the patch here, seeing nothing in the first page of text. On Wed, Feb 11, 2015 at 05:13:13PM +0000, Mark Rutland wrote: > --- > include/linux/interrupt.h | 5 +++++ > kernel/irq/pm.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 47 insertions(+), 2 deletions(-) > > 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. I feel we should do better documenting this; at the very least refer to Documentation/power/suspend-and-interrupts.txt and ideally put a scary note in telling people that if they use this as a bandaid to make the warn go away, they will end up with a broken system. Now ideally every driver that employs this should also have a comment next to it how it does indeed behave nicely, such that we can 'quickly' see people have indeed thought about things and not just slapped it on to make the WARN go away. > 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) > + for (action = desc->action; action; action = action->next) > + if (!(action->flags & safe_flags)) > + return false; In general I prefer braces around the for loop even though C does not strictly require it. Its just too easy to confuse multi-line statements with multiple statements. Extra braces comfort the brain in this case. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] > > The "suspend" part is kind of a distraction to me here, because that really > > only is about sharing an IRQ with a timer and the "your interrupt handler > > may be called when the device is suspended" part is just a consequence of that. > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging people to > > abuse this for other purposes not related to timers. > > Sorry to be late to the bike-shed party, but what about: [...] > arch/arm/mach-omap2/mux.c: omap_hwmod_mux_handle_irq, IRQF_SHARED | IRQF_NO_SUSPEND, > arch/arm/mach-omap2/pm34xx.c: _prcm_int_handle_io, IRQF_SHARED | IRQF_NO_SUSPEND, "pm_io", > drivers/pinctrl/pinctrl-single.c: IRQF_SHARED | IRQF_NO_SUSPEND, These are chained IRQ handlers. If any of these have a chained timer irq then the IRQF_NO_SUSPEND may be legitimate. I can't imagine why these would be shared, however. It also looks like these abuse IRQF_NO_SUSPEND for wakeup interrupts. > drivers/rtc/rtc-pl031.c: .irqflags = IRQF_SHARED | IRQF_NO_SUSPEND, This looks to be an abuse and should use {enable,disable}_irq_wake. However, we'd then need to handle mismatch with wakeup interrupts (which is effectively the same problem as sharing with a timer). > drivers/mfd/ab8500-debugfs.c: IRQF_SHARED | IRQF_NO_SUSPEND, > drivers/mfd/ab8500-gpadc.c: IRQF_NO_SUSPEND | IRQF_SHARED, "ab8500-gpadc-sw", > drivers/mfd/ab8500-gpadc.c: IRQF_NO_SUSPEND | IRQF_SHARED, "ab8500-gpadc-hw", > drivers/power/ab8500_btemp.c: IRQF_SHARED | IRQF_NO_SUSPEND, > drivers/power/ab8500_charger.c: IRQF_SHARED | IRQF_NO_SUSPEND, > drivers/power/ab8500_fg.c: IRQF_SHARED | IRQF_NO_SUSPEND, > drivers/usb/phy/phy-ab8500-usb.c: IRQF_NO_SUSPEND | IRQF_SHARED, > drivers/usb/phy/phy-ab8500-usb.c: IRQF_NO_SUSPEND | IRQF_SHARED, > drivers/usb/phy/phy-ab8500-usb.c: IRQF_NO_SUSPEND | IRQF_SHARED, All the *ab8500* look cargo-culted. There's other nonsense in these (e.g. mutex_lock in irq handlers...). I suspect these are not legitimate. > drivers/watchdog/intel-mid_wdt.c: IRQF_SHARED | IRQF_NO_SUSPEND, "watchdog", Watchdogs could be a legitimate case, but this driver relies on another timer and the timeout irq handler simply calls panic(), which seems a little extreme. > Is there a single legitimate user in that list? If so, the TIMER name > might be misleading. The watchdog case could be legitimate, and with drivers corrected to use {enable,disable}_irq_wake we'll need to handle mismatch for wakeup interrupts too. Having separate flags for sharing with timers and sharing with wakeup sources seems redundant, and IRQF_SHARED_TIMER_OK would be misleading. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, February 16, 2015 12:23:43 PM Mark Rutland wrote: > [...] > > > > The "suspend" part is kind of a distraction to me here, because that really > > > only is about sharing an IRQ with a timer and the "your interrupt handler > > > may be called when the device is suspended" part is just a consequence of that. > > > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging people to > > > abuse this for other purposes not related to timers. > > > > Sorry to be late to the bike-shed party, but what about: > > [...] > > > arch/arm/mach-omap2/mux.c: omap_hwmod_mux_handle_irq, IRQF_SHARED | IRQF_NO_SUSPEND, > > arch/arm/mach-omap2/pm34xx.c: _prcm_int_handle_io, IRQF_SHARED | IRQF_NO_SUSPEND, "pm_io", > > drivers/pinctrl/pinctrl-single.c: IRQF_SHARED | IRQF_NO_SUSPEND, > > These are chained IRQ handlers. If any of these have a chained timer irq > then the IRQF_NO_SUSPEND may be legitimate. I can't imagine why these > would be shared, however. > > It also looks like these abuse IRQF_NO_SUSPEND for wakeup interrupts. > > > drivers/rtc/rtc-pl031.c: .irqflags = IRQF_SHARED | IRQF_NO_SUSPEND, > > This looks to be an abuse and should use {enable,disable}_irq_wake. > > However, we'd then need to handle mismatch with wakeup interrupts (which > is effectively the same problem as sharing with a timer). IRQF_NO_SUSPEND and wakeup fundamentally don't match due to the way wakeup is implemented in the IRQ core now. Unless drivers with IRQF_NO_SUSPEND do the wakeup behind the core's back which is just disgusting and should never happen.
On Thu, Feb 19, 2015 at 01:16:50AM +0000, Rafael J. Wysocki wrote: > On Monday, February 16, 2015 12:23:43 PM Mark Rutland wrote: > > [...] > > > > > > The "suspend" part is kind of a distraction to me here, because that really > > > > only is about sharing an IRQ with a timer and the "your interrupt handler > > > > may be called when the device is suspended" part is just a consequence of that. > > > > > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging people to > > > > abuse this for other purposes not related to timers. > > > > > > Sorry to be late to the bike-shed party, but what about: > > > > [...] > > > > > arch/arm/mach-omap2/mux.c: omap_hwmod_mux_handle_irq, IRQF_SHARED | IRQF_NO_SUSPEND, > > > arch/arm/mach-omap2/pm34xx.c: _prcm_int_handle_io, IRQF_SHARED | IRQF_NO_SUSPEND, "pm_io", > > > drivers/pinctrl/pinctrl-single.c: IRQF_SHARED | IRQF_NO_SUSPEND, > > > > These are chained IRQ handlers. If any of these have a chained timer irq > > then the IRQF_NO_SUSPEND may be legitimate. I can't imagine why these > > would be shared, however. > > > > It also looks like these abuse IRQF_NO_SUSPEND for wakeup interrupts. > > > > > drivers/rtc/rtc-pl031.c: .irqflags = IRQF_SHARED | IRQF_NO_SUSPEND, > > > > This looks to be an abuse and should use {enable,disable}_irq_wake. > > > > However, we'd then need to handle mismatch with wakeup interrupts (which > > is effectively the same problem as sharing with a timer). > > IRQF_NO_SUSPEND and wakeup fundamentally don't match due to the way > wakeup is implemented in the IRQ core now. > > Unless drivers with IRQF_NO_SUSPEND do the wakeup behind the core's back > which is just disgusting and should never happen. I completely agree that using IRQF_NO_SUSPEND in that manner is a disgusting abuse. It seems like some drivers are abusing it for wakeup, and those need to be corrected. If those are corrected, the same issue with mismatch will happen with those wakeup interrupts, and we need to fix that somehow given people seem to already be relying on being able to share a line with a wakeup device. I propose we add a new IRQF_BIKESHED, meaning that this interrupt line may be shared with an IRQF_NO_SUSPEND or wakeup interrupt handler. Specifically: * This driver ensures that its device will be quiesced during suspend, and will not assert interrupts. * This handler can be called spuriously during suspend (or we somehow mask out IRQF_BIKSHED irq actions in the core). * It will be documented and enforced that each use of IRQF_BIKESHED must have an associated comment explaining why the driver has to use it, and how it meets the requirements. With that in place we can audit+fix the drivers sharing the line to use IRQF_BIKESHED, which solves the warning Boris is seeing. In parallel with that we can audit+fix the IRQF_NO_SUSPEND abuses to use the correct infrastructure. Does that make any sense? I'll have a go at patches on the assumption that it's not the absolute worst idea, unless/until corrected otherwise. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, February 19, 2015 11:23:46 AM Mark Rutland wrote: > On Thu, Feb 19, 2015 at 01:16:50AM +0000, Rafael J. Wysocki wrote: > > On Monday, February 16, 2015 12:23:43 PM Mark Rutland wrote: > > > [...] > > > > > > > > The "suspend" part is kind of a distraction to me here, because that really > > > > > only is about sharing an IRQ with a timer and the "your interrupt handler > > > > > may be called when the device is suspended" part is just a consequence of that. > > > > > > > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging people to > > > > > abuse this for other purposes not related to timers. > > > > > > > > Sorry to be late to the bike-shed party, but what about: > > > > > > [...] > > > > > > > arch/arm/mach-omap2/mux.c: omap_hwmod_mux_handle_irq, IRQF_SHARED | IRQF_NO_SUSPEND, > > > > arch/arm/mach-omap2/pm34xx.c: _prcm_int_handle_io, IRQF_SHARED | IRQF_NO_SUSPEND, "pm_io", > > > > drivers/pinctrl/pinctrl-single.c: IRQF_SHARED | IRQF_NO_SUSPEND, > > > > > > These are chained IRQ handlers. If any of these have a chained timer irq > > > then the IRQF_NO_SUSPEND may be legitimate. I can't imagine why these > > > would be shared, however. > > > > > > It also looks like these abuse IRQF_NO_SUSPEND for wakeup interrupts. > > > > > > > drivers/rtc/rtc-pl031.c: .irqflags = IRQF_SHARED | IRQF_NO_SUSPEND, > > > > > > This looks to be an abuse and should use {enable,disable}_irq_wake. > > > > > > However, we'd then need to handle mismatch with wakeup interrupts (which > > > is effectively the same problem as sharing with a timer). > > > > IRQF_NO_SUSPEND and wakeup fundamentally don't match due to the way > > wakeup is implemented in the IRQ core now. > > > > Unless drivers with IRQF_NO_SUSPEND do the wakeup behind the core's back > > which is just disgusting and should never happen. > > I completely agree that using IRQF_NO_SUSPEND in that manner is a > disgusting abuse. It seems like some drivers are abusing it for wakeup, > and those need to be corrected. > > If those are corrected, the same issue with mismatch will happen with > those wakeup interrupts, and we need to fix that somehow given people > seem to already be relying on being able to share a line with a wakeup > device. The way we handle wakeup interrupts in the core prevents that, because wakeup is handled at the IRQ level rather than at a handler (irqaction) level. Interrupt handlers from irqactions are not run for wakeup interrupts at all after suspend_device_irqs(), so if you have anyone sharing an IRQ with a wakeup source, their interrupt handler won't be run anyway then. > I propose we add a new IRQF_BIKESHED, meaning that this interrupt line > may be shared with an IRQF_NO_SUSPEND or wakeup interrupt handler. As I said, sharing an IRQ with a wakeup source is OK (worst case you'll cause spurious wakeup interrupts to occur if your device is not suspended properly). The problem is *entirely* about IRQF_NO_SUSPEND. Moreover, I don't think any watchdogs have a legitimate reason to use IRQF_NO_SUSPEND, because quite frankly if you don't want your watchdog to abort system suspends in progress, then you have no reason to run that watchdog during system suspend at all. Conversely, if you want the watchdog to abort system suspends in progress, its wakeup interrupt should be a wakeup one. So I claim that the only things having legitimate reasons to ever use IRQF_NO_SUSPEND are (a) timers and (b) IPIs. There really are no other cases to worry about in my view, but I may be overlooking something. Anyway, I wouldn't like to add flags allowing driver writers to do fishy things in a hush-hush manner. The warning trigger is there for a good reason and if we allow it to be avoided, that has to be for a good reason too. In other words, it shouldn't be too easy to do that.
[...] > > > IRQF_NO_SUSPEND and wakeup fundamentally don't match due to the way > > > wakeup is implemented in the IRQ core now. > > > > > > Unless drivers with IRQF_NO_SUSPEND do the wakeup behind the core's back > > > which is just disgusting and should never happen. > > > > I completely agree that using IRQF_NO_SUSPEND in that manner is a > > disgusting abuse. It seems like some drivers are abusing it for wakeup, > > and those need to be corrected. > > > > If those are corrected, the same issue with mismatch will happen with > > those wakeup interrupts, and we need to fix that somehow given people > > seem to already be relying on being able to share a line with a wakeup > > device. > > The way we handle wakeup interrupts in the core prevents that, because > wakeup is handled at the IRQ level rather than at a handler (irqaction) > level. Interrupt handlers from irqactions are not run for wakeup > interrupts at all after suspend_device_irqs(), so if you have anyone > sharing an IRQ with a wakeup source, their interrupt handler won't be > run anyway then. I had missed that fact; thank you for correcting me! > > I propose we add a new IRQF_BIKESHED, meaning that this interrupt line > > may be shared with an IRQF_NO_SUSPEND or wakeup interrupt handler. > > As I said, sharing an IRQ with a wakeup source is OK (worst case you'll > cause spurious wakeup interrupts to occur if your device is not suspended > properly). The problem is *entirely* about IRQF_NO_SUSPEND. I see that now. > So I claim that the only things having legitimate reasons to ever use > IRQF_NO_SUSPEND are (a) timers and (b) IPIs. There really are no other > cases to worry about in my view, but I may be overlooking something. Assuming you also include any chained irqchip handlers necessary for those timers or IPIs, that makes sense to me. > Anyway, I wouldn't like to add flags allowing driver writers to do fishy > things in a hush-hush manner. The warning trigger is there for a good > reason and if we allow it to be avoided, that has to be for a good reason > too. In other words, it shouldn't be too easy to do that. Sure. Given all of the above I'll go back to the IRQF_SHARED_TIMER_OK approach you proposed, along with documentation updates and comments at usage sites to make it clear when it is valid to use. Thank you for bearing with me so far. Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Boris, On Wed, Feb 11, 2015 at 04:38:23PM +0000, Boris Brezillon wrote: [...] > For the list of impacted drivers, you can have a look at this series [1] > (patches 2 to 5), and I'll take care of the testing part once every one > has agreed on the solution ;-). > > [1]https://lkml.org/lkml/2014/12/15/552 Looking at those: * The pmc looks like it could be a valid use of the new flag. It also seems to function as an irqchip. Do any of its child IRQs need to be handled during the suspend-resume cycle? If so using IRQF_NO_SUSPEND would seem to be valid. * atmel_serial seems to be intended to be used as a wakeup device (given it calls device_set_wakeup_enable). Therefore it needs to call enable_irq_wake, and when it does so it can share an IRQ with other interrupts, just not IRQF_NO_SUSPEND interrupts. None of the approaches thus far can fix the fundamental mismatch between wakeup interrupts and IRQF_NO_SUSPEND interrupts. * Similarly, rtc-at91rm9200 and rtc-at91sam9 are intended to be used as wakeup devices. They call enable_irq_wake (though don't bother checking the return value). They can share an IRQ with other interrupts, just not IRQF_NO_SUSPEND interrupts. * at91sam9_wdt seems to be fundamentally incompatible with suspend. If the watchdog cannot be disabled, and you need to handle it during suspend, then it needs to be a wakeup interrupt, not an IRQF_NO_SUSPEND interrupt. As far as I can see, the flag or virtual irqchip approaches only help the PMC case, and even then might not be necessary. All the others seem to be relying on guarantees the genirq layer don't provide, and fixing that would mean moving them further from IRQF_NO_SUSPEND. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On Fri, 20 Feb 2015 14:22:08 +0000 Mark Rutland <mark.rutland@arm.com> wrote: > Hi Boris, > > On Wed, Feb 11, 2015 at 04:38:23PM +0000, Boris Brezillon wrote: > > [...] > > > For the list of impacted drivers, you can have a look at this series [1] > > (patches 2 to 5), and I'll take care of the testing part once every one > > has agreed on the solution ;-). > > > > [1]https://lkml.org/lkml/2014/12/15/552 > > Looking at those: > > * The pmc looks like it could be a valid use of the new flag. It also > seems to function as an irqchip. > > Do any of its child IRQs need to be handled during the suspend-resume > cycle? If so using IRQF_NO_SUSPEND would seem to be valid. No they don't, they are used for clock activation only, and thus should be disabled on suspend. > > * atmel_serial seems to be intended to be used as a wakeup device (given > it calls device_set_wakeup_enable). Therefore it needs to call > enable_irq_wake, and when it does so it can share an IRQ with other > interrupts, just not IRQF_NO_SUSPEND interrupts. I'll have a look, but AFAIR serial core already taking care of that. > > None of the approaches thus far can fix the fundamental mismatch > between wakeup interrupts and IRQF_NO_SUSPEND interrupts. > > * Similarly, rtc-at91rm9200 and rtc-at91sam9 are intended to be used as > wakeup devices. They call enable_irq_wake (though don't bother > checking the return value). They can share an IRQ with other > interrupts, just not IRQF_NO_SUSPEND interrupts. Yep. > > * at91sam9_wdt seems to be fundamentally incompatible with suspend. If > the watchdog cannot be disabled, and you need to handle it during > suspend, then it needs to be a wakeup interrupt, not an > IRQF_NO_SUSPEND interrupt. You forgot the PIT timer, which is the one in cause here (no other drivers are specifying this IRQF_NO_SUSPEND flag), and, as you already know, a timer sets the IRQF_NO_SUSPEND flag. > > As far as I can see, the flag or virtual irqchip approaches only help > the PMC case, and even then might not be necessary. All the others seem > to be relying on guarantees the genirq layer don't provide, and fixing > that would mean moving them further from IRQF_NO_SUSPEND. I don't know what you're trying to prove here, but I never said my goal was to set IRQF_NO_SUSPEND flags on existing at91 drivers ? The problem here, is that all those IPs are sharing the irq line with a timer which sets IRQF_NO_SUSPEND. I just want to be able to share an irq line with a timer and other devices that do not set IRQF_NO_SUSPEND. Could we focus on that ? Best Regards, Boris
> > * The pmc looks like it could be a valid use of the new flag. It also > > seems to function as an irqchip. > > > > Do any of its child IRQs need to be handled during the suspend-resume > > cycle? If so using IRQF_NO_SUSPEND would seem to be valid. > > No they don't, they are used for clock activation only, and thus should > be disabled on suspend. Ok. So the IRQF_SHARED_TIMER_OK flag would make sense here. > > * atmel_serial seems to be intended to be used as a wakeup device (given > > it calls device_set_wakeup_enable). Therefore it needs to call > > enable_irq_wake, and when it does so it can share an IRQ with other > > interrupts, just not IRQF_NO_SUSPEND interrupts. > > I'll have a look, but AFAIR serial core already taking care of that. > > > > > None of the approaches thus far can fix the fundamental mismatch > > between wakeup interrupts and IRQF_NO_SUSPEND interrupts. > > > > * Similarly, rtc-at91rm9200 and rtc-at91sam9 are intended to be used as > > wakeup devices. They call enable_irq_wake (though don't bother > > checking the return value). They can share an IRQ with other > > interrupts, just not IRQF_NO_SUSPEND interrupts. > > Yep. > > > > > * at91sam9_wdt seems to be fundamentally incompatible with suspend. If > > the watchdog cannot be disabled, and you need to handle it during > > suspend, then it needs to be a wakeup interrupt, not an > > IRQF_NO_SUSPEND interrupt. If they're not shared with an IRQF_NO_SUSPEND IRQ, then everything is already OK. If they are shared with an IRQF_NO_SUSPEND IRQ, then the fundamental problem is not solved by any approach so far. > You forgot the PIT timer, which is the one in cause here (no other > drivers are specifying this IRQF_NO_SUSPEND flag), and, as you already > know, a timer sets the IRQF_NO_SUSPEND flag. > > > As far as I can see, the flag or virtual irqchip approaches only help > > the PMC case, and even then might not be necessary. All the others seem > > to be relying on guarantees the genirq layer don't provide, and fixing > > that would mean moving them further from IRQF_NO_SUSPEND. > > I don't know what you're trying to prove here, but I never said my goal > was to set IRQF_NO_SUSPEND flags on existing at91 drivers ? > The problem here, is that all those IPs are sharing the irq line with a > timer which sets IRQF_NO_SUSPEND. As Rafael and I described, sharing an IRQ between a wakeup device (the serial, rtc, and wdt) and an IRQF_NO_SUSPEND device is broken. You have the choice between losing wakeup events or masking timer events. So those device above which operate as wakeup devices cannot share a line with a timer. > I just want to be able to share an irq line with a timer and other > devices that do not set IRQF_NO_SUSPEND. > Could we focus on that ? I'm trying to focus on the cases we can actually salvage. An IRQ cannot be shared between a device with IRQF_NO_SUSPEND and a device that wishes to operate as a wakeup device, because the semantics don't align. One wishes to handle IRQs continuously and one wants to abort suspend at the first IRQ (waiting until part-way through the resume before handling it). So those devices above which wish to operate as wakeup devices are either irrelevant or unsalvageable with the current approaches. The flag or demux chip only happens to mask the problem in the absence of strict sanity checking. If you want to be able to share the line then you will need to fundamentally rework the way wakeup interrupts work in order to be able to decide when you've encountered a real wakeup event. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, Thanks for the clarification, and sorry if I've been a bit harsh to you in my previous answer, but this whole shared irq thing is starting to drive me crazy. On Fri, 20 Feb 2015 15:16:56 +0000 Mark Rutland <mark.rutland@arm.com> wrote: [...] > > An IRQ cannot be shared between a device with IRQF_NO_SUSPEND and a > device that wishes to operate as a wakeup device, because the semantics > don't align. One wishes to handle IRQs continuously and one wants to > abort suspend at the first IRQ (waiting until part-way through the > resume before handling it). > > So those devices above which wish to operate as wakeup devices are > either irrelevant or unsalvageable with the current approaches. > > The flag or demux chip only happens to mask the problem in the absence > of strict sanity checking. > > If you want to be able to share the line then you will need to > fundamentally rework the way wakeup interrupts work in order to be able > to decide when you've encountered a real wakeup event. Okay, I'll try to summarize the discussion we had on IRC regarding this aspect. On at91 platforms, irq line 1 is shared by a timer (PIT) and some devices that can register themselves as wakeup sources (especially the RTC IP). I doubt at91 users will agree on dropping either of these features (the timer or the wakeup on RTC/UART/...), so, can we try to find a solution that would both make irq, DT and at91 guys happy (that doesn't sound like an easy task :-)) ? The whole problem here is that we can't have both IRQF_NO_SUSPEND flag set on one of the shared action and others that are configuring the irq as a wakeup source, because it would always lead to the system being woken up (even when the only thing we were supposed to do is handle the timer event). This is because irq_may_run [1], which is called to decide whether we should handle this irq or just wake the system up [2], will always return true if at least one of the shared action has tagged the irq line as a wakeup source. Sorry for summarizing things you most likely already know, but I want to be sure I'm actually understanding it correctly. Now, let's look at how this could be solved. Here is a proposal [3] that does the following: 1/ prevent a system wakeup when at least one of the action handler has set the IRQF_NO_SUSPEND flag 2/ Add a few helpers to deal with system wakeup from drivers code 3/ Rework the at91 RTC driver to show how such weird cases could be handled Of course, I'll need the IRQF_SHARED_TIMER_OK patch to prevent the WARN_ON backtrace. Please, let me know if I missed anything important, share your opinion on this proposal, and feel free to propose any other solution. [1]http://lxr.free-electrons.com/source/kernel/irq/chip.c#L348 [2]http://lxr.free-electrons.com/source/kernel/irq/chip.c#L386 [3]http://code.bulix.org/gboee1-87936
On Mon, Feb 23, 2015 at 05:00:57PM +0000, Boris Brezillon wrote: > Hi Mark, > > Thanks for the clarification, and sorry if I've been a bit harsh to you > in my previous answer, but this whole shared irq thing is starting to > drive me crazy. No worries. Having lost a few days exploring the core and call sites, and having seen how widesrpead IRQF_NO_SUSPEND misuse is, it's beginning to grate on me too. [...] > On at91 platforms, irq line 1 is shared by a timer (PIT) and some > devices that can register themselves as wakeup sources (especially the > RTC IP). > I doubt at91 users will agree on dropping either of these features (the > timer or the wakeup on RTC/UART/...), so, can we try to find a solution > that would both make irq, DT and at91 guys happy (that doesn't sound > like an easy task :-)) ? >From the DT side, I think all the necessary information is there. We know how the interrupts are wired, so nothing needs to change w.r.t. the topology description. I hope that we have sufficient information to when a device may operate and raise interrupts during suspend. So the fun part is how we solve this within Linux. > The whole problem here is that we can't have both IRQF_NO_SUSPEND flag > set on one of the shared action and others that are configuring the irq > as a wakeup source, because it would always lead to the system being > woken up (even when the only thing we were supposed to do is handle the > timer event). > > This is because irq_may_run [1], which is called to decide whether we > should handle this irq or just wake the system up [2], will always > return true if at least one of the shared action has tagged the irq > line as a wakeup source. I assume you mean we return false in this case (having triggered the wakeup within irq_pm_check_wakeup, which returned true), but otherwise agreed. I can envisage problems if the irq handler of a wakeup device can't be run safely until resume time, though I'm not sure if that happens in practice given the device is necessarily going to be active. > Sorry for summarizing things you most likely already know, but I want > to be sure I'm actually understanding it correctly. > > Now, let's look at how this could be solved. > Here is a proposal [3] that does the following: This would be a lot easier to follow/review as an RFC post to the mailing list. Otherwise I have some high-level comments on the stuff below, which I think matches the shape of what we discussed on IRC. > 1/ prevent a system wakeup when at least one of the action handler > has set the IRQF_NO_SUSPEND flag We might need to add some logic to enable_irq_wake and irq_pm_install_action to prevent some of the horrible mismatch cases we can get here (e.g. if we have a wakeup handler, a IRQF_NO_SUSPEND handler, and another handler which is neither). We may need to reconsider temporarily stashing the other potential interrupts. Do we perhaps need an IRQF_SHARED_WAKEUP_SIBLING_OK for timer drivers to assert their handlers are safe for the whole suspend period rather than just the period they expect to be enabled for? Or do those always happen to be safe in practice? > 2/ Add a few helpers to deal with system wakeup from drivers code The irq_pm_force_wakeup part looks like what I had in mind. > 3/ Rework the at91 RTC driver to show how such weird cases could be > handled It might be simpler to do this with a PM notifier within the driver rather than having to traverse all the irq_descs, though perhaps not. > Of course, I'll need the IRQF_SHARED_TIMER_OK patch to prevent the > WARN_ON backtrace. That should be fine; it's backed up in the list archive ;) > Please, let me know if I missed anything important, share your opinion > on this proposal, and feel free to propose any other solution. Hopefully the above covers that! Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 23 Feb 2015 18:14:48 +0000 Mark Rutland <mark.rutland@arm.com> wrote: [...] > > This is because irq_may_run [1], which is called to decide whether we > > should handle this irq or just wake the system up [2], will always > > return true if at least one of the shared action has tagged the irq > > line as a wakeup source. > > I assume you mean we return false in this case (having triggered the > wakeup within irq_pm_check_wakeup, which returned true), but otherwise > agreed. Yep, I meant 'return false'. > > I can envisage problems if the irq handler of a wakeup device can't be > run safely until resume time, though I'm not sure if that happens in > practice given the device is necessarily going to be active. Isn't that the purpose of the IRQF_NO_SUSPEND_SAFE/IRQF_SHARED_TIMER_OK/IRQF_SHARED_WAKEUP_SIBLING_OK flag ? > > > Sorry for summarizing things you most likely already know, but I want > > to be sure I'm actually understanding it correctly. > > > > Now, let's look at how this could be solved. > > Here is a proposal [3] that does the following: > > This would be a lot easier to follow/review as an RFC post to the > mailing list. Yep, that was the plan, just wanted to make sure I had correctly understood the problem before posting an RFC. > Otherwise I have some high-level comments on the stuff > below, which I think matches the shape of what we discussed on IRC. > > > 1/ prevent a system wakeup when at least one of the action handler > > has set the IRQF_NO_SUSPEND flag > > We might need to add some logic to enable_irq_wake and > irq_pm_install_action to prevent some of the horrible mismatch cases we > can get here (e.g. if we have a wakeup handler, a IRQF_NO_SUSPEND > handler, and another handler which is neither). We may need to > reconsider temporarily stashing the other potential interrupts. Actually if we force users to pass the IRQF_XXX_SAFE (I'm tired writing all the potential names :-)), when mixing IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND handlers, we shouldn't bother deactivating normal handlers (those without IRQF_NO_SUSPEND), 'cause they claimed they could safely be called in suspended state. > > Do we perhaps need an IRQF_SHARED_WAKEUP_SIBLING_OK for timer drivers to > assert their handlers are safe for the whole suspend period rather than > just the period they expect to be enabled for? Or do those always > happen to be safe in practice? I thought they were always safe... > > > 2/ Add a few helpers to deal with system wakeup from drivers code > > The irq_pm_force_wakeup part looks like what I had in mind. > > > 3/ Rework the at91 RTC driver to show how such weird cases could be > > handled > > It might be simpler to do this with a PM notifier within the driver > rather than having to traverse all the irq_descs, though perhaps not. I'm not sure to understand that one. Where am I traversing irq_descs (irq_to_desc, which is called when testing wakeup_armed status, is a direct table indexing operation) ? Moreover, I'm not sure when the PM_POST_SUSPEND event is sent, and testing the WAKEUP_ARMED flag should be safe in all cases, right ? > > > Of course, I'll need the IRQF_SHARED_TIMER_OK patch to prevent the > > WARN_ON backtrace. > > That should be fine; it's backed up in the list archive ;) > > > Please, let me know if I missed anything important, share your opinion > > on this proposal, and feel free to propose any other solution. > > Hopefully the above covers that! Yes it does. Thanks for the review. Best Regards, Boris
On Friday, February 20, 2015 10:31:44 AM Mark Rutland wrote: > [...] [cut] > Given all of the above I'll go back to the IRQF_SHARED_TIMER_OK approach > you proposed, along with documentation updates and comments at usage > sites to make it clear when it is valid to use. > > Thank you for bearing with me so far. No prob. :-) Actually, there's one more approach possible. Thomas might not like it, but here it goes anyway for consideration. What about making it possible to provide a special irqaction to be called while suspended for the shared IRQF_NO_SUSPEND interrupts. Say we have a "suspended_action" pointer in struct irq_desc in addition to "action" and we swap them in suspend_device_irq() if desc->no_suspend_depth is nonzero and "suspended_action" is not NULL (and then back in resume_irq()). Then, the platform might supply "suspended_action" that will do the right thing for the IRQ. So the requirement would be to have "suspended_action" set to start with and then the WARN_ON() in irq_pm_install_action() may check if that is present and only trigger the warning if it isn't. Thoughs? Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rafael, On Tue, 24 Feb 2015 02:02:59 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote: > On Friday, February 20, 2015 10:31:44 AM Mark Rutland wrote: > > [...] > > [cut] > > > Given all of the above I'll go back to the IRQF_SHARED_TIMER_OK approach > > you proposed, along with documentation updates and comments at usage > > sites to make it clear when it is valid to use. > > > > Thank you for bearing with me so far. > > No prob. :-) > > Actually, there's one more approach possible. Thomas might not like it, but > here it goes anyway for consideration. > > What about making it possible to provide a special irqaction to be called > while suspended for the shared IRQF_NO_SUSPEND interrupts. > > Say we have a "suspended_action" pointer in struct irq_desc in addition to > "action" and we swap them in suspend_device_irq() if desc->no_suspend_depth is > nonzero and "suspended_action" is not NULL (and then back in resume_irq()). > Then, the platform might supply "suspended_action" that will do the right > thing for the IRQ. > > So the requirement would be to have "suspended_action" set to start with > and then the WARN_ON() in irq_pm_install_action() may check if that is present > and only trigger the warning if it isn't. > > Thoughs? That should work for my case. Note that I also proposed a version that does not touch the irq_desc struct, but I had to add an irq_is_wakeup_armed helper function to test in which state the irq handler is called. Your solution would make this easier. Could you remind me what was Thomas concern regarding this suspended action list approach ? Boris
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. + +Required properties: +- compatible: Should be "virtual,irq-demux". +- interrupt-controller: Identifies the node as an interrupt controller. +- interrupts-extended or interrupt-parent and interrupts: Reference the source + interrupt connected to this dumb demuxer. +- #interrupt-cells: The number of cells to define the interrupts (should be 1). + The only cell is the IRQ number. +- irqs: u32 bitfield specifying the interrupts provided by the demuxer. + +Examples: + /* + * virtual demuxer controller + */ + virt_irq1_demux: virt-irq-demux@1 { + compatible = "virtual,irq-demux"; + interrupt-controller; + #interrupt-cells = <1>; + interrupts-extended = <&aic 1 IRQ_TYPE_LEVEL_HIGH 7>; + irqs = <0x3f>; + }; + + /* + * Device connected on this dumb demuxer + */ + dma: dma-controller@ffffec00 { + compatible = "atmel,at91sam9g45-dma"; + reg = <0xffffec00 0x200>; + interrupts-extended = <&virt_irq1_demux 0>; + };