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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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?

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?

> 
>>  	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"