linux-kernel - Re: [PATCH v2 1/2] genirq: Retain depth for managed IRQs across CPU hotplug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aEnQT3PdjdNJGi25@google.com>
Date: Wed, 11 Jun 2025 11:51:59 -0700
From: Brian Norris <briannorris@...omium.org>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Aleksandrs Vinarskis <alex.vinarskis@...il.com>,
	Tsai Sung-Fu <danielsftsai@...gle.com>,
	Douglas Anderson <dianders@...omium.org>,
	linux-kernel@...r.kernel.org, Johan Hovold <johan@...nel.org>
Subject: Re: [PATCH v2 1/2] genirq: Retain depth for managed IRQs across CPU
 hotplug

Hi Thomas,

On Wed, Jun 11, 2025 at 10:50:32AM +0200, Thomas Gleixner wrote:
> On Wed, Jun 11 2025 at 08:50, Thomas Gleixner wrote:
> > On Tue, Jun 10 2025 at 13:07, Brian Norris wrote:
> >> On Mon, Jun 09, 2025 at 08:19:58PM +0200, Aleksandrs Vinarskis wrote:
> >>  
> >>  void irq_startup_managed(struct irq_desc *desc)
> >>  {
> >> +	struct irq_data *d = irq_desc_get_irq_data(desc);
> >> +
> >>  	/*
> >>  	 * Only start it up when the disable depth is 1, so that a disable,
> >>  	 * hotunplug, hotplug sequence does not end up enabling it during
> >>  	 * hotplug unconditionally.
> >>  	 */
> >>  	desc->depth--;
> >> +	irqd_clr_managed_shutdown(d);
> >
> > If depth > 0, then it's still shutdown and the subsequent enable
> > operation which brings it down to 0 will take care of it. So what are
> > you trying to solve here?
> 
> I found the previous version which has an explanation for this. That

Right, I didn't update and carry the explanations here, as I figured I'd
format this all as proper patches after getting Alex's testing
feedbabck. (I think I'll split to two patches, since there are two
distinct bugs I'm fixing in here.)

> makes sense. You really want this to be:
> 
> 	struct irq_data *d = irq_desc_get_irq_data(desc);
> 
>         /* Proper comment */
> 	irqd_clr_managed_shutdown(d);

Ack, will add a comment.

>   	/*
>   	 * Only start it up when the disable depth is 1, so that a disable,
>   	 * hotunplug, hotplug sequence does not end up enabling it during
>   	 * hotplug unconditionally.
>   	 */
>   	desc->depth--;
> 
> ...
> 
> >>  	if (!desc->depth)
> >>  		irq_startup(desc, IRQ_RESEND, IRQ_START_COND);
> >>  }
> >> diff --git a/kernel/irq/cpuhotplug.c b/kernel/irq/cpuhotplug.c
> >> index f07529ae4895..755346ea9819 100644
> >> --- a/kernel/irq/cpuhotplug.c
> >> +++ b/kernel/irq/cpuhotplug.c
> >> @@ -210,13 +210,6 @@ static void irq_restore_affinity_of_irq(struct irq_desc *desc, unsigned int cpu)
> >>  	    !irq_data_get_irq_chip(data) || !cpumask_test_cpu(cpu, affinity))
> >>  		return;
> >>  
> >> -	/*
> >> -	 * Don't restore suspended interrupts here when a system comes back
> >> -	 * from S3. They are reenabled via resume_device_irqs().
> >> -	 */
> >> -	if (desc->istate & IRQS_SUSPENDED)
> >> -		return;
> >> -
> >
> > Huch? Care to read:
> >
> > a60dd06af674 ("genirq/cpuhotplug: Skip suspended interrupts when restoring affinity")
> 
> Never mind. After staring long enough at it, this should work because
> suspend increments depth and shutdown does too.

Yeah, that one definitely deserves an explanation :)

I wrote out a commit message already, but didn't include it here yet, as
I was only looking for testing. Sorry to have sent you staring so long
at it.

One snippet:

    This effectively reverts commit a60dd06af674 ("genirq/cpuhotplug: Skip
    suspended interrupts when restoring affinity"), because it is replaced
    by commit 788019eb559f ("genirq: Retain disable depth for managed
    interrupts across CPU hotplug").

IOW, the depth-tracking we added accomplishes the same goal as commit
a60dd06af674, but including both causes further bugs/imbalances.

Brian