[v2] genirq: Cleanups and conversion to lock guards

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

Posted by Thomas Gleixner 7 months, 3 weeks ago

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;
 
-	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);
+			}
+		} 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);
+			}
 		}
 	}
-
-out_unlock:
-	irq_put_desc_busunlock(desc, flags);
 	return ret;
 }
 EXPORT_SYMBOL(irq_set_irq_wake);

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

Posted by Jiri Slaby 7 months, 3 weeks ago

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

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

Posted by Thomas Gleixner 7 months, 3 weeks ago

On Wed, Apr 30 2025 at 08:37, Jiri Slaby wrote:
> 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.
>> -	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;
>>   
>> +		/*
>> +		 * 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.

Duh, yes.

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

Posted by Thomas Gleixner 7 months, 3 weeks ago

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>
---
V2a: Fix the return value for the on/off paths - Jiry
---
 kernel/irq/manage.c |   65 ++++++++++++++++++++++++----------------------------
 1 file changed, 30 insertions(+), 35 deletions(-)

--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -846,45 +846,40 @@ 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;
+	scoped_irqdesc_get_and_lock(irq, IRQ_GET_DESC_CHECK_GLOBAL) {
+		struct irq_desc *desc = scoped_irqdesc;
+		int ret = 0;
 
-	if (!desc)
-		return -EINVAL;
+		/* Don't use NMIs as wake up interrupts please */
+		if (irq_is_nmi(desc))
+			return -EINVAL;
 
-	/* Don't use NMIs as wake up interrupts please */
-	if (irq_is_nmi(desc)) {
-		ret = -EINVAL;
-		goto out_unlock;
-	}
-
-	/* 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);
+			}
+		} 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);
+			}
 		}
+		return ret;
 	}
-
-out_unlock:
-	irq_put_desc_busunlock(desc, flags);
-	return ret;
+	return -EINVAL;
 }
 EXPORT_SYMBOL(irq_set_irq_wake);

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

Posted by Jon Hunter 7 months, 1 week ago

Hi Thomas,

On 30/04/2025 13:48, 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>
> ---
> V2a: Fix the return value for the on/off paths - Jiry
> ---
>   kernel/irq/manage.c |   65 ++++++++++++++++++++++++----------------------------
>   1 file changed, 30 insertions(+), 35 deletions(-)
> 
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -846,45 +846,40 @@ 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;
> +	scoped_irqdesc_get_and_lock(irq, IRQ_GET_DESC_CHECK_GLOBAL) {


I noticed a suspend regression on some of our Tegra boards and bisect 
pointed to this commit. I made the following change and this does appear 
to fix it ...

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 2861e11acf3a..c94837382037 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -846,7 +846,7 @@ static int set_irq_wake_real(unsigned int irq, 
unsigned int on)
   */
  int irq_set_irq_wake(unsigned int irq, unsigned int on)
  {
-       scoped_irqdesc_get_and_lock(irq, IRQ_GET_DESC_CHECK_GLOBAL) {
+       scoped_irqdesc_get_and_buslock(irq, IRQ_GET_DESC_CHECK_GLOBAL) {
                 struct irq_desc *desc = scoped_irqdesc;
                 int ret = 0;

Hence, I wanted to ask if this should still be using the buslock scope here?

Thanks!
Jon

-- 
nvpublic

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

Posted by Thomas Gleixner 7 months, 1 week ago

On Tue, May 13 2025 at 18:32, Jon Hunter wrote:
> On 30/04/2025 13:48, Thomas Gleixner wrote:
>> @@ -846,45 +846,40 @@ 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;
>> +	scoped_irqdesc_get_and_lock(irq, IRQ_GET_DESC_CHECK_GLOBAL) {
>
>
> I noticed a suspend regression on some of our Tegra boards and bisect 
> pointed to this commit. I made the following change and this does appear 
> to fix it ...
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 2861e11acf3a..c94837382037 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -846,7 +846,7 @@ static int set_irq_wake_real(unsigned int irq, 
> unsigned int on)
>    */
>   int irq_set_irq_wake(unsigned int irq, unsigned int on)
>   {
> -       scoped_irqdesc_get_and_lock(irq, IRQ_GET_DESC_CHECK_GLOBAL) {
> +       scoped_irqdesc_get_and_buslock(irq, IRQ_GET_DESC_CHECK_GLOBAL) {
>                  struct irq_desc *desc = scoped_irqdesc;
>                  int ret = 0;
>
> Hence, I wanted to ask if this should still be using the buslock scope here?

Of course. My bad. Care to send a patch with a proper change log?

Thanks

        tglx

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

Posted by Jon Hunter 7 months, 1 week ago

On 13/05/2025 23:55, Thomas Gleixner wrote:
> On Tue, May 13 2025 at 18:32, Jon Hunter wrote:
>> On 30/04/2025 13:48, Thomas Gleixner wrote:
>>> @@ -846,45 +846,40 @@ 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;
>>> +	scoped_irqdesc_get_and_lock(irq, IRQ_GET_DESC_CHECK_GLOBAL) {
>>
>>
>> I noticed a suspend regression on some of our Tegra boards and bisect
>> pointed to this commit. I made the following change and this does appear
>> to fix it ...
>>
>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>> index 2861e11acf3a..c94837382037 100644
>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -846,7 +846,7 @@ static int set_irq_wake_real(unsigned int irq,
>> unsigned int on)
>>     */
>>    int irq_set_irq_wake(unsigned int irq, unsigned int on)
>>    {
>> -       scoped_irqdesc_get_and_lock(irq, IRQ_GET_DESC_CHECK_GLOBAL) {
>> +       scoped_irqdesc_get_and_buslock(irq, IRQ_GET_DESC_CHECK_GLOBAL) {
>>                   struct irq_desc *desc = scoped_irqdesc;
>>                   int ret = 0;
>>
>> Hence, I wanted to ask if this should still be using the buslock scope here?
> 
> Of course. My bad. Care to send a patch with a proper change log?

Yes no problem! Thanks for confirming.

Jon

-- 
nvpublic

[tip: irq/core] genirq/manage: Rework irq_set_irq_wake()

Posted by tip-bot2 for Thomas Gleixner 7 months, 2 weeks ago

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     8589e325ba4f3effe0d47924d38a5c6aef7a5512
Gitweb:        https://git.kernel.org/tip/8589e325ba4f3effe0d47924d38a5c6aef7a5512
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Wed, 30 Apr 2025 14:48:15 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Wed, 07 May 2025 09:08:15 +02:00

genirq/manage: Rework irq_set_irq_wake()

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>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/all/87ldrhq0hc.ffs@tglx

---
 kernel/irq/manage.c | 65 ++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 35 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index b8f5985..d1ff1e8 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -846,45 +846,40 @@ static int set_irq_wake_real(unsigned int irq, unsigned int on)
  */
 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;
-
-	if (!desc)
-		return -EINVAL;
+	scoped_irqdesc_get_and_lock(irq, IRQ_GET_DESC_CHECK_GLOBAL) {
+		struct irq_desc *desc = scoped_irqdesc;
+		int ret = 0;
 
-	/* 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);
+			}
+		} 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);
+			}
 		}
+		return ret;
 	}
-
-out_unlock:
-	irq_put_desc_busunlock(desc, flags);
-	return ret;
+	return -EINVAL;
 }
 EXPORT_SYMBOL(irq_set_irq_wake);