irqchip: qcom: pdc: Introduce irq_set_wake call

Add irqchip specific flags for msmgpio irqchip to mask non wakeirqs during suspend and mask before setting irq type.

Masking before changing type should make sure any spurious interrupt is not detected during this operation.

Fixes e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")

Acked-by Linus Walleij <[email protected]>

Reviewed-by Douglas Anderson <[email protected]>

Signed-off-by Maulik Shah <[email protected]>

---
 drivers/pinctrl/qcom/pinctrl-msm.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/pinctrl/qcom/pinctrl-msm.c +2 -0

index a2567e7..1c23f5c 100644
@@ -1243,6 +1243,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 	pctrl->irq_chip.irq_set_affinity = msm_gpio_irq_set_affinity;
 	pctrl->irq_chip.irq_set_vcpu_affinity = msm_gpio_irq_set_vcpu_affinity;
+	pctrl->irq_chip.flags = IRQCHIP_MASK_ON_SUSPEND |
+				IRQCHIP_SET_TYPE_MASKED;
 
 	np = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
 	if (np) {

Signature · 2 lines

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

msmgpio irqchip was not using return value of irq_set_irq_wake() callback since previously GIC-v3 irqchip neither had IRQCHIP_SKIP_SET_WAKE flag nor it implemented .irq_set_wake callback. This lead to irq_set_irq_wake() return error -ENXIO.

However from 'commit 4110b5cbb014 ("irqchip/gic-v3: Allow interrupt to be configured as wake-up sources")' GIC irqchip has IRQCHIP_SKIP_SET_WAKE flag.

Use return value from irq_set_irq_wake() and irq_chip_set_wake_parent() instead of always returning success.

Fixes e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")

Signed-off-by Maulik Shah <[email protected]>

Reviewed-by Douglas Anderson <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

---
 drivers/pinctrl/qcom/pinctrl-msm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

drivers/pinctrl/qcom/pinctrl-msm.c +3 -5

index 1c23f5c..1df2322 100644
@@ -1077,12 +1077,10 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 	 * when TLMM is powered on. To allow that, enable the GPIO
 	 * summary line to be wakeup capable at GIC.
 	 */
-	if (d->parent_data)
-		irq_chip_set_wake_parent(d, on);
-
-	irq_set_irq_wake(pctrl->irq, on);
+	if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
+		return irq_chip_set_wake_parent(d, on);
 
-	return 0;
+	return irq_set_irq_wake(pctrl->irq, on);
 }
 
 static int msm_gpio_irq_reqres(struct irq_data *d)

Signature · 2 lines

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

kernel/irq/pm.c +1 -0

index c6c7e18..2cc800b 100644
@@ -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;

kernel/irq/pm.c +1 -1

                /*
                 * We return true here to force the caller to issue
                 * synchronize_irq(). We need to make sure that the

@@ -93,7 +98,7 @@ static bool suspend_device_irq(struct irq_desc *desc)
         * chip level. The chip implementation indicates that with
         * IRQCHIP_MASK_ON_SUSPEND.
         */
-       if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND)
+       if (chipflags & IRQCHIP_MASK_ON_SUSPEND)
                mask_irq(desc);
        return true;
 }

Quoting Maulik Shah (2020-08-22 09:16:58)

Quoted 14 lines

kernel/irq/pm.c +2 -0

index c6c7e18..2cc800b 100644
@@ -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;
 

kernel/irq/pm.c void irq_pm_remove_action(struct irq_desc *desc, struct irqaction *action) · line 82

Quoted 5 lines

kernel/irq/pm.c +3 -0

        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?

Quoted 15 lines · Stephen Boyd

Quoting Maulik Shah (2020-08-22 09:16:58)

Quoted 14 lines

kernel/irq/pm.c +2 -0

index c6c7e18..2cc800b 100644
@@ -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;
 

Quoted 7 lines

kernel/irq/pm.c void irq_pm_remove_action(struct irq_desc *desc, struct irqaction *action) · line 82

Quoted 5 lines

kernel/irq/pm.c +3 -0

         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?

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? :)

Quoted 3 lines · Thomas Gleixner

Quoted 3 lines

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?

Quoted 8 lines · Maulik Shah

Quoted 3 lines · Thomas Gleixner

Quoted 3 lines

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.

That makes it correct in which way? Did not explode in my face is hardly proof of anything.

Quoted 4 lines

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.

No, that's just wrong because you again create inconsistent state.

Quoted 3 lines

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?

I don't care about the corner case. If the driver misses to do it is buggy in the first place. Silently papering over it is just mindless hackery.

There are two reasonable choices here:

1) Do the symmetric thing

2) Let the drivers call a new function disable_wakeup_irq_for_suspend() which marks the interrupt to be enabled from the core on suspend and remove the enable call on the resume callback of the driver.

Then you don't need the resume part in the core and state still is consistent.

I'm leaning towards #2 because that makes a lot of sense.

Quoted 4 lines · Thomas Gleixner

There are two reasonable choices here:

1) Do the symmetric thing

Quoted 8 lines

2) Let the drivers call a new function disable_wakeup_irq_for_suspend() which marks the interrupt to be enabled from the core on suspend and remove the enable call on the resume callback of the driver.

Then you don't need the resume part in the core and state still is consistent.

I'm leaning towards #2 because that makes a lot of sense.

IIUC, #2 requires that we change existing drivers that are currently using disable_irq() + enable_irq_wake(), right? Presumably, if we're going to do #2, we should declare that what drivers used to do is now considered illegal, right? Perhaps we could detect that and throw a warning so that they know that they need to change to use the new disable_wakeup_irq_for_suspend() API. Otherwise these drivers will work fine on some systems (like they always have) but will fail in weird corner cases for systems that are relying on drivers to call disable_wakeup_irq_for_suspend(). That doesn't sound super great to me...

Quoted 14 lines · Doug Anderson

Quoted 4 lines · Thomas Gleixner

There are two reasonable choices here:

1) Do the symmetric thing

Quoted 8 lines

2) Let the drivers call a new function disable_wakeup_irq_for_suspend() which marks the interrupt to be enabled from the core on suspend and remove the enable call on the resume callback of the driver.

Then you don't need the resume part in the core and state still is consistent.

I'm leaning towards #2 because that makes a lot of sense.

Quoted 10 lines

IIUC, #2 requires that we change existing drivers that are currently using disable_irq() + enable_irq_wake(), right? Presumably, if we're going to do #2, we should declare that what drivers used to do is now considered illegal, right? Perhaps we could detect that and throw a warning so that they know that they need to change to use the new disable_wakeup_irq_for_suspend() API. Otherwise these drivers will work fine on some systems (like they always have) but will fail in weird corner cases for systems that are relying on drivers to call disable_wakeup_irq_for_suspend(). That doesn't sound super great to me...

Hmm. With disable_irq() + enable_irq_wake() in the driver suspend path the driver already makes an implicit assumption about the underlying irq chip functionality, i.e. it expects that even with the interrupt disabled the irq chip can wake up the system.

Now with the new flag magic and #1 we are just working around the driver assumptions at the interrupt chip level.

That's inconsistent at best.

How many drivers are doing that sequence?

And the more important
question is why are they calling disable_irq() in the first place if they want to be woken up by that interrupt.

The point is that the core suspend code disables all interrupts which are not marked as wakeup enabled automatically and reenables them after resume. So why would any driver invoke disable_irq() in the suspend function at all? Historical raisins?

Specifically I think it gets back to the idea that, from a device driver's point of view, there isn't a separate concept of disabling an IRQ (turn it off and stop tracking it) and masking an IRQ (keep track of it but don't call my handler until I unmask). As I understand it drivers expect that the disable_irq() call is actually a mask and that an IRQ is never fully disabled unless released by the driver. It is a little unfortunate (IMO) that the function is called disable_irq() but as far as I understand that's historical.

Quoted 4 lines

The point is that the core suspend code disables all interrupts which are not marked as wakeup enabled automatically and reenables them after resume. So why would any driver invoke disable_irq() in the suspend function at all? Historical raisins?

One case I can imagine: pretend that there are two power rails controlling a device. One power rail controls the communication channel between the CPU and the peripheral and the other power rail controls whether the peripheral is on. At suspend time we want to keep the peripheral on but we can shut down the power to the communication channel.

One way you could do this is at suspend time:
disable_irq()
turn_off_comm_power()
enable_irq_wake()

You'd do the disable_irq() (AKA mask your interrupt) because you'd really want to make sure that your handler isn't called after you turned off the communication power. You want to leave the interrupt pending/masked until you are able to turn the communications channel back on and then you can query why the wakeup happened.

Now, admittedly, you could redesign the above driver to work any number of different ways. Maybe you could use the "noirq" suspend to turn off your comm power or maybe you could come up with another solution. However, since the above has always worked and is quite simple I guess that's what drivers use?

Quoted 8 lines · Doug Anderson

Specifically I think it gets back to the idea that, from a device driver's point of view, there isn't a separate concept of disabling an IRQ (turn it off and stop tracking it) and masking an IRQ (keep track of it but don't call my handler until I unmask). As I understand it drivers expect that the disable_irq() call is actually a mask and that an IRQ is never fully disabled unless released by the driver. It is a little unfortunate (IMO) that the function is called disable_irq() but as far as I understand that's historical.

Yes, the naming is historical but it always meant:

Don't invoke an interrupt handler. Whether that's achieved by actually masking it at the interrupt chip level in hardware or by software state in the core does not matter from the driver perspective.

Quoted 17 lines

Quoted 4 lines

The point is that the core suspend code disables all interrupts which are not marked as wakeup enabled automatically and reenables them after resume. So why would any driver invoke disable_irq() in the suspend function at all? Historical raisins?

One case I can imagine: pretend that there are two power rails controlling a device. One power rail controls the communication channel between the CPU and the peripheral and the other power rail controls whether the peripheral is on. At suspend time we want to keep the peripheral on but we can shut down the power to the communication channel.

One way you could do this is at suspend time:
disable_irq()
turn_off_comm_power()
enable_irq_wake()

Quoted 5 lines

You'd do the disable_irq() (AKA mask your interrupt) because you'd really want to make sure that your handler isn't called after you turned off the communication power. You want to leave the interrupt pending/masked until you are able to turn the communications channel back on and then you can query why the wakeup happened.

Ok.

Quoted 5 lines

Now, admittedly, you could redesign the above driver to work any number of different ways. Maybe you could use the "noirq" suspend to turn off your comm power or maybe you could come up with another solution. However, since the above has always worked and is quite simple I guess that's what drivers use?

That comm power case is a reasonable argument for having that sequence. So we need to make sure that the underlying interrupt chips do the right thing.

We have the following two cases:

1) irq chip does not have a irq_disable() callback and does not have IRQ_DISABLE_UNLAZY set

In that case the interrupt is not masked at the hardware level. It's just software state. If the interrupt fires while disabled it is marked pending and actually masked at the hardware level.

Actually there is a race condition which is not handled:

   disable_irq()
   ...

   interrupt fires
      mask and mark pending

   ....
   suspend_device_irq()
      if (wakeup source) {
         set_state(WAKEUP ARMED);

         return;
      }

   That pending interrupt will not prevent the machine from going into
   suspend and if it's an edge interrupt then an unmask in
   suspend_device_irq() won't help. Edge interrupts are not resent in
   hardware. They are fire and forget from the POV of the device
   hardware.

2) irq chip has a irq_disable() callback or has IRQ_DISABLE_UNLAZY set

In that case disable_irq() will mask it at the hardware level and it stays that way until enable_irq() is invoked.

#1 kinda works and the gap is reasonably trivial to fix in
suspend_device_irq() by checking the pending state and telling the PM core that there is a wakeup pending.

#2 Needs an indication from the chip flags that an interrupt which is
masked has to be unmasked when it is a enabled wakeup source.

I assume your problem is #2, right? If it's #1 then UNMASK_IF_WAKEUP is the wrong answer.

Quoted 5 lines · Thomas Gleixner

That pending interrupt will not prevent the machine from going into suspend and if it's an edge interrupt then an unmask in suspend_device_irq() won't help. Edge interrupts are not resent in hardware. They are fire and forget from the POV of the device hardware.

Ah, interesting. I didn't think about this case exactly. I might have a fix for it anyway. At some point in time I was thinking that the world could be solved by relying on lazily-disabled interrupts and I wrote up a patch to make sure that they woke things up. If you're willing to check out our gerrit you can look at:

https://crrev.com/c/2314693

...if not I can post it as a RFC for you.

I'm sure I've solved the problem in a completely incorrect and broken way, but hopefully the idea makes sense. In discussion we decided not to go this way because it looked like IRQ clients could request an IRQ with IRQ_DISABLE_UNLAZY and then that'd break us. :( ...but even so I think the patch is roughly right and would address your point #1.

Quoted 9 lines

2) irq chip has a irq_disable() callback or has IRQ_DISABLE_UNLAZY set

In that case disable_irq() will mask it at the hardware level and it stays that way until enable_irq() is invoked.

#1 kinda works and the gap is reasonably trivial to fix in
suspend_device_irq() by checking the pending state and telling the PM core that there is a wakeup pending.

Quoted 5 lines

#2 Needs an indication from the chip flags that an interrupt which is
masked has to be unmasked when it is a enabled wakeup source.

I assume your problem is #2, right? If it's #1 then UNMASK_IF_WAKEUP is the wrong answer.

Right, the problem is #2. We're not in the lazy mode.

Doug,

Quoted 15 lines · Doug Anderson

Quoted 5 lines · Thomas Gleixner

That pending interrupt will not prevent the machine from going into suspend and if it's an edge interrupt then an unmask in suspend_device_irq() won't help. Edge interrupts are not resent in hardware. They are fire and forget from the POV of the device hardware.

Ah, interesting. I didn't think about this case exactly. I might have a fix for it anyway. At some point in time I was thinking that the world could be solved by relying on lazily-disabled interrupts and I wrote up a patch to make sure that they woke things up. If you're willing to check out our gerrit you can look at:

https://crrev.com/c/2314693

Quoted 1 line

...if not I can post it as a RFC for you.

I actually tried despite my usual aversion against web interfaces. Aversion confirmed :)

You could have included the 5 lines of patch into your reply to spare me the experience. :)

Quoted 5 lines

I'm sure I've solved the problem in a completely incorrect and broken way, but hopefully the idea makes sense. In discussion we decided not to go this way because it looked like IRQ clients could request an IRQ with IRQ_DISABLE_UNLAZY and then that'd break us. :( ...but even so I think the patch is roughly right and would address your point #1.

Kinda :) But that's still incomplete because it does not handle the case where the interrupt arrives between disable_irq() and enable_irq_wake(). See below.

Quoted 12 lines

Quoted 12 lines

2) irq chip has a irq_disable() callback or has IRQ_DISABLE_UNLAZY set

In that case disable_irq() will mask it at the hardware level and it stays that way until enable_irq() is invoked.

#1 kinda works and the gap is reasonably trivial to fix in
suspend_device_irq() by checking the pending state and telling the PM core that there is a wakeup pending.

#2 Needs an indication from the chip flags that an interrupt which is
masked has to be unmasked when it is a enabled wakeup source.

Quoted 4 lines

Quoted 2 lines

I assume your problem is #2, right? If it's #1 then UNMASK_IF_WAKEUP is the wrong answer.

Right, the problem is #2. We're not in the lazy mode.

Right and that's where we want the new chip flag with the unmask if armed.

Quoted 2 lines · Thomas Gleixner

Right and that's where we want the new chip flag with the unmask if armed.

OK, so we're back in Maulik's court to spin, right? I think the last word before our tangent was at:

http://lore.kernel.org/r/[email protected]

There you were leaning towards #2 ("a new function disable_wakeup_irq_for_suspend()"). Presumably you'd now be suggesting #1 ("Do the symmetric thing") since I've pointed out the bunch of drivers that would need to change.

Set IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND flag to enable/unmask the wakeirqs during suspend entry.

Signed-off-by Maulik Shah <[email protected]>

---

Set IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND flag to enable/unmask the wakeirqs during suspend entry.

Signed-off-by Maulik Shah <[email protected]>

---
 drivers/pinctrl/qcom/pinctrl-msm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Set IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND flag to enable/unmask the wakeirqs during suspend entry.

Signed-off-by Maulik Shah <[email protected]>

---

Set IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND flag to enable/unmask the wakeirqs during suspend entry.

Signed-off-by Maulik Shah <[email protected]>

---
 drivers/irqchip/qcom-pdc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Kexec can directly boot into a new kernel without going to complete reboot. This can leave the previous kernel's configuration for PDC interrupts as is.

Clear previous kernel's configuration during init by setting interrupts in enable bank to zero. The IRQs specified in qcom,pdc-ranges property are the only ones that can be used by the new kernel so clear only those IRQs. The remaining ones may be in use by a different kernel and should not be set by new kernel.

Suggested-by Stephen Boyd <[email protected]>

Signed-off-by Maulik Shah <[email protected]>

---

Kexec can directly boot into a new kernel without going to complete reboot. This can leave the previous kernel's configuration for PDC interrupts as is.

Clear previous kernel's configuration during init by setting interrupts in enable bank to zero. The IRQs specified in qcom,pdc-ranges property are the only ones that can be used by the new kernel so clear only those IRQs. The remaining ones may be in use by a different kernel and should not be set by new kernel.

Suggested-by Stephen Boyd <[email protected]>

Signed-off-by Maulik Shah <[email protected]>

---
 drivers/irqchip/qcom-pdc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Changes in v5: