* [patch 0/2] x86/ioapic: Prevent inconsistent state
@ 2019-10-17 10:19 Thomas Gleixner
2019-10-17 10:19 ` [patch 1/2] x86/ioapic: Prevent inconsistent state when moving an interrupt Thomas Gleixner
2019-10-17 10:19 ` [patch 2/2] x86/ioapic: Rename misnamed functions Thomas Gleixner
0 siblings, 2 replies; 5+ messages in thread
From: Thomas Gleixner @ 2019-10-17 10:19 UTC (permalink / raw)
To: LKML; +Cc: x86, Sebastian Siewior, Andy Shevchenko
The following patch fell through the cracks:
https://lore.kernel.org/lkml/20180717162531.7d4dmhbu5ijqg2uw@linutronix.de/
Instead of applying it as is, I've split it up into two pieces:
- Fix the inconsistent mask state
- Rename the functions
so the fix can be trivialy backported.
Thanks,
tglx
^ permalink raw reply [flat|nested] 5+ messages in thread
* [patch 1/2] x86/ioapic: Prevent inconsistent state when moving an interrupt
2019-10-17 10:19 [patch 0/2] x86/ioapic: Prevent inconsistent state Thomas Gleixner
@ 2019-10-17 10:19 ` Thomas Gleixner
2019-10-24 10:13 ` [tip: x86/apic] " tip-bot2 for Thomas Gleixner
2019-10-17 10:19 ` [patch 2/2] x86/ioapic: Rename misnamed functions Thomas Gleixner
1 sibling, 1 reply; 5+ messages in thread
From: Thomas Gleixner @ 2019-10-17 10:19 UTC (permalink / raw)
To: LKML; +Cc: x86, Sebastian Siewior, Andy Shevchenko
There is an issue with threaded interrupts which are marked ONESHOT
and using the fasteoi handler.
if (IS_ONESHOT())
mask_irq();
....
cond_unmask_eoi_irq()
chip->irq_eoi();
if (setaffinity_pending) {
mask_ioapic();
...
move_affinity();
unmask_ioapic();
}
So if setaffinity is pending the interrupt will be moved and then
unconditionally unmasked at the ioapic level, which is wrong in two
aspects:
1) It should be kept masked up to the point where the threaded handler
finished.
2) The physical chip state and the software masked state are inconsistent
Guard both the mask and the unmask with a check for the software masked
state. If the line is marked masked then the ioapic line is also masked, so
both mask_ioapic() and unmask_ioapic() can be skipped safely.
Fixes: 3aa551c9b4c4 ("genirq: add threaded interrupt handler support")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
arch/x86/kernel/apic/io_apic.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1729,7 +1729,8 @@ static inline bool ioapic_irqd_mask(stru
{
/* If we are moving the irq we need to mask it */
if (unlikely(irqd_is_setaffinity_pending(data))) {
- mask_ioapic_irq(data);
+ if (!irqd_irq_masked(data))
+ mask_ioapic_irq(data);
return true;
}
return false;
@@ -1766,7 +1767,9 @@ static inline void ioapic_irqd_unmask(st
*/
if (!io_apic_level_ack_pending(data->chip_data))
irq_move_masked_irq(data);
- unmask_ioapic_irq(data);
+ /* If the irq is masked in the core, leave it */
+ if (!irqd_irq_masked(data))
+ unmask_ioapic_irq(data);
}
}
#else
^ permalink raw reply [flat|nested] 5+ messages in thread
* [patch 2/2] x86/ioapic: Rename misnamed functions
2019-10-17 10:19 [patch 0/2] x86/ioapic: Prevent inconsistent state Thomas Gleixner
2019-10-17 10:19 ` [patch 1/2] x86/ioapic: Prevent inconsistent state when moving an interrupt Thomas Gleixner
@ 2019-10-17 10:19 ` Thomas Gleixner
2019-10-24 10:13 ` [tip: x86/apic] " tip-bot2 for Thomas Gleixner
1 sibling, 1 reply; 5+ messages in thread
From: Thomas Gleixner @ 2019-10-17 10:19 UTC (permalink / raw)
To: LKML; +Cc: x86, Sebastian Siewior, Andy Shevchenko
ioapic_irqd_[un]mask() are misnomers as both functions do way more than
masking and unmasking the interrupt line. Both deal with the moving the
affinity of the interrupt within interrupt context. The mask/unmask is just
a tiny part of the functionality.
Rename them to ioapic_prepare/finish_move(), fixup the call sites and
rename the related variables in the code to reflect what this is about.
No functional change.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
arch/x86/kernel/apic/io_apic.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1725,7 +1725,7 @@ static bool io_apic_level_ack_pending(st
return false;
}
-static inline bool ioapic_irqd_mask(struct irq_data *data)
+static inline bool ioapic_prepare_move(struct irq_data *data)
{
/* If we are moving the irq we need to mask it */
if (unlikely(irqd_is_setaffinity_pending(data))) {
@@ -1736,9 +1736,9 @@ static inline bool ioapic_irqd_mask(stru
return false;
}
-static inline void ioapic_irqd_unmask(struct irq_data *data, bool masked)
+static inline void ioapic_finish_move(struct irq_data *data, bool moveit)
{
- if (unlikely(masked)) {
+ if (unlikely(moveit)) {
/* Only migrate the irq if the ack has been received.
*
* On rare occasions the broadcast level triggered ack gets
@@ -1773,11 +1773,11 @@ static inline void ioapic_irqd_unmask(st
}
}
#else
-static inline bool ioapic_irqd_mask(struct irq_data *data)
+static inline bool ioapic_prepare_move(struct irq_data *data)
{
return false;
}
-static inline void ioapic_irqd_unmask(struct irq_data *data, bool masked)
+static inline void ioapic_finish_move(struct irq_data *data, bool moveit)
{
}
#endif
@@ -1786,11 +1786,11 @@ static void ioapic_ack_level(struct irq_
{
struct irq_cfg *cfg = irqd_cfg(irq_data);
unsigned long v;
- bool masked;
+ bool moveit;
int i;
irq_complete_move(cfg);
- masked = ioapic_irqd_mask(irq_data);
+ moveit = ioapic_prepare_move(irq_data);
/*
* It appears there is an erratum which affects at least version 0x11
@@ -1845,7 +1845,7 @@ static void ioapic_ack_level(struct irq_
eoi_ioapic_pin(cfg->vector, irq_data->chip_data);
}
- ioapic_irqd_unmask(irq_data, masked);
+ ioapic_finish_move(irq_data, moveit);
}
static void ioapic_ir_ack_level(struct irq_data *irq_data)
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tip: x86/apic] x86/ioapic: Rename misnamed functions
2019-10-17 10:19 ` [patch 2/2] x86/ioapic: Rename misnamed functions Thomas Gleixner
@ 2019-10-24 10:13 ` tip-bot2 for Thomas Gleixner
0 siblings, 0 replies; 5+ messages in thread
From: tip-bot2 for Thomas Gleixner @ 2019-10-24 10:13 UTC (permalink / raw)
To: linux-tip-commits
Cc: Thomas Gleixner, Andy Shevchenko, Linus Torvalds, Peter Zijlstra,
Sebastian Siewior, Ingo Molnar, Borislav Petkov, linux-kernel
The following commit has been merged into the x86/apic branch of tip:
Commit-ID: 2579a4eefc04d1c23eef8f3f0db3309f955e5792
Gitweb: https://git.kernel.org/tip/2579a4eefc04d1c23eef8f3f0db3309f955e5792
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 17 Oct 2019 12:19:02 +02:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Thu, 24 Oct 2019 12:09:21 +02:00
x86/ioapic: Rename misnamed functions
ioapic_irqd_[un]mask() are misnomers as both functions do way more than
masking and unmasking the interrupt line. Both deal with the moving the
affinity of the interrupt within interrupt context. The mask/unmask is just
a tiny part of the functionality.
Rename them to ioapic_prepare/finish_move(), fixup the call sites and
rename the related variables in the code to reflect what this is about.
No functional change.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sebastian Siewior <bigeasy@linutronix.de>
Link: https://lkml.kernel.org/r/20191017101938.412489856@linutronix.de
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
arch/x86/kernel/apic/io_apic.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index f0262cb..913c886 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1725,7 +1725,7 @@ static bool io_apic_level_ack_pending(struct mp_chip_data *data)
return false;
}
-static inline bool ioapic_irqd_mask(struct irq_data *data)
+static inline bool ioapic_prepare_move(struct irq_data *data)
{
/* If we are moving the IRQ we need to mask it */
if (unlikely(irqd_is_setaffinity_pending(data))) {
@@ -1736,9 +1736,9 @@ static inline bool ioapic_irqd_mask(struct irq_data *data)
return false;
}
-static inline void ioapic_irqd_unmask(struct irq_data *data, bool masked)
+static inline void ioapic_finish_move(struct irq_data *data, bool moveit)
{
- if (unlikely(masked)) {
+ if (unlikely(moveit)) {
/* Only migrate the irq if the ack has been received.
*
* On rare occasions the broadcast level triggered ack gets
@@ -1773,11 +1773,11 @@ static inline void ioapic_irqd_unmask(struct irq_data *data, bool masked)
}
}
#else
-static inline bool ioapic_irqd_mask(struct irq_data *data)
+static inline bool ioapic_prepare_move(struct irq_data *data)
{
return false;
}
-static inline void ioapic_irqd_unmask(struct irq_data *data, bool masked)
+static inline void ioapic_finish_move(struct irq_data *data, bool moveit)
{
}
#endif
@@ -1786,11 +1786,11 @@ static void ioapic_ack_level(struct irq_data *irq_data)
{
struct irq_cfg *cfg = irqd_cfg(irq_data);
unsigned long v;
- bool masked;
+ bool moveit;
int i;
irq_complete_move(cfg);
- masked = ioapic_irqd_mask(irq_data);
+ moveit = ioapic_prepare_move(irq_data);
/*
* It appears there is an erratum which affects at least version 0x11
@@ -1845,7 +1845,7 @@ static void ioapic_ack_level(struct irq_data *irq_data)
eoi_ioapic_pin(cfg->vector, irq_data->chip_data);
}
- ioapic_irqd_unmask(irq_data, masked);
+ ioapic_finish_move(irq_data, moveit);
}
static void ioapic_ir_ack_level(struct irq_data *irq_data)
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [tip: x86/apic] x86/ioapic: Prevent inconsistent state when moving an interrupt
2019-10-17 10:19 ` [patch 1/2] x86/ioapic: Prevent inconsistent state when moving an interrupt Thomas Gleixner
@ 2019-10-24 10:13 ` tip-bot2 for Thomas Gleixner
0 siblings, 0 replies; 5+ messages in thread
From: tip-bot2 for Thomas Gleixner @ 2019-10-24 10:13 UTC (permalink / raw)
To: linux-tip-commits
Cc: Thomas Gleixner, Andy Shevchenko, Linus Torvalds, Peter Zijlstra,
Sebastian Siewior, Ingo Molnar, Borislav Petkov, linux-kernel
The following commit has been merged into the x86/apic branch of tip:
Commit-ID: df4393424af3fbdcd5c404077176082a8ce459c4
Gitweb: https://git.kernel.org/tip/df4393424af3fbdcd5c404077176082a8ce459c4
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 17 Oct 2019 12:19:01 +02:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Thu, 24 Oct 2019 12:09:21 +02:00
x86/ioapic: Prevent inconsistent state when moving an interrupt
There is an issue with threaded interrupts which are marked ONESHOT
and using the fasteoi handler:
if (IS_ONESHOT())
mask_irq();
....
cond_unmask_eoi_irq()
chip->irq_eoi();
if (setaffinity_pending) {
mask_ioapic();
...
move_affinity();
unmask_ioapic();
}
So if setaffinity is pending the interrupt will be moved and then
unconditionally unmasked at the ioapic level, which is wrong in two
aspects:
1) It should be kept masked up to the point where the threaded handler
finished.
2) The physical chip state and the software masked state are inconsistent
Guard both the mask and the unmask with a check for the software masked
state. If the line is marked masked then the ioapic line is also masked, so
both mask_ioapic() and unmask_ioapic() can be skipped safely.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sebastian Siewior <bigeasy@linutronix.de>
Fixes: 3aa551c9b4c4 ("genirq: add threaded interrupt handler support")
Link: https://lkml.kernel.org/r/20191017101938.321393687@linutronix.de
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
arch/x86/kernel/apic/io_apic.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index d6af97f..f0262cb 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1727,9 +1727,10 @@ static bool io_apic_level_ack_pending(struct mp_chip_data *data)
static inline bool ioapic_irqd_mask(struct irq_data *data)
{
- /* If we are moving the irq we need to mask it */
+ /* If we are moving the IRQ we need to mask it */
if (unlikely(irqd_is_setaffinity_pending(data))) {
- mask_ioapic_irq(data);
+ if (!irqd_irq_masked(data))
+ mask_ioapic_irq(data);
return true;
}
return false;
@@ -1766,7 +1767,9 @@ static inline void ioapic_irqd_unmask(struct irq_data *data, bool masked)
*/
if (!io_apic_level_ack_pending(data->chip_data))
irq_move_masked_irq(data);
- unmask_ioapic_irq(data);
+ /* If the IRQ is masked in the core, leave it: */
+ if (!irqd_irq_masked(data))
+ unmask_ioapic_irq(data);
}
}
#else
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-10-24 10:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-17 10:19 [patch 0/2] x86/ioapic: Prevent inconsistent state Thomas Gleixner
2019-10-17 10:19 ` [patch 1/2] x86/ioapic: Prevent inconsistent state when moving an interrupt Thomas Gleixner
2019-10-24 10:13 ` [tip: x86/apic] " tip-bot2 for Thomas Gleixner
2019-10-17 10:19 ` [patch 2/2] x86/ioapic: Rename misnamed functions Thomas Gleixner
2019-10-24 10:13 ` [tip: x86/apic] " tip-bot2 for Thomas Gleixner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.