[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAD=FV=Wg-gdry1a-LjJhuKgHRr=DXq4Hu0P8nJGAzf5viEcthA@mail.gmail.com>
Date: Tue, 24 Nov 2020 09:01:44 -0800
From: Doug Anderson <dianders@...omium.org>
To: Marc Zyngier <maz@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Jason Cooper <jason@...edaemon.net>,
Linus Walleij <linus.walleij@...aro.org>,
Maulik Shah <mkshah@...eaurora.org>,
Srinivas Ramana <sramana@...eaurora.org>,
Neeraj Upadhyay <neeraju@...eaurora.org>,
Rajendra Nayak <rnayak@...eaurora.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
linux-arm-msm <linux-arm-msm@...r.kernel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Stephen Boyd <swboyd@...omium.org>,
Andy Gross <agross@...nel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] irqchip: qcom-pdc: Fix phantom irq when changing
between rising/falling
Hi,
On Tue, Nov 24, 2020 at 1:00 AM Marc Zyngier <maz@...nel.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
Powered by blists - more mailing lists