[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d65e2be33a218751e7be3342e490e076@kernel.org>
Date: Tue, 24 Nov 2020 09:00:31 +0000
From: Marc Zyngier <maz@...nel.org>
To: Douglas Anderson <dianders@...omium.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>,
linux-gpio@...r.kernel.org, linux-arm-msm@...r.kernel.org,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Stephen Boyd <swboyd@...omium.org>,
Andy Gross <agross@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] irqchip: qcom-pdc: Fix phantom irq when changing
between rising/falling
On 2020-11-24 00:01, Douglas Anderson wrote:
> We have a problem if we use gpio-keys and configure wakeups such that
> we only want one edge to wake us up. AKA:
> wakeup-event-action = <EV_ACT_DEASSERTED>;
> wakeup-source;
>
> Specifically we end up with a phantom interrupt that blocks suspend if
> the line was already high and we want wakeups on rising edges (AKA we
> want the GPIO to go low and then high again before we wake up). The
> opposite is also problematic.
>
> Specifically, here's what's happening today:
> 1. Normally, gpio-keys configures to look for both edges. Due to the
> current workaround introduced in commit c3c0c2e18d94 ("pinctrl:
> qcom: Handle broken/missing PDC dual edge IRQs on sc7180"), if the
> line was high we'd configure for falling edges.
> 2. At suspend time, we change to look for rising edges.
> 3. After qcom_pdc_gic_set_type() runs, we get a phantom interrupt.
>
> We can solve this by just clearing the phantom interrupt.
>
> NOTE: it is possible that this could cause problems for a client with
> very specific needs, but there's not much we can do with this
> hardware. As an example, let's say the interrupt signal is currently
> high and the client is looking for falling edges. The client now
> changes to look for rising edges. The client could possibly expect
> that if the line has a short pulse low (and back high) that it would
> always be detected. Specifically no matter when the pulse happened,
> it should either have tripped the (old) falling edge trigger or the
> (new) rising edge trigger. We will simply not trip it. We could
> narrow down the race a bit by polling our parent before changing
> types, but no matter what we do there will still be a period of time
> where we can't tell the difference between a real transition (or more
> than one transition) and the phantom.
>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
>
> drivers/irqchip/qcom-pdc.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index bd39e9de6ecf..7d097164aadc 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -159,6 +159,8 @@ static int qcom_pdc_gic_set_type(struct irq_data
> *d, unsigned int type)
> {
> int pin_out = d->hwirq;
> enum pdc_irq_config_bits pdc_type;
> + enum pdc_irq_config_bits old_pdc_type;
> + int ret;
>
> if (pin_out == GPIO_NO_WAKE_IRQ)
> return 0;
> @@ -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/.
You could also make it conditional on the parent side having been
successful.
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...
> +
> + return ret;
> }
>
> static struct irq_chip qcom_pdc_gic_chip = {
It otherwise looks sensible. Is that a fix for 5.10?
M.
--
Jazz is not dead. It just smells funny...
Powered by blists - more mailing lists