| Message ID | 1458224359-32665-9-git-send-email-jonathanh@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jon, On 17/03/16 14:19, Jon Hunter wrote: > Some IRQ chips may be located in a power domain outside of the CPU > subsystem and hence will require device specific runtime power > management. In order to support such IRQ chips, add a pointer for a > device structure to the irq_chip structure, and if this pointer is > populated by the IRQ chip driver and CONFIG_PM is selected in the kernel > configuration, then the pm_runtime_get/put APIs for this chip will be > called when an IRQ is requested/freed, respectively. > > When entering system suspend and each interrupt is disabled if there is > no wake-up set for that interrupt. For an IRQ chip that utilises runtime > power management, print a warning message for each active interrupt that > has no wake-up set because these interrupts may be unnecessarily keeping > the IRQ chip enabled during system suspend. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > include/linux/irq.h | 5 +++++ > kernel/irq/chip.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ > kernel/irq/internals.h | 1 + > kernel/irq/manage.c | 14 +++++++++++--- > kernel/irq/pm.c | 3 +++ > 5 files changed, 72 insertions(+), 3 deletions(-) > > diff --git a/include/linux/irq.h b/include/linux/irq.h > index c4de62348ff2..82f36390048d 100644 > --- a/include/linux/irq.h > +++ b/include/linux/irq.h > @@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) > /** > * struct irq_chip - hardware interrupt chip descriptor > * > + * @parent: pointer to associated device > * @name: name for /proc/interrupts > * @irq_startup: start up the interrupt (defaults to ->enable if NULL) > * @irq_shutdown: shut down the interrupt (defaults to ->disable if NULL) > @@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) > * @flags: chip specific flags > */ > struct irq_chip { > + struct device *parent; Nit: Please don't call this just "parent". We have parent fields in irq_data and irq_domain structures, and they always are a pointer to the same type, indicating some form of stacking. Here, we're pointing to a different type altogether... How about calling it "dev", or "device" instead? It would make it much clearer that when crossing that pointer, we're in another subsystem altogether. I'll come back to the rest of the patch a bit later, but I wanted to put that one out right away... ;-) Thanks, M.
On Thu, 17 Mar 2016, Jon Hunter wrote: > /** > * struct irq_chip - hardware interrupt chip descriptor > * > + * @parent: pointer to associated device That's really a bad name. parent suggests that this is a parent interrupt chip and your explanation sucks as well. What's an associated device? Network card? > #include <linux/irqdesc.h> > #include <linux/kernel_stat.h> > +#include <linux/pm_runtime.h> > > #ifdef CONFIG_SPARSE_IRQ > # define IRQ_BITMAP_BITS (NR_IRQS + 8196) > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index b2a93a37f772..65878e7c7c82 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -1114,6 +1114,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) > if (!try_module_get(desc->owner)) > return -ENODEV; > > + ret = irq_chip_pm_get(&desc->irq_data); > + if (ret < 0) > + goto out_mput; So this call nests inside the chip_bus_lock() region. Is that intentional? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Marc, On 17/03/16 15:02, Marc Zyngier wrote: > Hi Jon, > > On 17/03/16 14:19, Jon Hunter wrote: >> Some IRQ chips may be located in a power domain outside of the CPU >> subsystem and hence will require device specific runtime power >> management. In order to support such IRQ chips, add a pointer for a >> device structure to the irq_chip structure, and if this pointer is >> populated by the IRQ chip driver and CONFIG_PM is selected in the kernel >> configuration, then the pm_runtime_get/put APIs for this chip will be >> called when an IRQ is requested/freed, respectively. >> >> When entering system suspend and each interrupt is disabled if there is >> no wake-up set for that interrupt. For an IRQ chip that utilises runtime >> power management, print a warning message for each active interrupt that >> has no wake-up set because these interrupts may be unnecessarily keeping >> the IRQ chip enabled during system suspend. >> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> include/linux/irq.h | 5 +++++ >> kernel/irq/chip.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> kernel/irq/internals.h | 1 + >> kernel/irq/manage.c | 14 +++++++++++--- >> kernel/irq/pm.c | 3 +++ >> 5 files changed, 72 insertions(+), 3 deletions(-) >> >> diff --git a/include/linux/irq.h b/include/linux/irq.h >> index c4de62348ff2..82f36390048d 100644 >> --- a/include/linux/irq.h >> +++ b/include/linux/irq.h >> @@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) >> /** >> * struct irq_chip - hardware interrupt chip descriptor >> * >> + * @parent: pointer to associated device >> * @name: name for /proc/interrupts >> * @irq_startup: start up the interrupt (defaults to ->enable if NULL) >> * @irq_shutdown: shut down the interrupt (defaults to ->disable if NULL) >> @@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) >> * @flags: chip specific flags >> */ >> struct irq_chip { >> + struct device *parent; > > Nit: Please don't call this just "parent". We have parent fields in > irq_data and irq_domain structures, and they always are a pointer to the > same type, indicating some form of stacking. Here, we're pointing to a > different type altogether... > > How about calling it "dev", or "device" instead? It would make it much > clearer that when crossing that pointer, we're in another subsystem > altogether. I will defer to Linus W here, as it was his request we make this 'parent' and not 'dev'. See ... http://marc.info/?l=linux-kernel&m=145035839623442&w=2 > I'll come back to the rest of the patch a bit later, but I wanted to put > that one out right away... ;-) Thanks, Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17/03/16 15:13, Jon Hunter wrote: > Hi Marc, > > On 17/03/16 15:02, Marc Zyngier wrote: >> Hi Jon, >> >> On 17/03/16 14:19, Jon Hunter wrote: >>> Some IRQ chips may be located in a power domain outside of the CPU >>> subsystem and hence will require device specific runtime power >>> management. In order to support such IRQ chips, add a pointer for a >>> device structure to the irq_chip structure, and if this pointer is >>> populated by the IRQ chip driver and CONFIG_PM is selected in the kernel >>> configuration, then the pm_runtime_get/put APIs for this chip will be >>> called when an IRQ is requested/freed, respectively. >>> >>> When entering system suspend and each interrupt is disabled if there is >>> no wake-up set for that interrupt. For an IRQ chip that utilises runtime >>> power management, print a warning message for each active interrupt that >>> has no wake-up set because these interrupts may be unnecessarily keeping >>> the IRQ chip enabled during system suspend. >>> >>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >>> --- >>> include/linux/irq.h | 5 +++++ >>> kernel/irq/chip.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>> kernel/irq/internals.h | 1 + >>> kernel/irq/manage.c | 14 +++++++++++--- >>> kernel/irq/pm.c | 3 +++ >>> 5 files changed, 72 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/linux/irq.h b/include/linux/irq.h >>> index c4de62348ff2..82f36390048d 100644 >>> --- a/include/linux/irq.h >>> +++ b/include/linux/irq.h >>> @@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) >>> /** >>> * struct irq_chip - hardware interrupt chip descriptor >>> * >>> + * @parent: pointer to associated device >>> * @name: name for /proc/interrupts >>> * @irq_startup: start up the interrupt (defaults to ->enable if NULL) >>> * @irq_shutdown: shut down the interrupt (defaults to ->disable if NULL) >>> @@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) >>> * @flags: chip specific flags >>> */ >>> struct irq_chip { >>> + struct device *parent; >> >> Nit: Please don't call this just "parent". We have parent fields in >> irq_data and irq_domain structures, and they always are a pointer to the >> same type, indicating some form of stacking. Here, we're pointing to a >> different type altogether... >> >> How about calling it "dev", or "device" instead? It would make it much >> clearer that when crossing that pointer, we're in another subsystem >> altogether. > > I will defer to Linus W here, as it was his request we make this > 'parent' and not 'dev'. See ... > > http://marc.info/?l=linux-kernel&m=145035839623442&w=2 Well, that contradicts the way use use the word "parent" in the IRQ subsystem, I guess. I'd settle for parent_device or something along those lines (but keep in mind I'm really bad at naming things). Anyway, enough bikeshedding... ;-) M.
On 17/03/16 15:02, Thomas Gleixner wrote: > On Thu, 17 Mar 2016, Jon Hunter wrote: >> /** >> * struct irq_chip - hardware interrupt chip descriptor >> * >> + * @parent: pointer to associated device > > That's really a bad name. parent suggests that this is a parent interrupt chip > and your explanation sucks as well. What's an associated device? Network card? Linus, can you re-iterate your concerns here about just using 'dev' for the name? >> #include <linux/irqdesc.h> >> #include <linux/kernel_stat.h> >> +#include <linux/pm_runtime.h> >> >> #ifdef CONFIG_SPARSE_IRQ >> # define IRQ_BITMAP_BITS (NR_IRQS + 8196) >> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c >> index b2a93a37f772..65878e7c7c82 100644 >> --- a/kernel/irq/manage.c >> +++ b/kernel/irq/manage.c >> @@ -1114,6 +1114,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) >> if (!try_module_get(desc->owner)) >> return -ENODEV; >> >> + ret = irq_chip_pm_get(&desc->irq_data); >> + if (ret < 0) >> + goto out_mput; > > So this call nests inside the chip_bus_lock() region. Is that intentional? Hmm ... I was trying to simplify the call paths, but yes I think it would be safer to move outside. Ok, will fix that. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jon, On 03/17/2016 04:19 PM, Jon Hunter wrote: > Some IRQ chips may be located in a power domain outside of the CPU > subsystem and hence will require device specific runtime power > management. In order to support such IRQ chips, add a pointer for a > device structure to the irq_chip structure, and if this pointer is > populated by the IRQ chip driver and CONFIG_PM is selected in the kernel > configuration, then the pm_runtime_get/put APIs for this chip will be > called when an IRQ is requested/freed, respectively. > > When entering system suspend and each interrupt is disabled if there is > no wake-up set for that interrupt. For an IRQ chip that utilises runtime > power management, print a warning message for each active interrupt that > has no wake-up set because these interrupts may be unnecessarily keeping > the IRQ chip enabled during system suspend. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > include/linux/irq.h | 5 +++++ > kernel/irq/chip.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ > kernel/irq/internals.h | 1 + > kernel/irq/manage.c | 14 +++++++++++--- > kernel/irq/pm.c | 3 +++ > 5 files changed, 72 insertions(+), 3 deletions(-) > > diff --git a/include/linux/irq.h b/include/linux/irq.h > index c4de62348ff2..82f36390048d 100644 > --- a/include/linux/irq.h > +++ b/include/linux/irq.h > @@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) > /** > * struct irq_chip - hardware interrupt chip descriptor > * > + * @parent: pointer to associated device > * @name: name for /proc/interrupts > * @irq_startup: start up the interrupt (defaults to ->enable if NULL) > * @irq_shutdown: shut down the interrupt (defaults to ->disable if NULL) > @@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) > * @flags: chip specific flags > */ [..] > > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c > index cea1de0161f1..ab436119084f 100644 > --- a/kernel/irq/pm.c > +++ b/kernel/irq/pm.c > @@ -83,6 +83,9 @@ static bool suspend_device_irq(struct irq_desc *desc) > * suspend_device_irqs(). > */ > return true; > + } else if (!irq_chip_pm_suspended(&desc->irq_data)) { > + pr_warn("irq %d has no wakeup set and has not been freed!\n", > + desc->irq_data.irq); Sry. But I did not get this part of the patch :( static bool suspend_device_irq(struct irq_desc *desc) { if (!desc->action || irq_desc_is_chained(desc) || desc->no_suspend_depth) { pr_err("skip irq %d\n", irq_desc_get_irq(desc)); return false; } if (irqd_is_wakeup_set(&desc->irq_data)) { irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED); /* * We return true here to force the caller to issue * synchronize_irq(). We need to make sure that the * IRQD_WAKEUP_ARMED is visible before we return from * suspend_device_irqs(). */ pr_err("wakeup irq %d\n", irq_desc_get_irq(desc)); return true; } ^^^^ Here you've added a warning desc->istate |= IRQS_SUSPENDED; __disable_irq(desc); ^^^^ Here non wakeup IRQs will be disabled pr_err("%s __disable_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc)); /* * Hardware which has no wakeup source configuration facility * requires that the non wakeup interrupts are masked at the * chip level. The chip implementation indicates that with * IRQCHIP_MASK_ON_SUSPEND. */ if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND) { mask_irq(desc); pr_err("%s mask_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc)); } return true; } As result, there should be a ton of warnings if one IRQ (for example in GPIO irqchip) is wakeup source, but all other are not. Also I'd like to note that: - it is not expected that any IRQs have to be freed on enter Suspend - Primary interrupt controller is expected to be suspended from syscore_suspend() - not Primary interrupt controllers may be Suspended from: -- dpm_suspend() or dpm_suspend_late() - usual case for external interrupt controllers GPIO expanders (I2C, SPI ..) -- dpm_suspend_noirq() - usual case for SoC peripherals like OMAP GPIO dpm_suspend_noirq |- suspend_device_irqs() |- device_suspend_noirq() [1] <-- OMAP GPIO do suspend here. -- as always, some arches/maches may require hacks in platform code. So, In my opinion, suspend has to be handled by each irqchip driver separately, most probably at suspend_noirq level [1], because only irqchip driver now sees a full picture and knows if it can suspend or not, and when, and how. (may require to use pm_runtime_force_suspend/resume()). I propose do not touch common/generic suspend code now. Any common code can be always refactored later once there will be real drivers updated to use irqchip RPM and which will support Suspend.
On 18/03/16 11:11, Grygorii Strashko wrote: > Hi Jon, > > On 03/17/2016 04:19 PM, Jon Hunter wrote: >> Some IRQ chips may be located in a power domain outside of the CPU >> subsystem and hence will require device specific runtime power >> management. In order to support such IRQ chips, add a pointer for a >> device structure to the irq_chip structure, and if this pointer is >> populated by the IRQ chip driver and CONFIG_PM is selected in the kernel >> configuration, then the pm_runtime_get/put APIs for this chip will be >> called when an IRQ is requested/freed, respectively. >> >> When entering system suspend and each interrupt is disabled if there is >> no wake-up set for that interrupt. For an IRQ chip that utilises runtime >> power management, print a warning message for each active interrupt that >> has no wake-up set because these interrupts may be unnecessarily keeping >> the IRQ chip enabled during system suspend. >> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> include/linux/irq.h | 5 +++++ >> kernel/irq/chip.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> kernel/irq/internals.h | 1 + >> kernel/irq/manage.c | 14 +++++++++++--- >> kernel/irq/pm.c | 3 +++ >> 5 files changed, 72 insertions(+), 3 deletions(-) >> >> diff --git a/include/linux/irq.h b/include/linux/irq.h >> index c4de62348ff2..82f36390048d 100644 >> --- a/include/linux/irq.h >> +++ b/include/linux/irq.h >> @@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) >> /** >> * struct irq_chip - hardware interrupt chip descriptor >> * >> + * @parent: pointer to associated device >> * @name: name for /proc/interrupts >> * @irq_startup: start up the interrupt (defaults to ->enable if NULL) >> * @irq_shutdown: shut down the interrupt (defaults to ->disable if NULL) >> @@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) >> * @flags: chip specific flags >> */ > > [..] > >> >> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c >> index cea1de0161f1..ab436119084f 100644 >> --- a/kernel/irq/pm.c >> +++ b/kernel/irq/pm.c >> @@ -83,6 +83,9 @@ static bool suspend_device_irq(struct irq_desc *desc) >> * suspend_device_irqs(). >> */ >> return true; >> + } else if (!irq_chip_pm_suspended(&desc->irq_data)) { >> + pr_warn("irq %d has no wakeup set and has not been freed!\n", >> + desc->irq_data.irq); > > Sry. But I did not get this part of the patch :( > > static bool suspend_device_irq(struct irq_desc *desc) > { > if (!desc->action || irq_desc_is_chained(desc) || > desc->no_suspend_depth) { > pr_err("skip irq %d\n", irq_desc_get_irq(desc)); > return false; > } > > if (irqd_is_wakeup_set(&desc->irq_data)) { > irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED); > /* > * We return true here to force the caller to issue > * synchronize_irq(). We need to make sure that the > * IRQD_WAKEUP_ARMED is visible before we return from > * suspend_device_irqs(). > */ > pr_err("wakeup irq %d\n", irq_desc_get_irq(desc)); > return true; > } > > ^^^^ Here you've added a warning Yes, to warn if the IRQ is enabled but not a wake-up source ... if (irqd_is_wakeup_set(&desc->irq_data)) { ... } else if (!irq_chip_pm_suspended(&desc->irq_data)) { ... } > desc->istate |= IRQS_SUSPENDED; > __disable_irq(desc); > > ^^^^ Here non wakeup IRQs will be disabled Yes, but this will not turn off the irqchip. It is legitimate for the chip to be enabled during suspend if an IRQ is enabled as a wakeup. The purpose of the warning is to report any IRQs that are enabled at this point, but NOT wake-up sources. These could be unintentionally be keeping the chip active when it does not need to be. > pr_err("%s __disable_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc)); > > /* > * Hardware which has no wakeup source configuration facility > * requires that the non wakeup interrupts are masked at the > * chip level. The chip implementation indicates that with > * IRQCHIP_MASK_ON_SUSPEND. > */ > if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND) { > mask_irq(desc); > pr_err("%s mask_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc)); > } > > return true; > } > > As result, there should be a ton of warnings if one IRQ (for example in GPIO irqchip) > is wakeup source, but all other are not. No there should not be. Remember this is an else-if and ONLY if an IRQ is not a wake-up source AND enabled will you get a warning. > Also I'd like to note that: > - it is not expected that any IRQs have to be freed on enter Suspend True, but surely they should have a wake-up enabled then? If not you are wasting power unnecessarily. I realise that this is different to how interrupts for irqchips work today, but when we discussed this before, the only way to ensure that we can power-down an irqchip with PM is if all IRQs are freed [0]. So it is a slightly different mindset for irqchips with PM, that may be we shouldn't hold references to IRQs forever if we are not using them. > - Primary interrupt controller is expected to be suspended from syscore_suspend() > - not Primary interrupt controllers may be Suspended from: > -- dpm_suspend() or dpm_suspend_late() - usual case for external interrupt controllers > GPIO expanders (I2C, SPI ..) > -- dpm_suspend_noirq() - usual case for SoC peripherals like OMAP GPIO > dpm_suspend_noirq > |- suspend_device_irqs() > |- device_suspend_noirq() [1] <-- OMAP GPIO do suspend here. > -- as always, some arches/maches may require hacks in platform code. > > So, In my opinion, suspend has to be handled by each irqchip driver separately, > most probably at suspend_noirq level [1], because only irqchip driver > now sees a full picture and knows if it can suspend or not, and when, and how. > (may require to use pm_runtime_force_suspend/resume()). I understand what you are saying, but at least in my mind if would be better if the clients of the IRQ chips using PM freed their interrupts when entering suspend. Quite possibly I am overlooking a use-case here or overhead of doing this, but it would avoid every irqchip having to handle this themselves and having a custom handler. > I propose do not touch common/generic suspend code now. Any common code can be always > refactored later once there will be real drivers updated to use irqchip RPM > and which will support Suspend. If this is strongly opposed, I would concede to making this a pr_debug() as I think it could be useful. Jon [0] http://marc.info/?l=linux-pm&m=145340595315514&w=2 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/18/2016 02:27 PM, Jon Hunter wrote: > > On 18/03/16 11:11, Grygorii Strashko wrote: >> Hi Jon, >> >> On 03/17/2016 04:19 PM, Jon Hunter wrote: >>> Some IRQ chips may be located in a power domain outside of the CPU >>> subsystem and hence will require device specific runtime power >>> management. In order to support such IRQ chips, add a pointer for a >>> device structure to the irq_chip structure, and if this pointer is >>> populated by the IRQ chip driver and CONFIG_PM is selected in the kernel >>> configuration, then the pm_runtime_get/put APIs for this chip will be >>> called when an IRQ is requested/freed, respectively. >>> >>> When entering system suspend and each interrupt is disabled if there is >>> no wake-up set for that interrupt. For an IRQ chip that utilises runtime >>> power management, print a warning message for each active interrupt that >>> has no wake-up set because these interrupts may be unnecessarily keeping >>> the IRQ chip enabled during system suspend. >>> >>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >>> --- >>> include/linux/irq.h | 5 +++++ >>> kernel/irq/chip.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>> kernel/irq/internals.h | 1 + >>> kernel/irq/manage.c | 14 +++++++++++--- >>> kernel/irq/pm.c | 3 +++ >>> 5 files changed, 72 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/linux/irq.h b/include/linux/irq.h >>> index c4de62348ff2..82f36390048d 100644 >>> --- a/include/linux/irq.h >>> +++ b/include/linux/irq.h >>> @@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) >>> /** >>> * struct irq_chip - hardware interrupt chip descriptor >>> * >>> + * @parent: pointer to associated device >>> * @name: name for /proc/interrupts >>> * @irq_startup: start up the interrupt (defaults to ->enable if NULL) >>> * @irq_shutdown: shut down the interrupt (defaults to ->disable if NULL) >>> @@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) >>> * @flags: chip specific flags >>> */ >> >> [..] >> >>> >>> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c >>> index cea1de0161f1..ab436119084f 100644 >>> --- a/kernel/irq/pm.c >>> +++ b/kernel/irq/pm.c >>> @@ -83,6 +83,9 @@ static bool suspend_device_irq(struct irq_desc *desc) >>> * suspend_device_irqs(). >>> */ >>> return true; >>> + } else if (!irq_chip_pm_suspended(&desc->irq_data)) { >>> + pr_warn("irq %d has no wakeup set and has not been freed!\n", >>> + desc->irq_data.irq); >> >> Sry. But I did not get this part of the patch :( >> >> static bool suspend_device_irq(struct irq_desc *desc) >> { >> if (!desc->action || irq_desc_is_chained(desc) || >> desc->no_suspend_depth) { >> pr_err("skip irq %d\n", irq_desc_get_irq(desc)); >> return false; >> } >> >> if (irqd_is_wakeup_set(&desc->irq_data)) { >> irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED); >> /* >> * We return true here to force the caller to issue >> * synchronize_irq(). We need to make sure that the >> * IRQD_WAKEUP_ARMED is visible before we return from >> * suspend_device_irqs(). >> */ >> pr_err("wakeup irq %d\n", irq_desc_get_irq(desc)); >> return true; >> } >> >> ^^^^ Here you've added a warning > > Yes, to warn if the IRQ is enabled but not a wake-up source ... > > if (irqd_is_wakeup_set(&desc->irq_data)) { > ... > } else if (!irq_chip_pm_suspended(&desc->irq_data)) { > ... > } > >> desc->istate |= IRQS_SUSPENDED; >> __disable_irq(desc); >> >> ^^^^ Here non wakeup IRQs will be disabled > > Yes, but this will not turn off the irqchip. It is legitimate for the > chip to be enabled during suspend if an IRQ is enabled as a wakeup. > > The purpose of the warning is to report any IRQs that are enabled at > this point, but NOT wake-up sources. These could be unintentionally be > keeping the chip active when it does not need to be. > >> pr_err("%s __disable_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc)); >> >> /* >> * Hardware which has no wakeup source configuration facility >> * requires that the non wakeup interrupts are masked at the >> * chip level. The chip implementation indicates that with >> * IRQCHIP_MASK_ON_SUSPEND. >> */ >> if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND) { >> mask_irq(desc); >> pr_err("%s mask_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc)); >> } >> >> return true; >> } >> >> As result, there should be a ton of warnings if one IRQ (for example in GPIO irqchip) >> is wakeup source, but all other are not. > > No there should not be. Remember this is an else-if and ONLY if an IRQ > is not a wake-up source AND enabled will you get a warning. Sorry, but I don't see the "AND enabled" check any where in this file and disabled irqs can be wakeup source - they shouldn't be masked. But I'll stop commenting until i reproduce it. Or do you mean free? > >> Also I'd like to note that: >> - it is not expected that any IRQs have to be freed on enter Suspend > > True, but surely they should have a wake-up enabled then? If not you are > wasting power unnecessarily. > > I realise that this is different to how interrupts for irqchips work > today, but when we discussed this before, the only way to ensure that we > can power-down an irqchip with PM is if all IRQs are freed [0]. So it is > a slightly different mindset for irqchips with PM, that may be we > shouldn't hold references to IRQs forever if we are not using them. > >> - Primary interrupt controller is expected to be suspended from syscore_suspend() >> - not Primary interrupt controllers may be Suspended from: >> -- dpm_suspend() or dpm_suspend_late() - usual case for external interrupt controllers >> GPIO expanders (I2C, SPI ..) >> -- dpm_suspend_noirq() - usual case for SoC peripherals like OMAP GPIO >> dpm_suspend_noirq >> |- suspend_device_irqs() >> |- device_suspend_noirq() [1] <-- OMAP GPIO do suspend here. >> -- as always, some arches/maches may require hacks in platform code. >> >> So, In my opinion, suspend has to be handled by each irqchip driver separately, >> most probably at suspend_noirq level [1], because only irqchip driver >> now sees a full picture and knows if it can suspend or not, and when, and how. >> (may require to use pm_runtime_force_suspend/resume()). > > I understand what you are saying, but at least in my mind if would be > better if the clients of the IRQ chips using PM freed their interrupts > when entering suspend. Quite possibly I am overlooking a use-case here > or overhead of doing this, ok. seems you do mean "free". oh :( That will require updating of all drivers (and if it will be taken into account that wakeup can be configured from sysfs + devm_ - it will be painful). > but it would avoid every irqchip having to > handle this themselves and having a custom handler. irqchip like TI OMAP GPIO will need custom handling any way even if it's not expected to be Powered off during Suspend or deep CPUIdle states, simply because its state in suspend is unknown - PM state managed automatically (and depends on many factors) and wakeup can be handled by special HW in case if GPIO bank was really switched off. >> I propose do not touch common/generic suspend code now. Any common code can be always >> refactored later once there will be real drivers updated to use irqchip RPM >> and which will support Suspend. > > If this is strongly opposed, I would concede to making this a pr_debug() > as I think it could be useful. Probably yes, because most of the drivers now and IRQ PM core are not ready for this approach.
On 18/03/16 14:23, Grygorii Strashko wrote: > On 03/18/2016 02:27 PM, Jon Hunter wrote: >> >> On 18/03/16 11:11, Grygorii Strashko wrote: >>> Hi Jon, >>> >>> On 03/17/2016 04:19 PM, Jon Hunter wrote: >>>> Some IRQ chips may be located in a power domain outside of the CPU >>>> subsystem and hence will require device specific runtime power >>>> management. In order to support such IRQ chips, add a pointer for a >>>> device structure to the irq_chip structure, and if this pointer is >>>> populated by the IRQ chip driver and CONFIG_PM is selected in the kernel >>>> configuration, then the pm_runtime_get/put APIs for this chip will be >>>> called when an IRQ is requested/freed, respectively. >>>> >>>> When entering system suspend and each interrupt is disabled if there is >>>> no wake-up set for that interrupt. For an IRQ chip that utilises runtime >>>> power management, print a warning message for each active interrupt that >>>> has no wake-up set because these interrupts may be unnecessarily keeping >>>> the IRQ chip enabled during system suspend. >>>> >>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >>>> --- >>>> include/linux/irq.h | 5 +++++ >>>> kernel/irq/chip.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> kernel/irq/internals.h | 1 + >>>> kernel/irq/manage.c | 14 +++++++++++--- >>>> kernel/irq/pm.c | 3 +++ >>>> 5 files changed, 72 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/include/linux/irq.h b/include/linux/irq.h >>>> index c4de62348ff2..82f36390048d 100644 >>>> --- a/include/linux/irq.h >>>> +++ b/include/linux/irq.h >>>> @@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) >>>> /** >>>> * struct irq_chip - hardware interrupt chip descriptor >>>> * >>>> + * @parent: pointer to associated device >>>> * @name: name for /proc/interrupts >>>> * @irq_startup: start up the interrupt (defaults to ->enable if NULL) >>>> * @irq_shutdown: shut down the interrupt (defaults to ->disable if NULL) >>>> @@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) >>>> * @flags: chip specific flags >>>> */ >>> >>> [..] >>> >>>> >>>> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c >>>> index cea1de0161f1..ab436119084f 100644 >>>> --- a/kernel/irq/pm.c >>>> +++ b/kernel/irq/pm.c >>>> @@ -83,6 +83,9 @@ static bool suspend_device_irq(struct irq_desc *desc) >>>> * suspend_device_irqs(). >>>> */ >>>> return true; >>>> + } else if (!irq_chip_pm_suspended(&desc->irq_data)) { >>>> + pr_warn("irq %d has no wakeup set and has not been freed!\n", >>>> + desc->irq_data.irq); >>> >>> Sry. But I did not get this part of the patch :( >>> >>> static bool suspend_device_irq(struct irq_desc *desc) >>> { >>> if (!desc->action || irq_desc_is_chained(desc) || >>> desc->no_suspend_depth) { >>> pr_err("skip irq %d\n", irq_desc_get_irq(desc)); >>> return false; >>> } >>> >>> if (irqd_is_wakeup_set(&desc->irq_data)) { >>> irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED); >>> /* >>> * We return true here to force the caller to issue >>> * synchronize_irq(). We need to make sure that the >>> * IRQD_WAKEUP_ARMED is visible before we return from >>> * suspend_device_irqs(). >>> */ >>> pr_err("wakeup irq %d\n", irq_desc_get_irq(desc)); >>> return true; >>> } >>> >>> ^^^^ Here you've added a warning >> >> Yes, to warn if the IRQ is enabled but not a wake-up source ... >> >> if (irqd_is_wakeup_set(&desc->irq_data)) { >> ... >> } else if (!irq_chip_pm_suspended(&desc->irq_data)) { >> ... >> } >> >>> desc->istate |= IRQS_SUSPENDED; >>> __disable_irq(desc); >>> >>> ^^^^ Here non wakeup IRQs will be disabled >> >> Yes, but this will not turn off the irqchip. It is legitimate for the >> chip to be enabled during suspend if an IRQ is enabled as a wakeup. >> >> The purpose of the warning is to report any IRQs that are enabled at >> this point, but NOT wake-up sources. These could be unintentionally be >> keeping the chip active when it does not need to be. >> >>> pr_err("%s __disable_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc)); >>> >>> /* >>> * Hardware which has no wakeup source configuration facility >>> * requires that the non wakeup interrupts are masked at the >>> * chip level. The chip implementation indicates that with >>> * IRQCHIP_MASK_ON_SUSPEND. >>> */ >>> if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND) { >>> mask_irq(desc); >>> pr_err("%s mask_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc)); >>> } >>> >>> return true; >>> } >>> >>> As result, there should be a ton of warnings if one IRQ (for example in GPIO irqchip) >>> is wakeup source, but all other are not. >> >> No there should not be. Remember this is an else-if and ONLY if an IRQ >> is not a wake-up source AND enabled will you get a warning. > > Sorry, but I don't see the "AND enabled" check any where in this file and > disabled irqs can be wakeup source - they shouldn't be masked. > But I'll stop commenting until i reproduce it. > > Or do you mean free? Yes, to be correct I mean not a wake-up source AND not freed (requested). >> >>> Also I'd like to note that: >>> - it is not expected that any IRQs have to be freed on enter Suspend >> >> True, but surely they should have a wake-up enabled then? If not you are >> wasting power unnecessarily. >> >> I realise that this is different to how interrupts for irqchips work >> today, but when we discussed this before, the only way to ensure that we >> can power-down an irqchip with PM is if all IRQs are freed [0]. So it is >> a slightly different mindset for irqchips with PM, that may be we >> shouldn't hold references to IRQs forever if we are not using them. >> >>> - Primary interrupt controller is expected to be suspended from syscore_suspend() >>> - not Primary interrupt controllers may be Suspended from: >>> -- dpm_suspend() or dpm_suspend_late() - usual case for external interrupt controllers >>> GPIO expanders (I2C, SPI ..) >>> -- dpm_suspend_noirq() - usual case for SoC peripherals like OMAP GPIO >>> dpm_suspend_noirq >>> |- suspend_device_irqs() >>> |- device_suspend_noirq() [1] <-- OMAP GPIO do suspend here. >>> -- as always, some arches/maches may require hacks in platform code. >>> >>> So, In my opinion, suspend has to be handled by each irqchip driver separately, >>> most probably at suspend_noirq level [1], because only irqchip driver >>> now sees a full picture and knows if it can suspend or not, and when, and how. >>> (may require to use pm_runtime_force_suspend/resume()). >> >> I understand what you are saying, but at least in my mind if would be >> better if the clients of the IRQ chips using PM freed their interrupts >> when entering suspend. Quite possibly I am overlooking a use-case here >> or overhead of doing this, > > ok. seems you do mean "free". > > oh :( That will require updating of all drivers (and if it will be taken into account that > wakeup can be configured from sysfs + devm_ - it will be painful). Will it? I know that there are a few gpio chips that have some hacked ways to get around the PM issue, but I wonder how many drivers this really impacts. What sysfs entries are you referring too? >> but it would avoid every irqchip having to >> handle this themselves and having a custom handler. > > irqchip like TI OMAP GPIO will need custom handling any way even if it's not expected > to be Powered off during Suspend or deep CPUIdle states, simply because its state > in suspend is unknown - PM state managed automatically (and depends on many factors) > and wakeup can be handled by special HW in case if GPIO bank was really switched off. > >>> I propose do not touch common/generic suspend code now. Any common code can be always >>> refactored later once there will be real drivers updated to use irqchip RPM >>> and which will support Suspend. >> >> If this is strongly opposed, I would concede to making this a pr_debug() >> as I think it could be useful. > > Probably yes, because most of the drivers now and IRQ PM core are not ready > for this approach. May be this calls for a new flag to not WARN if non-wakeup IRQs are not freed when entering suspend. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18/03/16 14:40, Jon Hunter wrote: > On 18/03/16 14:23, Grygorii Strashko wrote: >> On 03/18/2016 02:27 PM, Jon Hunter wrote: >>> >>> On 18/03/16 11:11, Grygorii Strashko wrote: [snip] >> oh :( That will require updating of all drivers (and if it will be taken into account that >> wakeup can be configured from sysfs + devm_ - it will be painful). > > Will it? I know that there are a few gpio chips that have some hacked > ways to get around the PM issue, but I wonder how many drivers this > really impacts. What sysfs entries are you referring too? Thinking about this some more, yes I guess it would impact all drivers that use a gpio but don't use it for a wake-up. I could see that could be a few drivers indeed. >>> but it would avoid every irqchip having to >>> handle this themselves and having a custom handler. >> >> irqchip like TI OMAP GPIO will need custom handling any way even if it's not expected >> to be Powered off during Suspend or deep CPUIdle states, simply because its state >> in suspend is unknown - PM state managed automatically (and depends on many factors) >> and wakeup can be handled by special HW in case if GPIO bank was really switched off. >> >>>> I propose do not touch common/generic suspend code now. Any common code can be always >>>> refactored later once there will be real drivers updated to use irqchip RPM >>>> and which will support Suspend. >>> >>> If this is strongly opposed, I would concede to making this a pr_debug() >>> as I think it could be useful. >> >> Probably yes, because most of the drivers now and IRQ PM core are not ready >> for this approach. > > May be this calls for a new flag to not WARN if non-wakeup IRQs are not > freed when entering suspend. Flag or pr_debug()? Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/18/2016 04:56 PM, Jon Hunter wrote: > > On 18/03/16 14:40, Jon Hunter wrote: >> On 18/03/16 14:23, Grygorii Strashko wrote: >>> On 03/18/2016 02:27 PM, Jon Hunter wrote: >>>> >>>> On 18/03/16 11:11, Grygorii Strashko wrote: > > [snip] > >>> oh :( That will require updating of all drivers (and if it will be taken into account that >>> wakeup can be configured from sysfs + devm_ - it will be painful). >> >> Will it? I know that there are a few gpio chips that have some hacked >> ways to get around the PM issue, but I wonder how many drivers this >> really impacts. What sysfs entries are you referring too? echo enabled > /sys/devices/platform/44000000.ocp/48020000.serial/tty/ttyS2/power/wakeup > > Thinking about this some more, yes I guess it would impact all drivers > that use a gpio but don't use it for a wake-up. I could see that could > be a few drivers indeed. yep. I've just tested it - gpio was requested through sysfs and configured as IRQ - do suspend the same is if GPIO is requested as IRQ only and not configured as wakeup source [ 319.669760] PM: late suspend of devices complete after 0.213 msecs [ 319.671195] irq 191 has no wakeup set and has not been freed! [ 319.673453] PM: noirq suspend of devices complete after 2.258 msecs this is very minimal configuration - the regular one is at ~30-50 devices most of them will use IRQ and only ~10% are used as wakeup sources. > >>>> but it would avoid every irqchip having to >>>> handle this themselves and having a custom handler. >>> >>> irqchip like TI OMAP GPIO will need custom handling any way even if it's not expected >>> to be Powered off during Suspend or deep CPUIdle states, simply because its state >>> in suspend is unknown - PM state managed automatically (and depends on many factors) >>> and wakeup can be handled by special HW in case if GPIO bank was really switched off. >>> >>>>> I propose do not touch common/generic suspend code now. Any common code can be always >>>>> refactored later once there will be real drivers updated to use irqchip RPM >>>>> and which will support Suspend. >>>> >>>> If this is strongly opposed, I would concede to making this a pr_debug() >>>> as I think it could be useful. >>> >>> Probably yes, because most of the drivers now and IRQ PM core are not ready >>> for this approach. >> >> May be this calls for a new flag to not WARN if non-wakeup IRQs are not >> freed when entering suspend. > > Flag or pr_debug()? > Honestly, I don't know how to proceed - minimum is pr_debug. My personal opinion is still the same - don't touch suspend core code now, within this series.
On 18/03/16 17:52, Grygorii Strashko wrote: > On 03/18/2016 04:56 PM, Jon Hunter wrote: >> >> On 18/03/16 14:40, Jon Hunter wrote: >>> On 18/03/16 14:23, Grygorii Strashko wrote: >>>> On 03/18/2016 02:27 PM, Jon Hunter wrote: >>>>> >>>>> On 18/03/16 11:11, Grygorii Strashko wrote: >> >> [snip] >> >>>> oh :( That will require updating of all drivers (and if it will be taken into account that >>>> wakeup can be configured from sysfs + devm_ - it will be painful). >>> >>> Will it? I know that there are a few gpio chips that have some hacked >>> ways to get around the PM issue, but I wonder how many drivers this >>> really impacts. What sysfs entries are you referring too? > > echo enabled > /sys/devices/platform/44000000.ocp/48020000.serial/tty/ttyS2/power/wakeup > >> >> Thinking about this some more, yes I guess it would impact all drivers >> that use a gpio but don't use it for a wake-up. I could see that could >> be a few drivers indeed. > > yep. I've just tested it > - gpio was requested through sysfs and configured as IRQ > - do suspend > > the same is if GPIO is requested as IRQ only and not configured as wakeup source > > [ 319.669760] PM: late suspend of devices complete after 0.213 msecs > [ 319.671195] irq 191 has no wakeup set and has not been freed! > [ 319.673453] PM: noirq suspend of devices complete after 2.258 msecs > > this is very minimal configuration - the regular one is at ~30-50 devices > most of them will use IRQ and only ~10% are used as wakeup sources. Then it is working as intended :-) However, if this is too verbose for some irqchips, then as I mentioned we can have a flag to avoid these messages. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 17, 2016 at 4:28 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 17/03/16 15:13, Jon Hunter wrote: >>>> struct irq_chip { >>>> + struct device *parent; >>> >>> Nit: Please don't call this just "parent". We have parent fields in >>> irq_data and irq_domain structures, and they always are a pointer to the >>> same type, indicating some form of stacking. Here, we're pointing to a >>> different type altogether... >>> >>> How about calling it "dev", or "device" instead? It would make it much >>> clearer that when crossing that pointer, we're in another subsystem >>> altogether. >> >> I will defer to Linus W here, as it was his request we make this >> 'parent' and not 'dev'. See ... >> >> http://marc.info/?l=linux-kernel&m=145035839623442&w=2 > > Well, that contradicts the way use use the word "parent" in the IRQ > subsystem, I guess. I'd settle for parent_device or something along > those lines (but keep in mind I'm really bad at naming things). > > Anyway, enough bikeshedding... ;-) I'm happy with .parent_device. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/irq.h b/include/linux/irq.h index c4de62348ff2..82f36390048d 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) /** * struct irq_chip - hardware interrupt chip descriptor * + * @parent: pointer to associated device * @name: name for /proc/interrupts * @irq_startup: start up the interrupt (defaults to ->enable if NULL) * @irq_shutdown: shut down the interrupt (defaults to ->disable if NULL) @@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) * @flags: chip specific flags */ struct irq_chip { + struct device *parent; const char *name; unsigned int (*irq_startup)(struct irq_data *data); void (*irq_shutdown)(struct irq_data *data); @@ -488,6 +490,9 @@ extern void handle_bad_irq(struct irq_desc *desc); extern void handle_nested_irq(unsigned int irq); extern int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg); +extern int irq_chip_pm_get(struct irq_data *data); +extern int irq_chip_pm_put(struct irq_data *data); +extern bool irq_chip_pm_suspended(struct irq_data *data); #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY extern void irq_chip_enable_parent(struct irq_data *data); extern void irq_chip_disable_parent(struct irq_data *data); diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index 2f9f2b0e79f2..c575b700e88a 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -1093,3 +1093,55 @@ int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) return 0; } + +/** + * irq_chip_pm_get - Enable power for an IRQ chip + * @data: Pointer to interrupt specific data + * + * Enable the power to the IRQ chip referenced by the interrupt data + * structure. + */ +int irq_chip_pm_get(struct irq_data *data) +{ + int retval = 0; + + if (IS_ENABLED(CONFIG_PM) && data->chip->parent) + retval = pm_runtime_get_sync(data->chip->parent); + + return (retval < 0) ? retval : 0; +} + +/** + * irq_chip_pm_put - Disable power for an IRQ chip + * @data: Pointer to interrupt specific data + * + * Disable the power to the IRQ chip referenced by the interrupt data + * structure, belongs. Note that power will only be disabled, once this + * function has been called for all IRQs that have called irq_chip_pm_get(). + */ +int irq_chip_pm_put(struct irq_data *data) +{ + int retval = 0; + + if (IS_ENABLED(CONFIG_PM) && data->chip->parent) + retval = pm_runtime_put(data->chip->parent); + + return (retval < 0) ? retval : 0; +} + +/** + * irq_chip_pm_suspended - Power status for an IRQ chip + * @data: Pointer to interrupt specific data + * + * Return the runtime power status for an IRQ chip referenced by the + * interrupt data structure. + */ +bool irq_chip_pm_suspended(struct irq_data *data) +{ + bool status = true; + + if (IS_ENABLED(CONFIG_PM) && data->chip->parent) + status = pm_runtime_status_suspended(data->chip->parent); + + return status; +} diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h index 09be2c903c6d..d5edcdc9382a 100644 --- a/kernel/irq/internals.h +++ b/kernel/irq/internals.h @@ -7,6 +7,7 @@ */ #include <linux/irqdesc.h> #include <linux/kernel_stat.h> +#include <linux/pm_runtime.h> #ifdef CONFIG_SPARSE_IRQ # define IRQ_BITMAP_BITS (NR_IRQS + 8196) diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index b2a93a37f772..65878e7c7c82 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -1114,6 +1114,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) if (!try_module_get(desc->owner)) return -ENODEV; + ret = irq_chip_pm_get(&desc->irq_data); + if (ret < 0) + goto out_mput; + new->irq = irq; /* @@ -1131,7 +1135,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) if (nested) { if (!new->thread_fn) { ret = -EINVAL; - goto out_mput; + goto out_pm; } /* * Replace the primary handler which was provided from @@ -1143,7 +1147,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) if (irq_settings_can_thread(desc)) { ret = irq_setup_forced_threading(new); if (ret) - goto out_mput; + goto out_pm; } } @@ -1155,7 +1159,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) if (new->thread_fn && !nested) { ret = setup_irq_thread(new, irq, false); if (ret) - goto out_mput; + goto out_pm; if (new->secondary) { ret = setup_irq_thread(new->secondary, irq, true); if (ret) @@ -1397,6 +1401,8 @@ out_thread: kthread_stop(t); put_task_struct(t); } +out_pm: + irq_chip_pm_put(&desc->irq_data); out_mput: module_put(desc->owner); return ret; @@ -1513,6 +1519,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id) } } + irq_chip_pm_put(&desc->irq_data); module_put(desc->owner); kfree(action->secondary); return action; @@ -1829,6 +1836,7 @@ static struct irqaction *__free_percpu_irq(unsigned int irq, void __percpu *dev_ unregister_handler_proc(irq, action); + irq_chip_pm_put(&desc->irq_data); module_put(desc->owner); return action; diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c index cea1de0161f1..ab436119084f 100644 --- a/kernel/irq/pm.c +++ b/kernel/irq/pm.c @@ -83,6 +83,9 @@ static bool suspend_device_irq(struct irq_desc *desc) * suspend_device_irqs(). */ return true; + } else if (!irq_chip_pm_suspended(&desc->irq_data)) { + pr_warn("irq %d has no wakeup set and has not been freed!\n", + desc->irq_data.irq); } desc->istate |= IRQS_SUSPENDED;
Some IRQ chips may be located in a power domain outside of the CPU subsystem and hence will require device specific runtime power management. In order to support such IRQ chips, add a pointer for a device structure to the irq_chip structure, and if this pointer is populated by the IRQ chip driver and CONFIG_PM is selected in the kernel configuration, then the pm_runtime_get/put APIs for this chip will be called when an IRQ is requested/freed, respectively. When entering system suspend and each interrupt is disabled if there is no wake-up set for that interrupt. For an IRQ chip that utilises runtime power management, print a warning message for each active interrupt that has no wake-up set because these interrupts may be unnecessarily keeping the IRQ chip enabled during system suspend. Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- include/linux/irq.h | 5 +++++ kernel/irq/chip.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ kernel/irq/internals.h | 1 + kernel/irq/manage.c | 14 +++++++++++--- kernel/irq/pm.c | 3 +++ 5 files changed, 72 insertions(+), 3 deletions(-)