* [PATCH] gpio/msm-v1: re-read IRQ flags on each iteration @ 2012-05-11 12:07 Linus Walleij 2012-05-11 17:41 ` Grant Likely 0 siblings, 1 reply; 6+ messages in thread From: Linus Walleij @ 2012-05-11 12:07 UTC (permalink / raw) To: Grant Likely, linux-kernel; +Cc: Julia Lawall, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio/msm-v1: re-read IRQ flags on each iteration 2012-05-11 12:07 [PATCH] gpio/msm-v1: re-read IRQ flags on each iteration Linus Walleij @ 2012-05-11 17:41 ` Grant Likely 2012-05-11 21:22 ` Linus Walleij 2012-05-16 1:09 ` David Brown 0 siblings, 2 replies; 6+ messages in thread From: Grant Likely @ 2012-05-11 17:41 UTC (permalink / raw) To: Linus Walleij, linux-kernel; +Cc: Julia Lawall, Linus Walleij 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio/msm-v1: re-read IRQ flags on each iteration 2012-05-11 17:41 ` Grant Likely @ 2012-05-11 21:22 ` Linus Walleij 2012-05-16 1:09 ` David Brown 1 sibling, 0 replies; 6+ messages in thread From: Linus Walleij @ 2012-05-11 21:22 UTC (permalink / raw) To: Grant Likely; +Cc: Linus Walleij, linux-kernel, Julia Lawall 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio/msm-v1: re-read IRQ flags on each iteration 2012-05-11 17:41 ` Grant Likely 2012-05-11 21:22 ` Linus Walleij @ 2012-05-16 1:09 ` David Brown 2012-05-16 7:23 ` Linus Walleij 1 sibling, 1 reply; 6+ messages in thread From: David Brown @ 2012-05-16 1:09 UTC (permalink / raw) To: Grant Likely; +Cc: Linus Walleij, linux-kernel, Julia Lawall, Linus Walleij 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio/msm-v1: re-read IRQ flags on each iteration 2012-05-16 1:09 ` David Brown @ 2012-05-16 7:23 ` Linus Walleij 2012-05-16 18:38 ` David Brown 0 siblings, 1 reply; 6+ messages in thread From: Linus Walleij @ 2012-05-16 7:23 UTC (permalink / raw) To: David Brown; +Cc: Grant Likely, Linus Walleij, linux-kernel, Julia Lawall 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio/msm-v1: re-read IRQ flags on each iteration 2012-05-16 7:23 ` Linus Walleij @ 2012-05-16 18:38 ` David Brown 0 siblings, 0 replies; 6+ messages in thread From: David Brown @ 2012-05-16 18:38 UTC (permalink / raw) To: Linus Walleij; +Cc: Grant Likely, Linus Walleij, linux-kernel, Julia Lawall 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-05-16 18:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-05-11 12:07 [PATCH] gpio/msm-v1: re-read IRQ flags on each iteration Linus Walleij 2012-05-11 17:41 ` Grant Likely 2012-05-11 21:22 ` Linus Walleij 2012-05-16 1:09 ` David Brown 2012-05-16 7:23 ` Linus Walleij 2012-05-16 18:38 ` David Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox