Re: [Intel-wired-lan] [PATCH iwl-next 4/4] ice: Fix VLAN and CRC strip co-existence

[Tony Nguyen <anthony.l.nguyen@intel.com>]

On 8/24/2023 5:50 AM, Ahmed Zaki wrote:
> From: Haiyue Wang <haiyue.wang@intel.com>
> 
> When VLAN strip is enabled, the CRC strip should not be allowed to be
> 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
> cause the Tx queue setup failure next time:
> "Failed to set LAN Tx queue context, error: ICE_ERR_PARAM"

Would this go better before the iavf patch so that we have a properly 
functioning PF before iavf tries to use it?

Also, where does this come from?

"Failed to set LAN Tx queue context, error: ICE_ERR_PARAM"

ICE_ERRs don't exist any more.

> 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..e1621aecd973 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;

I believe preference is to put this in the for loop now.

> +	for (i = 0; i < vsi->alloc_rxq; 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,
> @@ -2663,6 +2686,9 @@ static int ice_vc_query_rxdid(struct ice_vf *vf)
>   static int ice_vf_init_vlan_stripping(struct ice_vf *vf)
>   {
>   	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
> +	int err;

Seems like scope of this can be reduced.

> +	vf->vlan_strip_ena = 0;
>   
>   	if (!vsi)
>   		return -EINVAL;
> @@ -2673,10 +2699,15 @@ 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)) {
> +		err = vsi->inner_vlan_ops.ena_stripping(vsi, ETH_P_8021Q);
> +
> +		if (!err)

No newline between function call and error check please.

> +			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);
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan