eth: bnxt: fix IRQ notifier bugs

bnxt_irq_affinity_notify() copies into irq->cpu_mask. Cancel the notifier before freeing irq->cpu_mask.

Fixes c214410c47d6 ("bnxt_en: Add TPH support in BNXT driver")

Signed-off-by Jakub Kicinski <[email protected]>

---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/net/ethernet/broadcom/bnxt/bnxt.c +0 -0

index 7513618793da..4a2a44ffe669 100644

drivers/net/ethernet/broadcom/bnxt/bnxt.c +0 -2

                        if (irq->have_cpumask) {
                                irq_update_affinity_hint(irq->vector, NULL);
                                free_cpumask_var(irq->cpu_mask);
                                irq->have_cpumask = 0;
                        }
 
-                       bnxt_release_irq_notifier(irq);
-
                        free_irq(irq->vector, bp->bnapi[i]);
                }

Quoted 16 lines · Jakub Kicinski

bnxt_irq_affinity_notify() copies into irq->cpu_mask. Cancel the notifier before freeing irq->cpu_mask.

Fixes c214410c47d6 ("bnxt_en: Add TPH support in BNXT driver")

Signed-off-by Jakub Kicinski <[email protected]>

---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/net/ethernet/broadcom/bnxt/bnxt.c +0 -0

index 7513618793da..4a2a44ffe669 100644
@@ -11859,14 +11859,14 @@ static void bnxt_free_irq(struct bnxt *bp)
 

drivers/net/ethernet/broadcom/bnxt/bnxt.c static void bnxt_free_irq(struct bnxt *bp) · line 11863

Quoted 4 lines

drivers/net/ethernet/broadcom/bnxt/bnxt.c +2 -0

                irq = &bp->irq_tbl[map_idx];
                if (irq->requested) {
+                       bnxt_release_irq_notifier(irq);
+

bnxt_release_irq_notifier() calls irq_set_affinity_notifier() to set the notifier to NULL. But the call is not synchronous, right? A pending notifier is still possible after irq_set_affinity_notifier() returns, right?

The TPH support must have broken aRFS in bnxt. IRQ can only have one notifier, so installing the TPH notifier is overriding the one implicitly installed by irq_cpu_rmap_add().

Make sure we call cpu_rmap_update() from the TPH notifier.

We need to be careful with the ordering and not free the rmap until we unregistered the notifier. Note that moving the rmap freeing after the early return in bnxt_free_irq() is fine - there's no path that could leave rmap with irq_tbl being NULL.

Fixes c214410c47d6 ("bnxt_en: Add TPH support in BNXT driver")

Signed-off-by Jakub Kicinski <[email protected]>

bnxt_request_irq() has unnecessary level of indentation. Use continue instead. No need to re-fetch NUMA node for each IRQ, move to the function level.

No functional changes.

Signed-off-by Jakub Kicinski <[email protected]>

Unregistering IRQ affinity notifiers waits for the callback synchronously. bnxt takes the netdev instance lock in the notifier (to restart the queue) and cancels the work under the same lock. This may obviously deadlock.

Move the restart to the async service task. The queue restart isn't super time sensitive. Store the new TPH tag, schedule the task. Safely canceling the service task is already ironed out.

In bnxt_request_irq() the order of registering notifier, affinity and initial TPH programming has to be inverted. I think it was racy previously since user may trigger an update as soon as notifier is installed.

There's a small known gap - if pcie_tph_get_cpu_st() fails at init and the target tag is 0 we may miss programming the entry. This does not seem worth fixing, the code has skip-on-failure all over the place, anyway.

Fixes c214410c47d6 ("bnxt_en: Add TPH support in BNXT driver")

Signed-off-by Jakub Kicinski <[email protected]>

Quoted 14 lines · Jakub Kicinski

Unregistering IRQ affinity notifiers waits for the callback synchronously. bnxt takes the netdev instance lock in the notifier (to restart the queue) and cancels the work under the same lock. This may obviously deadlock.

Move the restart to the async service task. The queue restart isn't super time sensitive. Store the new TPH tag, schedule the task. Safely canceling the service task is already ironed out.

In bnxt_request_irq() the order of registering notifier, affinity and initial TPH programming has to be inverted. I think it was racy previously since user may trigger an update as soon as notifier is installed.

Quoted 7 lines

There's a small known gap - if pcie_tph_get_cpu_st() fails at init and the target tag is 0 we may miss programming the entry. This does not seem worth fixing, the code has skip-on-failure all over the place, anyway.

Fixes c214410c47d6 ("bnxt_en: Add TPH support in BNXT driver")

Signed-off-by Jakub Kicinski <[email protected]>

Thanks for the patches. We'll get the patches tested in our lab.