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

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

---

Can the subject be more specific? "pinctrl: qcom: Set IRQCHIP_SET_TYPE_MASKED flag"?

Quoting Maulik Shah (2020-08-10 04:20:54)

Quoted 10 lines

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

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

---

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

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 +0 -0

index a2567e7..90edf61 100644

msmgpio irqchip is not using return value of irq_set_wake call. Start using it.

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

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

Reviewed-by Douglas Anderson <[email protected]>

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

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

index 90edf61..c264561 100644

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

 }
 
 static int msm_gpio_irq_reqres(struct irq_data *d)

Quoting Maulik Shah (2020-08-10 04:20:55)

Quoted 2 lines

msmgpio irqchip is not using return value of irq_set_wake call. Start using it.

Does this work when the irq parent isn't setup in a hierarchy? I seem to recall that this was written this way because sometimes irq_set_irq_wake() would fail for the summary irq so it was a best effort setting of wake on the summary line.

Quoted 13 lines

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

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

Reviewed-by Douglas Anderson <[email protected]>

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

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

index 90edf61..c264561 100644

drivers/pinctrl/qcom/pinctrl-msm.c static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) · line 1081

Quoted 9 lines

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

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

So this bit is probably fine.

Quoted 1 line

drivers/pinctrl/qcom/pinctrl-msm.c static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) · line 1083

Quoted 2 lines

drivers/pinctrl/qcom/pinctrl-msm.c +1 -1

-       return 0;
+       return irq_set_irq_wake(pctrl->irq, on);

But this one is probably not fine.

Quoting Maulik Shah (2020-08-10 04:20:55)

Quoted 2 lines

msmgpio irqchip is not using return value of irq_set_wake call. Start using it.

Does this work when the irq parent isn't setup in a hierarchy?

I seem to
recall that this was written this way because sometimes irq_set_irq_wake() would fail for the summary irq so it was a best effort setting of wake on the summary line.

Quoted 13 lines

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

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

Reviewed-by Douglas Anderson <[email protected]>

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

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

index 90edf61..c264561 100644

drivers/pinctrl/qcom/pinctrl-msm.c static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) · line 1081

Quoted 9 lines

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

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

So this bit is probably fine.

Quoted 1 line

drivers/pinctrl/qcom/pinctrl-msm.c static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) · line 1083

Quoted 2 lines

drivers/pinctrl/qcom/pinctrl-msm.c +1 -1

-       return 0;
+       return irq_set_irq_wake(pctrl->irq, on);

But this one is probably not fine.

Quoted 4 lines

  }

static int msm_gpio_irq_reqres(struct irq_data *d)

Hi,

Quoted 4 lines · Stephen Boyd

Quoting Maulik Shah (2020-08-10 04:20:55)

Quoted 2 lines

msmgpio irqchip is not using return value of irq_set_wake call. Start using it.

Does this work when the irq parent isn't setup in a hierarchy?

yes it works fine even when parent isn't setup in hierarchy.

Quoted 4 lines

I seem to
recall that this was written this way because sometimes irq_set_irq_wake() would fail for the summary irq so it was a best effort setting of wake on the summary line.

Thanks for pointing this.

It was written this way since previously GIC driver neither had IRQCHIP_SKIP_SET_WAKE flag nor it implemented .irq_set_wake callback,

so the call to irq_set_irq_wake() to set_irq_wake_real() used to return error -ENXIO in past.

I see this is already taken care now in GIC drivers by adding IRQCHIP_SKIP_SET_WAKE flag.

msmgpio irqchip is not using return value of irq_set_wake call. Start using it.

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

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

Reviewed-by Douglas Anderson <[email protected]>

From: Douglas Anderson <[email protected]>

The "struct irq_chip" has two callbacks in it: irq_suspend() and irq_resume(). These two callbacks are interesting because sometimes an irq chip needs to know about suspend/resume, but they are a bit awkward because:
1. They are called once for the whole irq_chip, not once per IRQ. It's passed data for one of the IRQs enabled on that chip. That means it's up to the irq_chip driver to aggregate.
2. They are only called if you're using "generic-chip", which not everyone is.
3. The implementation uses syscore ops, which apparently have problems with s2idle.

Probably the old irq_suspend() and irq_resume() callbacks should be deprecated.

Let's introcuce a nicer API that works for all irq_chip devices. This

Hi,

Quoted 14 lines · Maulik Shah

From: Douglas Anderson <[email protected]>

The "struct irq_chip" has two callbacks in it: irq_suspend() and irq_resume(). These two callbacks are interesting because sometimes an irq chip needs to know about suspend/resume, but they are a bit awkward because:
1. They are called once for the whole irq_chip, not once per IRQ. It's passed data for one of the IRQs enabled on that chip. That means it's up to the irq_chip driver to aggregate.
2. They are only called if you're using "generic-chip", which not everyone is.
3. The implementation uses syscore ops, which apparently have problems with s2idle.

Quoted 4 lines

Probably the old irq_suspend() and irq_resume() callbacks should be deprecated.

Let's introcuce a nicer API that works for all irq_chip devices. This

You grabbed my patch (which is great, thanks!) but forgot to address Stephen's early feedback from <https://crrev.com/c/2321123>.
Specifically:

s/introcuce/introduce

include/linux/irq.h static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) · line 472

Quoted 8 lines

include/linux/irq.h +2 -0

@@ -468,10 +468,16 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
   * @irq_bus_sync_unlock:function to sync and unlock slow bus (i2c) chips
   * @irq_cpu_online:    configure an interrupt source for a secondary CPU
   * @irq_cpu_offline:   un-configure an interrupt source for a secondary CPU
+ * @irq_suspend_one:   called on an every irq to suspend it; called even if
+ *                     this IRQ is configured for wakeup

s/called on an/called on

include/linux/irq.h static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) · line 474

Quoted 2 lines

include/linux/irq.h +2 -0

+ * @irq_resume_one:    called on an every irq to resume it; called even if
+ *                     this IRQ is configured for wakeup

s/called on an/called on

-Doug

From: Douglas Anderson <[email protected]>

The "struct irq_chip" has two callbacks in it: irq_suspend() and irq_resume(). These two callbacks are interesting because sometimes an irq chip needs to know about suspend/resume, but they are a bit awkward because:
1. They are called once for the whole irq_chip, not once per IRQ. It's passed data for one of the IRQs enabled on that chip. That means it's up to the irq_chip driver to aggregate.
2. They are only called if you're using "generic-chip", which not everyone is.
3. The implementation uses syscore ops, which apparently have problems with s2idle.

Probably the old irq_suspend() and irq_resume() callbacks should be deprecated.

Let's introcuce a nicer API that works for all irq_chip devices.

This will be called by the core and is called once per IRQ. The core will call the suspend callback after doing its normal suspend operations and the resume before its normal resume operations.

kernel/irq/chip.c +5 -0

+void suspend_one_irq(struct irq_desc *desc)
+{
+	struct irq_chip *chip = desc->irq_data.chip;
+
+	if (chip->irq_suspend_one)

Quoted 13 lines · Maulik Shah

From: Douglas Anderson <[email protected]>

The "struct irq_chip" has two callbacks in it: irq_suspend() and irq_resume(). These two callbacks are interesting because sometimes an irq chip needs to know about suspend/resume, but they are a bit awkward because:
1. They are called once for the whole irq_chip, not once per IRQ. It's passed data for one of the IRQs enabled on that chip. That means it's up to the irq_chip driver to aggregate.
2. They are only called if you're using "generic-chip", which not everyone is.
3. The implementation uses syscore ops, which apparently have problems with s2idle.

The main point is that these callbacks are specific to generic chip and not used anywhere else.

Quoted 2 lines

Probably the old irq_suspend() and irq_resume() callbacks should be deprecated.

You need to analyze first what these callbacks actually do. :)

Quoted 1 line

Let's introcuce a nicer API that works for all irq_chip devices.

s/Let's intro/Intro/

Let's is pretty useless in a changelog especially if you read it some time after the patch got applied.

Quoted 3 lines

This will be called by the core and is called once per IRQ. The core will call the suspend callback after doing its normal suspend operations and the resume before its normal resume operations.

Will be? You are adding the code which calls that unconditionally even.

Quoted 10 lines

kernel/irq/chip.c +10 -0

+void suspend_one_irq(struct irq_desc *desc)
+{
+     struct irq_chip *chip = desc->irq_data.chip;
+
+     if (chip->irq_suspend_one)
+             chip->irq_suspend_one(&desc->irq_data);
+}
+
+void resume_one_irq(struct irq_desc *desc)
+{

kernel/irq/chip.c void unmask_threaded_irq(struct irq_desc *desc) · line 464

Quoted 5 lines

kernel/irq/chip.c +5 -0

+     struct irq_chip *chip = desc->irq_data.chip;
+
+     if (chip->irq_resume_one)
+             chip->irq_resume_one(&desc->irq_data);
+}

There not much of a point to have these in chip.c. The functionality is clearly pm.c only.

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

Quoted 8 lines

static bool suspend_device_irq(struct irq_desc *desc) {

kernel/irq/pm.c +3 -1

+     bool sync = false;
+
      if (!desc->action || irq_desc_is_chained(desc) ||
          desc->no_suspend_depth)
-             return false;
+             goto exit;

What?

If no_suspend_depth is > 0 why would you try to tell the irq chip that this line needs to be suspended?

If there is no action, then the interrupt line is in shut down state. What's the point of suspending it?

Chained interrupts are special and you really have to think hard whether calling suspend for them unconditionally is a good idea. What if a wakeup irq is connected to this chained thing?

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

Quoted 12 lines

      if (irqd_is_wakeup_set(&desc->irq_data)) {
              irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED);

kernel/irq/pm.c +3 -1

+
              /*
               * 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().
               */
-             return true;
+             sync = true;
+             goto exit;

So again. This interrupt is a wakeup source. What's the point of suspending it unconditionally.

kernel/irq/pm.c static bool suspend_device_irq(struct irq_desc *desc) · line 105

Quoted 12 lines

      }

      desc->istate |= IRQS_SUSPENDED;

kernel/irq/pm.c +4 -1

@@ -95,7 +99,10 @@ static bool suspend_device_irq(struct irq_desc *desc)
       */
      if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND)
              mask_irq(desc);
-     return true;
+
+exit:
+     suspend_one_irq(desc);
+     return sync;

So what happens in this case:

   CPU0                         CPU1
   interrupt                    suspend_device_irq()
     handle()                     chip->suspend_one()
       action()                 ...
       chip->fiddle();

????

What is the logic here and how is this going to work under all circumstances without having magic hacks in the irq chip to handle all the corner cases?

This needs way more thoughts vs. the various states and sync requirements. Just adding callbacks, invoking them unconditionally, not giving any rationale how the whole thing is supposed to work and then let everyone figure out how to deal with the state and corner case handling at the irq chip driver level does not cut it, really.

State handling is core functionality and if irq chip drivers have special requirements then they want to be communicated with flags and/or specialized callbacks.

Quoted 2 lines · Thomas Gleixner

The main point is that these callbacks are specific to generic chip and not used anywhere else.

I'm not sure I understand. This callback is used by drivers that use generic-chip but I don't think there's anything specific about generic-chip in these callbacks. Sure many of them use the generic-chip's "wake_active" tracking but a different IRQ chip could track "wake_active" itself without bringing in all of generic-chip and still might want to accomplish the same thing, right?

Quoted 4 lines

Quoted 2 lines

Probably the old irq_suspend() and irq_resume() callbacks should be deprecated.

You need to analyze first what these callbacks actually do. :)

See below. I intended my callbacks to be for the same type of thing as the existing ones, though perhaps either my naming or description was confusing.

Quoted 14 lines

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

Quoted 8 lines

static bool suspend_device_irq(struct irq_desc *desc) {

kernel/irq/pm.c +3 -1

+     bool sync = false;
+
      if (!desc->action || irq_desc_is_chained(desc) ||
          desc->no_suspend_depth)
-             return false;
+             goto exit;

What?

If no_suspend_depth is > 0 why would you try to tell the irq chip that this line needs to be suspended?

Quoted 6 lines

If there is no action, then the interrupt line is in shut down state. What's the point of suspending it?

Chained interrupts are special and you really have to think hard whether calling suspend for them unconditionally is a good idea. What if a wakeup irq is connected to this chained thing?

I think there is a confusion about what this callback is intended to do and that probably needs to be made clearer, either by renaming or by comments (or both). Let's think about these two things that we might be telling the IRQ:

a) Please disable yourself in preparation for suspending.

b) The system is suspending, please take any action you need to.

I believe you are reading this as a). I intended it to be b). Can you think of a name for these callbacks that would make it clearer? suspend_notify() / resume_notify() maybe?

Specifically the problem we're trying to address is when an IRQ is marked as "disabled" (driver called disable_irq()) but also marked as "wakeup" (driver called enable_irq_wake()). As per my understanding, this means:

* Don't call the interrupt handler for this interrupt until I call enable_irq() but keep tracking it (either in hardware or in software). Specifically it's a requirement that if the interrupt fires one or more times while masked the interrupt handler should be called as soon as enable_irq() is called.

* If this interrupt fires while the system is suspended then please wake the system up.

On some (many?) interrupt controllers a masked interrupt won't wake the system up. Thus we need some point in time where the interrupt controller can unmask interrupts in hardware so that they can act as wakeups.

Also: if an interrupt was masked lazily this could be a good time to ensure that these interrupts _won't_ wake the system up.

Thus the point of these callbacks is to provide a hook for IRQ chips to do this. Now that you understand the motivation perhaps you can suggest a better way to accomplish this if the approach in this patch is not OK.

I will note that a quick audit of existing users of the gernic-chip's irq_suspend() show that they are doing exactly this. So the point of my patch is to actually allow other IRQ chips (ones that aren't using generic-chip) to do this type of thing. At the same time my patch provides a way for current users of generic-chip to adapt their routines so they work without syscore (which, I guess, isn't compatible with s2idle).

Quoted 8 lines

So what happens in this case:

   CPU0                         CPU1
   interrupt                    suspend_device_irq()
     handle()                     chip->suspend_one()
       action()                 ...
       chip->fiddle();

Quoted 1 line

????

Ah, so I guess we need to move the call to suspend_one_irq() till after the (potential) call to synchronize_irq() in in suspend_device_irqs()?

Hopefully with the above explanation this makes more sense?

Quoted 11 lines

Specifically the problem we're trying to address is when an IRQ is marked as "disabled" (driver called disable_irq()) but also marked as "wakeup" (driver called enable_irq_wake()). As per my understanding, this means:

* Don't call the interrupt handler for this interrupt until I call enable_irq() but keep tracking it (either in hardware or in software). Specifically it's a requirement that if the interrupt fires one or more times while masked the interrupt handler should be called as soon as enable_irq() is called.

irq_disable() has two operating modes:

1) Immediately mask the interrupt at the irq chip level

2) Software disable it. If an interrupt is raised while disabled then the flow handler observes disabled state, masks it, marks it pending and returns without invoking any device handler.

On a subsequent irq_enable() the interrupt is unmasked if it was masked and if the interrupt is marked pending and the interrupt is not level type then it's attempted to retrigger it. Either in hardware or by a software replay mechanism.

Quoted 2 lines

* If this interrupt fires while the system is suspended then please wake the system up.

Well, that's kinda contradicting itself. If the interrupt is masked then what is the point? I'm surely missing something subtle here.

Quoted 4 lines

On some (many?) interrupt controllers a masked interrupt won't wake the system up. Thus we need some point in time where the interrupt controller can unmask interrupts in hardware so that they can act as wakeups.

So far nobody told me about this until now, but why exactly do we need yet another unspecified callback instead of simply telling the core via an irq chip flag that it should always unmask the interrupt if it is a wakeup source?

Quoted 2 lines

Also: if an interrupt was masked lazily this could be a good time to ensure that these interrupts _won't_ wake the system up.

Setting IRQCHIP_MASK_ON_SUSPEND does exactly that. No need for a chip driver to do any magic. You just have to use it.

So the really obvious counterpart for this is to have:

IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND

and then do:

kernel/irq/pm.c +2 -0

@@ -81,6 +81,8 @@ static bool suspend_device_irq(struct ir
                 * IRQD_WAKEUP_ARMED is visible before we return from
                 * suspend_device_irqs().
                 */
+               if (chip->flags & IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND)
+                       unmask_irq(desc);
                return true;
        }
 

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

The magic behind the back of the core code unmask brings core state and hardware state out of sync. So if for whatever reason the interrupt is raised in the CPU before the resume path can mask it again, then the flow handler will see disabled state, invoke mask_irq() which does nothing because core state is masked and if that's a level irq it will come back forever.

Quoted 4 lines

Thus the point of these callbacks is to provide a hook for IRQ chips to do this. Now that you understand the motivation perhaps you can suggest a better way to accomplish this if the approach in this patch is not OK.

See above.

Quoted 7 lines

I will note that a quick audit of existing users of the gernic-chip's irq_suspend() show that they are doing exactly this. So the point of my patch is to actually allow other IRQ chips (ones that aren't using generic-chip) to do this type of thing. At the same time my patch provides a way for current users of generic-chip to adapt their routines so they work without syscore (which, I guess, isn't compatible with s2idle).

If that's the main problem which is solved in these callbacks, then I really have to ask why this has not been raised years ago. Why can't people talk?

IIRC back then when the callbacks for GC were added the reason was that the affected chips needed a way to save and restore the full chip state because the hardware lost it during suspend. S2idle did not exist back then at least not in it's current form. Oh well...

But gust replacing them by something which is yet another sinkhole for horrible hacks behind the core code is not making it any better.

I fear another sweep through the unpleasantries of chip drivers is due sooner than later. Aside of finding time, I need to find my eyecancer protection glasses and check my schnaps stock.

Quoted 8 lines

Quoted 8 lines

So what happens in this case:

   CPU0                         CPU1
   interrupt                    suspend_device_irq()
     handle()                     chip->suspend_one()
       action()                 ...
       chip->fiddle();

Quoted 5 lines

Quoted 1 line

????

Ah, so I guess we need to move the call to suspend_one_irq() till after the (potential) call to synchronize_irq() in in suspend_device_irqs()?

For what you are trying to achieve, no. IRQCHIP_MASK_ON_SUSPEND is already safe.

If we add IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND then there is no sync problem either.

Quoted 1 line

Hopefully with the above explanation this makes more sense?

At least the explanation helped to understand the problem, while the changelog was pretty useless in that regard:

"These two callbacks are interesting because sometimes an irq chip
needs to know about suspend/resume."

Really valuable and precise technical information.

But aside of the confusion, even with your explanation of what you are trying to solve, I really want a coherent explanation why this should be done for any of those:

1) an interrupt which has no action, i.e. an interrupt which has no active users and is in the worst case completely deactivated or was never activated to begin with.

In the inactive case it might be in a state where unmask issues an invalid vector, causes hardware malfunction or hits undefined software state in the chip drivers in the hierarchy.

If you want to be woken up by irq X, then request irq X which ensures that irq X is in a usable state at all levels of the stack. If you call disable_irq() or mark the interrupt with IRQ_NOAUTOEN, fine, it's still consistent state.

2) interrupts which have no_suspend_depth > 0 which means that there is an action requested which explicitely says: don't touch me on suspend.

If that driver invokes disable_irq() then it can keep the pieces.

3) chained interrupts

They are never disabled and never masked. So why would anything need to be done here?

Side note: they should not exist at all, but that's a different story.

If you don't have coherent explanations, then please just don't touch that condition at all.

Hint: "Sometimes a chip needs to know" does not qualify :)

Quoted 3 lines · Thomas Gleixner

Quoted 2 lines

* If this interrupt fires while the system is suspended then please wake the system up.

Quoted 2 lines

Well, that's kinda contradicting itself. If the interrupt is masked then what is the point? I'm surely missing something subtle here.

This is how I've always been told that the API works and there are at least a handful of drivers in the kernel whose suspend routines both enable wakeup and call disable_irq(). Isn't this also documented as of commit f9f21cea3113 ("genirq: Clarify that irq wake state is orthogonal to enable/disable")?

Quoted 3 lines

If that's the main problem which is solved in these callbacks, then I really have to ask why this has not been raised years ago. Why can't people talk?

Not all of us have the big picture that you do to know how things ought to work, I guess. If nothing else someone looking at this problem would think: "this must be a common problem, let's go see how all the other places do it" and then they find how everyone else is doing it and do it that way. It requires the grander picture that a maintainer has in order to say: whoa, everyone's copying the same hack--let's come up with a better solution.

Quoted 4 lines

"These two callbacks are interesting because sometimes an irq chip
needs to know about suspend/resume."

Really valuable and precise technical information.

Funny to get yelled at for not providing a detailed enough changelog. Usually people complain that my changelogs are too detailed. Sigh.

Quoted 1 line

Hint: "Sometimes a chip needs to know" does not qualify :)

Clearly I am not coherent. ;-) My only goal was to help enable interrupts that were disabled / marked as wakeup (as per above, documented to be OK) to work on Qualcomm chips. This specifically affects me because a driver that I need to work (cros_ec) does this.

If IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND is good to add then it sounds like a great plan to me.

Doug,

Quoted 4 lines · Doug Anderson

Quoted 3 lines · Thomas Gleixner

Quoted 2 lines

* If this interrupt fires while the system is suspended then please wake the system up.

Quoted 8 lines

Quoted 2 lines

Well, that's kinda contradicting itself. If the interrupt is masked then what is the point? I'm surely missing something subtle here.

This is how I've always been told that the API works and there are at least a handful of drivers in the kernel whose suspend routines both enable wakeup and call disable_irq(). Isn't this also documented as of commit f9f21cea3113 ("genirq: Clarify that irq wake state is orthogonal to enable/disable")?

Fair enough. The wording there is unfortunate and I probably should have spent more brain cycles before applying it. It suggests that this is a pure driver problem. I should have asked some of the questions I asked now back then :(

Quoted 11 lines

Quoted 3 lines

If that's the main problem which is solved in these callbacks, then I really have to ask why this has not been raised years ago. Why can't people talk?

Not all of us have the big picture that you do to know how things ought to work, I guess. If nothing else someone looking at this problem would think: "this must be a common problem, let's go see how all the other places do it" and then they find how everyone else is doing it and do it that way. It requires the grander picture that a maintainer has in order to say: whoa, everyone's copying the same hack--let's come up with a better solution.

That's not the point. I know how these things happen, but I fail to understand why nobody ever looks at this and says: OMG, I need to do yet another variant of copy&pasta of the same thing every other driver does. Why is there no infrastructure for that?

Asking that question does not require a maintainer who always encouraged people to talk about exactly these kind of things instead of going off and creating the gazillionst broken copy of the same thing with yet another wart working around core code problems and thereby violating layering and introducing bugs which wouldn't exist otherwise.

Spare me all the $corp reasons. I've heard all of them and if not then the not yet known reason won't be any more convincing. :)

One of the most underutilized strengths of FOSS is that you can go and ask someone who has the big picture in his head before you go off and waste time on distangling copy&pasta, dealing with the resulting obvious bugs and then the latent ones which only surface 3 month after the product has shipped. Or like in this case figure out that the copy&pasta road is a dead end and then create something new without seeing the big picture and having analyzed completely what consequences this might have.

I don't know how much hours you and others spent on this. I surely know that after you gave me proper context it took me less than an hour to figure out that one problem you were trying to solve was already solved and the other one was just a matter of doing the counterpart of it. I definitely spent way more time on reviewing and debating.

So if you had asked upfront, I probably would have spent quite some time on it as well depending on the quality of the question and explanation but the total amount on both sides would have been significantly lower, which I consider a win-win situation.

Of course I know that my $corp MBA foo is close to zero, so I just can be sure that it would have been a win for me :)

Seriously, we need to promote a 'talk to each other' culture very actively. The people with the big picture in their head, aka maintainers, are happy to answer questions and they also want that others come forth and say "this makes no sense" instead of silently accepting that the five other drivers do something stupid. This would help to solve some of the well known problems:

- Maintainer scalability

I rather discuss a problem with you at the conceptual level upfront instead of reviewing patches after the fact and having to tell you that it's all wrong. The latter takes way more time.

Having a quick and dirty POC for illustration is fine and usually useful.

- Maintainer blinders

Maintainers need input from the outside as any other people because they become blind to shortcomings in the area they are responsible for as any other person. Especially if they maintain complex and highly active subsystems.

- Submitter frustration

You spent a huge amount of time to come up with a solution for something and then you get told by the maintainer/reviewer that the time spent was wasted and your solution is crap. It does not matter much what the politeness level of that message is. It sets you back and causes frustration on both ends.

- Turn around times

A lot of time can be spared by talking to each other early. A half baken POC patch is fine for opening such a discussion, but going down all the way and then having the talk over the final patch review is more than suboptimal and causes grief on both sides.

Quoted 7 lines

Quoted 4 lines

"These two callbacks are interesting because sometimes an irq chip
needs to know about suspend/resume."

Really valuable and precise technical information.

Funny to get yelled at for not providing a detailed enough changelog. Usually people complain that my changelogs are too detailed. Sigh.

The complaint you might get from me about an overly detailed changelog is that it has redundant or pointless information in it, e.g.

- the 500 lines of debug dump containing about 10 lines of valuable information which you already decoded and condensed in order to figure the problem out.

- anecdotes around the discovery which carry zero information and often show that that the scope of the problem was not fully understood.

- pointless examples of how to trigger the fail

- In depth explanaations of what the patch does instead of a concise explanation at the conceptual level.

You won't hear me complain about a concise and coherent in depth technical explanation of a problem.

Writing changelogs is an art and I surely look at some of my own changelogs written long ago and yell at myself from time to time.

Reading a patch goes top down obviously:

1) Subject line
2) Changelog
3) Patch.

If I have to rumage for my crystal ball before #3 then I already spent more time than necessary. If the thing is some random feature then I might just say: try again. But if I get the sense that it is about a bug or has some smell of a shorrcoming in the core code then I have to bite the bullet and decode it the hard way. Not the most efficient way. And from experience I can tell you that if #1 and #2 are already problematic then #3 needs some serious scrutiny in most cases.

Quoted 6 lines

Quoted 1 line

Hint: "Sometimes a chip needs to know" does not qualify :)

Clearly I am not coherent. ;-) My only goal was to help enable interrupts that were disabled / marked as wakeup (as per above, documented to be OK) to work on Qualcomm chips. This specifically affects me because a driver that I need to work (cros_ec) does this.

Mission acoomplished :)

Quoted 2 lines

If IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND is good to add then it sounds like a great plan to me.

If it solves the problem and from what you explained it should do so then this is definitely the right way to go.

Quoted 2 lines · Thomas Gleixner

Having a quick and dirty POC for illustration is fine and usually useful.

OK, I will try to remember that, in the future, I should send questions rather than patches to you. I'm always learning the

workflows of the different maintainers, so sorry for killing so much time. :(

Quoted 2 lines

If it solves the problem and from what you explained it should do so then this is definitely the right way to go.

Wonderful! Looking forward to Maulik's post doing it this way.

Hi,

Quoted 23 lines · Thomas Gleixner

Quoted 10 lines

Specifically the problem we're trying to address is when an IRQ is marked as "disabled" (driver called disable_irq()) but also marked as "wakeup" (driver called enable_irq_wake()). As per my understanding, this means:

* Don't call the interrupt handler for this interrupt until I call enable_irq() but keep tracking it (either in hardware or in software). Specifically it's a requirement that if the interrupt fires one or more times while masked the interrupt handler should be called as soon as enable_irq() is called.

irq_disable() has two operating modes:

1) Immediately mask the interrupt at the irq chip level

2) Software disable it. If an interrupt is raised while disabled then the flow handler observes disabled state, masks it, marks it pending and returns without invoking any device handler.

On a subsequent irq_enable() the interrupt is unmasked if it was masked and if the interrupt is marked pending and the interrupt is not level type then it's attempted to retrigger it. Either in hardware or by a software replay mechanism.

Quoted 4 lines

Quoted 2 lines

* If this interrupt fires while the system is suspended then please wake the system up.

Well, that's kinda contradicting itself. If the interrupt is masked then what is the point? I'm surely missing something subtle here.

This is how I've always been told that the API works and there are at least a handful of drivers in the kernel whose suspend routines both enable wakeup and call disable_irq(). Isn't this also documented as of commit f9f21cea3113 ("genirq: Clarify that irq wake state is orthogonal to enable/disable")?

Quoted 29 lines

Quoted 4 lines

On some (many?) interrupt controllers a masked interrupt won't wake the system up. Thus we need some point in time where the interrupt controller can unmask interrupts in hardware so that they can act as wakeups.

So far nobody told me about this until now, but why exactly do we need yet another unspecified callback instead of simply telling the core via an irq chip flag that it should always unmask the interrupt if it is a wakeup source?

Quoted 2 lines

Also: if an interrupt was masked lazily this could be a good time to ensure that these interrupts _won't_ wake the system up.

Setting IRQCHIP_MASK_ON_SUSPEND does exactly that. No need for a chip driver to do any magic. You just have to use it.

So the really obvious counterpart for this is to have:

IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND

and then do:

kernel/irq/pm.c +2 -0

@@ -81,6 +81,8 @@ static bool suspend_device_irq(struct ir
                  * IRQD_WAKEUP_ARMED is visible before we return from
                  * suspend_device_irqs().
                  */
+               if (chip->flags & IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND)
+                       unmask_irq(desc);
                 return true;
         }
 

Quoted 2 lines

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

This sounds wonderful to me. Maulik: I think you could replace quite a few of the patches in the series and just use that.

Quoted 22 lines

The magic behind the back of the core code unmask brings core state and hardware state out of sync. So if for whatever reason the interrupt is raised in the CPU before the resume path can mask it again, then the flow handler will see disabled state, invoke mask_irq() which does nothing because core state is masked and if that's a level irq it will come back forever.

Quoted 4 lines

Thus the point of these callbacks is to provide a hook for IRQ chips to do this. Now that you understand the motivation perhaps you can suggest a better way to accomplish this if the approach in this patch is not OK.

See above.

Quoted 7 lines

I will note that a quick audit of existing users of the gernic-chip's irq_suspend() show that they are doing exactly this. So the point of my patch is to actually allow other IRQ chips (ones that aren't using generic-chip) to do this type of thing. At the same time my patch provides a way for current users of generic-chip to adapt their routines so they work without syscore (which, I guess, isn't compatible with s2idle).

Quoted 3 lines

If that's the main problem which is solved in these callbacks, then I really have to ask why this has not been raised years ago. Why can't people talk?

Not all of us have the big picture that you do to know how things ought to work, I guess. If nothing else someone looking at this problem would think: "this must be a common problem, let's go see how all the other places do it" and then they find how everyone else is doing it and do it that way. It requires the grander picture that a maintainer has in order to say: whoa, everyone's copying the same hack--let's come up with a better solution.

Quoted 30 lines

IIRC back then when the callbacks for GC were added the reason was that the affected chips needed a way to save and restore the full chip state because the hardware lost it during suspend. S2idle did not exist back then at least not in it's current form. Oh well...

But gust replacing them by something which is yet another sinkhole for horrible hacks behind the core code is not making it any better.

I fear another sweep through the unpleasantries of chip drivers is due sooner than later. Aside of finding time, I need to find my eyecancer protection glasses and check my schnaps stock.

Quoted 8 lines

Quoted 8 lines

So what happens in this case:

    CPU0                         CPU1
    interrupt                    suspend_device_irq()
      handle()                     chip->suspend_one()
        action()                 ...
        chip->fiddle();

Quoted 4 lines

Quoted 1 line

????

Ah, so I guess we need to move the call to suspend_one_irq() till after the (potential) call to synchronize_irq() in in suspend_device_irqs()?

For what you are trying to achieve, no. IRQCHIP_MASK_ON_SUSPEND is already safe.

If we add IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND then there is no sync problem either.

Quoted 8 lines

Quoted 1 line

Hopefully with the above explanation this makes more sense?

At least the explanation helped to understand the problem, while the changelog was pretty useless in that regard:

"These two callbacks are interesting because sometimes an irq chip
needs to know about suspend/resume."

Really valuable and precise technical information.

Funny to get yelled at for not providing a detailed enough changelog. Usually people complain that my changelogs are too detailed. Sigh.

Quoted 28 lines

But aside of the confusion, even with your explanation of what you are trying to solve, I really want a coherent explanation why this should be done for any of those:

1) an interrupt which has no action, i.e. an interrupt which has no active users and is in the worst case completely deactivated or was never activated to begin with.

In the inactive case it might be in a state where unmask issues an invalid vector, causes hardware malfunction or hits undefined software state in the chip drivers in the hierarchy.

If you want to be woken up by irq X, then request irq X which ensures that irq X is in a usable state at all levels of the stack. If you call disable_irq() or mark the interrupt with IRQ_NOAUTOEN, fine, it's still consistent state.

2) interrupts which have no_suspend_depth > 0 which means that there is an action requested which explicitely says: don't touch me on suspend.

If that driver invokes disable_irq() then it can keep the pieces.

3) chained interrupts

They are never disabled and never masked. So why would anything need to be done here?

Quoted 7 lines

Side note: they should not exist at all, but that's a different story.

If you don't have coherent explanations, then please just don't touch that condition at all.

Hint: "Sometimes a chip needs to know" does not qualify :)

Clearly I am not coherent. ;-) My only goal was to help enable interrupts that were disabled / marked as wakeup (as per above, documented to be OK) to work on Qualcomm chips. This specifically affects me because a driver that I need to work (cros_ec) does this. If IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND is good to add then it sounds like a great plan to me.

-Doug

Quoted 1 line · Doug Anderson

On Thu, Aug 13, 2020 at 3:09 PM Thomas Gleixner <[email protected]> wrote:

(quoted diff) +2 -0

+               if (chip->flags & IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND)
+                       unmask_irq(desc);

I tried this patch and it didnot work as is.

Calling unmask_irq() only invoke's chip's .irq_unmask callback but the underlying irq_chip have .irq_enable also present.

Replacing the call with irq_enable() internally takes care of either invoking chip's .irq_enable (if its present) else it invokes unmask_irq().

+
+               if (chip->flags & IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND) +                       irq_enable(desc);

probably IRQCHIP_UNMASK_WAKEUP_ON_SUSPEND should also be renamed to IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND.

From: Douglas Anderson <[email protected]>

This goes with the new irq_suspend_one() and irq_resume_one() callbacks and allow us to easily pass things up to our parent.

Signed-off-by Douglas Anderson <[email protected]>

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

---
 include/linux/irq.h |  2 ++
 kernel/irq/chip.c   | 28 ++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

Hi,

Quoted 11 lines · Maulik Shah

From: Douglas Anderson <[email protected]>

This goes with the new irq_suspend_one() and irq_resume_one() callbacks and allow us to easily pass things up to our parent.

Signed-off-by Douglas Anderson <[email protected]>

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

---
  include/linux/irq.h |  2 ++
  kernel/irq/chip.c   | 28 ++++++++++++++++++++++++++++
  2 files changed, 30 insertions(+)

Thanks for posting my patch. Small nit here is that when I saw the patches listed together I realized that I forgot to capitalize "introduce" in ${SUBJECT}. The two patches right next to each other that both start with "introduce" where one has a capital and one doesn't look weird. Hopefully you can fix in the next version?

Thanks!

-Doug

Clear previous kernel's configuration during init by resetting interrupts in enable bank to zero.

Suggested-by Stephen Boyd <[email protected]>

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

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

drivers/irqchip/qcom-pdc.c +2 -1

index dfcdfc5..80e0dfb 100644
@@ -389,7 +389,8 @@ static const struct irq_domain_ops qcom_pdc_gpio_ops = {
 
 static int pdc_setup_pin_mapping(struct device_node *np)
 {
-	int ret, n;
+	int ret, n, i;
+	u32 irq_index, reg_index, val;
 
 	n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
 	if (n <= 0 || n % 3)

Quoted 24 lines · Maulik Shah

Clear previous kernel's configuration during init by resetting interrupts in enable bank to zero.

Suggested-by Stephen Boyd <[email protected]>

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

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

drivers/irqchip/qcom-pdc.c +2 -1

index dfcdfc5..80e0dfb 100644
@@ -389,7 +389,8 @@ static const struct irq_domain_ops qcom_pdc_gpio_ops = {
 
  static int pdc_setup_pin_mapping(struct device_node *np)
  {
-	int ret, n;
+	int ret, n, i;
+	u32 irq_index, reg_index, val;
 
  	n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
  	if (n <= 0 || n % 3)

drivers/irqchip/qcom-pdc.c static int pdc_setup_pin_mapping(struct device_node *np) · line 424

Quoted 6 lines

drivers/irqchip/qcom-pdc.c +3 -0

@@ -418,6 +419,15 @@ static int pdc_setup_pin_mapping(struct device_node *np)
  						 &pdc_region[n].cnt);
  		if (ret)
  			return ret;
+
+		for (i = pdc_region[n].pin_base; i < pdc_region[n].pin_base +
+						 pdc_region[n].cnt; i++) {

how about making the for loop slightly easier to read by moving pin_base inside the loop?

	for (i = 0; i < pdc_region[n].cnt; i++) {
         	reg_index = (i + pdc_region[n].pin_base) >> 5;
         	irq_index = (i + pdc_region[n].pin_base) & 0x1f;

		[...]
         }

Clear previous kernel's configuration during init by resetting interrupts in enable bank to zero.

Suggested-by Stephen Boyd <[email protected]>

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

Quoting Maulik Shah (2020-08-10 04:21:00)

Quoted 2 lines

Clear previous kernel's configuration during init by resetting interrupts in enable bank to zero.

Can you please add some more information here about why we're not clearing all the pdc irqs and only the ones that are listed in DT?

Is
that because the pdc is shared between exception levels of the CPU and so some irqs shouldn't be used? Does the DT binding need to change to only list the hwirqs that are usable by the OS instead of the ones that are usable for the entire system? The binding doesn't mention this at all so I am just guessing here.

Quoted 3 lines

Suggested-by Stephen Boyd <[email protected]>

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

Hi,

Quoted 5 lines · Stephen Boyd

Quoting Maulik Shah (2020-08-10 04:21:00)

Quoted 2 lines

Clear previous kernel's configuration during init by resetting interrupts in enable bank to zero.

Can you please add some more information here about why we're not clearing all the pdc irqs and only the ones that are listed in DT?

sure.

Quoted 6 lines

Is
that because the pdc is shared between exception levels of the CPU and so some irqs shouldn't be used? Does the DT binding need to change to only list the hwirqs that are usable by the OS instead of the ones that are usable for the entire system? The binding doesn't mention this at all so I am just guessing here.

The IRQs specified in qcom,pdc-ranges property in DT are the only ones that can be used in the current OS for the PDC.

So instead of setting entire register to zero (each reg supports 32 interrupts enable bit) only clearing the ones that can be used.