This patch series backports a bunch of patches related IRQ handling
with respect to freeing the irq line while IRQ is in flight at CPU
or at the hardware level.
Recently we saw this issue in serial 8250 driver where the IRQ was being
freed while the irq was in flight or not yet delivered to the CPU. As a
result the irqchip was going into a wedged state and IRQ was not getting
delivered to the cpu. These patches helped fixed the issue in 4.14
kernel.
Let us know if more patches need backporting.
Lukas Wunner (2):
genirq: Update code comments wrt recycled thread_mask
genirq: Synchronize only with single thread on free_irq()
Thomas Gleixner (4):
genirq: Delay deactivation in free_irq()
genirq: Fix misleading synchronize_irq() documentation
genirq: Add optional hardware synchronization for shutdown
x86/ioapic: Implement irq_get_irqchip_state() callback
arch/x86/kernel/apic/io_apic.c | 46 ++++++++++++++
kernel/irq/autoprobe.c | 6 +-
kernel/irq/chip.c | 6 ++
kernel/irq/cpuhotplug.c | 2 +-
kernel/irq/internals.h | 5 ++
kernel/irq/manage.c | 106 ++++++++++++++++++++++-----------
6 files changed, 133 insertions(+), 38 deletions(-)
--
2.37.1
From: Thomas Gleixner <[email protected]>
commit 62e0468650c30f0298822c580f382b16328119f6 upstream.
free_irq() ensures that no hardware interrupt handler is executing on a
different CPU before actually releasing resources and deactivating the
interrupt completely in a domain hierarchy.
But that does not catch the case where the interrupt is on flight at the
hardware level but not yet serviced by the target CPU. That creates an
interesing race condition:
CPU 0 CPU 1 IRQ CHIP
interrupt is raised
sent to CPU1
Unable to handle
immediately
(interrupts off,
deep idle delay)
mask()
...
free()
shutdown()
synchronize_irq()
release_resources()
do_IRQ()
-> resources are not available
That might be harmless and just trigger a spurious interrupt warning, but
some interrupt chips might get into a wedged state.
Utilize the existing irq_get_irqchip_state() callback for the
synchronization in free_irq().
synchronize_hardirq() is not using this mechanism as it might actually
deadlock unter certain conditions, e.g. when called with interrupts
disabled and the target CPU is the one on which the synchronization is
invoked. synchronize_irq() uses it because that function cannot be called
from non preemtible contexts as it might sleep.
No functional change intended and according to Marc the existing GIC
implementations where the driver supports the callback should be able
to cope with that core change. Famous last words.
Fixes: 464d12309e1b ("x86/vector: Switch IOAPIC to global reservation mode")
Reported-by: Robert Hodaszi <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Marc Zyngier <[email protected]>
Tested-by: Marc Zyngier <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
Signed-off-by: Rishabh Bhatnagar <[email protected]>
---
kernel/irq/internals.h | 4 ++++
kernel/irq/manage.c | 53 +++++++++++++++++++++++++++---------------
2 files changed, 38 insertions(+), 19 deletions(-)
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 3de3821ab5fe..a3d7150bb295 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -93,6 +93,10 @@ static inline void irq_mark_irq(unsigned int irq) { }
extern void irq_mark_irq(unsigned int irq);
#endif
+extern int __irq_get_irqchip_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool *state);
+
extern void init_kstat_irqs(struct irq_desc *desc, int node, int nr);
irqreturn_t __handle_irq_event_percpu(struct irq_desc *desc, unsigned int *flags);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index a72d7ae0418b..fb3ea75cc0fb 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -35,8 +35,9 @@ static int __init setup_forced_irqthreads(char *arg)
early_param("threadirqs", setup_forced_irqthreads);
#endif
-static void __synchronize_hardirq(struct irq_desc *desc)
+static void __synchronize_hardirq(struct irq_desc *desc, bool sync_chip)
{
+ struct irq_data *irqd = irq_desc_get_irq_data(desc);
bool inprogress;
do {
@@ -52,6 +53,20 @@ static void __synchronize_hardirq(struct irq_desc *desc)
/* Ok, that indicated we're done: double-check carefully. */
raw_spin_lock_irqsave(&desc->lock, flags);
inprogress = irqd_irq_inprogress(&desc->irq_data);
+
+ /*
+ * If requested and supported, check at the chip whether it
+ * is in flight at the hardware level, i.e. already pending
+ * in a CPU and waiting for service and acknowledge.
+ */
+ if (!inprogress && sync_chip) {
+ /*
+ * Ignore the return code. inprogress is only updated
+ * when the chip supports it.
+ */
+ __irq_get_irqchip_state(irqd, IRQCHIP_STATE_ACTIVE,
+ &inprogress);
+ }
raw_spin_unlock_irqrestore(&desc->lock, flags);
/* Oops, that failed? */
@@ -74,13 +89,18 @@ static void __synchronize_hardirq(struct irq_desc *desc)
* Returns: false if a threaded handler is active.
*
* This function may be called - with care - from IRQ context.
+ *
+ * It does not check whether there is an interrupt in flight at the
+ * hardware level, but not serviced yet, as this might deadlock when
+ * called with interrupts disabled and the target CPU of the interrupt
+ * is the current CPU.
*/
bool synchronize_hardirq(unsigned int irq)
{
struct irq_desc *desc = irq_to_desc(irq);
if (desc) {
- __synchronize_hardirq(desc);
+ __synchronize_hardirq(desc, false);
return !atomic_read(&desc->threads_active);
}
@@ -98,13 +118,17 @@ EXPORT_SYMBOL(synchronize_hardirq);
*
* Can only be called from preemptible code as it might sleep when
* an interrupt thread is associated to @irq.
+ *
+ * It optionally makes sure (when the irq chip supports that method)
+ * that the interrupt is not pending in any CPU and waiting for
+ * service.
*/
void synchronize_irq(unsigned int irq)
{
struct irq_desc *desc = irq_to_desc(irq);
if (desc) {
- __synchronize_hardirq(desc);
+ __synchronize_hardirq(desc, true);
/*
* We made sure that no hardirq handler is
* running. Now verify that no threaded handlers are
@@ -1635,8 +1659,12 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
unregister_handler_proc(irq, action);
- /* Make sure it's not being used on another CPU: */
- synchronize_hardirq(irq);
+ /*
+ * Make sure it's not being used on another CPU and if the chip
+ * supports it also make sure that there is no (not yet serviced)
+ * interrupt in flight at the hardware level.
+ */
+ __synchronize_hardirq(desc, true);
#ifdef CONFIG_DEBUG_SHIRQ
/*
@@ -2187,7 +2215,6 @@ int irq_get_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
{
struct irq_desc *desc;
struct irq_data *data;
- struct irq_chip *chip;
unsigned long flags;
int err = -EINVAL;
@@ -2197,19 +2224,7 @@ int irq_get_irqchip_state(unsigned int irq, enum irqchip_irq_state which,
data = irq_desc_get_irq_data(desc);
- do {
- chip = irq_data_get_irq_chip(data);
- if (chip->irq_get_irqchip_state)
- break;
-#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
- data = data->parent_data;
-#else
- data = NULL;
-#endif
- } while (data);
-
- if (data)
- err = chip->irq_get_irqchip_state(data, which, state);
+ err = __irq_get_irqchip_state(data, which, state);
irq_put_desc_busunlock(desc, flags);
return err;
--
2.37.1
On Thu, Sep 29, 2022 at 09:06:45PM +0000, Rishabh Bhatnagar wrote:
> This patch series backports a bunch of patches related IRQ handling
> with respect to freeing the irq line while IRQ is in flight at CPU
> or at the hardware level.
> Recently we saw this issue in serial 8250 driver where the IRQ was being
> freed while the irq was in flight or not yet delivered to the CPU. As a
> result the irqchip was going into a wedged state and IRQ was not getting
> delivered to the cpu. These patches helped fixed the issue in 4.14
> kernel.
Why is the serial driver freeing an irq while the system is running?
Ah, this could happen on a tty hangup, right?
> Let us know if more patches need backporting.
What hardware platform were these patches tested on to verify they work
properly? And why can't they move to 4.19 or newer if they really need
this fix? What's preventing that?
As Amazon doesn't seem to be testing 4.14.y -rc releases, I find it odd
that you all did this backport. Is this a kernel that you all care
about?
thanks,
greg k-h
On 10/2/22, 8:30 AM, "Greg KH" <[email protected]> wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
On Thu, Sep 29, 2022 at 09:06:45PM +0000, Rishabh Bhatnagar wrote:
> This patch series backports a bunch of patches related IRQ handling
> with respect to freeing the irq line while IRQ is in flight at CPU
> or at the hardware level.
> Recently we saw this issue in serial 8250 driver where the IRQ was being
> freed while the irq was in flight or not yet delivered to the CPU. As a
> result the irqchip was going into a wedged state and IRQ was not getting
> delivered to the cpu. These patches helped fixed the issue in 4.14
> kernel.
Why is the serial driver freeing an irq while the system is running?
Ah, this could happen on a tty hangup, right?
Yes, exactly during tty hangup we see this sequence happening.
It doesn't happen on every hangup but can be reproduced within 10 tries. We didn't see the same
behavior in 5.10 and hence found these commits.
> Let us know if more patches need backporting.
What hardware platform were these patches tested on to verify they work
properly? And why can't they move to 4.19 or newer if they really need
this fix? What's preventing that?
As Amazon doesn't seem to be testing 4.14.y -rc releases, I find it odd
that you all did this backport. Is this a kernel that you all care
about?
These were tested on Intel x86_64 (Xeon Platinum 8259).
Amazon linux 2 still supports 4.14 kernel for our customers, so we would need to fix that.
thanks,
greg k-h
(putting my @amazon.com hat on)
On Sun, 2022-10-02 at 17:30 +0200, Greg KH wrote:
> On Thu, Sep 29, 2022 at 09:06:45PM +0000, Rishabh Bhatnagar wrote:
> > This patch series backports a bunch of patches related IRQ handling
> > with respect to freeing the irq line while IRQ is in flight at CPU
> > or at the hardware level.
> > Recently we saw this issue in serial 8250 driver where the IRQ was
> > being
> > freed while the irq was in flight or not yet delivered to the CPU.
> > As a
> > result the irqchip was going into a wedged state and IRQ was not
> > getting
> > delivered to the cpu. These patches helped fixed the issue in 4.14
> > kernel.
>
> Why is the serial driver freeing an irq while the system is running?
> Ah, this could happen on a tty hangup, right?
Right. Rishabh answered that separately.
> > Let us know if more patches need backporting.
>
> What hardware platform were these patches tested on to verify they
> work properly? And why can't they move to 4.19 or newer if they
> really need this fix? What's preventing that?
>
> As Amazon doesn't seem to be testing 4.14.y -rc releases, I find it
> odd that you all did this backport. Is this a kernel that you all
> care about?
These were tested on a collection of EC2 instances, virtual and metal I
believe (Rishabh, please confirm).
Amazon Linux 2 runs 4.14 or 5.10. Unfortunately we still have to
support customers running the former.
We'll be including these patches in our releases, we thought it would
be nice to have them in -stable as well for the sake of whoever else
might be still using this kernel. No huge deal if they don't.
As for testing -rc's, yes, we need to get better at that (and publish
what we test). Point taken :-)
Cheers,
Ben.
On 10/6/22 8:07 PM, Herrenschmidt, Benjamin wrote:
> (putting my @amazon.com hat on)
>
> On Sun, 2022-10-02 at 17:30 +0200, Greg KH wrote:
>
>
>> On Thu, Sep 29, 2022 at 09:06:45PM +0000, Rishabh Bhatnagar wrote:
>>> This patch series backports a bunch of patches related IRQ handling
>>> with respect to freeing the irq line while IRQ is in flight at CPU
>>> or at the hardware level.
>>> Recently we saw this issue in serial 8250 driver where the IRQ was
>>> being
>>> freed while the irq was in flight or not yet delivered to the CPU.
>>> As a
>>> result the irqchip was going into a wedged state and IRQ was not
>>> getting
>>> delivered to the cpu. These patches helped fixed the issue in 4.14
>>> kernel.
>> Why is the serial driver freeing an irq while the system is running?
>> Ah, this could happen on a tty hangup, right?
> Right. Rishabh answered that separately.
>
>>> Let us know if more patches need backporting.
>> What hardware platform were these patches tested on to verify they
>> work properly? And why can't they move to 4.19 or newer if they
>> really need this fix? What's preventing that?
>>
>> As Amazon doesn't seem to be testing 4.14.y -rc releases, I find it
>> odd that you all did this backport. Is this a kernel that you all
>> care about?
> These were tested on a collection of EC2 instances, virtual and metal I
> believe (Rishabh, please confirm).
Yes these patches were tested on multiple virt/metal EC2 instances.
>
> Amazon Linux 2 runs 4.14 or 5.10. Unfortunately we still have to
> support customers running the former.
>
> We'll be including these patches in our releases, we thought it would
> be nice to have them in -stable as well for the sake of whoever else
> might be still using this kernel. No huge deal if they don't.
>
> As for testing -rc's, yes, we need to get better at that (and publish
> what we test). Point taken :-)
>
> Cheers,
> Ben.
>
On 10/9/22 10:50 AM, Bhatnagar, Rishabh wrote:
>
> On 10/6/22 8:07 PM, Herrenschmidt, Benjamin wrote:
>> (putting my @amazon.com hat on)
>>
>> On Sun, 2022-10-02 at 17:30 +0200, Greg KH wrote:
>>
>>
>>> On Thu, Sep 29, 2022 at 09:06:45PM +0000, Rishabh Bhatnagar wrote:
>>>> This patch series backports a bunch of patches related IRQ handling
>>>> with respect to freeing the irq line while IRQ is in flight at CPU
>>>> or at the hardware level.
>>>> Recently we saw this issue in serial 8250 driver where the IRQ was
>>>> being
>>>> freed while the irq was in flight or not yet delivered to the CPU.
>>>> As a
>>>> result the irqchip was going into a wedged state and IRQ was not
>>>> getting
>>>> delivered to the cpu. These patches helped fixed the issue in 4.14
>>>> kernel.
>>> Why is the serial driver freeing an irq while the system is running?
>>> Ah, this could happen on a tty hangup, right?
>> Right. Rishabh answered that separately.
>>
>>>> Let us know if more patches need backporting.
>>> What hardware platform were these patches tested on to verify they
>>> work properly? And why can't they move to 4.19 or newer if they
>>> really need this fix? What's preventing that?
>>>
>>> As Amazon doesn't seem to be testing 4.14.y -rc releases, I find it
>>> odd that you all did this backport. Is this a kernel that you all
>>> care about?
>> These were tested on a collection of EC2 instances, virtual and metal I
>> believe (Rishabh, please confirm).
> Yes these patches were tested on multiple virt/metal EC2 instances.
>>
>> Amazon Linux 2 runs 4.14 or 5.10. Unfortunately we still have to
>> support customers running the former.
>>
>> We'll be including these patches in our releases, we thought it would
>> be nice to have them in -stable as well for the sake of whoever else
>> might be still using this kernel. No huge deal if they don't.
>>
>> As for testing -rc's, yes, we need to get better at that (and publish
>> what we test). Point taken :-)
>>
>> Cheers,
>> Ben.
>>
Hi Greg
Let us know if you think it would be beneficial to take these backports
for 4.14 stable.
We can drop this patch set otherwise.
Thanks alot,
Rishabh
On Fri, Oct 14, 2022 at 12:00:31PM -0700, Bhatnagar, Rishabh wrote:
>
> On 10/9/22 10:50 AM, Bhatnagar, Rishabh wrote:
> >
> > On 10/6/22 8:07 PM, Herrenschmidt, Benjamin wrote:
> > > (putting my @amazon.com hat on)
> > >
> > > On Sun, 2022-10-02 at 17:30 +0200, Greg KH wrote:
> > >
> > >
> > > > On Thu, Sep 29, 2022 at 09:06:45PM +0000, Rishabh Bhatnagar wrote:
> > > > > This patch series backports a bunch of patches related IRQ handling
> > > > > with respect to freeing the irq line while IRQ is in flight at CPU
> > > > > or at the hardware level.
> > > > > Recently we saw this issue in serial 8250 driver where the IRQ was
> > > > > being
> > > > > freed while the irq was in flight or not yet delivered to the CPU.
> > > > > As a
> > > > > result the irqchip was going into a wedged state and IRQ was not
> > > > > getting
> > > > > delivered to the cpu. These patches helped fixed the issue in 4.14
> > > > > kernel.
> > > > Why is the serial driver freeing an irq while the system is running?
> > > > Ah, this could happen on a tty hangup, right?
> > > Right. Rishabh answered that separately.
> > >
> > > > > Let us know if more patches need backporting.
> > > > What hardware platform were these patches tested on to verify they
> > > > work properly?? And why can't they move to 4.19 or newer if they
> > > > really need this fix?? What's preventing that?
> > > >
> > > > As Amazon doesn't seem to be testing 4.14.y -rc releases, I find it
> > > > odd that you all did this backport.? Is this a kernel that you all
> > > > care about?
> > > These were tested on a collection of EC2 instances, virtual and metal I
> > > believe (Rishabh, please confirm).
> > Yes these patches were tested on multiple virt/metal EC2 instances.
> > >
> > > Amazon Linux 2 runs 4.14 or 5.10. Unfortunately we still have to
> > > support customers running the former.
> > >
> > > We'll be including these patches in our releases, we thought it would
> > > be nice to have them in -stable as well for the sake of whoever else
> > > might be still using this kernel. No huge deal if they don't.
> > >
> > > As for testing -rc's, yes, we need to get better at that (and publish
> > > what we test). Point taken :-)
> > >
> > > Cheers,
> > > Ben.
> > >
> Hi Greg
>
> Let us know if you think it would be beneficial to take these backports for
> 4.14 stable.
Give me some time after -rc1 is out to review this then as we are
swamped right now.
> We can drop this patch set otherwise.
You can do whatever you want with your tree :)
thanks,
greg k-h
On Thu, Sep 29, 2022 at 09:06:45PM +0000, Rishabh Bhatnagar wrote:
> This patch series backports a bunch of patches related IRQ handling
> with respect to freeing the irq line while IRQ is in flight at CPU
> or at the hardware level.
> Recently we saw this issue in serial 8250 driver where the IRQ was being
> freed while the irq was in flight or not yet delivered to the CPU. As a
> result the irqchip was going into a wedged state and IRQ was not getting
> delivered to the cpu. These patches helped fixed the issue in 4.14
> kernel.
> Let us know if more patches need backporting.
>
> Lukas Wunner (2):
> genirq: Update code comments wrt recycled thread_mask
> genirq: Synchronize only with single thread on free_irq()
>
> Thomas Gleixner (4):
> genirq: Delay deactivation in free_irq()
> genirq: Fix misleading synchronize_irq() documentation
> genirq: Add optional hardware synchronization for shutdown
> x86/ioapic: Implement irq_get_irqchip_state() callback
>
> arch/x86/kernel/apic/io_apic.c | 46 ++++++++++++++
> kernel/irq/autoprobe.c | 6 +-
> kernel/irq/chip.c | 6 ++
> kernel/irq/cpuhotplug.c | 2 +-
> kernel/irq/internals.h | 5 ++
> kernel/irq/manage.c | 106 ++++++++++++++++++++++-----------
> 6 files changed, 133 insertions(+), 38 deletions(-)
A simple build test breaks with this series applied:
ld: kernel/irq/manage.o: in function `__synchronize_hardirq':
manage.c:(.text+0x149): undefined reference to `__irq_get_irqchip_state'
ld: kernel/irq/manage.o: in function `irq_get_irqchip_state':
manage.c:(.text+0x5a2): undefined reference to `__irq_get_irqchip_state'
make: *** [Makefile:1038: vmlinux] Error 1
How did you test this?
{sigh}
I'm dropping all of these from my queue. I think you need to just keep
this in your device-specific tree as obviously this is not ready to be
backported anywhere.
greg k-h