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

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 09/04/2018 08:17 AM, Shahaf Shuler wrote:
> Hi Ferruh,
>
> Monday, September 3, 2018 5:45 PM, Ferruh Yigit:
>> 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) ?

OMG, how can I overlook it on my review. Really good catch.