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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Mon, Mar 9, 2020 at 3:54 PM Marc Zyngier <maz@xxxxxxxxxx> 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