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

[Jon Hunter]

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