| Message ID | 20190315200759.139479-1-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | genirq: Respect IRQCHIP_SKIP_SET_WAKE in irq_chip_set_wake_parent() | expand |
On Fri, 15 Mar 2019, Stephen Boyd wrote: > This function returns an error if a child interrupt controller calls > irq_chip_set_wake_parent() but that parent interrupt controller has the > IRQCHIP_SKIP_SET_WAKE flag. Let's return 0 for success instead because > there isn't anything to do. > > There's also the possibility that a parent indicates that we should skip > it, but the grandparent has an .irq_set_wake callback. Let's iterate > through the parent chain as long as the IRQCHIP_SKIP_SET_WAKE flag isn't > set so we can find the first parent that needs to handle the wake > configuration. This fixes a problem on my Qualcomm sdm845 device where > I'm trying to enable wake on an irq from the gpio controller that's a > child of the qcom pdc interrupt controller. The qcom pdc interrupt > controller has the IRQCHIP_SKIP_SET_WAKE flag set, and so does the > grandparent (ARM GIC), causing this function to return a failure because > the parent controller doesn't have the .irq_set_wake callback set. It took me some time to distangle that changelog.... and I don't think that this is the right thing to do. set_irq_wake_real() returns 0 when the topmost chip has SKIP_SET_WAKE set. So let's assume we have the following chains: chip A -> chip B chip A -> chip B -> chip C chip A has SKIP_SET_WAKE not set chip B has SKIP_SET_WAKE set chip C has SKIP_SET_WAKE not set and invokes irq_chip_set_wake_parent() Now assume we have interrupt X connected to chip B and interrupt Y connected to chip C. If irq_set_wake() is called for interrupt X, then the function returns without trying to invoke the set_wake() callback of chip A. If irq_set_wake() is called for interrupt Y, irq_chip_set_wake_parent() is invoked from chip C which then skips chip B, but tries to invoke the callback on chip A. That's inconsistent and changes the existing behaviour. So IMO, the right thing to do is to return 0 from irq_chip_set_wake_parent() when the parent has SKIP_SET_WAKE set and not to try to follow the whole chain. That should fix your problem nicely w/o changing behaviour. Thanks, tglx ---- diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index 3faef4a77f71..51128bea3846 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -1449,6 +1449,10 @@ int irq_chip_set_vcpu_affinity_parent(struct irq_data *data, void *vcpu_info) int irq_chip_set_wake_parent(struct irq_data *data, unsigned int on) { data = data->parent_data; + + if (data->chip->flags & IRQCHIP_SKIP_SET_WAKE) + return 0; + if (data->chip->irq_set_wake) return data->chip->irq_set_wake(data, on);
Quoting Thomas Gleixner (2019-03-21 02:26:26) > On Fri, 15 Mar 2019, Stephen Boyd wrote: > > > This function returns an error if a child interrupt controller calls > > irq_chip_set_wake_parent() but that parent interrupt controller has the > > IRQCHIP_SKIP_SET_WAKE flag. Let's return 0 for success instead because > > there isn't anything to do. > > > > There's also the possibility that a parent indicates that we should skip > > it, but the grandparent has an .irq_set_wake callback. Let's iterate > > through the parent chain as long as the IRQCHIP_SKIP_SET_WAKE flag isn't > > set so we can find the first parent that needs to handle the wake > > configuration. This fixes a problem on my Qualcomm sdm845 device where > > I'm trying to enable wake on an irq from the gpio controller that's a > > child of the qcom pdc interrupt controller. The qcom pdc interrupt > > controller has the IRQCHIP_SKIP_SET_WAKE flag set, and so does the > > grandparent (ARM GIC), causing this function to return a failure because > > the parent controller doesn't have the .irq_set_wake callback set. > > It took me some time to distangle that changelog.... and I don't think that > this is the right thing to do. Yes, your diagram would be a useful addition to the commit text. > > set_irq_wake_real() returns 0 when the topmost chip has SKIP_SET_WAKE set. Just to confirm, the topmost chip would be chip B or chip C below? > > So let's assume we have the following chains: > > chip A -> chip B > > chip A -> chip B -> chip C > > chip A has SKIP_SET_WAKE not set > chip B has SKIP_SET_WAKE set > chip C has SKIP_SET_WAKE not set and invokes irq_chip_set_wake_parent() > > Now assume we have interrupt X connected to chip B and interrupt Y > connected to chip C. > > If irq_set_wake() is called for interrupt X, then the function returns > without trying to invoke the set_wake() callback of chip A. It's not clear to me that having SKIP_SET_WAKE set means "completely ignore set wake for irqs from this domain" vs. "skip setting wake here because the .irq_set_wake() is intentionally omitted for this chip". Reading Santosh's reasoning in commit 60f96b41f71d ("genirq: Add IRQCHIP_SKIP_SET_WAKE flag") just further confuses me because it sounds like the latter. > > If irq_set_wake() is called for interrupt Y, irq_chip_set_wake_parent() is > invoked from chip C which then skips chip B, but tries to invoke the > callback on chip A. > > That's inconsistent and changes the existing behaviour. So IMO, the right > thing to do is to return 0 from irq_chip_set_wake_parent() when the parent > has SKIP_SET_WAKE set and not to try to follow the whole chain. That should > fix your problem nicely w/o changing behaviour. Ok. I understand that with hierarchical chips you want it to be explicit in the code that a parent chip needs to be called or not. This works for me, and it's actually how I had originally solved this problem. Will you merge your patch or do you want me to resend it with some updated commit text?
Stephen, On Fri, 22 Mar 2019, Stephen Boyd wrote: > Quoting Thomas Gleixner (2019-03-21 02:26:26) > > > > set_irq_wake_real() returns 0 when the topmost chip has SKIP_SET_WAKE set. > > Just to confirm, the topmost chip would be chip B or chip C below? Yes. A is the parent of B, resp. the grandparent of C > > > > So let's assume we have the following chains: > > > > chip A -> chip B > > > > chip A -> chip B -> chip C > > > > chip A has SKIP_SET_WAKE not set > > chip B has SKIP_SET_WAKE set > > chip C has SKIP_SET_WAKE not set and invokes irq_chip_set_wake_parent() > > > > Now assume we have interrupt X connected to chip B and interrupt Y > > connected to chip C. > > > > If irq_set_wake() is called for interrupt X, then the function returns > > without trying to invoke the set_wake() callback of chip A. > > It's not clear to me that having SKIP_SET_WAKE set means "completely > ignore set wake for irqs from this domain" vs. "skip setting wake here > because the .irq_set_wake() is intentionally omitted for this chip". That's a really good question, but I'd say that if one part of the hierarchy does not require set wake, then this means no further action required. > Reading Santosh's reasoning in commit 60f96b41f71d ("genirq: Add > IRQCHIP_SKIP_SET_WAKE flag") just further confuses me because it sounds > like the latter. That preceeds hierarchical irq domains. So back then a hierarchy was expressed by weird callbacks, hardcoded dependencies, etc. That means if the top level chip had SKIP_SET_WAKE set, the whole hierarchy was excluded. > > If irq_set_wake() is called for interrupt Y, irq_chip_set_wake_parent() is > > invoked from chip C which then skips chip B, but tries to invoke the > > callback on chip A. > > > > That's inconsistent and changes the existing behaviour. So IMO, the right > > thing to do is to return 0 from irq_chip_set_wake_parent() when the parent > > has SKIP_SET_WAKE set and not to try to follow the whole chain. That should > > fix your problem nicely w/o changing behaviour. > > Ok. I understand that with hierarchical chips you want it to be explicit > in the code that a parent chip needs to be called or not. This works for I think so. > me, and it's actually how I had originally solved this problem. Will you > merge your patch or do you want me to resend it with some updated commit > text? Please send a new version. Thanks, tglx
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index 3faef4a77f71..280d612ba71b 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -1448,7 +1448,13 @@ int irq_chip_set_vcpu_affinity_parent(struct irq_data *data, void *vcpu_info) */ int irq_chip_set_wake_parent(struct irq_data *data, unsigned int on) { - data = data->parent_data; + for (data = data->parent_data; data; data = data->parent_data) + if (!(data->chip->flags & IRQCHIP_SKIP_SET_WAKE)) + break; + + if (!data) + return 0; + if (data->chip->irq_set_wake) return data->chip->irq_set_wake(data, on);
This function returns an error if a child interrupt controller calls irq_chip_set_wake_parent() but that parent interrupt controller has the IRQCHIP_SKIP_SET_WAKE flag. Let's return 0 for success instead because there isn't anything to do. There's also the possibility that a parent indicates that we should skip it, but the grandparent has an .irq_set_wake callback. Let's iterate through the parent chain as long as the IRQCHIP_SKIP_SET_WAKE flag isn't set so we can find the first parent that needs to handle the wake configuration. This fixes a problem on my Qualcomm sdm845 device where I'm trying to enable wake on an irq from the gpio controller that's a child of the qcom pdc interrupt controller. The qcom pdc interrupt controller has the IRQCHIP_SKIP_SET_WAKE flag set, and so does the grandparent (ARM GIC), causing this function to return a failure because the parent controller doesn't have the .irq_set_wake callback set. Cc: Lina Iyer <ilina@codeaurora.org> Cc: Marc Zyngier <marc.zyngier@arm.com> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- kernel/irq/chip.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)