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

Jiri Slaby
2025-04-30T06:37:55Z

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