* [PATCH] regmap: irq: clear status when disable irq
@ 2013-10-15 5:03 Yi Zhang
2013-10-15 11:48 ` Mark Brown
0 siblings, 1 reply; 6+ messages in thread
From: Yi Zhang @ 2013-10-15 5:03 UTC (permalink / raw)
To: broonie; +Cc: yizhang, zhouqiao, zhangwm, yizhang.mrvl, linux-kernel
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;
Signed-off-by: Yi Zhang <yizhang@marvell.com>
---
drivers/base/regmap/regmap-irq.c | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index d10456f..0784cf4 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -61,7 +61,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
{
struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
struct regmap *map = d->map;
- int i, ret;
+ int i, j, ret, bits_length;
u32 reg;
if (d->chip->runtime_pm) {
@@ -105,6 +105,30 @@ 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 all the masked interrupts uncondictionly,
+ * OR if there is masked interrupt which hasn't been Acked,
+ * it'll be ignored in irq handler, then may introduce irq storm
+ */
+ if (d->chip->ack_base) {
+ reg = d->chip->ack_base +
+ (i * map->reg_stride * d->irq_reg_stride);
+
+ bits_length = d->map->format.val_bytes * BITS_PER_BYTE;
+ for (j = 0; j < bits_length; j++) {
+ d->mask_buf[i] &= (0x1 << j);
+ if (!d->mask_buf[i])
+ ret = regmap_update_bits(d->map, reg,
+ (0x1 << j), 0);
+ if (ret != 0)
+ dev_err(d->map->dev, "Failed to ack 0x%x: %d\n",
+ reg, ret);
+ }
+ /* no need to sync status_buf, update in irq handler */
+ }
}
if (d->chip->runtime_pm)
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] regmap: irq: clear status when disable irq
2013-10-15 5:03 [PATCH] regmap: irq: clear status when disable irq Yi Zhang
@ 2013-10-15 11:48 ` Mark Brown
2013-10-15 12:03 ` Yi Zhang
0 siblings, 1 reply; 6+ messages in thread
From: Mark Brown @ 2013-10-15 11:48 UTC (permalink / raw)
To: Yi Zhang; +Cc: zhouqiao, zhangwm, yizhang.mrvl, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 893 bytes --]
On Tue, Oct 15, 2013 at 01:03:58PM +0800, Yi Zhang wrote:
> + bits_length = d->map->format.val_bytes * BITS_PER_BYTE;
> + for (j = 0; j < bits_length; j++) {
> + d->mask_buf[i] &= (0x1 << j);
> + if (!d->mask_buf[i])
> + ret = regmap_update_bits(d->map, reg,
> + (0x1 << j), 0);
> + if (ret != 0)
> + dev_err(d->map->dev, "Failed to ack 0x%x: %d\n",
> + reg, ret);
I don't entirely understand this code - what's the loop doing? It
looks like it's trying to acknowledge things bit by bit but it's doing
this by updating mask_buf so it looks like it'll corrupt the set of
masked interrupts. After the zeroth iteration any bits other than bit 0
should get cleared by the &= (0x1 << j). I'd have expected to just
write mask_buf back or something similar.
It's possible I'm missing something here but if that is the case perhaps
some comments would be good.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] regmap: irq: clear status when disable irq
2013-10-15 11:48 ` Mark Brown
@ 2013-10-15 12:03 ` Yi Zhang
0 siblings, 0 replies; 6+ messages in thread
From: Yi Zhang @ 2013-10-15 12:03 UTC (permalink / raw)
To: Mark Brown; +Cc: Yi Zhang, zhouqiao, zhangwm, linux-kernel
2013/10/15 Mark Brown <broonie@kernel.org>:
> On Tue, Oct 15, 2013 at 01:03:58PM +0800, Yi Zhang wrote:
>
>> + bits_length = d->map->format.val_bytes * BITS_PER_BYTE;
>> + for (j = 0; j < bits_length; j++) {
>> + d->mask_buf[i] &= (0x1 << j);
Yes, this line is wrong, I shouldn't change the mask_buf; I'll change it;
my fault; sorry;
>> + if (!d->mask_buf[i])
>> + ret = regmap_update_bits(d->map, reg,
>> + (0x1 << j), 0);
>> + if (ret != 0)
>> + dev_err(d->map->dev, "Failed to ack 0x%x: %d\n",
>> + reg, ret);
>
> I don't entirely understand this code - what's the loop doing? It
> looks like it's trying to acknowledge things bit by bit but it's doing
> this by updating mask_buf so it looks like it'll corrupt the set of
> masked interrupts. After the zeroth iteration any bits other than bit 0
> should get cleared by the &= (0x1 << j). I'd have expected to just
> write mask_buf back or something similar.
>
> It's possible I'm missing something here but if that is the case perhaps
> some comments would be good.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] regmap: irq: clear status when disable irq
@ 2013-10-14 4:23 Yi Zhang
2013-10-14 11:09 ` Mark Brown
0 siblings, 1 reply; 6+ messages in thread
From: Yi Zhang @ 2013-10-14 4:23 UTC (permalink / raw)
To: broonie; +Cc: yizhang, qiaozhou, zhangwm, yizhang.mrvl, linux-kernel
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 <yizhang@marvell.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] regmap: irq: clear status when disable irq
2013-10-14 4:23 Yi Zhang
@ 2013-10-14 11:09 ` Mark Brown
2013-10-14 11:55 ` Yi Zhang
0 siblings, 1 reply; 6+ messages in thread
From: Mark Brown @ 2013-10-14 11:09 UTC (permalink / raw)
To: Yi Zhang; +Cc: qiaozhou, zhangwm, yizhang.mrvl, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 810 bytes --]
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.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] regmap: irq: clear status when disable irq
2013-10-14 11:09 ` Mark Brown
@ 2013-10-14 11:55 ` Yi Zhang
0 siblings, 0 replies; 6+ messages in thread
From: Yi Zhang @ 2013-10-14 11:55 UTC (permalink / raw)
To: Mark Brown; +Cc: Yi Zhang, zhangwm, linux-kernel, zhouqiao
2013/10/14 Mark Brown <broonie@kernel.org>:
> 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;
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-10-15 12:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-15 5:03 [PATCH] regmap: irq: clear status when disable irq Yi Zhang
2013-10-15 11:48 ` Mark Brown
2013-10-15 12:03 ` Yi Zhang
-- strict thread matches above, loose matches on Subject: below --
2013-10-14 4:23 Yi Zhang
2013-10-14 11:09 ` Mark Brown
2013-10-14 11:55 ` Yi Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).