Re: [dpdk-dev] [PATCH] ethdev: make default behavior CRC strip on Rx

[Shahaf Shuler <shahafs@mellanox.com>]

Hi Ferruh, 

Monday, September 3, 2018 5:45 PM, Ferruh Yigit:
> Subject: [PATCH] ethdev: make default behavior CRC strip on Rx
> 
> Removed DEV_RX_OFFLOAD_CRC_STRIP offload flag.
> Without any specific Rx offload flag, default behavior by PMDs is to
> strip CRC.
> 
> PMDs that support keeping CRC should advertise
> DEV_RX_OFFLOAD_KEEP_CRC
> Rx offload capability.
> 
> Applications that require keeping CRC should check PMD capability first
> and if it is supported can enable this feature by setting
> DEV_RX_OFFLOAD_KEEP_CRC in Rx offload flag in rte_eth_dev_configure()
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

[...]

> diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
> index 1f7bfd441..718f4b1d9 100644
> --- a/drivers/net/mlx5/mlx5_rxq.c
> +++ b/drivers/net/mlx5/mlx5_rxq.c
> @@ -388,7 +388,6 @@ mlx5_get_rx_queue_offloads(struct rte_eth_dev
> *dev)
>  			     DEV_RX_OFFLOAD_TIMESTAMP |
>  			     DEV_RX_OFFLOAD_JUMBO_FRAME);
> 
> -	offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
>  	if (config->hw_fcs_strip)
>  		offloads |= DEV_RX_OFFLOAD_KEEP_CRC;
> 
> @@ -1438,7 +1437,7 @@ mlx5_rxq_new(struct rte_eth_dev *dev, uint16_t
> idx, uint16_t desc,
>  	tmpl->rxq.vlan_strip = !!(offloads &
> DEV_RX_OFFLOAD_VLAN_STRIP);
>  	/* By default, FCS (CRC) is stripped by hardware. */
>  	tmpl->rxq.crc_present = 0;
> -	if (rte_eth_dev_must_keep_crc(offloads)) {
> +	if (offloads | DEV_RX_OFFLOAD_KEEP_CRC) {

I don't understand this logic, and it exists on many other location in the patch.
Shouldn't it be (offloads & DEV_RX_OFFLOAD_KEEP_CRC) ? 


>  		if (config->hw_fcs_strip) {
>  			tmpl->rxq.crc_present = 1;
>  		} else {

Also I think the CRC offload should have an entry on the port caps printed by testpmt "show port caps 0" (see port_offload_cap_display()).