linux-kernel - Re: [patch V2 35/45] genirq/manage: Rework irq_set_irq

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: <e9a0abc5-7ee0-4ee1-9e19-37d43a5d41de@kernel.org>
Date: Wed, 30 Apr 2025 08:37:52 +0200
From: Jiri Slaby <jirislaby@...nel.org>
To: Thomas Gleixner <tglx@...utronix.de>, LKML <linux-kernel@...r.kernel.org>
Cc: Peter Zijlstra <peterz@...radead.org>
Subject: Re: [patch V2 35/45] genirq/manage: Rework irq_set_irq_wake()

On 29. 04. 25, 8:55, Thomas Gleixner wrote:
> Use the new guards to get and lock the interrupt descriptor and tidy up the
> code.
> 
> No functional change.
> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> 
> ---
>   kernel/irq/manage.c |   61 +++++++++++++++++++++++-----------------------------
>   1 file changed, 28 insertions(+), 33 deletions(-)
> 
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -845,44 +845,39 @@ static int set_irq_wake_real(unsigned in
>    */
>   int irq_set_irq_wake(unsigned int irq, unsigned int on)
>   {
> -	unsigned long flags;
> -	struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
> -	int ret = 0;
> +	int ret = -EINVAL;

Hmm...

> -	if (!desc)
> -		return -EINVAL;
> +	scoped_irqdesc_get_and_lock(irq, IRQ_GET_DESC_CHECK_GLOBAL) {
> +		struct irq_desc *desc = scoped_irqdesc;
>   
> -	/* Don't use NMIs as wake up interrupts please */
> -	if (irq_is_nmi(desc)) {
> -		ret = -EINVAL;
> -		goto out_unlock;
> -	}
> +		/* Don't use NMIs as wake up interrupts please */
> +		if (irq_is_nmi(desc))
> +			return -EINVAL;
>   
> -	/* wakeup-capable irqs can be shared between drivers that
> -	 * don't need to have the same sleep mode behaviors.
> -	 */
> -	if (on) {
> -		if (desc->wake_depth++ == 0) {
> -			ret = set_irq_wake_real(irq, on);
> -			if (ret)
> -				desc->wake_depth = 0;
> -			else
> -				irqd_set(&desc->irq_data, IRQD_WAKEUP_STATE);
> -		}
> -	} else {
> -		if (desc->wake_depth == 0) {
> -			WARN(1, "Unbalanced IRQ %d wake disable\n", irq);
> -		} else if (--desc->wake_depth == 0) {
> -			ret = set_irq_wake_real(irq, on);
> -			if (ret)
> -				desc->wake_depth = 1;
> -			else
> -				irqd_clear(&desc->irq_data, IRQD_WAKEUP_STATE);
> +		/*
> +		 * wakeup-capable irqs can be shared between drivers that
> +		 * don't need to have the same sleep mode behaviors.
> +		 */
> +		if (on) {
> +			if (desc->wake_depth++ == 0) {
> +				ret = set_irq_wake_real(irq, on);
> +				if (ret)
> +					desc->wake_depth = 0;
> +				else
> +					irqd_set(&desc->irq_data, IRQD_WAKEUP_STATE);
> +			}

So in this (imaginary) else branch (i.e. desc->wake_depth++ != 0), you 
return EINVAL now?

Previously, it was 0 (correctly), if I am looking correctly.

> +		} else {
> +			if (desc->wake_depth == 0) {
> +				WARN(1, "Unbalanced IRQ %d wake disable\n", irq);

And here too.

> +			} else if (--desc->wake_depth == 0) {
> +				ret = set_irq_wake_real(irq, on);
> +				if (ret)
> +					desc->wake_depth = 1;
> +				else
> +					irqd_clear(&desc->irq_data, IRQD_WAKEUP_STATE);
> +			}
>   		}
>   	}
> -
> -out_unlock:
> -	irq_put_desc_busunlock(desc, flags);
>   	return ret;
>   }
>   EXPORT_SYMBOL(irq_set_irq_wake);
> 
> 
> 


-- 
js
suse labs