[POSSIBLE BUG] behavior change in irq_can_handle_pm() introduced in 8d39d6ec4db5d

[Luigi Rizzo]

BACKGROUND (just to explain how I found the issue; it may exist regardless):

I have some code (soon to be posted here) to implement interrupt moderation
in software using using per-CPU hrtimers. The basic logic is the following:

- if the system decides an irq needs moderation, it calls disable_irq_nosync(),
  adds the irq_desc in a per-cpu list, and keeps IRQD_IRQ_INPROGRESS set
  to prevent migration. The first desc inserted in the list also start
an hrtimer;

- when the timer fires, the callback clears the bit and calls enable_irq()
  on all linked irq_desc's

The relevant code is the following:

@@ -207,x +208,x @@ irqreturn_t handle_irq_event(struct irq_desc *desc)

        raw_spin_lock(&desc->lock);
+       /* if moderation kicks in, disable_irq_nosync() and set an
hrtimer. Keep the bit set to prevent migration */
+       if (irq_moderation_has_started_timer_and_disabled_irq(desc))
+               return ret;
        irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);
        return ret;

and the timer callback does the following:

+       list_for_each_entry_safe(desc, next, &ms->descs, ms_node) {
+               list_del(&desc->ms_node);
+               INIT_LIST_HEAD(&desc->ms_node);
+               /* To prevent migration, IRQD_IRQ_INPROGRESS was left
on. Clear it now. */
+               raw_spin_lock(&desc->lock); /* maybe not necessary ? */
+               irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);
+               raw_spin_unlock(&desc->lock);
+               enable_irq(desc->irq_data.irq); /* valid if the chip
sync_lock/unlock are null */
+       }

ISSUE:
After the following change enable_irq() almost immediately causes a deadlock
(it used to work before):

8d39d6ec4db5d genirq: Prevent migration live lock in handle_edge_irq()
 kernel/irq/chip.c | 41 +++++++++++++++++++++++++++++++++++++++--

likely because the extra checks in irq_can_handle_pm(), specifically
the chunk below:

@@ -501,x +504,x @@ static bool irq_can_handle_pm(struct irq_desc *desc)
                        return false;
                return irq_wait_on_inprogress(desc);
        }
-       return false;
...
+        * If the interrupt affinity was moved to this CPU and the
+        * interrupt is currently handled on the previous target CPU, then
+        * busy wait for INPROGRESS to be cleared.
...
+       aff = irq_data_get_effective_affinity_mask(irqd);
+       if (cpumask_first(aff) != smp_processor_id())    <==== is this correct ?
+               return false;
+       return irq_wait_on_inprogress(desc);

QUESTION:
I am not sure the condition indicated by the arrow matches the comment.
Can someone clarify ?

Otherwise, for my use case, I have successfully tested the following change,
which reverts to the previous behavior at least for my specific use case:

@@ -501,x +504,x @@ static bool irq_can_handle_pm(struct irq_desc *desc)
                        return false;
                return irq_wait_on_inprogress(desc);
        }
+       if (irqd_irq_disabled(irqd))
+               return false;

thanks
luigi