Message-ID: <155329862018.20095.14024385652448015564@swboyd.mtv.corp.google.com>
Date:   Fri, 22 Mar 2019 16:50:20 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-gpio@...r.kernel.org, Lina Iyer <ilina@...eaurora.org>,
        Marc Zyngier <marc.zyngier@....com>
Subject: Re: [PATCH] genirq: Respect IRQCHIP_SKIP_SET_WAKE in irq_chip_set_wake_parent()

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?

Powered by blists - more mailing lists