irq/irq_sim: add locking
PATCH 1/1 irq/irq_sim: add locking
Two threads can try to fire the irq_sim with different offsets and will end up fighting for the irq_work asignment. To fix it: add a mutex and lock it before firing.
--- include/linux/irq_sim.h | 1 + kernel/irq/irq_sim.c | 5 +++++ 2 files changed, 6 insertions(+)
include/linux/irq_sim.h +1 -0
@@ -29,6 +29,7 @@ struct irq_sim {int irq_base;unsigned int irq_count;struct irq_sim_irq_ctx *irqs; +struct mutex lock; };int irq_sim_init (struct irq_sim *sim,unsigned int num_irqs);
kernel/irq/irq_sim.c +5 -0
@@ -74,6 +74,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) }init_irq_work (&sim->work_ctx.work, irq_sim_handle_irq); +mutex_init (&sim->lock); sim->irq_count = num_irqs;return sim->irq_base;
@@ -142,10 +143,14 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_init);*/ void irq_sim_fire (struct irq_sim *sim,unsigned int offset) { +mutex_lock (&sim->lock); +if (sim->irqs[offset].enabled) { sim->work_ctx.irq =irq_sim_irqnum (sim, offset);irq_work_queue (&sim->work_ctx.work); } + +mutex_unlock (&sim->lock); }EXPORT_SYMBOL_GPL (irq_sim_fire);
Signature · 1 line
2.19.1
Hello Bartosz,
Quoted 37 lines · Bartosz Golaszewski
Two threads can try to fire the
irq_simwith different offsets and will end up fighting for theirq_workasignment. To fix it: add a mutex and lock it before firing.Suggested-by Uwe Kleine-König <[email protected]>Signed-off-by Bartosz Golaszewski <[email protected]>--- include/linux/irq_sim.h | 1 + kernel/irq/irq_sim.c | 5 +++++ 2 files changed, 6 insertions(+)include/linux/irq_sim.h +1
-0@@ -29,6 +29,7 @@ struct irq_sim {int irq_base;unsigned int irq_count;struct irq_sim_irq_ctx *irqs; +struct mutex lock; };int irq_sim_init (struct irq_sim *sim,unsigned int num_irqs);kernel/irq/irq_sim.c +1
-0@@ -74,6 +74,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) }init_irq_work (&sim->work_ctx.work, irq_sim_handle_irq); +mutex_init (&sim->lock); sim->irq_count = num_irqs;return sim->irq_base;@@ -142,10 +143,14 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_init);*/
kernel/irq/irq_sim.c EXPORT_SYMBOL_GPL(devm_irq_sim_init); · line 153
Quoted 10 lines
kernel/irq/irq_sim.c +4
-0void irq_sim_fire (struct irq_sim *sim,unsigned int offset) { +mutex_lock (&sim->lock); +if (sim->irqs[offset].enabled) { sim->work_ctx.irq =irq_sim_irqnum (sim, offset);irq_work_queue (&sim->work_ctx.work); } + +mutex_unlock (&sim->lock);
This doesn't fix the issue I think. irq_work_queue() only schedules the work function. If after irq_sim_fire() returned but before the worker runs another irq_sim_fire() is issued the value is still overwritten.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
czw., 8 lis 2018 o 20:41 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 47 lines
Hello Bartosz,
Quoted 43 lines · Bartosz Golaszewski
Two threads can try to fire the
irq_simwith different offsets and will end up fighting for theirq_workasignment. To fix it: add a mutex and lock it before firing.Suggested-by Uwe Kleine-König <[email protected]>Signed-off-by Bartosz Golaszewski <[email protected]>--- include/linux/irq_sim.h | 1 + kernel/irq/irq_sim.c | 5 +++++ 2 files changed, 6 insertions(+)include/linux/irq_sim.h +1
-0@@ -29,6 +29,7 @@ struct irq_sim {int irq_base;unsigned int irq_count;struct irq_sim_irq_ctx *irqs; +struct mutex lock; };int irq_sim_init (struct irq_sim *sim,unsigned int num_irqs);kernel/irq/irq_sim.c +3
-0@@ -74,6 +74,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) }init_irq_work (&sim->work_ctx.work, irq_sim_handle_irq); +mutex_init (&sim->lock); sim->irq_count = num_irqs;return sim->irq_base;@@ -142,10 +143,14 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_init);*/ void irq_sim_fire (struct irq_sim *sim,unsigned int offset) { +mutex_lock (&sim->lock); +if (sim->irqs[offset].enabled) { sim->work_ctx.irq =irq_sim_irqnum (sim, offset);
Quoted 9 lines
kernel/irq/irq_sim.c EXPORT_SYMBOL_GPL(devm_irq_sim_init); · line 153
Quoted 4 lines
kernel/irq/irq_sim.c +2
-0irq_work_queue (&sim->work_ctx.work); } + +mutex_unlock (&sim->lock);This doesn't fix the issue I think.
irq_work_queue()only schedules the work function. If afterirq_sim_fire()returned but before the worker runs anotherirq_sim_fire()is issued the value is still overwritten.
Looking at irq_work_queue(): while there may be some arch-specific details deeper down the stack, it seems that unless the work is IRQ_WORK_LAZY, the handler should be executed immediately. I'll verify tomorrow though.
Bart
Hello,
Quoted 19 lines · Bartosz Golaszewski
czw., 8 lis 2018 o 20:41 Uwe Kleine-König
<[email protected]> napisał(a):Quoted 8 lines
Quoted 8 lines
kernel/irq/irq_sim.c +2
-0@@ -142,10 +143,14 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_init);*/ void irq_sim_fire (struct irq_sim *sim,unsigned int offset) { +mutex_lock (&sim->lock); +if (sim->irqs[offset].enabled) { sim->work_ctx.irq =irq_sim_irqnum (sim, offset);Quoted 8 lines
kernel/irq/irq_sim.c EXPORT_SYMBOL_GPL(devm_irq_sim_init); · line 153
Quoted 4 lines
kernel/irq/irq_sim.c +2
-0irq_work_queue (&sim->work_ctx.work); } + +mutex_unlock (&sim->lock);This doesn't fix the issue I think.
irq_work_queue()only schedules the work function. If afterirq_sim_fire()returned but before the worker runs anotherirq_sim_fire()is issued the value is still overwritten.
Quoted 4 lines
Looking at
irq_work_queue(): while there may be some arch-specific details deeper down the stack, it seems that unless the work isIRQ_WORK_LAZY, the handler should be executed immediately. I'll verify tomorrow though.
not considering the IRQ_WORK_LAZY case irq_work_queue adds the work struct to a list and then calls arch_irq_work_raise(). The default implementation for this function is empty. alpha, arm, arm64, powerpc, sparc and x86 have alternative implementations. Quickly looking at the arm one: It is only used on SMP. Also given that all relevant code of irq_work_queue is protected by preempt_disable/preempt_enable this cannot atomically call the work function, otherwise it would run with preemption disabled which isn't the case AFAIK.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
Quoted 17 lines · Uwe Kleine-König
Quoted 16 lines · Bartosz Golaszewski
kernel/irq/irq_sim.c +3
-0@@ -74,6 +74,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) }init_irq_work (&sim->work_ctx.work, irq_sim_handle_irq); +mutex_init (&sim->lock); sim->irq_count = num_irqs;return sim->irq_base;@@ -142,10 +143,14 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_init);*/ void irq_sim_fire (struct irq_sim *sim,unsigned int offset) { +mutex_lock (&sim->lock); +if (sim->irqs[offset].enabled) { sim->work_ctx.irq =irq_sim_irqnum (sim, offset);
Quoted 8 lines
kernel/irq/irq_sim.c EXPORT_SYMBOL_GPL(devm_irq_sim_init); · line 153
Quoted 4 lines
kernel/irq/irq_sim.c +2
-0irq_work_queue (&sim->work_ctx.work); } + +mutex_unlock (&sim->lock);This doesn't fix the issue I think.
irq_work_queue()only schedules the work function. If afterirq_sim_fire()returned but before the worker runs anotherirq_sim_fire()is issued the value is still overwritten.
Right. So the obvious solution is to avoid the irq number store and use a bitfield instead.
struct irq_sim_work_ctx {
...
unsigned long pending;
};fire(sim, offset)
{
if (!sim->irqs[offset].enabled)
return;
set_bit(offset, &sim->work_ctx.pending);
....and in the work handler do:
handle(work)
{
struct irq_sim_work_ctx *ctx = container_of(work,....);
while (ctx->pending) {
offs = ffs(ctx->pending);
clr_bit(offs, &ctx->pending);
handle_simple_irq(offs);
}
}Or something like that.
Thanks,
tglx
pt., 9 lis 2018 o 11:19 Thomas Gleixner <[email protected]> napisał(a):
Quoted 57 lines · Uwe Kleine-König
Quoted 17 lines · Bartosz Golaszewski
Quoted 16 lines
kernel/irq/irq_sim.c +3
-0@@ -74,6 +74,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) }init_irq_work (&sim->work_ctx.work, irq_sim_handle_irq); +mutex_init (&sim->lock); sim->irq_count = num_irqs;return sim->irq_base;@@ -142,10 +143,14 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_init);*/ void irq_sim_fire (struct irq_sim *sim,unsigned int offset) { +mutex_lock (&sim->lock); +if (sim->irqs[offset].enabled) { sim->work_ctx.irq =irq_sim_irqnum (sim, offset);Quoted 8 lines
kernel/irq/irq_sim.c EXPORT_SYMBOL_GPL(devm_irq_sim_init); · line 153
Quoted 4 lines
kernel/irq/irq_sim.c +2
-0irq_work_queue (&sim->work_ctx.work); } + +mutex_unlock (&sim->lock);This doesn't fix the issue I think.
irq_work_queue()only schedules the work function. If afterirq_sim_fire()returned but before the worker runs anotherirq_sim_fire()is issued the value is still overwritten.Right. So the obvious solution is to avoid the irq number store and use a bitfield instead.
struct irq_sim_work_ctx { ... unsigned long pending; };
fire(sim, offset){ if (!sim->irqs[offset].enabled) return; set_bit(offset, &sim->work_ctx.pending); ....and in the work handler do:
handle(work)
{ struct irq_sim_work_ctx *ctx = container_of(work,....); while (ctx->pending) { offs = ffs(ctx->pending); clr_bit(offs, &ctx->pending); handle_simple_irq(offs); } }
Quoted 5 lines
Or something like that.
Thanks,
tglx
Actually on ARM and x86 with smp the handler is called between the mutex lock and unlock alright but this solution looks much better. I'll resend a v2.
Thanks!
Bartosz