diff mbox series

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

Message ID 20250227211610.1154503-1-anthony.l.nguyen@intel.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net,v2] idpf: synchronize pending IRQs after disable | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 17 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 50 this patch: 50
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2025-02-28--03-00 (tests: 894)

Commit Message

Tony Nguyen Feb. 27, 2025, 9:16 p.m. UTC 
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(-)

Comments

Jakub Kicinski Feb. 28, 2025, 10:40 p.m. UTC | #1 
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.
Ahmed Zaki Feb. 28, 2025, 11:59 p.m. UTC | #2 
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.
Jakub Kicinski March 1, 2025, 12:47 a.m. UTC | #3 
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 :/
diff mbox series

Patch

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);
+	}
 }
 
 /**