Re: [patch V2 35/45] genirq/manage: Rework irq_set_irq_wake()

[Jiri Slaby <jirislaby@kernel.org>]

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@linutronix.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