2020-01-06 15:45:47

by chengkaitao

Subject: [RESEND v2] irq: Refactor irq_wait_for_interrupt info to simplify the code

From: Kaitao Cheng <[email protected]>

Cleanup extra if(test_and_clear_bit), and put the other one in front.

Signed-off-by: Kaitao Cheng <[email protected]>
---
kernel/irq/manage.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 1753486b440c..7266d0d30fa9 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -863,21 +863,15 @@ static int irq_wait_for_interrupt(struct irqaction *action)
for (;;) {
set_current_state(TASK_INTERRUPTIBLE);

- if (kthread_should_stop()) {
- /* may need to run one last time */
- if (test_and_clear_bit(IRQTF_RUNTHREAD,
- &action->thread_flags)) {
- __set_current_state(TASK_RUNNING);
- return 0;
- }
+ if (test_and_clear_bit(IRQTF_RUNTHREAD,
+ &action->thread_flags)) {
__set_current_state(TASK_RUNNING);
- return -1;
+ return 0;
}

- if (test_and_clear_bit(IRQTF_RUNTHREAD,
- &action->thread_flags)) {
+ if (kthread_should_stop()) {
__set_current_state(TASK_RUNNING);
- return 0;
+ return -1;
}
schedule();
}
--
2.20.1

2020-01-15 13:44:44

by Thomas Gleixner

Subject: Re: [RESEND v2] irq: Refactor irq_wait_for_interrupt info to simplify the code

chengkaitao <[email protected]> writes:
> From: Kaitao Cheng <[email protected]>
>
> Cleanup extra if(test_and_clear_bit), and put the other one in front.

That simplifies the code but opens a race window:

CPU 0 CPU 1
irq_wait_for_interrupt()
has not yet reached schedule()
free_irq()
remove_action();
synchronize_irq();

#ifdef CONFIG_DEBUG_SHIRQ
action->handler() if (test_and_clear_bit())
---> bit is not set yet
--> SET thread running
#endif

kthread_stop() if (kthread_stop())

---> Leave with bit set and thread active count != 0

That's just the most obvious example...

Thanks,

tglx