clear the status bit if the mask register doesn't prevent
the chip level irq from being asserted
OR in the following sequence, there will be irq storm happens:
1) interrupt is triggered;
2) another thread disables it(the mask bit is set);
3) _Then_ the interrupt thread is not ACKed(the status bit is not cleared),
and it's ignored;
4) if the irq is still asserted because of the uncleared status bit,
the irq storm happens;
Change-Id: I371201f365c5a8470073a393068cfeb4e3d14a03
Signed-off-by: Yi Zhang <[email protected]>
---
drivers/base/regmap/regmap-irq.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index d10456f..cd655b8 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -105,6 +105,27 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
"Failed to sync wakes in %x: %d\n",
reg, ret);
}
+
+ if (!d->chip->init_ack_masked)
+ continue;
+
+ /* Ack masked but set interrupts */
+ reg = d->chip->status_base +
+ (i * map->reg_stride * d->irq_reg_stride);
+ ret = regmap_read(d->map, reg, &d->status_buf[i]);
+ if (ret != 0)
+ dev_err(d->map->dev, "Failed to read IRQ status: %d\n",
+ ret);
+
+ if (d->status_buf[i] && d->chip->ack_base) {
+ reg = d->chip->ack_base +
+ (i * map->reg_stride * d->irq_reg_stride);
+ ret = regmap_write(d->map, reg,
+ d->status_buf[i] & d->mask_buf[i]);
+ if (ret != 0)
+ dev_err(d->map->dev, "Failed to ack 0x%x: %d\n",
+ reg, ret);
+ }
}
if (d->chip->runtime_pm)
--
1.7.9.5
On Mon, Oct 14, 2013 at 12:23:53PM +0800, Yi Zhang wrote:
> Change-Id: I371201f365c5a8470073a393068cfeb4e3d14a03
Don't include noise like this in upstream submissions.
> + /* Ack masked but set interrupts */
> + reg = d->chip->status_base +
> + (i * map->reg_stride * d->irq_reg_stride);
> + ret = regmap_read(d->map, reg, &d->status_buf[i]);
> + if (ret != 0)
> + dev_err(d->map->dev, "Failed to read IRQ status: %d\n",
> + ret);
No, this isn't good - it'll read the hardware interrupt status again.
This will break any devices that are clear on read since enabled
interrupts will also be read. I'd suggest unconditionally acknowledging
all masked interrupts as the simplest approach, obviously it'd be better
to only acknowledge newly masked interrupts but that is more complex to
implement.
2013/10/14 Mark Brown <[email protected]>:
> On Mon, Oct 14, 2013 at 12:23:53PM +0800, Yi Zhang wrote:
>
>> Change-Id: I371201f365c5a8470073a393068cfeb4e3d14a03
>
> Don't include noise like this in upstream submissions.
>
Thanks Mark, it's my fault; I'll remove it;
>> + /* Ack masked but set interrupts */
>> + reg = d->chip->status_base +
>> + (i * map->reg_stride * d->irq_reg_stride);
>> + ret = regmap_read(d->map, reg, &d->status_buf[i]);
>> + if (ret != 0)
>> + dev_err(d->map->dev, "Failed to read IRQ status: %d\n",
>> + ret);
>
> No, this isn't good - it'll read the hardware interrupt status again.
> This will break any devices that are clear on read since enabled
> interrupts will also be read. I'd suggest unconditionally acknowledging
> all masked interrupts as the simplest approach, obviously it'd be better
> to only acknowledge newly masked interrupts but that is more complex to
> implement.
Yes, you are right; I'll change according to your advice and send it out later;
thanks very much for pointing this;