Re: [PATCH 08/15] genirq: Add runtime power management support for IRQ chips

[Grygorii Strashko]

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@xxxxxxxxxx>
> ---
>   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.


-- 
regards,
-grygorii