Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines
On Wednesday, March 04, 2015 07:42:46 PM Mark Rutland wrote: > Hi Rafael, > > I'm a little late to the party here, but I have just a couple of minor > comments... > > [...] > > > Link: http://marc.info/?l=linux-kernel=142252777602084=2 > > Link: http://marc.info/?t=142252775300011=1=2 > > Linx: https://lkml.org/lkml/2014/12/15/552 > > s/x/k/ ? Yes, thanks! > > Reported-by: Boris Brezillon > > Signed-off-by: Rafael J. Wysocki > > --- > > include/linux/interrupt.h |5 + > > include/linux/irqdesc.h |1 + > > kernel/irq/manage.c |7 ++- > > kernel/irq/pm.c |7 ++- > > 4 files changed, 18 insertions(+), 2 deletions(-) > > > > Index: linux-pm/include/linux/interrupt.h > > === > > --- linux-pm.orig/include/linux/interrupt.h > > +++ linux-pm/include/linux/interrupt.h > > @@ -57,6 +57,10 @@ > > * IRQF_NO_THREAD - Interrupt cannot be threaded > > * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device > > *resume time. > > + * IRQF_COND_SUSPEND - If the IRQ is shared with a NO_SUSPEND user, > > execute this > > + *interrupt handler after suspending interrupts. For system > > + *wakeup devices users need to implement wakeup detection > > in > > + *their interrupt handlers. > > It's probably worth documenting this in suspend-and-interrupts.txt, as > this invalidates some of the "IRQF_NO_SUSPEND and enable_irq_wake()" > section. I'll send a patch momentarily to that effect. > > Otherwise, this patch looks good, thanks for handling this! > > Acked-by: Mark Rutland Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines
Hi Rafael, I'm a little late to the party here, but I have just a couple of minor comments... [...] > Link: http://marc.info/?l=linux-kernel=142252777602084=2 > Link: http://marc.info/?t=142252775300011=1=2 > Linx: https://lkml.org/lkml/2014/12/15/552 s/x/k/ ? > Reported-by: Boris Brezillon > Signed-off-by: Rafael J. Wysocki > --- > include/linux/interrupt.h |5 + > include/linux/irqdesc.h |1 + > kernel/irq/manage.c |7 ++- > kernel/irq/pm.c |7 ++- > 4 files changed, 18 insertions(+), 2 deletions(-) > > Index: linux-pm/include/linux/interrupt.h > === > --- linux-pm.orig/include/linux/interrupt.h > +++ linux-pm/include/linux/interrupt.h > @@ -57,6 +57,10 @@ > * IRQF_NO_THREAD - Interrupt cannot be threaded > * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device > *resume time. > + * IRQF_COND_SUSPEND - If the IRQ is shared with a NO_SUSPEND user, execute > this > + *interrupt handler after suspending interrupts. For system > + *wakeup devices users need to implement wakeup detection in > + *their interrupt handlers. It's probably worth documenting this in suspend-and-interrupts.txt, as this invalidates some of the "IRQF_NO_SUSPEND and enable_irq_wake()" section. I'll send a patch momentarily to that effect. Otherwise, this patch looks good, thanks for handling this! Acked-by: Mark Rutland Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines
Hi Rafael, I'm a little late to the party here, but I have just a couple of minor comments... [...] Link: http://marc.info/?l=linux-kernelm=142252777602084w=2 Link: http://marc.info/?t=142252775300011r=1w=2 Linx: https://lkml.org/lkml/2014/12/15/552 s/x/k/ ? Reported-by: Boris Brezillon boris.brezil...@free-electrons.com Signed-off-by: Rafael J. Wysocki rafael.j.wyso...@intel.com --- include/linux/interrupt.h |5 + include/linux/irqdesc.h |1 + kernel/irq/manage.c |7 ++- kernel/irq/pm.c |7 ++- 4 files changed, 18 insertions(+), 2 deletions(-) Index: linux-pm/include/linux/interrupt.h === --- linux-pm.orig/include/linux/interrupt.h +++ linux-pm/include/linux/interrupt.h @@ -57,6 +57,10 @@ * IRQF_NO_THREAD - Interrupt cannot be threaded * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device *resume time. + * IRQF_COND_SUSPEND - If the IRQ is shared with a NO_SUSPEND user, execute this + *interrupt handler after suspending interrupts. For system + *wakeup devices users need to implement wakeup detection in + *their interrupt handlers. It's probably worth documenting this in suspend-and-interrupts.txt, as this invalidates some of the IRQF_NO_SUSPEND and enable_irq_wake() section. I'll send a patch momentarily to that effect. Otherwise, this patch looks good, thanks for handling this! Acked-by: Mark Rutland mark.rutl...@arm.com Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines
On Wednesday, March 04, 2015 07:42:46 PM Mark Rutland wrote: Hi Rafael, I'm a little late to the party here, but I have just a couple of minor comments... [...] Link: http://marc.info/?l=linux-kernelm=142252777602084w=2 Link: http://marc.info/?t=142252775300011r=1w=2 Linx: https://lkml.org/lkml/2014/12/15/552 s/x/k/ ? Yes, thanks! Reported-by: Boris Brezillon boris.brezil...@free-electrons.com Signed-off-by: Rafael J. Wysocki rafael.j.wyso...@intel.com --- include/linux/interrupt.h |5 + include/linux/irqdesc.h |1 + kernel/irq/manage.c |7 ++- kernel/irq/pm.c |7 ++- 4 files changed, 18 insertions(+), 2 deletions(-) Index: linux-pm/include/linux/interrupt.h === --- linux-pm.orig/include/linux/interrupt.h +++ linux-pm/include/linux/interrupt.h @@ -57,6 +57,10 @@ * IRQF_NO_THREAD - Interrupt cannot be threaded * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device *resume time. + * IRQF_COND_SUSPEND - If the IRQ is shared with a NO_SUSPEND user, execute this + *interrupt handler after suspending interrupts. For system + *wakeup devices users need to implement wakeup detection in + *their interrupt handlers. It's probably worth documenting this in suspend-and-interrupts.txt, as this invalidates some of the IRQF_NO_SUSPEND and enable_irq_wake() section. I'll send a patch momentarily to that effect. Otherwise, this patch looks good, thanks for handling this! Acked-by: Mark Rutland mark.rutl...@arm.com Thanks! -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines
On Fri, Feb 27, 2015 at 11:13:57PM +0100, Rafael J. Wysocki wrote: > On Friday, February 27, 2015 09:38:59 AM Peter Zijlstra wrote: > > Seems good to me. Should I take this through tip/irq ? > > I can apply it along with the previous IRQF_NO_SUSPEND documentation patch > from Mark Rutland if you ACK it for me. :-) Works for me, Acked-by: Peter Zijlstra (Intel) > > Also, should we warn if people use enable_irq_wake() where there is only > > a single descriptor with NO_SUSPEND? > > We probably should do that, but that would be a separate patch IMO? Agreed. Just wanted to raise the point. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines
On Friday, February 27, 2015 09:38:59 AM Peter Zijlstra wrote: > On Fri, Feb 27, 2015 at 12:07:55AM +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki > > > > It currently is required that all users of NO_SUSPEND interrupt > > lines pass the IRQF_NO_SUSPEND flag when requesting the IRQ or the > > WARN_ON_ONCE() in irq_pm_install_action() will trigger. That is > > done to warn about situations in which unprepared interrupt handlers > > may be run unnecessarily for suspended devices and may attempt to > > access those devices by mistake. However, it may cause drivers > > that have no technical reasons for using IRQF_NO_SUSPEND to set > > that flag just because they happen to share the interrupt line > > with something like a timer. > > > > Moreover, the generic handling of wakeup interrupts introduced by > > commit 9ce7a25849e8 (genirq: Simplify wakeup mechanism) only works > > for IRQs without any NO_SUSPEND users, so the drivers of wakeup > > devices needing to use shared NO_SUSPEND interrupt lines for > > signaling system wakeup generally have to detect wakeup in their > > interrupt handlers. Thus if they happen to share an interrupt line > > with a NO_SUSPEND user, they also need to request that their > > interrupt handlers be run after suspend_device_irqs(). > > > > In both cases the reason for using IRQF_NO_SUSPEND is not because > > the driver in question has a genuine need to run its interrupt > > handler after suspend_device_irqs(), but because it happens to > > share the line with some other NO_SUSPEND user. Otherwise, the > > driver would do without IRQF_NO_SUSPEND just fine. > > > > To make it possible to specify that condition explicitly, introduce > > a new IRQ action handler flag for shared IRQs, IRQF_COND_SUSPEND, > > that, when set, will indicate to the IRQ core that the interrupt > > user is generally fine with suspending the IRQ, but it also can > > tolerate handler invocations after suspend_device_irqs() and, in > > particular, it is capable of detecting system wakeup and triggering > > it as appropriate from its interrupt handler. > > > > That will allow us to work around a problem with a shared timer > > interrupt line on at91 platforms. > > > > Link: http://marc.info/?l=linux-kernel=142252777602084=2 > > Link: http://marc.info/?t=142252775300011=1=2 > > Linx: https://lkml.org/lkml/2014/12/15/552 > > Reported-by: Boris Brezillon > > Signed-off-by: Rafael J. Wysocki > > Seems good to me. Should I take this through tip/irq ? I can apply it along with the previous IRQF_NO_SUSPEND documentation patch from Mark Rutland if you ACK it for me. :-) > Also, should we warn if people use enable_irq_wake() where there is only > a single descriptor with NO_SUSPEND? We probably should do that, but that would be a separate patch IMO? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines
On Fri, Feb 27, 2015 at 12:07:55AM +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki > > It currently is required that all users of NO_SUSPEND interrupt > lines pass the IRQF_NO_SUSPEND flag when requesting the IRQ or the > WARN_ON_ONCE() in irq_pm_install_action() will trigger. That is > done to warn about situations in which unprepared interrupt handlers > may be run unnecessarily for suspended devices and may attempt to > access those devices by mistake. However, it may cause drivers > that have no technical reasons for using IRQF_NO_SUSPEND to set > that flag just because they happen to share the interrupt line > with something like a timer. > > Moreover, the generic handling of wakeup interrupts introduced by > commit 9ce7a25849e8 (genirq: Simplify wakeup mechanism) only works > for IRQs without any NO_SUSPEND users, so the drivers of wakeup > devices needing to use shared NO_SUSPEND interrupt lines for > signaling system wakeup generally have to detect wakeup in their > interrupt handlers. Thus if they happen to share an interrupt line > with a NO_SUSPEND user, they also need to request that their > interrupt handlers be run after suspend_device_irqs(). > > In both cases the reason for using IRQF_NO_SUSPEND is not because > the driver in question has a genuine need to run its interrupt > handler after suspend_device_irqs(), but because it happens to > share the line with some other NO_SUSPEND user. Otherwise, the > driver would do without IRQF_NO_SUSPEND just fine. > > To make it possible to specify that condition explicitly, introduce > a new IRQ action handler flag for shared IRQs, IRQF_COND_SUSPEND, > that, when set, will indicate to the IRQ core that the interrupt > user is generally fine with suspending the IRQ, but it also can > tolerate handler invocations after suspend_device_irqs() and, in > particular, it is capable of detecting system wakeup and triggering > it as appropriate from its interrupt handler. > > That will allow us to work around a problem with a shared timer > interrupt line on at91 platforms. > > Link: http://marc.info/?l=linux-kernel=142252777602084=2 > Link: http://marc.info/?t=142252775300011=1=2 > Linx: https://lkml.org/lkml/2014/12/15/552 > Reported-by: Boris Brezillon > Signed-off-by: Rafael J. Wysocki Seems good to me. Should I take this through tip/irq ? Also, should we warn if people use enable_irq_wake() where there is only a single descriptor with NO_SUSPEND? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines
On Fri, Feb 27, 2015 at 11:13:57PM +0100, Rafael J. Wysocki wrote: On Friday, February 27, 2015 09:38:59 AM Peter Zijlstra wrote: Seems good to me. Should I take this through tip/irq ? I can apply it along with the previous IRQF_NO_SUSPEND documentation patch from Mark Rutland if you ACK it for me. :-) Works for me, Acked-by: Peter Zijlstra (Intel) pet...@infradead.org Also, should we warn if people use enable_irq_wake() where there is only a single descriptor with NO_SUSPEND? We probably should do that, but that would be a separate patch IMO? Agreed. Just wanted to raise the point. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines
On Friday, February 27, 2015 09:38:59 AM Peter Zijlstra wrote: On Fri, Feb 27, 2015 at 12:07:55AM +0100, Rafael J. Wysocki wrote: From: Rafael J. Wysocki rafael.j.wyso...@intel.com It currently is required that all users of NO_SUSPEND interrupt lines pass the IRQF_NO_SUSPEND flag when requesting the IRQ or the WARN_ON_ONCE() in irq_pm_install_action() will trigger. That is done to warn about situations in which unprepared interrupt handlers may be run unnecessarily for suspended devices and may attempt to access those devices by mistake. However, it may cause drivers that have no technical reasons for using IRQF_NO_SUSPEND to set that flag just because they happen to share the interrupt line with something like a timer. Moreover, the generic handling of wakeup interrupts introduced by commit 9ce7a25849e8 (genirq: Simplify wakeup mechanism) only works for IRQs without any NO_SUSPEND users, so the drivers of wakeup devices needing to use shared NO_SUSPEND interrupt lines for signaling system wakeup generally have to detect wakeup in their interrupt handlers. Thus if they happen to share an interrupt line with a NO_SUSPEND user, they also need to request that their interrupt handlers be run after suspend_device_irqs(). In both cases the reason for using IRQF_NO_SUSPEND is not because the driver in question has a genuine need to run its interrupt handler after suspend_device_irqs(), but because it happens to share the line with some other NO_SUSPEND user. Otherwise, the driver would do without IRQF_NO_SUSPEND just fine. To make it possible to specify that condition explicitly, introduce a new IRQ action handler flag for shared IRQs, IRQF_COND_SUSPEND, that, when set, will indicate to the IRQ core that the interrupt user is generally fine with suspending the IRQ, but it also can tolerate handler invocations after suspend_device_irqs() and, in particular, it is capable of detecting system wakeup and triggering it as appropriate from its interrupt handler. That will allow us to work around a problem with a shared timer interrupt line on at91 platforms. Link: http://marc.info/?l=linux-kernelm=142252777602084w=2 Link: http://marc.info/?t=142252775300011r=1w=2 Linx: https://lkml.org/lkml/2014/12/15/552 Reported-by: Boris Brezillon boris.brezil...@free-electrons.com Signed-off-by: Rafael J. Wysocki rafael.j.wyso...@intel.com Seems good to me. Should I take this through tip/irq ? I can apply it along with the previous IRQF_NO_SUSPEND documentation patch from Mark Rutland if you ACK it for me. :-) Also, should we warn if people use enable_irq_wake() where there is only a single descriptor with NO_SUSPEND? We probably should do that, but that would be a separate patch IMO? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] genirq / PM: Add flag for shared NO_SUSPEND interrupt lines
On Fri, Feb 27, 2015 at 12:07:55AM +0100, Rafael J. Wysocki wrote: From: Rafael J. Wysocki rafael.j.wyso...@intel.com It currently is required that all users of NO_SUSPEND interrupt lines pass the IRQF_NO_SUSPEND flag when requesting the IRQ or the WARN_ON_ONCE() in irq_pm_install_action() will trigger. That is done to warn about situations in which unprepared interrupt handlers may be run unnecessarily for suspended devices and may attempt to access those devices by mistake. However, it may cause drivers that have no technical reasons for using IRQF_NO_SUSPEND to set that flag just because they happen to share the interrupt line with something like a timer. Moreover, the generic handling of wakeup interrupts introduced by commit 9ce7a25849e8 (genirq: Simplify wakeup mechanism) only works for IRQs without any NO_SUSPEND users, so the drivers of wakeup devices needing to use shared NO_SUSPEND interrupt lines for signaling system wakeup generally have to detect wakeup in their interrupt handlers. Thus if they happen to share an interrupt line with a NO_SUSPEND user, they also need to request that their interrupt handlers be run after suspend_device_irqs(). In both cases the reason for using IRQF_NO_SUSPEND is not because the driver in question has a genuine need to run its interrupt handler after suspend_device_irqs(), but because it happens to share the line with some other NO_SUSPEND user. Otherwise, the driver would do without IRQF_NO_SUSPEND just fine. To make it possible to specify that condition explicitly, introduce a new IRQ action handler flag for shared IRQs, IRQF_COND_SUSPEND, that, when set, will indicate to the IRQ core that the interrupt user is generally fine with suspending the IRQ, but it also can tolerate handler invocations after suspend_device_irqs() and, in particular, it is capable of detecting system wakeup and triggering it as appropriate from its interrupt handler. That will allow us to work around a problem with a shared timer interrupt line on at91 platforms. Link: http://marc.info/?l=linux-kernelm=142252777602084w=2 Link: http://marc.info/?t=142252775300011r=1w=2 Linx: https://lkml.org/lkml/2014/12/15/552 Reported-by: Boris Brezillon boris.brezil...@free-electrons.com Signed-off-by: Rafael J. Wysocki rafael.j.wyso...@intel.com Seems good to me. Should I take this through tip/irq ? Also, should we warn if people use enable_irq_wake() where there is only a single descriptor with NO_SUSPEND? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
