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

[Marc Zyngier <maz@kernel.org>]

On 2020-03-09 15:03, Linus Walleij wrote:
> 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'd rather we have the minimal change that makes your system runnable.
If making irq_eoi depend on some QC magic, fine by me. Having an 
unbalanced
enable/disable setup looks pretty fragile.

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

I don't think anyone knows what they are outside of the usual QC 
circles.

         M.
-- 
Jazz is not dead. It just smells funny...