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

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

On 9/4/2018 6:54 AM, Andrew Rybchenko wrote:
> 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.

Same here, new version coming soon.