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: <87o6uuoe5j.ffs@tglx>
Date: Wed, 11 Jun 2025 10:50:32 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Brian Norris <briannorris@...omium.org>, Aleksandrs Vinarskis
 <alex.vinarskis@...il.com>
Cc: 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

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
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);

  	/*
  	 * 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.

Thanks,

        tglx