Re: [PATCH v2] pinctrl: qcom: Assign irq_disable/eoi conditionally

From: Linus Walleij <linus.walleij@linaro.org>
To: Marc Zyngier <maz@kernel.org>
Cc: "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Lina Iyer <ilina@codeaurora.org>,
	Stephen Boyd <swboyd@chromium.org>,
	stable <stable@vger.kernel.org>
Subject: Re: [PATCH v2] pinctrl: qcom: Assign irq_disable/eoi conditionally
Date: Mon, 9 Mar 2020 16:03:26 +0100	[thread overview]
Message-ID: <CACRpkdamdgMh-z6AHqEptAw_o9JtCu1-RXDVWkqVJsoQTpc2NQ@mail.gmail.com> (raw)
In-Reply-To: <1be9151d00160ef26a3900e0e6a5fd14@kernel.org>

On Mon, Mar 9, 2020 at 3:54 PM Marc Zyngier <maz@kernel.org> wrote:

> On 2020-03-09 12:52, Linus Walleij wrote:

> > ChangeLog v1->v2:
> > - Noticed that the previous solution doesn't actually work,
> >   the machine hangs and reboots intead (even if it got rid of
> >   the most obvious crash). Make a more thorough solution that
> >   completely avoids using these callbacks if we don't have
> >   a parent.
>
> What is the problem with disable exactly?

There is no problem with .irq_disable, the system still works
if I keep that. But since the original patch added these two
callbacks for hierarchical I just moved them both to be
conditional.

The .irq_eoi callback is the culprit.

> >       pctrl->irq_chip.name = "msmgpio";
> >       pctrl->irq_chip.irq_enable = msm_gpio_irq_enable;
> > -     pctrl->irq_chip.irq_disable = msm_gpio_irq_disable;
>
> I find it really odd to have the enable callback, but not the disable.
> What is the rational for that? Can we drop the enable as well for old
> platforms and only use mask/unmask instead?

Hm I'm just working with the regression, and before the
patch I'm fixing the driver actually had just the .irq_enable
callback, so I'm restoring that state.

Would you prefer a patch where I just move the assignment
of the .irq_eoi callback to be conditional?

I have no idea *why* .irq_eoi() locks up the system, I suspect
one of those irqchip internal semantics that are sometimes
not entirely clear.

Yours,
Linus Walleij