[PATCH net v2] idpf: synchronize pending IRQs after disable

[PATCH net v2] idpf: synchronize pending IRQs after disable

[Tony Nguyen]

From: Ahmed Zaki <ahmed.zaki@intel.com>

IDPF deinits all interrupts in idpf_vport_intr_deinit() by first disabling
the interrupt registers in idpf_vport_intr_dis_irq_all(), then later on frees
the irqs in idpf_vport_intr_rel_irq().

Prevent any races by waiting for pending IRQ handler after it is disabled.
This will ensure the IRQ is cleanly freed afterwards.

Fixes: d4d558718266 ("idpf: initialize interrupts and enable vport")
Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
Suggested-by: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>
Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Tested-by: Samuel Salin <Samuel.salin@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
v1 (originally from): https://lore.kernel.org/netdev/20250224190647.3601930-4-anthony.l.nguyen@intel.com/
- Rewrote commit message

 drivers/net/ethernet/intel/idpf/idpf_txrx.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
index 977741c41498..70387e091e69 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
@@ -3593,10 +3593,15 @@ static void idpf_vport_intr_rel_irq(struct idpf_vport *vport)
 static void idpf_vport_intr_dis_irq_all(struct idpf_vport *vport)
 {
 	struct idpf_q_vector *q_vector = vport->q_vectors;
-	int q_idx;
+	int q_idx, vidx, irq_num;
+
+	for (q_idx = 0; q_idx < vport->num_q_vectors; q_idx++) {
+		vidx = vport->q_vector_idxs[q_idx];
+		irq_num = vport->adapter->msix_entries[vidx].vector;
 
-	for (q_idx = 0; q_idx < vport->num_q_vectors; q_idx++)
 		writel(0, q_vector[q_idx].intr_reg.dyn_ctl);
+		synchronize_irq(irq_num);
+	}
 }
 
 /**
-- 
2.47.1

Re: [PATCH net v2] idpf: synchronize pending IRQs after disable

[Jakub Kicinski]

On Thu, 27 Feb 2025 13:16:09 -0800 Tony Nguyen wrote:
> IDPF deinits all interrupts in idpf_vport_intr_deinit() by first disabling
> the interrupt registers in idpf_vport_intr_dis_irq_all(), then later on frees
> the irqs in idpf_vport_intr_rel_irq().
> 
> Prevent any races by waiting for pending IRQ handler after it is disabled.
> This will ensure the IRQ is cleanly freed afterwards.

You need to explain what is racing with what. Most drivers are fine
with just ordering the teardown carefully. What is racing with the IRQ,
and why can idpf_vport_intr_dis_irq_all() not be moved after that thing
is disabled.
-- 
pw-bot: cr

Re: [PATCH net v2] idpf: synchronize pending IRQs after disable

[Ahmed Zaki]

On 2025-02-28 3:40 p.m., Jakub Kicinski wrote:
> On Thu, 27 Feb 2025 13:16:09 -0800 Tony Nguyen wrote:
>> IDPF deinits all interrupts in idpf_vport_intr_deinit() by first disabling
>> the interrupt registers in idpf_vport_intr_dis_irq_all(), then later on frees
>> the irqs in idpf_vport_intr_rel_irq().
>>
>> Prevent any races by waiting for pending IRQ handler after it is disabled.
>> This will ensure the IRQ is cleanly freed afterwards.
> 
> You need to explain what is racing with what. Most drivers are fine
> with just ordering the teardown carefully. What is racing with the IRQ,
> and why can idpf_vport_intr_dis_irq_all() not be moved after that thing
> is disabled.

Most drivers call synchronize_irq() in the same order as this patch, for ex:
bnxt_disable_int_sync()
iavf_irq_disable()
ice_vsi_dis_irq()


The order is:
1 - disable IRQ registers
2 - synchronize_irq()     <-- currently missed in IDPF
3 - delete napis
4 - free IRQs

May be "races" is the wrong word, I will try to re-word.

Re: [PATCH net v2] idpf: synchronize pending IRQs after disable

[Jakub Kicinski]

On Fri, 28 Feb 2025 16:59:47 -0700 Ahmed Zaki wrote:
> Most drivers call synchronize_irq() in the same order as this patch, for ex:
> bnxt_disable_int_sync()
> iavf_irq_disable()
> ice_vsi_dis_irq()
> 
> 
> The order is:
> 1 - disable IRQ registers
> 2 - synchronize_irq()     <-- currently missed in IDPF
> 3 - delete napis
> 4 - free IRQs
> 
> May be "races" is the wrong word, I will try to re-word.

I still have no idea what the problem is.

If you can't explain what the bug is let's just drop this patch :/