| Message ID | 20241218014417.3786-1-emil.s.tantilov@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [iwl-net] idpf: fix transaction timeouts on reset | expand |
From: Emil Tantilov <emil.s.tantilov@intel.com> Date: Tue, 17 Dec 2024 17:44:17 -0800 > Restore the call to idpf_vc_xn_shutdown() at the beginning of > idpf_vc_core_deinit() provided the function is not called on > remove. In the reset path this call is needed to prevent mailbox > transactions from timing out. > > Fixes: 09d0fb5cb30e ("idpf: deinit virtchnl transaction manager after vport and vectors") > Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com> > Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com> > --- > Testing hints: > echo 1 > /sys/class/net/<netif>/device/reset > --- > drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > index d46c95f91b0d..0387794daf17 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > @@ -3080,9 +3080,15 @@ void idpf_vc_core_deinit(struct idpf_adapter *adapter) > if (!test_bit(IDPF_VC_CORE_INIT, adapter->flags)) > return; > > + /* Avoid transaction timeouts when called during reset */ > + if (!test_bit(IDPF_REMOVE_IN_PROG, adapter->flags)) > + idpf_vc_xn_shutdown(adapter->vcxn_mngr); > + > idpf_deinit_task(adapter); > idpf_intr_rel(adapter); > - idpf_vc_xn_shutdown(adapter->vcxn_mngr); > + > + if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags)) > + idpf_vc_xn_shutdown(adapter->vcxn_mngr); Why test it two times... bool reset; ... reset = test_bit(REMOVE_IN_PROG); if (!reset) vc_xn_shutdown(); deinit_task(); intr_rel(); if (reset) vc_xn_shutdown(); BTW can't we just move that call unconditionally? > > cancel_delayed_work_sync(&adapter->serv_task); > cancel_delayed_work_sync(&adapter->mbx_task); Thanks, Olek
On 12/18/2024 7:13 AM, Alexander Lobakin wrote: > From: Emil Tantilov <emil.s.tantilov@intel.com> > Date: Tue, 17 Dec 2024 17:44:17 -0800 > >> Restore the call to idpf_vc_xn_shutdown() at the beginning of >> idpf_vc_core_deinit() provided the function is not called on >> remove. In the reset path this call is needed to prevent mailbox >> transactions from timing out. >> >> Fixes: 09d0fb5cb30e ("idpf: deinit virtchnl transaction manager after vport and vectors") >> Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com> >> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com> >> --- >> Testing hints: >> echo 1 > /sys/class/net/<netif>/device/reset >> --- >> drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c >> index d46c95f91b0d..0387794daf17 100644 >> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c >> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c >> @@ -3080,9 +3080,15 @@ void idpf_vc_core_deinit(struct idpf_adapter *adapter) >> if (!test_bit(IDPF_VC_CORE_INIT, adapter->flags)) >> return; >> >> + /* Avoid transaction timeouts when called during reset */ >> + if (!test_bit(IDPF_REMOVE_IN_PROG, adapter->flags)) >> + idpf_vc_xn_shutdown(adapter->vcxn_mngr); >> + >> idpf_deinit_task(adapter); >> idpf_intr_rel(adapter); >> - idpf_vc_xn_shutdown(adapter->vcxn_mngr); >> + >> + if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags)) >> + idpf_vc_xn_shutdown(adapter->vcxn_mngr); > > Why test it two times... > > bool reset; > > ... > > reset = test_bit(REMOVE_IN_PROG); > if (!reset) > vc_xn_shutdown(); > > deinit_task(); > intr_rel(); > > if (reset) > vc_xn_shutdown(); Good point, I will update in v2. > > BTW can't we just move that call unconditionally? This will be a revert of 09d0fb5cb30e: https://lore.kernel.org/intel-wired-lan/20240904095418.6426-1-larysa.zaremba@intel.com/ Which based on internal discussions should be OK, since the control plane (CP) is expected to cleanup regardless. The consensus was that driver communicates with CP when it can, hence the checks. Thanks, Emil > >> >> cancel_delayed_work_sync(&adapter->serv_task); >> cancel_delayed_work_sync(&adapter->mbx_task); > > Thanks, > Olek
diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c index d46c95f91b0d..0387794daf17 100644 --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c @@ -3080,9 +3080,15 @@ void idpf_vc_core_deinit(struct idpf_adapter *adapter) if (!test_bit(IDPF_VC_CORE_INIT, adapter->flags)) return; + /* Avoid transaction timeouts when called during reset */ + if (!test_bit(IDPF_REMOVE_IN_PROG, adapter->flags)) + idpf_vc_xn_shutdown(adapter->vcxn_mngr); + idpf_deinit_task(adapter); idpf_intr_rel(adapter); - idpf_vc_xn_shutdown(adapter->vcxn_mngr); + + if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags)) + idpf_vc_xn_shutdown(adapter->vcxn_mngr); cancel_delayed_work_sync(&adapter->serv_task); cancel_delayed_work_sync(&adapter->mbx_task);
Restore the call to idpf_vc_xn_shutdown() at the beginning of idpf_vc_core_deinit() provided the function is not called on remove. In the reset path this call is needed to prevent mailbox transactions from timing out. Fixes: 09d0fb5cb30e ("idpf: deinit virtchnl transaction manager after vport and vectors") Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com> --- Testing hints: echo 1 > /sys/class/net/<netif>/device/reset --- drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)