Re: [PATCH 1/3] irqchip: qcom-pdc: Fix phantom irq when changing between rising/falling

[Doug Anderson <dianders@chromium.org>]

Hi,

On Tue, Nov 24, 2020 at 1:00 AM Marc Zyngier <maz@kernel.org> wrote:
>
> > @@ -187,9 +189,24 @@ static int qcom_pdc_gic_set_type(struct irq_data
> > *d, unsigned int type)
> >               return -EINVAL;
> >       }
> >
> > +     old_pdc_type = pdc_reg_read(IRQ_i_CFG, pin_out);
> >       pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
> >
> > -     return irq_chip_set_type_parent(d, type);
> > +     ret = irq_chip_set_type_parent(d, type);
> > +
> > +     /*
> > +      * When we change types the PDC can give a phantom interrupt.
> > +      * Clear it.  Specifically the phantom shows up if a line is already
> > +      * high and we change to rising or if a line is already low and we
> > +      * change to falling but let's be consistent and clear it always.
> > +      *
> > +      * Doing this works because we have IRQCHIP_SET_TYPE_MASKED so the
> > +      * interrupt will be cleared before the rest of the system sees it.
> > +      */
> > +     if (old_pdc_type != pdc_type)
> > +             irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
>
> nit: s/0/false/.

I'll fix this.


> You could also make it conditional on the parent side having been
> successful.

Good idea.


> And while we're looking at this: do you need to rollback the PDC state
> if the GIC side has failed? It's all very hypothetical, but just in
> case...

I'm going to go ahead and say "no", but I'll make this change if you
insist.  Specifically:

* We're still leaving things in a self-consistent state if we fail to
clear the parent, we'll just get a spurious interrupt.  It won't cause
any crashes / hangs / whatever.

* Since it seems very unlikely we'd ever trip this and if we ever do
it's not the end of the world, I'd rather not add extra code.

Hopefully that's OK.

-Doug