diff mbox series

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

Message ID 20201123160139.1.I2702919afc253e2a451bebc3b701b462b2d22344@changeid
State Superseded
Headers show
Series [1/3] irqchip: qcom-pdc: Fix phantom irq when changing between rising/falling | expand

Commit Message

Doug Anderson Nov. 24, 2020, 12:01 a.m. UTC
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@chromium.org>
---

 drivers/irqchip/qcom-pdc.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Comments

Maulik Shah Nov. 24, 2020, 7:03 a.m. UTC | #1
Hi Doug,

Thanks for the patch. Looks good to me and tested.

Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
Tested-by: Maulik Shah <mkshah@codeaurora.org>

Thanks,
Maulik

On 11/24/2020 5:31 AM, 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@chromium.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);
> +
> +	return ret;
>   }
>   
>   static struct irq_chip qcom_pdc_gic_chip = {
Linus Walleij Nov. 24, 2020, 8:28 a.m. UTC | #2
On Tue, Nov 24, 2020 at 1:02 AM Douglas Anderson <dianders@chromium.org> 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;


I would need Marc's ACK to apply this with the other patches
to the pinctrl tree, but I can't really see if maybe it is OK to
apply it separately?

Also are these patches supposed to all go in as fixes or
for v5.11?

Yours,
Linus Walleij
Marc Zyngier Nov. 24, 2020, 9 a.m. UTC | #3
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@chromium.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.
Doug Anderson Nov. 24, 2020, 4:55 p.m. UTC | #4
Hi,

On Tue, Nov 24, 2020 at 12:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Nov 24, 2020 at 1:02 AM Douglas Anderson <dianders@chromium.org> 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;
>
> I would need Marc's ACK to apply this with the other patches
> to the pinctrl tree, but I can't really see if maybe it is OK to
> apply it separately?

I'll make an explicit note after the cut in the patch, but to also
respond here: we can apply this patch on its own.  The only reason I
sent as one series is because they address similar issues, this patch
stands on its own.  Patch #3 needs #2 but patch #2/#3 don't need patch
#1.

> Also are these patches supposed to all go in as fixes or
> for v5.11?

Wherever it makes sense.

-Doug
Doug Anderson Nov. 24, 2020, 5:01 p.m. UTC | #5
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
Maulik Shah Nov. 24, 2020, 5:42 p.m. UTC | #6
Hi Linus,

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

Can you please hold this change. I am checking with HW folks if above 
commented behaviour is expected/is valid case to set the irq type rising 
edge when the line is already high.

Will keep posting update here.

Thanks,
Maulik

On 11/24/2020 10:25 PM, Doug Anderson wrote:
> Hi,

>

> On Tue, Nov 24, 2020 at 12:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:

>> On Tue, Nov 24, 2020 at 1:02 AM Douglas Anderson <dianders@chromium.org> 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;

>> I would need Marc's ACK to apply this with the other patches

>> to the pinctrl tree, but I can't really see if maybe it is OK to

>> apply it separately?

> I'll make an explicit note after the cut in the patch, but to also

> respond here: we can apply this patch on its own.  The only reason I

> sent as one series is because they address similar issues, this patch

> stands on its own.  Patch #3 needs #2 but patch #2/#3 don't need patch

> #1.

>

>> Also are these patches supposed to all go in as fixes or

>> for v5.11?

> Wherever it makes sense.

>

> -Doug


-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Doug Anderson Nov. 24, 2020, 5:46 p.m. UTC | #7
Hi,

On Tue, Nov 24, 2020 at 9:42 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> Hi Linus,
>
> +        * 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.
> +        *
>
> Can you please hold this change. I am checking with HW folks if above
> commented behaviour is expected/is valid case to set the irq type rising
> edge when the line is already high.
>
> Will keep posting update here.
>
> Thanks,
> Maulik

Thanks for the update.  I'm still going to post a v2 because I think
patch #1 in the series should land and it seems nice to keep them
together.  I'll add a note to the patch indicating your request to
wait for an ack before landing.

-Doug
diff mbox series

Patch

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);
+
+	return ret;
 }
 
 static struct irq_chip qcom_pdc_gic_chip = {