[PATCH] gpio/msm-v1: re-read IRQ flags on each iteration

[PATCH] gpio/msm-v1: re-read IRQ flags on each iteration

[Linus Walleij]

From: Linus Walleij <linus.walleij@linaro.org>

We have recently found a number or erroneous IRQ handlers in
the kernel where the flag iterator loop miss IRQs that get
raised when the loop is executing. This was spotted in the
MSM v1 GPIO driver by Julia Lawall using this cocinelle
snippet:

@@
expression pending,gedr,e1;
statement S;
@@

*pending = readl(gedr);
... when != pending = e1
while (pending) S

Reported-by: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpio-msm-v1.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-msm-v1.c b/drivers/gpio/gpio-msm-v1.c
index 52a4d42..68ca760 100644
--- a/drivers/gpio/gpio-msm-v1.c
+++ b/drivers/gpio/gpio-msm-v1.c
@@ -567,9 +567,9 @@ static void msm_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 
 	for (i = 0; i < msm_gpio_count; i++) {
 		struct msm_gpio_chip *msm_chip = &msm_gpio_chips[i];
-		val = readl(msm_chip->regs.int_status);
-		val &= msm_chip->int_enable[0];
-		while (val) {
+
+		while (val = (readl(msm_chip->regs.int_status) &
+			      msm_chip->int_enable[0])) {
 			mask = val & -val;
 			j = fls(mask) - 1;
 			/* printk("%s %08x %08x bit %d gpio %d irq %d\n",
-- 
1.7.9.2

Re: [PATCH] gpio/msm-v1: re-read IRQ flags on each iteration

[Grant Likely]

On Fri, 11 May 2012 14:07:12 +0200, Linus Walleij <linus.walleij@stericsson.com> wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> We have recently found a number or erroneous IRQ handlers in
> the kernel where the flag iterator loop miss IRQs that get
> raised when the loop is executing. This was spotted in the
> MSM v1 GPIO driver by Julia Lawall using this cocinelle
> snippet:
> 
> @@
> expression pending,gedr,e1;
> statement S;
> @@
> 
> *pending = readl(gedr);
> ... when != pending = e1
> while (pending) S
> 
> Reported-by: Julia Lawall <julia.lawall@lip6.fr>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Thanks Linus,

I'll need an ack from an msm developer before applying this.

g.

> ---
>  drivers/gpio/gpio-msm-v1.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-msm-v1.c b/drivers/gpio/gpio-msm-v1.c
> index 52a4d42..68ca760 100644
> --- a/drivers/gpio/gpio-msm-v1.c
> +++ b/drivers/gpio/gpio-msm-v1.c
> @@ -567,9 +567,9 @@ static void msm_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>  
>  	for (i = 0; i < msm_gpio_count; i++) {
>  		struct msm_gpio_chip *msm_chip = &msm_gpio_chips[i];
> -		val = readl(msm_chip->regs.int_status);
> -		val &= msm_chip->int_enable[0];
> -		while (val) {
> +
> +		while (val = (readl(msm_chip->regs.int_status) &
> +			      msm_chip->int_enable[0])) {
>  			mask = val & -val;
>  			j = fls(mask) - 1;
>  			/* printk("%s %08x %08x bit %d gpio %d irq %d\n",
> -- 
> 1.7.9.2
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

Re: [PATCH] gpio/msm-v1: re-read IRQ flags on each iteration

[Linus Walleij]

On Fri, May 11, 2012 at 7:41 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Fri, 11 May 2012 14:07:12 +0200, Linus Walleij <linus.walleij@stericsson.com> wrote:

> I'll need an ack from an msm developer before applying this.

Yep I forward it to David Brown when I realized I forgot him in the Cc...

Yours,
Linus Walleij

Re: [PATCH] gpio/msm-v1: re-read IRQ flags on each iteration

[David Brown]

On Fri, May 11, 2012 at 11:41:12AM -0600, Grant Likely wrote:
> On Fri, 11 May 2012 14:07:12 +0200, Linus Walleij <linus.walleij@stericsson.com> wrote:
> > From: Linus Walleij <linus.walleij@linaro.org>
> > 
> > We have recently found a number or erroneous IRQ handlers in
> > the kernel where the flag iterator loop miss IRQs that get
> > raised when the loop is executing. This was spotted in the
> > MSM v1 GPIO driver by Julia Lawall using this cocinelle
> > snippet:
> > 
> > @@
> > expression pending,gedr,e1;
> > statement S;
> > @@
> > 
> > *pending = readl(gedr);
> > ... when != pending = e1
> > while (pending) S
> > 
> > Reported-by: Julia Lawall <julia.lawall@lip6.fr>
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Thanks Linus,
> 
> I'll need an ack from an msm developer before applying this.

I though I saw Jeff Ohlstein reply, but I'm not finding that message.
I believe this patch is actually incorrect.  The register needs to
only be read once, and the bits walked through.  If another interrupt
comes in during the loop, the interrupt will remain asserted, and we
will re-enter the irq handler.  I suppose it could be wrapped in yet
another loop, but I'm not sure it is worth it.

I'm pretty sure the code works as it is, since it is the same as the
code in the Android kernel.

David

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] gpio/msm-v1: re-read IRQ flags on each iteration

[Linus Walleij]

On Wed, May 16, 2012 at 3:09 AM, David Brown <davidb@codeaurora.org> wrote:
> On Fri, May 11, 2012 at 11:41:12AM -0600, Grant Likely wrote:

>> I'll need an ack from an msm developer before applying this.
>
> I though I saw Jeff Ohlstein reply, but I'm not finding that message.

He was replying to a similar patch affecting the DMA controller
interrupt-like construct.

> I believe this patch is actually incorrect.  The register needs to
> only be read once, and the bits walked through.  If another interrupt
> comes in during the loop, the interrupt will remain asserted, and we
> will re-enter the irq handler.  I suppose it could be wrapped in yet
> another loop, but I'm not sure it is worth it.

OK the code doesn't say really. (It did say something like that
in the DMA controller). It's quite uncommon with these type of
latched-clear registers (reading IRQ status clears the flags) type
of hardware so maybe it should be commented.

> I'm pretty sure the code works as it is, since it is the same as the
> code in the Android kernel.

The problem with this bug is that it often works until you get a
demanding case. On the VIC I guess it wasn't noticed until tested with
a device that raised a storm of IRQs so the handler started missing
them.

On a GPIO this would be the case where a GPIO IRQ is asserted
in real quick succession, like the same IRQ being asserted while
the previous IRQ on the same line is being handled and where
missing one of these IRQs creates problems.

It's gonna be error phenomena of the type "it looks like we're
missing some IRQs from our touchscreen". etc.

Yours,
Linus Walleij

Re: [PATCH] gpio/msm-v1: re-read IRQ flags on each iteration

[David Brown]

On Wed, May 16, 2012 at 09:23:33AM +0200, Linus Walleij wrote:
> On Wed, May 16, 2012 at 3:09 AM, David Brown <davidb@codeaurora.org> wrote:
> > On Fri, May 11, 2012 at 11:41:12AM -0600, Grant Likely wrote:
> 
> >> I'll need an ack from an msm developer before applying this.
> >
> > I though I saw Jeff Ohlstein reply, but I'm not finding that message.
> 
> He was replying to a similar patch affecting the DMA controller
> interrupt-like construct.
> 
> > I believe this patch is actually incorrect.  The register needs to
> > only be read once, and the bits walked through.  If another interrupt
> > comes in during the loop, the interrupt will remain asserted, and we
> > will re-enter the irq handler.  I suppose it could be wrapped in yet
> > another loop, but I'm not sure it is worth it.
> 
> OK the code doesn't say really. (It did say something like that
> in the DMA controller). It's quite uncommon with these type of
> latched-clear registers (reading IRQ status clears the flags) type
> of hardware so maybe it should be commented.

I'll see if I can get confirmation on it.

> > I'm pretty sure the code works as it is, since it is the same as the
> > code in the Android kernel.
> 
> The problem with this bug is that it often works until you get a
> demanding case. On the VIC I guess it wasn't noticed until tested with
> a device that raised a storm of IRQs so the handler started missing
> them.

At one point, the target got a lot of use, so I'm going to guess the
current code is correct.  I'll see if I can dig up a definitive
answer, though.  Since I don't have one to even test on, I'm a little
reluctant to change the code, though.

David

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.