Re: [dpdk-dev] [PATCH] ethdev: add new offload flag to keep CRC

[Shahaf Shuler <shahafs@mellanox.com>]

Wednesday, June 20, 2018 7:13 PM. Ferruh Yigit:
> Subject: Re: [dpdk-dev] [PATCH] ethdev: add new offload flag to keep CRC
> 
> On 6/20/2018 2:44 PM, Shahaf Shuler wrote:
> >
> > Hi Ferruh,
> >
> > Tuesday, June 19, 2018 9:03 PM, Ferruh Yigit
> >> Subject: [dpdk-dev] [PATCH] ethdev: add new offload flag to keep CRC
> >>
> >
> > [...]
> >
> >
> >> diff --git a/drivers/net/mlx5/mlx5_rxq.c
> >> b/drivers/net/mlx5/mlx5_rxq.c index de3f869ed..28cf168aa 100644
> >> --- a/drivers/net/mlx5/mlx5_rxq.c
> >> +++ b/drivers/net/mlx5/mlx5_rxq.c
> >> @@ -388,6 +388,9 @@ mlx5_get_rx_queue_offloads(struct rte_eth_dev
> >> *dev)
> >>
> >>  	if (config->hw_fcs_strip)
> >>  		offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
> >> +	else
> >> +		offloads |= DEV_RX_OFFLOAD_KEEP_CRC;
> >> +
> >
> > I think it should be:
> > if (config->hw_fcs_strip) {
> > 	offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
> > 	offloads |= DEV_RX_OFFLOAD_KEEP_CRC;
> > }
> >
> > The hw_fcs_strip is the capability from device which allows the PMD to
> toggle the CRC stripping.
> 
> From below logic, (how "crc_present" set), hw_fcs_strip looks like capability
> from device that says keeping CRC is supported. If so it the original code was
> not clear to me, why to report CRC stripping only if HW supports keeping
> CRC?

What we report is the option to toggle the CRC stripping by the PMD, and this is what the hw_fcs_strip capability is about. 
The default behavior of HW in case this option is not supported is to remove the CRC. 

I> 
> Following makes more sense to me, based on below code, report CRC
> stripping capability by default and report KEEP CRC capability if device
> supports it:
> 
>  offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
>  if (config->hw_fcs_strip)
> 	offloads |= DEV_RX_OFFLOAD_KEEP_CRC;
> 
> What do you think?

Yes it is better. Thanks. 

> 
> >
> >>  	if (config->hw_csum)
> >>  		offloads |= (DEV_RX_OFFLOAD_IPV4_CKSUM |
> >>  			     DEV_RX_OFFLOAD_UDP_CKSUM |
> >> @@ -1419,17 +1422,17 @@ mlx5_rxq_new(struct rte_eth_dev *dev,
> >> uint16_t idx, uint16_t desc,
> >>  	/* Configure VLAN stripping. */
> >>  	tmpl->rxq.vlan_strip = !!(offloads &
> DEV_RX_OFFLOAD_VLAN_STRIP);
> >>  	/* By default, FCS (CRC) is stripped by hardware. */
> >> -	if (offloads & DEV_RX_OFFLOAD_CRC_STRIP) {
> >> -		tmpl->rxq.crc_present = 0;
> >> -	} else if (config->hw_fcs_strip) {
> >> -		tmpl->rxq.crc_present = 1;
> >> -	} else {
> >> -		DRV_LOG(WARNING,
> >> -			"port %u CRC stripping has been disabled but will"
> >> -			" still be performed by hardware, make sure
> >> MLNX_OFED"
> >> -			" and firmware are up to date",
> >> -			dev->data->port_id);
> >> -		tmpl->rxq.crc_present = 0;
> >> +	tmpl->rxq.crc_present = 0;
> >> +	if (rte_eth_dev_is_keep_crc(offloads)) {
> >> +		if (config->hw_fcs_strip) {
> >> +			tmpl->rxq.crc_present = 1;
> >> +		} else {
> >> +			DRV_LOG(WARNING,
> >> +				"port %u CRC stripping has been disabled but
> >> will"
> >> +				" still be performed by hardware, make sure
> >> MLNX_OFED"
> >> +				" and firmware are up to date",
> >> +				dev->data->port_id);
> >> +		}
> >>  	}
> >>  	DRV_LOG(DEBUG,
> >>  		"port %u CRC stripping is %s, %u bytes will be subtracted
> from"