* [PATCH] pinctrl/amd: fix masking of GPIO interrupts
@ 2017-10-02 4:00 Daniel Drake
2017-10-06 3:26 ` Shah, Nehal-bakulchandra
2017-10-11 7:33 ` Linus Walleij
0 siblings, 2 replies; 3+ messages in thread
From: Daniel Drake @ 2017-10-02 4:00 UTC (permalink / raw)
To: ken.xue, shyam-sundar.s-k, nehal-bakulchandra.shah, linus.walleij
Cc: linux, linux-gpio
On Asus laptop models X505BA, X505BP, X542BA and X542BP, the i2c-hid
touchpad (using a GPIO for interrupts) becomes unresponsive after a
few minutes of usage, or after placing two fingers on the touchpad,
which seems to have the effect of queuing up a large amount of input
data to be transferred.
When the touchpad is in unresponsive state, we observed that the GPIO
level-triggered interrupt is still at it's active level, however the
pinctrl-amd driver is not receiving/dispatching more interrupts at this
point.
After the initial interrupt arrives, amd_gpio_irq_mask() is called
however we then see amd_gpio_irq_handler() being called repeatedly for
the same irq; the interrupt mask is not taking effect because of the
following sequence of events:
- amd_gpio_irq_handler fires, reads and caches pin reg
- amd_gpio_irq_handler calls generic_handle_irq()
- During IRQ handling, amd_gpio_irq_mask() is called and modifies pin reg
- amd_gpio_irq_handler clears interrupt by writing cached value
The stale cached value written at the final stage undoes the masking.
Fix this by re-reading the register before clearing the interrupt.
I also spotted that the interrupt-clearing code can race against
amd_gpio_irq_mask() / amd_gpio_irq_unmask(), so add locking there.
Presumably this race was leading to the loss of interrupts.
After these changes, the touchpad appears to be working fine.
Signed-off-by: Daniel Drake <drake@endlessm.com>
---
drivers/pinctrl/pinctrl-amd.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 3f6b34febbf1..433af328d981 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -534,8 +534,16 @@ static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
continue;
irq = irq_find_mapping(gc->irqdomain, irqnr + i);
generic_handle_irq(irq);
- /* Clear interrupt */
+
+ /* Clear interrupt.
+ * We must read the pin register again, in case the
+ * value was changed while executing
+ * generic_handle_irq() above.
+ */
+ raw_spin_lock_irqsave(&gpio_dev->lock, flags);
+ regval = readl(regs + i);
writel(regval, regs + i);
+ raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
ret = IRQ_HANDLED;
}
}
--
2.11.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] pinctrl/amd: fix masking of GPIO interrupts
2017-10-02 4:00 [PATCH] pinctrl/amd: fix masking of GPIO interrupts Daniel Drake
@ 2017-10-06 3:26 ` Shah, Nehal-bakulchandra
2017-10-11 7:33 ` Linus Walleij
1 sibling, 0 replies; 3+ messages in thread
From: Shah, Nehal-bakulchandra @ 2017-10-06 3:26 UTC (permalink / raw)
To: Daniel Drake, ken.xue, shyam-sundar.s-k, linus.walleij; +Cc: linux, linux-gpio
On 10/2/2017 9:30 AM, Daniel Drake wrote:
> On Asus laptop models X505BA, X505BP, X542BA and X542BP, the i2c-hid
> touchpad (using a GPIO for interrupts) becomes unresponsive after a
> few minutes of usage, or after placing two fingers on the touchpad,
> which seems to have the effect of queuing up a large amount of input
> data to be transferred.
>
> When the touchpad is in unresponsive state, we observed that the GPIO
> level-triggered interrupt is still at it's active level, however the
> pinctrl-amd driver is not receiving/dispatching more interrupts at this
> point.
>
> After the initial interrupt arrives, amd_gpio_irq_mask() is called
> however we then see amd_gpio_irq_handler() being called repeatedly for
> the same irq; the interrupt mask is not taking effect because of the
> following sequence of events:
> - amd_gpio_irq_handler fires, reads and caches pin reg
> - amd_gpio_irq_handler calls generic_handle_irq()
> - During IRQ handling, amd_gpio_irq_mask() is called and modifies pin reg
> - amd_gpio_irq_handler clears interrupt by writing cached value
>
> The stale cached value written at the final stage undoes the masking.
> Fix this by re-reading the register before clearing the interrupt.
>
> I also spotted that the interrupt-clearing code can race against
> amd_gpio_irq_mask() / amd_gpio_irq_unmask(), so add locking there.
> Presumably this race was leading to the loss of interrupts.
>
> After these changes, the touchpad appears to be working fine.
>
> Signed-off-by: Daniel Drake <drake@endlessm.com>
> ---
> drivers/pinctrl/pinctrl-amd.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index 3f6b34febbf1..433af328d981 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -534,8 +534,16 @@ static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
> continue;
> irq = irq_find_mapping(gc->irqdomain, irqnr + i);
> generic_handle_irq(irq);
> - /* Clear interrupt */
> +
> + /* Clear interrupt.
> + * We must read the pin register again, in case the
> + * value was changed while executing
> + * generic_handle_irq() above.
> + */
> + raw_spin_lock_irqsave(&gpio_dev->lock, flags);
> + regval = readl(regs + i);
> writel(regval, regs + i);
> + raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
> ret = IRQ_HANDLED;
> }
> }
>
Hi Darek
Looks good .
Thanks
Nehal Shah
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] pinctrl/amd: fix masking of GPIO interrupts
2017-10-02 4:00 [PATCH] pinctrl/amd: fix masking of GPIO interrupts Daniel Drake
2017-10-06 3:26 ` Shah, Nehal-bakulchandra
@ 2017-10-11 7:33 ` Linus Walleij
1 sibling, 0 replies; 3+ messages in thread
From: Linus Walleij @ 2017-10-11 7:33 UTC (permalink / raw)
To: Daniel Drake
Cc: Ken Xue, S-k, Shyam-sundar, Shah, Nehal-bakulchandra, linux, linux-gpio
On Mon, Oct 2, 2017 at 6:00 AM, Daniel Drake <drake@endlessm.com> wrote:
> On Asus laptop models X505BA, X505BP, X542BA and X542BP, the i2c-hid
> touchpad (using a GPIO for interrupts) becomes unresponsive after a
> few minutes of usage, or after placing two fingers on the touchpad,
> which seems to have the effect of queuing up a large amount of input
> data to be transferred.
>
> When the touchpad is in unresponsive state, we observed that the GPIO
> level-triggered interrupt is still at it's active level, however the
> pinctrl-amd driver is not receiving/dispatching more interrupts at this
> point.
>
> After the initial interrupt arrives, amd_gpio_irq_mask() is called
> however we then see amd_gpio_irq_handler() being called repeatedly for
> the same irq; the interrupt mask is not taking effect because of the
> following sequence of events:
> - amd_gpio_irq_handler fires, reads and caches pin reg
> - amd_gpio_irq_handler calls generic_handle_irq()
> - During IRQ handling, amd_gpio_irq_mask() is called and modifies pin reg
> - amd_gpio_irq_handler clears interrupt by writing cached value
>
> The stale cached value written at the final stage undoes the masking.
> Fix this by re-reading the register before clearing the interrupt.
>
> I also spotted that the interrupt-clearing code can race against
> amd_gpio_irq_mask() / amd_gpio_irq_unmask(), so add locking there.
> Presumably this race was leading to the loss of interrupts.
>
> After these changes, the touchpad appears to be working fine.
>
> Signed-off-by: Daniel Drake <drake@endlessm.com>
Patch applied for fixes with Nehal-bakulchandra's ACK.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-10-11 7:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-02 4:00 [PATCH] pinctrl/amd: fix masking of GPIO interrupts Daniel Drake
2017-10-06 3:26 ` Shah, Nehal-bakulchandra
2017-10-11 7:33 ` Linus Walleij
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.