| Message ID | 20230418095255.165004-1-kamil.maziarz@intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | [net,v2] ice: Fix ice VF reset during iavf initialization | expand |
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of > Kamil Maziarz > Sent: wtorek, 18 kwietnia 2023 11:53 > To: intel-wired-lan@lists.osuosl.org > Cc: Wesierski, DawidX <dawidx.wesierski@intel.com>; Maziarz, Kamil > <kamil.maziarz@intel.com> > Subject: [Intel-wired-lan] [PATCH net v2] ice: Fix ice VF reset during iavf > initialization > > From: Dawid Wesierski <dawidx.wesierski@intel.com> > > Fix the current implementation that causes ice_trigger_vf_reset() to start > resetting the VF even when the VF is still resetting itself and initializing > adminq. This leads to a series of -53 errors (failed to init adminq) from the > IAVF. > > Change the state of the vf_state field to be not active when the IAVF asks for > a reset. To avoid issues caused by the VF being reset too early, make sure to > wait until receiving the message on the message box to know the exact state > of the IAVF driver. > > Fixes: c54d209c78b8 ("ice: Wait for VF to be reset/ready before > configuration") > Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com> > Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com> > Acked-by: Jacob Keller <Jacob.e.keller@intel.com> > --- > v2: changed "reseted" to "reset", changed fixes tag, removed space > --- > drivers/net/ethernet/intel/ice/ice_sriov.c | 8 ++++---- > drivers/net/ethernet/intel/ice/ice_vf_lib.c | 19 +++++++++++++++++++ > drivers/net/ethernet/intel/ice/ice_vf_lib.h | 1 + > drivers/net/ethernet/intel/ice/ice_virtchnl.c | 1 + > 4 files changed, 25 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c > b/drivers/net/ethernet/intel/ice/ice_sriov.c > index 0cc05e54a781..d4206db7d6d5 100644 > --- a/drivers/net/ethernet/intel/ice/ice_sriov.c > +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Kamil Maziarz píše v Út 18. 04. 2023 v 11:52 +0200: > From: Dawid Wesierski <dawidx.wesierski@intel.com> > > Fix the current implementation that causes ice_trigger_vf_reset() > to start resetting the VF even when the VF is still resetting itself > and initializing adminq. This leads to a series of -53 errors > (failed to init adminq) from the IAVF. > > Change the state of the vf_state field to be not active when the IAVF > asks for a reset. To avoid issues caused by the VF being reset too > early, make sure to wait until receiving the message on the message > box to know the exact state of the IAVF driver. > > Fixes: c54d209c78b8 ("ice: Wait for VF to be reset/ready before > configuration") > Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com> > Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com> > Acked-by: Jacob Keller <Jacob.e.keller@intel.com> > --- > v2: changed "reseted" to "reset", changed fixes tag, removed space > --- > drivers/net/ethernet/intel/ice/ice_sriov.c | 8 ++++---- > drivers/net/ethernet/intel/ice/ice_vf_lib.c | 19 > +++++++++++++++++++ > drivers/net/ethernet/intel/ice/ice_vf_lib.h | 1 + > drivers/net/ethernet/intel/ice/ice_virtchnl.c | 1 + > 4 files changed, 25 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c > b/drivers/net/ethernet/intel/ice/ice_sriov.c > index 0cc05e54a781..d4206db7d6d5 100644 > --- a/drivers/net/ethernet/intel/ice/ice_sriov.c > +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c > @@ -1181,7 +1181,7 @@ int ice_set_vf_spoofchk(struct net_device > *netdev, int vf_id, bool ena) > if (!vf) > return -EINVAL; > > - ret = ice_check_vf_ready_for_cfg(vf); > + ret = ice_check_vf_ready_for_reset(vf); > if (ret) > goto out_put_vf; > > @@ -1296,7 +1296,7 @@ int ice_set_vf_mac(struct net_device *netdev, > int vf_id, u8 *mac) > goto out_put_vf; > } > > - ret = ice_check_vf_ready_for_cfg(vf); > + ret = ice_check_vf_ready_for_reset(vf); > if (ret) > goto out_put_vf; > > @@ -1350,7 +1350,7 @@ int ice_set_vf_trust(struct net_device *netdev, > int vf_id, bool trusted) > return -EOPNOTSUPP; > } > > - ret = ice_check_vf_ready_for_cfg(vf); > + ret = ice_check_vf_ready_for_reset(vf); > if (ret) > goto out_put_vf; > > @@ -1663,7 +1663,7 @@ ice_set_vf_port_vlan(struct net_device *netdev, > int vf_id, u16 vlan_id, u8 qos, > if (!vf) > return -EINVAL; > > - ret = ice_check_vf_ready_for_cfg(vf); > + ret = ice_check_vf_ready_for_reset(vf); > if (ret) > goto out_put_vf; > > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c > b/drivers/net/ethernet/intel/ice/ice_vf_lib.c > index 0e57bd1b85fd..95878c95e953 100644 > --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c > @@ -185,6 +185,25 @@ int ice_check_vf_ready_for_cfg(struct ice_vf > *vf) > return 0; > } > > +/** > + * ice_check_vf_ready_for_reset - check if VF is ready to be reset > + * @vf: VF to check if it's ready to be reset > + * > + * The purpose of this function is to ensure that the VF is not in > reset, > + * disabled, and is both initialized and active, thus enabling us to > safely > + * initialize another reset. > + */ > +int ice_check_vf_ready_for_reset(struct ice_vf *vf) > +{ > + int ret; > + > + ret = ice_check_vf_ready_for_cfg(vf); > + if (!ret && !test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) > + ret = -EAGAIN; > + > + return ret; > +} > + > /** > * ice_trigger_vf_reset - Reset a VF on HW > * @vf: pointer to the VF structure > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h > b/drivers/net/ethernet/intel/ice/ice_vf_lib.h > index ef30f05b5d02..3fc6a0a8d955 100644 > --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h > +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h > @@ -215,6 +215,7 @@ u16 ice_get_num_vfs(struct ice_pf *pf); > struct ice_vsi *ice_get_vf_vsi(struct ice_vf *vf); > bool ice_is_vf_disabled(struct ice_vf *vf); > int ice_check_vf_ready_for_cfg(struct ice_vf *vf); > +int ice_check_vf_ready_for_reset(struct ice_vf *vf); > void ice_set_vf_state_dis(struct ice_vf *vf); > bool ice_is_any_vf_in_unicast_promisc(struct ice_pf *pf); > void > diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c > b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > index e24e3f5017ca..d8c66baf4eb4 100644 > --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c > +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > @@ -3908,6 +3908,7 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, > struct ice_rq_event_info *event) > ice_vc_notify_vf_link_state(vf); > break; > case VIRTCHNL_OP_RESET_VF: > + clear_bit(ICE_VF_STATE_ACTIVE, vf->vf_states); > ops->reset_vf(vf); > break; > case VIRTCHNL_OP_ADD_ETH_ADDR: Hi Dawid, How was this patch tested? Why are we waiting for ICE_VF_STATE_ACTIVE? As I understand ICE_VF_STATE_INIT is the state where we can start the reset again. After this patch we are not able to attach VF to VM: # virsh attach-interface v0 hostdev --managed 0000:41:01.0 --mac 52:54:00:b4:aa:bb # error: Failed to attach interface # error: Cannot set interface MAC to 52:54:00:b4:aa:bb for ifname enp65s0f0np0 vf 0: Resource temporarily unavailable Regards, Petr
Kamil Maziarz píše v Čt 01. 01. 1970 v 00:00 +0000: > From: Dawid Wesierski <dawidx.wesierski@intel.com> > > Fix the current implementation that causes ice_trigger_vf_reset() > to start resetting the VF even when the VF is still resetting itself > and initializing adminq. This leads to a series of -53 errors > (failed to init adminq) from the IAVF. > > Change the state of the vf_state field to be not active when the IAVF > asks for a reset. To avoid issues caused by the VF being reset too > early, make sure to wait until receiving the message on the message > box to know the exact state of the IAVF driver. > > Fixes: c54d209c78b8 ("ice: Wait for VF to be reset/ready before > configuration") > Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com> > Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com> > Acked-by: Jacob Keller <Jacob.e.keller@intel.com> > --- > v2: changed "reseted" to "reset", changed fixes tag, removed space > --- > drivers/net/ethernet/intel/ice/ice_sriov.c | 8 ++++---- > drivers/net/ethernet/intel/ice/ice_vf_lib.c | 19 > +++++++++++++++++++ > drivers/net/ethernet/intel/ice/ice_vf_lib.h | 1 + > drivers/net/ethernet/intel/ice/ice_virtchnl.c | 1 + > 4 files changed, 25 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c > b/drivers/net/ethernet/intel/ice/ice_sriov.c > index 0cc05e54a781..d4206db7d6d5 100644 > --- a/drivers/net/ethernet/intel/ice/ice_sriov.c > +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c > @@ -1181,7 +1181,7 @@ int ice_set_vf_spoofchk(struct net_device > *netdev, int vf_id, bool ena) > if (!vf) > return -EINVAL; > > - ret = ice_check_vf_ready_for_cfg(vf); > + ret = ice_check_vf_ready_for_reset(vf); > if (ret) > goto out_put_vf; > > @@ -1296,7 +1296,7 @@ int ice_set_vf_mac(struct net_device *netdev, > int vf_id, u8 *mac) > goto out_put_vf; > } > > - ret = ice_check_vf_ready_for_cfg(vf); > + ret = ice_check_vf_ready_for_reset(vf); > if (ret) > goto out_put_vf; > > @@ -1350,7 +1350,7 @@ int ice_set_vf_trust(struct net_device *netdev, > int vf_id, bool trusted) > return -EOPNOTSUPP; > } > > - ret = ice_check_vf_ready_for_cfg(vf); > + ret = ice_check_vf_ready_for_reset(vf); > if (ret) > goto out_put_vf; > > @@ -1663,7 +1663,7 @@ ice_set_vf_port_vlan(struct net_device *netdev, > int vf_id, u16 vlan_id, u8 qos, > if (!vf) > return -EINVAL; > > - ret = ice_check_vf_ready_for_cfg(vf); > + ret = ice_check_vf_ready_for_reset(vf); > if (ret) > goto out_put_vf; > > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c > b/drivers/net/ethernet/intel/ice/ice_vf_lib.c > index 0e57bd1b85fd..95878c95e953 100644 > --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c > @@ -185,6 +185,25 @@ int ice_check_vf_ready_for_cfg(struct ice_vf > *vf) > return 0; > } > > +/** > + * ice_check_vf_ready_for_reset - check if VF is ready to be reset > + * @vf: VF to check if it's ready to be reset > + * > + * The purpose of this function is to ensure that the VF is not in > reset, > + * disabled, and is both initialized and active, thus enabling us to > safely > + * initialize another reset. > + */ > +int ice_check_vf_ready_for_reset(struct ice_vf *vf) > +{ > + int ret; > + > + ret = ice_check_vf_ready_for_cfg(vf); > + if (!ret && !test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) > + ret = -EAGAIN; > + > + return ret; > +} > + > /** > * ice_trigger_vf_reset - Reset a VF on HW > * @vf: pointer to the VF structure > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h > b/drivers/net/ethernet/intel/ice/ice_vf_lib.h > index ef30f05b5d02..3fc6a0a8d955 100644 > --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h > +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h > @@ -215,6 +215,7 @@ u16 ice_get_num_vfs(struct ice_pf *pf); > struct ice_vsi *ice_get_vf_vsi(struct ice_vf *vf); > bool ice_is_vf_disabled(struct ice_vf *vf); > int ice_check_vf_ready_for_cfg(struct ice_vf *vf); > +int ice_check_vf_ready_for_reset(struct ice_vf *vf); > void ice_set_vf_state_dis(struct ice_vf *vf); > bool ice_is_any_vf_in_unicast_promisc(struct ice_pf *pf); > void > diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c > b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > index e24e3f5017ca..d8c66baf4eb4 100644 > --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c > +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > @@ -3908,6 +3908,7 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, > struct ice_rq_event_info *event) > ice_vc_notify_vf_link_state(vf); > break; > case VIRTCHNL_OP_RESET_VF: > + clear_bit(ICE_VF_STATE_ACTIVE, vf->vf_states); > ops->reset_vf(vf); > break; > case VIRTCHNL_OP_ADD_ETH_ADDR: Hi, How was this patch tested? Why are we waiting for ICE_VF_STATE_ACTIVE? As I understand ICE_VF_STATE_INIT is the state where we can start the reset again. After this patch we are not able to attach VF to VM: # virsh attach-interface v0 hostdev --managed 0000:41:01.0 --mac 52:54:00:b4:aa:bb # error: Failed to attach interface # error: Cannot set interface MAC to 52:54:00:b4:aa:bb for ifname enp65s0f0np0 vf 0: Resource temporarily unavailable Regards, Petr
diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c index 0cc05e54a781..d4206db7d6d5 100644 --- a/drivers/net/ethernet/intel/ice/ice_sriov.c +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c @@ -1181,7 +1181,7 @@ int ice_set_vf_spoofchk(struct net_device *netdev, int vf_id, bool ena) if (!vf) return -EINVAL; - ret = ice_check_vf_ready_for_cfg(vf); + ret = ice_check_vf_ready_for_reset(vf); if (ret) goto out_put_vf; @@ -1296,7 +1296,7 @@ int ice_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac) goto out_put_vf; } - ret = ice_check_vf_ready_for_cfg(vf); + ret = ice_check_vf_ready_for_reset(vf); if (ret) goto out_put_vf; @@ -1350,7 +1350,7 @@ int ice_set_vf_trust(struct net_device *netdev, int vf_id, bool trusted) return -EOPNOTSUPP; } - ret = ice_check_vf_ready_for_cfg(vf); + ret = ice_check_vf_ready_for_reset(vf); if (ret) goto out_put_vf; @@ -1663,7 +1663,7 @@ ice_set_vf_port_vlan(struct net_device *netdev, int vf_id, u16 vlan_id, u8 qos, if (!vf) return -EINVAL; - ret = ice_check_vf_ready_for_cfg(vf); + ret = ice_check_vf_ready_for_reset(vf); if (ret) goto out_put_vf; diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c index 0e57bd1b85fd..95878c95e953 100644 --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c @@ -185,6 +185,25 @@ int ice_check_vf_ready_for_cfg(struct ice_vf *vf) return 0; } +/** + * ice_check_vf_ready_for_reset - check if VF is ready to be reset + * @vf: VF to check if it's ready to be reset + * + * The purpose of this function is to ensure that the VF is not in reset, + * disabled, and is both initialized and active, thus enabling us to safely + * initialize another reset. + */ +int ice_check_vf_ready_for_reset(struct ice_vf *vf) +{ + int ret; + + ret = ice_check_vf_ready_for_cfg(vf); + if (!ret && !test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) + ret = -EAGAIN; + + return ret; +} + /** * ice_trigger_vf_reset - Reset a VF on HW * @vf: pointer to the VF structure diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h b/drivers/net/ethernet/intel/ice/ice_vf_lib.h index ef30f05b5d02..3fc6a0a8d955 100644 --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h @@ -215,6 +215,7 @@ u16 ice_get_num_vfs(struct ice_pf *pf); struct ice_vsi *ice_get_vf_vsi(struct ice_vf *vf); bool ice_is_vf_disabled(struct ice_vf *vf); int ice_check_vf_ready_for_cfg(struct ice_vf *vf); +int ice_check_vf_ready_for_reset(struct ice_vf *vf); void ice_set_vf_state_dis(struct ice_vf *vf); bool ice_is_any_vf_in_unicast_promisc(struct ice_pf *pf); void diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c index e24e3f5017ca..d8c66baf4eb4 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c @@ -3908,6 +3908,7 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event) ice_vc_notify_vf_link_state(vf); break; case VIRTCHNL_OP_RESET_VF: + clear_bit(ICE_VF_STATE_ACTIVE, vf->vf_states); ops->reset_vf(vf); break; case VIRTCHNL_OP_ADD_ETH_ADDR: