| Message ID | 20230905185020.3613223-4-ahmed.zaki@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support rx-fcs on/off for VFs | expand |
Dear Ahmed, dear Haiyue, Thank you for the patch. Maybe reword the commit message summary to: ice: Check CRC strip requirement for VLAN strip Am 05.09.23 um 20:50 schrieb Ahmed Zaki: > From: Haiyue Wang <haiyue.wang@intel.com> > > When VLAN strip is enabled, the CRC strip should not be allowed to be must not be disabled or: VLAN strip requires CRC strip according to datasheet …. > disabled. And when CRC strip is disabled, the VLAN strip should not be > allowed to be enabled. > > It needs to check CRC strip disable setting parameter firstly before > configuring the Rx/Tx queues, otherwise, in current error handling, > the already set Tx queue context doesn't rollback correctly, it will The verb roll back is spelled with space (simple past is rolled back). > cause the Tx queue setup failure next time: > "Failed to set LAN Tx queue context" > > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com> > Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_vf_lib.h | 3 + > drivers/net/ethernet/intel/ice/ice_virtchnl.c | 64 ++++++++++++++++--- > 2 files changed, 58 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h b/drivers/net/ethernet/intel/ice/ice_vf_lib.h > index 48fea6fa0362..31a082e8a827 100644 > --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h > +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h > @@ -123,6 +123,9 @@ struct ice_vf { > u8 num_req_qs; /* num of queue pairs requested by VF */ > u16 num_mac; > u16 num_vf_qs; /* num of queue configured per VF */ > + u8 vlan_strip_ena; /* Outer and Inner VLAN strip enable */ > +#define ICE_INNER_VLAN_STRIP_ENA BIT(0) > +#define ICE_OUTER_VLAN_STRIP_ENA BIT(1) > struct ice_mdd_vf_events mdd_rx_events; > struct ice_mdd_vf_events mdd_tx_events; > DECLARE_BITMAP(opcodes_allowlist, VIRTCHNL_OP_MAX); > diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > index c0c3e524c523..21e62ebdfac2 100644 > --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c > +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > @@ -1623,6 +1623,15 @@ static int ice_vc_cfg_qs_msg(struct ice_vf *vf, u8 *msg) > goto error_param; > } > > + for (i = 0; i < qci->num_queue_pairs; i++) { > + if (!qci->qpair[i].rxq.crc_disable) > + continue; > + > + if (!(vf->driver_caps & VIRTCHNL_VF_OFFLOAD_CRC) || > + vf->vlan_strip_ena) > + goto error_param; > + } > + > for (i = 0; i < qci->num_queue_pairs; i++) { > qpi = &qci->qpair[i]; > if (qpi->txq.vsi_id != qci->vsi_id || > @@ -1669,11 +1678,6 @@ static int ice_vc_cfg_qs_msg(struct ice_vf *vf, u8 *msg) > vsi->rx_rings[i]->dma = qpi->rxq.dma_ring_addr; > vsi->rx_rings[i]->count = qpi->rxq.ring_len; > > - if (qpi->rxq.crc_disable && > - !(vf->driver_caps & VIRTCHNL_VF_OFFLOAD_CRC)) { > - goto error_param; > - } > - > if (qpi->rxq.crc_disable) > vsi->rx_rings[q_idx]->flags |= > ICE_RX_FLAGS_CRC_STRIP_DIS; > @@ -2425,6 +2429,21 @@ static int ice_vc_remove_vlan_msg(struct ice_vf *vf, u8 *msg) > return ice_vc_process_vlan_msg(vf, msg, false); > } > > +/** > + * ice_vsi_is_rxq_crc_strip_dis - check if Rx queue CRC strip is disabled or not > + * @vsi: pointer to the VF VSI info > + */ > +static bool ice_vsi_is_rxq_crc_strip_dis(struct ice_vsi *vsi) > +{ > + u16 i; Please use the non-fixed length type `unsigned int` [1]. > + > + ice_for_each_alloc_rxq(vsi, i) > + if (vsi->rx_rings[i]->flags & ICE_RX_FLAGS_CRC_STRIP_DIS) > + return true; > + > + return false; > +} > + > /** > * ice_vc_ena_vlan_stripping > * @vf: pointer to the VF info > @@ -2454,6 +2473,8 @@ static int ice_vc_ena_vlan_stripping(struct ice_vf *vf) > > if (vsi->inner_vlan_ops.ena_stripping(vsi, ETH_P_8021Q)) > v_ret = VIRTCHNL_STATUS_ERR_PARAM; > + else > + vf->vlan_strip_ena |= ICE_INNER_VLAN_STRIP_ENA; > > error_param: > return ice_vc_send_msg_to_vf(vf, VIRTCHNL_OP_ENABLE_VLAN_STRIPPING, > @@ -2489,6 +2510,8 @@ static int ice_vc_dis_vlan_stripping(struct ice_vf *vf) > > if (vsi->inner_vlan_ops.dis_stripping(vsi)) > v_ret = VIRTCHNL_STATUS_ERR_PARAM; > + else > + vf->vlan_strip_ena &= ~ICE_INNER_VLAN_STRIP_ENA; > > error_param: > return ice_vc_send_msg_to_vf(vf, VIRTCHNL_OP_DISABLE_VLAN_STRIPPING, > @@ -2664,6 +2687,8 @@ static int ice_vf_init_vlan_stripping(struct ice_vf *vf) > { > struct ice_vsi *vsi = ice_get_vf_vsi(vf); > > + vf->vlan_strip_ena = 0; > + > if (!vsi) > return -EINVAL; > > @@ -2673,10 +2698,16 @@ static int ice_vf_init_vlan_stripping(struct ice_vf *vf) > if (ice_vf_is_port_vlan_ena(vf) && !ice_is_dvm_ena(&vsi->back->hw)) > return 0; > > - if (ice_vf_vlan_offload_ena(vf->driver_caps)) > - return vsi->inner_vlan_ops.ena_stripping(vsi, ETH_P_8021Q); > - else > - return vsi->inner_vlan_ops.dis_stripping(vsi); > + if (ice_vf_vlan_offload_ena(vf->driver_caps)) { > + int err; > + > + err = vsi->inner_vlan_ops.ena_stripping(vsi, ETH_P_8021Q); > + if (!err) > + vf->vlan_strip_ena |= ICE_INNER_VLAN_STRIP_ENA; > + return err; > + } > + > + return vsi->inner_vlan_ops.dis_stripping(vsi); > } > > static u16 ice_vc_get_max_vlan_fltrs(struct ice_vf *vf) > @@ -3450,6 +3481,11 @@ static int ice_vc_ena_vlan_stripping_v2_msg(struct ice_vf *vf, u8 *msg) > goto out; > } > > + if (ice_vsi_is_rxq_crc_strip_dis(vsi)) { > + v_ret = VIRTCHNL_STATUS_ERR_NOT_SUPPORTED; > + goto out; > + } > + > ethertype_setting = strip_msg->outer_ethertype_setting; > if (ethertype_setting) { > if (ice_vc_ena_vlan_offload(vsi, > @@ -3470,6 +3506,8 @@ static int ice_vc_ena_vlan_stripping_v2_msg(struct ice_vf *vf, u8 *msg) > * enabled, is extracted in L2TAG1. > */ > ice_vsi_update_l2tsel(vsi, l2tsel); > + > + vf->vlan_strip_ena |= ICE_OUTER_VLAN_STRIP_ENA; > } > } > > @@ -3481,6 +3519,9 @@ static int ice_vc_ena_vlan_stripping_v2_msg(struct ice_vf *vf, u8 *msg) > goto out; > } > > + if (ethertype_setting) > + vf->vlan_strip_ena |= ICE_INNER_VLAN_STRIP_ENA; > + > out: > return ice_vc_send_msg_to_vf(vf, VIRTCHNL_OP_ENABLE_VLAN_STRIPPING_V2, > v_ret, NULL, 0); > @@ -3542,6 +3583,8 @@ static int ice_vc_dis_vlan_stripping_v2_msg(struct ice_vf *vf, u8 *msg) > * in L2TAG1. > */ > ice_vsi_update_l2tsel(vsi, l2tsel); > + > + vf->vlan_strip_ena &= ~ICE_OUTER_VLAN_STRIP_ENA; > } > } > > @@ -3551,6 +3594,9 @@ static int ice_vc_dis_vlan_stripping_v2_msg(struct ice_vf *vf, u8 *msg) > goto out; > } > > + if (ethertype_setting) > + vf->vlan_strip_ena &= ~ICE_INNER_VLAN_STRIP_ENA; > + > out: > return ice_vc_send_msg_to_vf(vf, VIRTCHNL_OP_DISABLE_VLAN_STRIPPING_V2, > v_ret, NULL, 0); Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de> Kind regards, Paul [1]: https://web.archive.org/web/*/https://notabs.org/coding/smallIntsBigPenalty.htm
On 2023-09-05 22:09, Paul Menzel wrote: > Dear Ahmed, dear Haiyue, > > > Thank you for the patch. Maybe reword the commit message summary to: > > ice: Check CRC strip requirement for VLAN strip > > Am 05.09.23 um 20:50 schrieb Ahmed Zaki: >> From: Haiyue Wang <haiyue.wang@intel.com> >> >> When VLAN strip is enabled, the CRC strip should not be allowed to be > > must not be disabled > > or: > > VLAN strip requires CRC strip according to datasheet …. > >> disabled. And when CRC strip is disabled, the VLAN strip should not be >> allowed to be enabled. >> >> It needs to check CRC strip disable setting parameter firstly before >> configuring the Rx/Tx queues, otherwise, in current error handling, >> the already set Tx queue context doesn't rollback correctly, it will > > The verb roll back is spelled with space (simple past is rolled back). > >> <..> >> >> +/** >> + * ice_vsi_is_rxq_crc_strip_dis - check if Rx queue CRC strip is >> disabled or not >> + * @vsi: pointer to the VF VSI info >> + */ >> +static bool ice_vsi_is_rxq_crc_strip_dis(struct ice_vsi *vsi) >> +{ >> + u16 i; > > Please use the non-fixed length type `unsigned int` [1]. > >> + >> + ice_for_each_alloc_rxq(vsi, i) >> + if (vsi->rx_rings[i]->flags & ICE_RX_FLAGS_CRC_STRIP_DIS) >> + return true; <..> >> + vf->vlan_strip_ena &= ~ICE_OUTER_VLAN_STRIP_ENA; >> } >> } >> @@ -3551,6 +3594,9 @@ static int >> ice_vc_dis_vlan_stripping_v2_msg(struct ice_vf *vf, u8 *msg) >> goto out; >> } >> + if (ethertype_setting) >> + vf->vlan_strip_ena &= ~ICE_INNER_VLAN_STRIP_ENA; >> + >> out: >> return ice_vc_send_msg_to_vf(vf, >> VIRTCHNL_OP_DISABLE_VLAN_STRIPPING_V2, >> v_ret, NULL, 0); > > Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de> > > > Kind regards, > > Paul > > > [1]: > https://web.archive.org/web/*/https://notabs.org/coding/smallIntsBigPenalty.htm Thanks for the review. This and your other comments will be included in v7. Ahmed
diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h b/drivers/net/ethernet/intel/ice/ice_vf_lib.h index 48fea6fa0362..31a082e8a827 100644 --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h @@ -123,6 +123,9 @@ struct ice_vf { u8 num_req_qs; /* num of queue pairs requested by VF */ u16 num_mac; u16 num_vf_qs; /* num of queue configured per VF */ + u8 vlan_strip_ena; /* Outer and Inner VLAN strip enable */ +#define ICE_INNER_VLAN_STRIP_ENA BIT(0) +#define ICE_OUTER_VLAN_STRIP_ENA BIT(1) struct ice_mdd_vf_events mdd_rx_events; struct ice_mdd_vf_events mdd_tx_events; DECLARE_BITMAP(opcodes_allowlist, VIRTCHNL_OP_MAX); diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c index c0c3e524c523..21e62ebdfac2 100644 --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c @@ -1623,6 +1623,15 @@ static int ice_vc_cfg_qs_msg(struct ice_vf *vf, u8 *msg) goto error_param; } + for (i = 0; i < qci->num_queue_pairs; i++) { + if (!qci->qpair[i].rxq.crc_disable) + continue; + + if (!(vf->driver_caps & VIRTCHNL_VF_OFFLOAD_CRC) || + vf->vlan_strip_ena) + goto error_param; + } + for (i = 0; i < qci->num_queue_pairs; i++) { qpi = &qci->qpair[i]; if (qpi->txq.vsi_id != qci->vsi_id || @@ -1669,11 +1678,6 @@ static int ice_vc_cfg_qs_msg(struct ice_vf *vf, u8 *msg) vsi->rx_rings[i]->dma = qpi->rxq.dma_ring_addr; vsi->rx_rings[i]->count = qpi->rxq.ring_len; - if (qpi->rxq.crc_disable && - !(vf->driver_caps & VIRTCHNL_VF_OFFLOAD_CRC)) { - goto error_param; - } - if (qpi->rxq.crc_disable) vsi->rx_rings[q_idx]->flags |= ICE_RX_FLAGS_CRC_STRIP_DIS; @@ -2425,6 +2429,21 @@ static int ice_vc_remove_vlan_msg(struct ice_vf *vf, u8 *msg) return ice_vc_process_vlan_msg(vf, msg, false); } +/** + * ice_vsi_is_rxq_crc_strip_dis - check if Rx queue CRC strip is disabled or not + * @vsi: pointer to the VF VSI info + */ +static bool ice_vsi_is_rxq_crc_strip_dis(struct ice_vsi *vsi) +{ + u16 i; + + ice_for_each_alloc_rxq(vsi, i) + if (vsi->rx_rings[i]->flags & ICE_RX_FLAGS_CRC_STRIP_DIS) + return true; + + return false; +} + /** * ice_vc_ena_vlan_stripping * @vf: pointer to the VF info @@ -2454,6 +2473,8 @@ static int ice_vc_ena_vlan_stripping(struct ice_vf *vf) if (vsi->inner_vlan_ops.ena_stripping(vsi, ETH_P_8021Q)) v_ret = VIRTCHNL_STATUS_ERR_PARAM; + else + vf->vlan_strip_ena |= ICE_INNER_VLAN_STRIP_ENA; error_param: return ice_vc_send_msg_to_vf(vf, VIRTCHNL_OP_ENABLE_VLAN_STRIPPING, @@ -2489,6 +2510,8 @@ static int ice_vc_dis_vlan_stripping(struct ice_vf *vf) if (vsi->inner_vlan_ops.dis_stripping(vsi)) v_ret = VIRTCHNL_STATUS_ERR_PARAM; + else + vf->vlan_strip_ena &= ~ICE_INNER_VLAN_STRIP_ENA; error_param: return ice_vc_send_msg_to_vf(vf, VIRTCHNL_OP_DISABLE_VLAN_STRIPPING, @@ -2664,6 +2687,8 @@ static int ice_vf_init_vlan_stripping(struct ice_vf *vf) { struct ice_vsi *vsi = ice_get_vf_vsi(vf); + vf->vlan_strip_ena = 0; + if (!vsi) return -EINVAL; @@ -2673,10 +2698,16 @@ static int ice_vf_init_vlan_stripping(struct ice_vf *vf) if (ice_vf_is_port_vlan_ena(vf) && !ice_is_dvm_ena(&vsi->back->hw)) return 0; - if (ice_vf_vlan_offload_ena(vf->driver_caps)) - return vsi->inner_vlan_ops.ena_stripping(vsi, ETH_P_8021Q); - else - return vsi->inner_vlan_ops.dis_stripping(vsi); + if (ice_vf_vlan_offload_ena(vf->driver_caps)) { + int err; + + err = vsi->inner_vlan_ops.ena_stripping(vsi, ETH_P_8021Q); + if (!err) + vf->vlan_strip_ena |= ICE_INNER_VLAN_STRIP_ENA; + return err; + } + + return vsi->inner_vlan_ops.dis_stripping(vsi); } static u16 ice_vc_get_max_vlan_fltrs(struct ice_vf *vf) @@ -3450,6 +3481,11 @@ static int ice_vc_ena_vlan_stripping_v2_msg(struct ice_vf *vf, u8 *msg) goto out; } + if (ice_vsi_is_rxq_crc_strip_dis(vsi)) { + v_ret = VIRTCHNL_STATUS_ERR_NOT_SUPPORTED; + goto out; + } + ethertype_setting = strip_msg->outer_ethertype_setting; if (ethertype_setting) { if (ice_vc_ena_vlan_offload(vsi, @@ -3470,6 +3506,8 @@ static int ice_vc_ena_vlan_stripping_v2_msg(struct ice_vf *vf, u8 *msg) * enabled, is extracted in L2TAG1. */ ice_vsi_update_l2tsel(vsi, l2tsel); + + vf->vlan_strip_ena |= ICE_OUTER_VLAN_STRIP_ENA; } } @@ -3481,6 +3519,9 @@ static int ice_vc_ena_vlan_stripping_v2_msg(struct ice_vf *vf, u8 *msg) goto out; } + if (ethertype_setting) + vf->vlan_strip_ena |= ICE_INNER_VLAN_STRIP_ENA; + out: return ice_vc_send_msg_to_vf(vf, VIRTCHNL_OP_ENABLE_VLAN_STRIPPING_V2, v_ret, NULL, 0); @@ -3542,6 +3583,8 @@ static int ice_vc_dis_vlan_stripping_v2_msg(struct ice_vf *vf, u8 *msg) * in L2TAG1. */ ice_vsi_update_l2tsel(vsi, l2tsel); + + vf->vlan_strip_ena &= ~ICE_OUTER_VLAN_STRIP_ENA; } } @@ -3551,6 +3594,9 @@ static int ice_vc_dis_vlan_stripping_v2_msg(struct ice_vf *vf, u8 *msg) goto out; } + if (ethertype_setting) + vf->vlan_strip_ena &= ~ICE_INNER_VLAN_STRIP_ENA; + out: return ice_vc_send_msg_to_vf(vf, VIRTCHNL_OP_DISABLE_VLAN_STRIPPING_V2, v_ret, NULL, 0);