Message ID | 20250508184715.7631-1-emil.s.tantilov@intel.com |
---|---|
State | Under Review |
Delegated to: | Anthony Nguyen |
Headers | show |
Series | [iwl-net] idpf: avoid mailbox timeout delays during reset | expand |
> -----Original Message----- > From: Tantilov, Emil S <emil.s.tantilov@intel.com> > Sent: Thursday, May 8, 2025 8:47 PM > To: intel-wired-lan@lists.osuosl.org > Cc: netdev@vger.kernel.org; decot@google.com; willemb@google.com; > Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Chittim, > Madhu <madhu.chittim@intel.com>; Loktionov, Aleksandr > <aleksandr.loktionov@intel.com>; Kitszel, Przemyslaw > <przemyslaw.kitszel@intel.com>; andrew+netdev@lunn.ch; Hay, Joshua A > <joshua.a.hay@intel.com>; Zaki, Ahmed <ahmed.zaki@intel.com> > Subject: [PATCH iwl-net] idpf: avoid mailbox timeout delays during reset > > Mailbox operations are not possible while the driver is in reset. > Operations that require MBX exchange with the control plane will result in long > delays if executed while a reset is in progress: > > ethtool -L <inf> combined 8& echo 1 > /sys/class/net/<inf>/device/reset idpf > 0000:83:00.0: HW reset detected idpf 0000:83:00.0: Device HW Reset > initiated idpf 0000:83:00.0: Transaction timed-out (op:504 cookie:be00 > vc_op:504 salt:be timeout:2000ms) idpf 0000:83:00.0: Transaction timed- > out (op:508 cookie:bf00 vc_op:508 salt:bf timeout:2000ms) idpf > 0000:83:00.0: Transaction timed-out (op:512 cookie:c000 vc_op:512 salt:c0 > timeout:2000ms) idpf 0000:83:00.0: Transaction timed-out (op:510 > cookie:c100 vc_op:510 salt:c1 timeout:2000ms) idpf 0000:83:00.0: > Transaction timed-out (op:509 cookie:c200 vc_op:509 salt:c2 > timeout:60000ms) idpf 0000:83:00.0: Transaction timed-out (op:509 > cookie:c300 vc_op:509 salt:c3 timeout:60000ms) idpf 0000:83:00.0: > Transaction timed-out (op:505 cookie:c400 vc_op:505 salt:c4 > timeout:60000ms) idpf 0000:83:00.0: Failed to configure queues for vport 0, > -62 > > Disable mailbox communication in case of a reset, unless it's done during a > driver load, where the virtchnl operations are needed to configure the device. > > Fixes: 8077c727561aa ("idpf: add controlq init and reset checks") > Co-developed-by: Joshua Hay <joshua.a.hay@intel.com> > Signed-off-by: Joshua Hay <joshua.a.hay@intel.com> > Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com> > Reviewed-by: Ahmed Zaki <ahmed.zaki@intel.com> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com> > --- > drivers/net/ethernet/intel/idpf/idpf_lib.c | 18 +++++++++++++----- > .../net/ethernet/intel/idpf/idpf_virtchnl.c | 2 +- > .../net/ethernet/intel/idpf/idpf_virtchnl.h | 1 + > 3 files changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c > b/drivers/net/ethernet/intel/idpf/idpf_lib.c > index 3a033ce19cda..2ed801398971 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c > +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c > @@ -1816,11 +1816,19 @@ void idpf_vc_event_task(struct work_struct > *work) > if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags)) > return; > > - if (test_bit(IDPF_HR_FUNC_RESET, adapter->flags) || > - test_bit(IDPF_HR_DRV_LOAD, adapter->flags)) { > - set_bit(IDPF_HR_RESET_IN_PROG, adapter->flags); > - idpf_init_hard_reset(adapter); > - } > + if (test_bit(IDPF_HR_FUNC_RESET, adapter->flags)) > + goto func_reset; > + > + if (test_bit(IDPF_HR_DRV_LOAD, adapter->flags)) > + goto drv_load; > + > + return; > + > +func_reset: > + idpf_vc_xn_shutdown(adapter->vcxn_mngr); > +drv_load: > + set_bit(IDPF_HR_RESET_IN_PROG, adapter->flags); > + idpf_init_hard_reset(adapter); > } > > /** > diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > index 3d2413b8684f..5d2ca007f682 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > @@ -376,7 +376,7 @@ static void idpf_vc_xn_init(struct > idpf_vc_xn_manager *vcxn_mngr) > * All waiting threads will be woken-up and their transaction aborted. Further > * operations on that object will fail. > */ > -static void idpf_vc_xn_shutdown(struct idpf_vc_xn_manager *vcxn_mngr) > +void idpf_vc_xn_shutdown(struct idpf_vc_xn_manager *vcxn_mngr) > { > int i; > > diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h > b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h > index 83da5d8da56b..23271cf0a216 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h > +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h > @@ -66,5 +66,6 @@ int idpf_send_get_stats_msg(struct idpf_vport *vport); > int idpf_send_set_sriov_vfs_msg(struct idpf_adapter *adapter, u16 num_vfs); > int idpf_send_get_set_rss_key_msg(struct idpf_vport *vport, bool get); int > idpf_send_get_set_rss_lut_msg(struct idpf_vport *vport, bool get); > +void idpf_vc_xn_shutdown(struct idpf_vc_xn_manager *vcxn_mngr); > > #endif /* _IDPF_VIRTCHNL_H_ */ > -- > 2.17.2
On Thu, May 08, 2025 at 11:47:15AM -0700, Emil Tantilov wrote: > Mailbox operations are not possible while the driver is in reset. > Operations that require MBX exchange with the control plane will result > in long delays if executed while a reset is in progress: > > ethtool -L <inf> combined 8& echo 1 > /sys/class/net/<inf>/device/reset > idpf 0000:83:00.0: HW reset detected > idpf 0000:83:00.0: Device HW Reset initiated > idpf 0000:83:00.0: Transaction timed-out (op:504 cookie:be00 vc_op:504 salt:be timeout:2000ms) > idpf 0000:83:00.0: Transaction timed-out (op:508 cookie:bf00 vc_op:508 salt:bf timeout:2000ms) > idpf 0000:83:00.0: Transaction timed-out (op:512 cookie:c000 vc_op:512 salt:c0 timeout:2000ms) > idpf 0000:83:00.0: Transaction timed-out (op:510 cookie:c100 vc_op:510 salt:c1 timeout:2000ms) > idpf 0000:83:00.0: Transaction timed-out (op:509 cookie:c200 vc_op:509 salt:c2 timeout:60000ms) > idpf 0000:83:00.0: Transaction timed-out (op:509 cookie:c300 vc_op:509 salt:c3 timeout:60000ms) > idpf 0000:83:00.0: Transaction timed-out (op:505 cookie:c400 vc_op:505 salt:c4 timeout:60000ms) > idpf 0000:83:00.0: Failed to configure queues for vport 0, -62 > > Disable mailbox communication in case of a reset, unless it's done during > a driver load, where the virtchnl operations are needed to configure the > device. > > Fixes: 8077c727561aa ("idpf: add controlq init and reset checks") > Co-developed-by: Joshua Hay <joshua.a.hay@intel.com> > Signed-off-by: Joshua Hay <joshua.a.hay@intel.com> > Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com> > Reviewed-by: Ahmed Zaki <ahmed.zaki@intel.com> Reviewed-by: Simon Horman <horms@kernel.org>
Dear Emil, Thank you for the patch. Am 08.05.25 um 20:47 schrieb Emil Tantilov: > Mailbox operations are not possible while the driver is in reset. > Operations that require MBX exchange with the control plane will result > in long delays if executed while a reset is in progress: > > ethtool -L <inf> combined 8& echo 1 > /sys/class/net/<inf>/device/reset > idpf 0000:83:00.0: HW reset detected > idpf 0000:83:00.0: Device HW Reset initiated > idpf 0000:83:00.0: Transaction timed-out (op:504 cookie:be00 vc_op:504 salt:be timeout:2000ms) > idpf 0000:83:00.0: Transaction timed-out (op:508 cookie:bf00 vc_op:508 salt:bf timeout:2000ms) > idpf 0000:83:00.0: Transaction timed-out (op:512 cookie:c000 vc_op:512 salt:c0 timeout:2000ms) > idpf 0000:83:00.0: Transaction timed-out (op:510 cookie:c100 vc_op:510 salt:c1 timeout:2000ms) > idpf 0000:83:00.0: Transaction timed-out (op:509 cookie:c200 vc_op:509 salt:c2 timeout:60000ms) > idpf 0000:83:00.0: Transaction timed-out (op:509 cookie:c300 vc_op:509 salt:c3 timeout:60000ms) > idpf 0000:83:00.0: Transaction timed-out (op:505 cookie:c400 vc_op:505 salt:c4 timeout:60000ms) > idpf 0000:83:00.0: Failed to configure queues for vport 0, -62 > > Disable mailbox communication in case of a reset, unless it's done during > a driver load, where the virtchnl operations are needed to configure the > device. Is the Linux kernel going to log something now, that the mailbox operation is ignored? > Fixes: 8077c727561aa ("idpf: add controlq init and reset checks") > Co-developed-by: Joshua Hay <joshua.a.hay@intel.com> > Signed-off-by: Joshua Hay <joshua.a.hay@intel.com> > Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com> > Reviewed-by: Ahmed Zaki <ahmed.zaki@intel.com> > --- > drivers/net/ethernet/intel/idpf/idpf_lib.c | 18 +++++++++++++----- > .../net/ethernet/intel/idpf/idpf_virtchnl.c | 2 +- > .../net/ethernet/intel/idpf/idpf_virtchnl.h | 1 + > 3 files changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c > index 3a033ce19cda..2ed801398971 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c > +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c > @@ -1816,11 +1816,19 @@ void idpf_vc_event_task(struct work_struct *work) > if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags)) > return; > > - if (test_bit(IDPF_HR_FUNC_RESET, adapter->flags) || > - test_bit(IDPF_HR_DRV_LOAD, adapter->flags)) { > - set_bit(IDPF_HR_RESET_IN_PROG, adapter->flags); > - idpf_init_hard_reset(adapter); > - } > + if (test_bit(IDPF_HR_FUNC_RESET, adapter->flags)) > + goto func_reset; > + > + if (test_bit(IDPF_HR_DRV_LOAD, adapter->flags)) > + goto drv_load; > + > + return; > + > +func_reset: > + idpf_vc_xn_shutdown(adapter->vcxn_mngr); > +drv_load: > + set_bit(IDPF_HR_RESET_IN_PROG, adapter->flags); > + idpf_init_hard_reset(adapter); > } > > /** > diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > index 3d2413b8684f..5d2ca007f682 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c > @@ -376,7 +376,7 @@ static void idpf_vc_xn_init(struct idpf_vc_xn_manager *vcxn_mngr) > * All waiting threads will be woken-up and their transaction aborted. Further > * operations on that object will fail. > */ > -static void idpf_vc_xn_shutdown(struct idpf_vc_xn_manager *vcxn_mngr) > +void idpf_vc_xn_shutdown(struct idpf_vc_xn_manager *vcxn_mngr) > { > int i; > > diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h > index 83da5d8da56b..23271cf0a216 100644 > --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h > +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h > @@ -66,5 +66,6 @@ int idpf_send_get_stats_msg(struct idpf_vport *vport); > int idpf_send_set_sriov_vfs_msg(struct idpf_adapter *adapter, u16 num_vfs); > int idpf_send_get_set_rss_key_msg(struct idpf_vport *vport, bool get); > int idpf_send_get_set_rss_lut_msg(struct idpf_vport *vport, bool get); > +void idpf_vc_xn_shutdown(struct idpf_vc_xn_manager *vcxn_mngr); > > #endif /* _IDPF_VIRTCHNL_H_ */ Kind regards, Paul
On 5/12/2025 4:46 AM, Paul Menzel wrote: > Dear Emil, > > > Thank you for the patch. > > Am 08.05.25 um 20:47 schrieb Emil Tantilov: >> Mailbox operations are not possible while the driver is in reset. >> Operations that require MBX exchange with the control plane will result >> in long delays if executed while a reset is in progress: >> >> ethtool -L <inf> combined 8& echo 1 > /sys/class/net/<inf>/device/reset >> idpf 0000:83:00.0: HW reset detected >> idpf 0000:83:00.0: Device HW Reset initiated >> idpf 0000:83:00.0: Transaction timed-out (op:504 cookie:be00 vc_op:504 >> salt:be timeout:2000ms) >> idpf 0000:83:00.0: Transaction timed-out (op:508 cookie:bf00 vc_op:508 >> salt:bf timeout:2000ms) >> idpf 0000:83:00.0: Transaction timed-out (op:512 cookie:c000 vc_op:512 >> salt:c0 timeout:2000ms) >> idpf 0000:83:00.0: Transaction timed-out (op:510 cookie:c100 vc_op:510 >> salt:c1 timeout:2000ms) >> idpf 0000:83:00.0: Transaction timed-out (op:509 cookie:c200 vc_op:509 >> salt:c2 timeout:60000ms) >> idpf 0000:83:00.0: Transaction timed-out (op:509 cookie:c300 vc_op:509 >> salt:c3 timeout:60000ms) >> idpf 0000:83:00.0: Transaction timed-out (op:505 cookie:c400 vc_op:505 >> salt:c4 timeout:60000ms) >> idpf 0000:83:00.0: Failed to configure queues for vport 0, -62 >> >> Disable mailbox communication in case of a reset, unless it's done during >> a driver load, where the virtchnl operations are needed to configure the >> device. > > Is the Linux kernel going to log something now, that the mailbox > operation is ignored? Strictly speaking, the mailbox operations are not ignored, they are disabled on purpose, because they are not possible during a reset. Only the timeouts will be absent in the scenario shown above. The error regarding the queue configuration will still be logged in dmesg. Thanks, Emil > >> Fixes: 8077c727561aa ("idpf: add controlq init and reset checks") >> Co-developed-by: Joshua Hay <joshua.a.hay@intel.com> >> Signed-off-by: Joshua Hay <joshua.a.hay@intel.com> >> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com> >> Reviewed-by: Ahmed Zaki <ahmed.zaki@intel.com> >> --- >> drivers/net/ethernet/intel/idpf/idpf_lib.c | 18 +++++++++++++----- >> .../net/ethernet/intel/idpf/idpf_virtchnl.c | 2 +- >> .../net/ethernet/intel/idpf/idpf_virtchnl.h | 1 + >> 3 files changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ >> ethernet/intel/idpf/idpf_lib.c >> index 3a033ce19cda..2ed801398971 100644 >> --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c >> +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c >> @@ -1816,11 +1816,19 @@ void idpf_vc_event_task(struct work_struct *work) >> if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags)) >> return; >> - if (test_bit(IDPF_HR_FUNC_RESET, adapter->flags) || >> - test_bit(IDPF_HR_DRV_LOAD, adapter->flags)) { >> - set_bit(IDPF_HR_RESET_IN_PROG, adapter->flags); >> - idpf_init_hard_reset(adapter); >> - } >> + if (test_bit(IDPF_HR_FUNC_RESET, adapter->flags)) >> + goto func_reset; >> + >> + if (test_bit(IDPF_HR_DRV_LOAD, adapter->flags)) >> + goto drv_load; >> + >> + return; >> + >> +func_reset: >> + idpf_vc_xn_shutdown(adapter->vcxn_mngr); >> +drv_load: >> + set_bit(IDPF_HR_RESET_IN_PROG, adapter->flags); >> + idpf_init_hard_reset(adapter); >> } >> /** >> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/ >> drivers/net/ethernet/intel/idpf/idpf_virtchnl.c >> index 3d2413b8684f..5d2ca007f682 100644 >> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c >> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c >> @@ -376,7 +376,7 @@ static void idpf_vc_xn_init(struct >> idpf_vc_xn_manager *vcxn_mngr) >> * All waiting threads will be woken-up and their transaction >> aborted. Further >> * operations on that object will fail. >> */ >> -static void idpf_vc_xn_shutdown(struct idpf_vc_xn_manager *vcxn_mngr) >> +void idpf_vc_xn_shutdown(struct idpf_vc_xn_manager *vcxn_mngr) >> { >> int i; >> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h b/ >> drivers/net/ethernet/intel/idpf/idpf_virtchnl.h >> index 83da5d8da56b..23271cf0a216 100644 >> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h >> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h >> @@ -66,5 +66,6 @@ int idpf_send_get_stats_msg(struct idpf_vport *vport); >> int idpf_send_set_sriov_vfs_msg(struct idpf_adapter *adapter, u16 >> num_vfs); >> int idpf_send_get_set_rss_key_msg(struct idpf_vport *vport, bool get); >> int idpf_send_get_set_rss_lut_msg(struct idpf_vport *vport, bool get); >> +void idpf_vc_xn_shutdown(struct idpf_vc_xn_manager *vcxn_mngr); >> #endif /* _IDPF_VIRTCHNL_H_ */ > > > Kind regards, > > Paul
diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c index 3a033ce19cda..2ed801398971 100644 --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c @@ -1816,11 +1816,19 @@ void idpf_vc_event_task(struct work_struct *work) if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags)) return; - if (test_bit(IDPF_HR_FUNC_RESET, adapter->flags) || - test_bit(IDPF_HR_DRV_LOAD, adapter->flags)) { - set_bit(IDPF_HR_RESET_IN_PROG, adapter->flags); - idpf_init_hard_reset(adapter); - } + if (test_bit(IDPF_HR_FUNC_RESET, adapter->flags)) + goto func_reset; + + if (test_bit(IDPF_HR_DRV_LOAD, adapter->flags)) + goto drv_load; + + return; + +func_reset: + idpf_vc_xn_shutdown(adapter->vcxn_mngr); +drv_load: + set_bit(IDPF_HR_RESET_IN_PROG, adapter->flags); + idpf_init_hard_reset(adapter); } /** diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c index 3d2413b8684f..5d2ca007f682 100644 --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c @@ -376,7 +376,7 @@ static void idpf_vc_xn_init(struct idpf_vc_xn_manager *vcxn_mngr) * All waiting threads will be woken-up and their transaction aborted. Further * operations on that object will fail. */ -static void idpf_vc_xn_shutdown(struct idpf_vc_xn_manager *vcxn_mngr) +void idpf_vc_xn_shutdown(struct idpf_vc_xn_manager *vcxn_mngr) { int i; diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h index 83da5d8da56b..23271cf0a216 100644 --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.h @@ -66,5 +66,6 @@ int idpf_send_get_stats_msg(struct idpf_vport *vport); int idpf_send_set_sriov_vfs_msg(struct idpf_adapter *adapter, u16 num_vfs); int idpf_send_get_set_rss_key_msg(struct idpf_vport *vport, bool get); int idpf_send_get_set_rss_lut_msg(struct idpf_vport *vport, bool get); +void idpf_vc_xn_shutdown(struct idpf_vc_xn_manager *vcxn_mngr); #endif /* _IDPF_VIRTCHNL_H_ */