Re: [PATCH v5 3/6] genirq/PM: Introduce IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND flag

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 8/26/2020 3:08 AM, Thomas Gleixner wrote:
On Tue, Aug 25 2020 at 03:12, Stephen Boyd wrote:
Quoting Maulik Shah (2020-08-22 09:16:58)
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index c6c7e18..2cc800b 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -69,12 +69,17 @@ void irq_pm_remove_action(struct irq_desc *desc, struct irqaction *action)
static bool suspend_device_irq(struct irq_desc *desc)
  {
+       unsigned long chipflags = irq_desc_get_chip(desc)->flags;
+
         if (!desc->action || irq_desc_is_chained(desc) ||
             desc->no_suspend_depth)
                 return false;
if (irqd_is_wakeup_set(&desc->irq_data)) {
                 irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED);
+
+               if (chipflags & IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND)
+                       irq_enable(desc);
Where is the corresponding change to resume_irq()? Don't we need to
disable an irq if it was disabled on suspend and forcibly enabled here?
I should have added comment explaining why i did not added.
I thought of having corresponding change to resume_irq() but i did not kept intentionally since i didn't
observe any issue in my testing.

Actually the drivers which called (disable_irq() + enable_irq_wake()), are invoking enable_irq() in the resume path everytime. With the driver's call to enable_irq() things are restoring back already.

If above is not true in some corner case, then the IRQ handler of driver won't get invoked, in such case,
why even to wake up with such IRQs in the first place, right?

However If we don't want to rely on the drivers doing things correctly, state can be restored in resume_irq()
I explored this, During suspend,

1. Some IRQs are unmasked/enabled + marked for wakeup
2. Some IRQs are masked/disabled + marked for wakeup

So have to track and restore only IRQs in category (2).
With current patch we don't have way to track IRQ is in (1) or (2).
It may be done with the new IRQD flag saying like IRQD_IRQ_ENABLED_ON_SUSPEND

During suspend,
First check if the IRQ was in disabled/masked state to invoke irq_enable() only for category (2) and set the new flag.

    if (irqd_irq_disabled(&desc->irq_data) && (chipflags & IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND)) {
        irq_enable(desc);
        irq_state_set_enabled_on_suspend(desc); => this will set new IRQD_IRQ_ENABLED_ON_SUSPEND
    }

During resume,
Simply calling irq_disable(desc); don't work in resume_irq(), since by default this API tries to lazily disable at HW, which won't quite restore the state,
So instead of adding below

    if (irqd_irq_disabled(&desc->irq_data) && (chipflags & IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND)
    && (irqd_is_enabled_on_suspend(desc)))
    {
        irq_disable(desc);
        irq_state_clear_enabled_on_suspend(desc); => clear flag
    }

we can replicate the irq_disable() with removal of lazy part, something like,

    if (irqd_irq_disabled(&desc->irq_data) && (chipflags & IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND) &&     (irqd_is_enabled_on_suspend(desc))) { ==> The new flag used to determine if IRQ was enabled during suspend path, then only restore.
        irq_state_set_disabled(desc);
        if (desc->irq_data.chip->irq_disable) {
desc->irq_data.chip->irq_disable(&desc->irq_data);
            irq_state_set_masked(desc);
        } else {
        mask_irq(desc);
    }
    irq_state_clear_enabled_on_suspend(desc);
}

which is matching exactly reverse of what is done in suspend entry.
Let me know if above is good i can include this in v6.

Thanks,
Maulik

That part was below the POC code I provided in the fine print:

  "plus the counterpart in the resume path. This also ensures that state is
   consistent."

Who reads the fine print? :)

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux