ethdev: add new offload flag to keep CRC

Message ID 20180619180230.72585-1-ferruh.yigit@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: add new offload flag to keep CRC |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Ferruh Yigit June 19, 2018, 6:02 p.m. UTC
  DEV_RX_OFFLOAD_KEEP_CRC offload flag added. PMDs that supports keeping
CRC should advertise this offload capability.

DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
default behavior in PMDs are to keep the CRC until this flag removed

Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
- Setting both KEEP_CRC & CRC_STRIP is INVALID
- Setting only CRC_STRIP PMD should strip the CRC
- Setting only KEEP_CRC PMD should keep the CRC
- Not setting both PMD should keep the CRC

A helper function rte_eth_dev_is_keep_crc() has been added to be able to
change the no flag behavior with minimal changes in PMDs.

The PMDs that doesn't report the DEV_RX_OFFLOAD_KEEP_CRC offload can
remove rte_eth_dev_is_keep_crc() checks next release, related code
commented to help the maintenance task.

And DEV_RX_OFFLOAD_CRC_STRIP has been added to virtual drivers since
they don't use CRC at all, when an application requires this offload
virtual PMDs should not return error.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 doc/guides/rel_notes/deprecation.rst      | 10 ---------
 drivers/net/af_packet/rte_eth_af_packet.c |  1 +
 drivers/net/avp/avp_ethdev.c              |  1 +
 drivers/net/axgbe/axgbe_ethdev.c          |  4 +++-
 drivers/net/axgbe/axgbe_rxtx.c            |  6 +++--
 drivers/net/bnxt/bnxt_ethdev.c            |  1 +
 drivers/net/bnxt/bnxt_rxq.c               |  3 +--
 drivers/net/cxgbe/cxgbe_ethdev.c          |  6 ++++-
 drivers/net/e1000/em_rxtx.c               | 20 ++++++++++-------
 drivers/net/e1000/igb_ethdev.c            |  4 ++--
 drivers/net/e1000/igb_rxtx.c              | 27 ++++++++++++++---------
 drivers/net/fm10k/fm10k_ethdev.c          |  6 +++--
 drivers/net/i40e/i40e_ethdev.c            |  1 +
 drivers/net/i40e/i40e_ethdev_vf.c         |  3 ++-
 drivers/net/i40e/i40e_rxtx.c              |  6 +++--
 drivers/net/ixgbe/ixgbe_ethdev.c          |  4 ++--
 drivers/net/ixgbe/ixgbe_ipsec.c           |  2 +-
 drivers/net/ixgbe/ixgbe_rxtx.c            | 25 ++++++++++++---------
 drivers/net/kni/rte_eth_kni.c             |  1 +
 drivers/net/mlx4/mlx4_rxq.c               | 23 ++++++++++---------
 drivers/net/mlx5/mlx5_rxq.c               | 25 ++++++++++++---------
 drivers/net/mvpp2/mrvl_ethdev.c           |  8 ++++---
 drivers/net/nfp/nfp_net.c                 |  9 +++++---
 drivers/net/null/rte_eth_null.c           |  1 +
 drivers/net/octeontx/octeontx_ethdev.c    |  5 ++++-
 drivers/net/pcap/rte_eth_pcap.c           |  1 +
 drivers/net/qede/qede_ethdev.c            |  2 ++
 drivers/net/ring/rte_eth_ring.c           |  1 +
 drivers/net/sfc/sfc_rx.c                  |  5 ++++-
 drivers/net/softnic/rte_eth_softnic.c     |  1 +
 drivers/net/szedata2/rte_eth_szedata2.c   |  3 ++-
 drivers/net/thunderx/nicvf_ethdev.c       |  5 ++++-
 drivers/net/vhost/rte_eth_vhost.c         |  3 ++-
 drivers/net/virtio/virtio_ethdev.c        |  3 ++-
 drivers/net/vmxnet3/vmxnet3_ethdev.c      |  3 ++-
 lib/librte_ethdev/rte_ethdev.c            |  9 ++++++++
 lib/librte_ethdev/rte_ethdev.h            |  5 +++++
 lib/librte_ethdev/rte_ethdev_driver.h     | 20 +++++++++++++++++
 38 files changed, 172 insertions(+), 91 deletions(-)
  

Comments

Andrew Rybchenko June 20, 2018, 7:42 a.m. UTC | #1
On 06/19/2018 09:02 PM, Ferruh Yigit wrote:
> DEV_RX_OFFLOAD_KEEP_CRC offload flag added. PMDs that supports keeping
> CRC should advertise this offload capability.
>
> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
> default behavior in PMDs are to keep the CRC until this flag removed
>
> Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
> - Setting both KEEP_CRC & CRC_STRIP is INVALID
> - Setting only CRC_STRIP PMD should strip the CRC
> - Setting only KEEP_CRC PMD should keep the CRC
> - Not setting both PMD should keep the CRC
>
> A helper function rte_eth_dev_is_keep_crc() has been added to be able to
> change the no flag behavior with minimal changes in PMDs.
>
> The PMDs that doesn't report the DEV_RX_OFFLOAD_KEEP_CRC offload can
> remove rte_eth_dev_is_keep_crc() checks next release, related code
> commented to help the maintenance task.
>
> And DEV_RX_OFFLOAD_CRC_STRIP has been added to virtual drivers since
> they don't use CRC at all, when an application requires this offload
> virtual PMDs should not return error.
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---

<...>

> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> index c9c825e3f..09a42f8c2 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -325,6 +325,26 @@ typedef int (*ethdev_uninit_t)(struct rte_eth_dev *ethdev);
>   int __rte_experimental
>   rte_eth_dev_destroy(struct rte_eth_dev *ethdev, ethdev_uninit_t ethdev_uninit);
>   
> +/**
> + * PMD helper function to check if keeping CRC is requested
> + *
> + * @param rx_offloads
> + *   offloads variable
> + *
> + * @return
> + *   Return positive if keeping CRC is requested,
> + *   zero if stripping CRC is requested
> + */
> +static inline int
> +rte_eth_dev_is_keep_crc(uint64_t rx_offloads)
> +{
> +	if (rx_offloads & DEV_RX_OFFLOAD_CRC_STRIP)
> +		return 0;
> +
> +	/* no KEEP_CRC or CRC_STRIP offload flags means keep CRC */
> +	return 1;
> +}
> +
>   #ifdef __cplusplus
>   }
>   #endif

A couple of control questions about the function:
  - shouldn't __rte_experimental be used?
  - if the function remains in the future, it will be a bit asymmetric 
vs other
    offload flags. Right now it is clear why the function is introduced, but
    it is the question if the function should remain or go away in the 
future
    (as far as I know no other offload flag has similar function to check).

above things are really minor, ethdev and net/sfc
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
  
Allain Legacy June 20, 2018, 10:54 a.m. UTC | #2
> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Tuesday, June 19, 2018 2:03 PM
<...>
> Subject: [PATCH] ethdev: add new offload flag to keep CRC
> 
> DEV_RX_OFFLOAD_KEEP_CRC offload flag added. PMDs that supports
> keeping CRC should advertise this offload capability.
> 
> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release default
> behavior in PMDs are to keep the CRC until this flag removed
> 
> Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
> - Setting both KEEP_CRC & CRC_STRIP is INVALID
> - Setting only CRC_STRIP PMD should strip the CRC
> - Setting only KEEP_CRC PMD should keep the CRC
> - Not setting both PMD should keep the CRC
> 
> A helper function rte_eth_dev_is_keep_crc() has been added to be able to
> change the no flag behavior with minimal changes in PMDs.
> 
> The PMDs that doesn't report the DEV_RX_OFFLOAD_KEEP_CRC offload can
> remove rte_eth_dev_is_keep_crc() checks next release, related code
> commented to help the maintenance task.
> 
> And DEV_RX_OFFLOAD_CRC_STRIP has been added to virtual drivers since
> they don't use CRC at all, when an application requires this offload virtual
> PMDs should not return error.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---

For net/avp:

Acked-by:  Allain Legacy <allain.legacy@windriver.com>
  
Shahaf Shuler June 20, 2018, 1:44 p.m. UTC | #3
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.

>  	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"
  
Ferruh Yigit June 20, 2018, 4:12 p.m. UTC | #4
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"
  
Ferruh Yigit June 20, 2018, 5:24 p.m. UTC | #5
On 6/20/2018 8:42 AM, Andrew Rybchenko wrote:
> On 06/19/2018 09:02 PM, Ferruh Yigit wrote:
>> DEV_RX_OFFLOAD_KEEP_CRC offload flag added. PMDs that supports keeping
>> CRC should advertise this offload capability.
>>
>> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
>> default behavior in PMDs are to keep the CRC until this flag removed
>>
>> Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
>> - Setting both KEEP_CRC & CRC_STRIP is INVALID
>> - Setting only CRC_STRIP PMD should strip the CRC
>> - Setting only KEEP_CRC PMD should keep the CRC
>> - Not setting both PMD should keep the CRC
>>
>> A helper function rte_eth_dev_is_keep_crc() has been added to be able to
>> change the no flag behavior with minimal changes in PMDs.
>>
>> The PMDs that doesn't report the DEV_RX_OFFLOAD_KEEP_CRC offload can
>> remove rte_eth_dev_is_keep_crc() checks next release, related code
>> commented to help the maintenance task.
>>
>> And DEV_RX_OFFLOAD_CRC_STRIP has been added to virtual drivers since
>> they don't use CRC at all, when an application requires this offload
>> virtual PMDs should not return error.
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
> 
> <...>
> 
>> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
>> index c9c825e3f..09a42f8c2 100644
>> --- a/lib/librte_ethdev/rte_ethdev_driver.h
>> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
>> @@ -325,6 +325,26 @@ typedef int (*ethdev_uninit_t)(struct rte_eth_dev *ethdev);
>>  int __rte_experimental
>>  rte_eth_dev_destroy(struct rte_eth_dev *ethdev, ethdev_uninit_t ethdev_uninit);
>>  
>> +/**
>> + * PMD helper function to check if keeping CRC is requested
>> + *
>> + * @param rx_offloads
>> + *   offloads variable
>> + *
>> + * @return
>> + *   Return positive if keeping CRC is requested,
>> + *   zero if stripping CRC is requested
>> + */
>> +static inline int
>> +rte_eth_dev_is_keep_crc(uint64_t rx_offloads)
>> +{
>> +	if (rx_offloads & DEV_RX_OFFLOAD_CRC_STRIP)
>> +		return 0;
>> +
>> +	/* no KEEP_CRC or CRC_STRIP offload flags means keep CRC */
>> +	return 1;
>> +}
>> +
>>  #ifdef __cplusplus
>>  }
>>  #endif
> 
> A couple of control questions about the function:
>  - shouldn't __rte_experimental be used?

This is an internal function, not API, so I think doesn't require to be
experimental.

>  - if the function remains in the future, it will be a bit asymmetric vs other
>    offload flags. Right now it is clear why the function is introduced, but
>    it is the question if the function should remain or go away in the future
>    (as far as I know no other offload flag has similar function to check).

No other offloads don't have similar functions, this is kind special.

There will be more changes related CRC next release, CRC_STRIP will be removed
and no flag will mean strip CRC. So the conditions to is_keep_crc will be changed.
This function is to manage this change easier, localize the information in to
single function to make it easy to update later.

> 
> above things are really minor, ethdev and net/sfc
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
>
  
Andrew Rybchenko June 20, 2018, 5:39 p.m. UTC | #6
On 06/20/2018 08:24 PM, Ferruh Yigit wrote:
> On 6/20/2018 8:42 AM, Andrew Rybchenko wrote:
>> On 06/19/2018 09:02 PM, Ferruh Yigit wrote:
>>> DEV_RX_OFFLOAD_KEEP_CRC offload flag added. PMDs that supports keeping
>>> CRC should advertise this offload capability.
>>>
>>> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
>>> default behavior in PMDs are to keep the CRC until this flag removed
>>>
>>> Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
>>> - Setting both KEEP_CRC & CRC_STRIP is INVALID
>>> - Setting only CRC_STRIP PMD should strip the CRC
>>> - Setting only KEEP_CRC PMD should keep the CRC
>>> - Not setting both PMD should keep the CRC
>>>
>>> A helper function rte_eth_dev_is_keep_crc() has been added to be able to
>>> change the no flag behavior with minimal changes in PMDs.
>>>
>>> The PMDs that doesn't report the DEV_RX_OFFLOAD_KEEP_CRC offload can
>>> remove rte_eth_dev_is_keep_crc() checks next release, related code
>>> commented to help the maintenance task.
>>>
>>> And DEV_RX_OFFLOAD_CRC_STRIP has been added to virtual drivers since
>>> they don't use CRC at all, when an application requires this offload
>>> virtual PMDs should not return error.
>>>
>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>> ---
>> <...>
>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
>>> index c9c825e3f..09a42f8c2 100644
>>> --- a/lib/librte_ethdev/rte_ethdev_driver.h
>>> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
>>> @@ -325,6 +325,26 @@ typedef int (*ethdev_uninit_t)(struct rte_eth_dev *ethdev);
>>>   int __rte_experimental
>>>   rte_eth_dev_destroy(struct rte_eth_dev *ethdev, ethdev_uninit_t ethdev_uninit);
>>>   
>>> +/**
>>> + * PMD helper function to check if keeping CRC is requested
>>> + *
>>> + * @param rx_offloads
>>> + *   offloads variable
>>> + *
>>> + * @return
>>> + *   Return positive if keeping CRC is requested,
>>> + *   zero if stripping CRC is requested
>>> + */
>>> +static inline int
>>> +rte_eth_dev_is_keep_crc(uint64_t rx_offloads)
>>> +{
>>> +	if (rx_offloads & DEV_RX_OFFLOAD_CRC_STRIP)
>>> +		return 0;
>>> +
>>> +	/* no KEEP_CRC or CRC_STRIP offload flags means keep CRC */
>>> +	return 1;
>>> +}
>>> +
>>>   #ifdef __cplusplus
>>>   }
>>>   #endif
>> A couple of control questions about the function:
>>   - shouldn't __rte_experimental be used?
> This is an internal function, not API, so I think doesn't require to be
> experimental.

Just to make my thoughts clear: description does not say that it is an 
internal.
So, nothing prevents external entities to use it. Changes will be API 
breakage.

>>   - if the function remains in the future, it will be a bit asymmetric vs other
>>     offload flags. Right now it is clear why the function is introduced, but
>>     it is the question if the function should remain or go away in the future
>>     (as far as I know no other offload flag has similar function to check).
> No other offloads don't have similar functions, this is kind special.
>
> There will be more changes related CRC next release, CRC_STRIP will be removed
> and no flag will mean strip CRC. So the conditions to is_keep_crc will be changed.
> This function is to manage this change easier, localize the information in to
> single function to make it easy to update later.

It is perfectly clear why it is required right now and introduced (as I said
from the very beginning).
Yes, it is will be the history which explains why it is so, but if we make
a step forward and discard the history it will look asymmetric -
it will be a function which checks single bit. It is really minor and
100% up to you.

Many thanks for reply.
  
Ferruh Yigit June 20, 2018, 6:12 p.m. UTC | #7
On 6/20/2018 6:39 PM, Andrew Rybchenko wrote:
> On 06/20/2018 08:24 PM, Ferruh Yigit wrote:
>> On 6/20/2018 8:42 AM, Andrew Rybchenko wrote:
>>> On 06/19/2018 09:02 PM, Ferruh Yigit wrote:
>>>> DEV_RX_OFFLOAD_KEEP_CRC offload flag added. PMDs that supports keeping
>>>> CRC should advertise this offload capability.
>>>>
>>>> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
>>>> default behavior in PMDs are to keep the CRC until this flag removed
>>>>
>>>> Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
>>>> - Setting both KEEP_CRC & CRC_STRIP is INVALID
>>>> - Setting only CRC_STRIP PMD should strip the CRC
>>>> - Setting only KEEP_CRC PMD should keep the CRC
>>>> - Not setting both PMD should keep the CRC
>>>>
>>>> A helper function rte_eth_dev_is_keep_crc() has been added to be able to
>>>> change the no flag behavior with minimal changes in PMDs.
>>>>
>>>> The PMDs that doesn't report the DEV_RX_OFFLOAD_KEEP_CRC offload can
>>>> remove rte_eth_dev_is_keep_crc() checks next release, related code
>>>> commented to help the maintenance task.
>>>>
>>>> And DEV_RX_OFFLOAD_CRC_STRIP has been added to virtual drivers since
>>>> they don't use CRC at all, when an application requires this offload
>>>> virtual PMDs should not return error.
>>>>
>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> ---
>>> <...>
>>>
>>>> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
>>>> index c9c825e3f..09a42f8c2 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev_driver.h
>>>> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
>>>> @@ -325,6 +325,26 @@ typedef int (*ethdev_uninit_t)(struct rte_eth_dev *ethdev);
>>>>  int __rte_experimental
>>>>  rte_eth_dev_destroy(struct rte_eth_dev *ethdev, ethdev_uninit_t ethdev_uninit);
>>>>  
>>>> +/**
>>>> + * PMD helper function to check if keeping CRC is requested
>>>> + *
>>>> + * @param rx_offloads
>>>> + *   offloads variable
>>>> + *
>>>> + * @return
>>>> + *   Return positive if keeping CRC is requested,
>>>> + *   zero if stripping CRC is requested
>>>> + */
>>>> +static inline int
>>>> +rte_eth_dev_is_keep_crc(uint64_t rx_offloads)
>>>> +{
>>>> +	if (rx_offloads & DEV_RX_OFFLOAD_CRC_STRIP)
>>>> +		return 0;
>>>> +
>>>> +	/* no KEEP_CRC or CRC_STRIP offload flags means keep CRC */
>>>> +	return 1;
>>>> +}
>>>> +
>>>>  #ifdef __cplusplus
>>>>  }
>>>>  #endif
>>> A couple of control questions about the function:
>>>  - shouldn't __rte_experimental be used?
>> This is an internal function, not API, so I think doesn't require to be
>> experimental.
> 
> Just to make my thoughts clear: description does not say that it is an internal.
> So, nothing prevents external entities to use it. Changes will be API breakage.

rte_ethdev_driver.h is not public header, it is just for PMDs.

> 
>>>  - if the function remains in the future, it will be a bit asymmetric vs other
>>>    offload flags. Right now it is clear why the function is introduced, but
>>>    it is the question if the function should remain or go away in the future
>>>    (as far as I know no other offload flag has similar function to check).
>> No other offloads don't have similar functions, this is kind special.
>>
>> There will be more changes related CRC next release, CRC_STRIP will be removed
>> and no flag will mean strip CRC. So the conditions to is_keep_crc will be changed.
>> This function is to manage this change easier, localize the information in to
>> single function to make it easy to update later.
> 
> It is perfectly clear why it is required right now and introduced (as I said
> from the very beginning).
> Yes, it is will be the history which explains why it is so, but if we make
> a step forward and discard the history it will look asymmetric -
> it will be a function which checks single bit. It is really minor and
> 100% up to you.

I see, right it will be just a wrapper to bit check. In this patch it helps to
revert to logic, from strip_crc to keep_crc. In next release I am OK to remove
function and return back to bit check in PMDs, will this be more reasonable?
  
Andrew Rybchenko June 20, 2018, 9:16 p.m. UTC | #8
On 20.06.2018 21:12, Ferruh Yigit wrote:
> On 6/20/2018 6:39 PM, Andrew Rybchenko wrote:
>> On 06/20/2018 08:24 PM, Ferruh Yigit wrote:
>>> On 6/20/2018 8:42 AM, Andrew Rybchenko wrote:
>>>> On 06/19/2018 09:02 PM, Ferruh Yigit wrote:
>>>>> DEV_RX_OFFLOAD_KEEP_CRC offload flag added. PMDs that supports keeping
>>>>> CRC should advertise this offload capability.
>>>>>
>>>>> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
>>>>> default behavior in PMDs are to keep the CRC until this flag removed
>>>>>
>>>>> Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
>>>>> - Setting both KEEP_CRC & CRC_STRIP is INVALID
>>>>> - Setting only CRC_STRIP PMD should strip the CRC
>>>>> - Setting only KEEP_CRC PMD should keep the CRC
>>>>> - Not setting both PMD should keep the CRC
>>>>>
>>>>> A helper function rte_eth_dev_is_keep_crc() has been added to be able to
>>>>> change the no flag behavior with minimal changes in PMDs.
>>>>>
>>>>> The PMDs that doesn't report the DEV_RX_OFFLOAD_KEEP_CRC offload can
>>>>> remove rte_eth_dev_is_keep_crc() checks next release, related code
>>>>> commented to help the maintenance task.
>>>>>
>>>>> And DEV_RX_OFFLOAD_CRC_STRIP has been added to virtual drivers since
>>>>> they don't use CRC at all, when an application requires this offload
>>>>> virtual PMDs should not return error.
>>>>>
>>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>> ---
>>>> <...>
>>>>
>>>>> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
>>>>> index c9c825e3f..09a42f8c2 100644
>>>>> --- a/lib/librte_ethdev/rte_ethdev_driver.h
>>>>> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
>>>>> @@ -325,6 +325,26 @@ typedef int (*ethdev_uninit_t)(struct rte_eth_dev *ethdev);
>>>>>   int __rte_experimental
>>>>>   rte_eth_dev_destroy(struct rte_eth_dev *ethdev, ethdev_uninit_t ethdev_uninit);
>>>>>   
>>>>> +/**
>>>>> + * PMD helper function to check if keeping CRC is requested
>>>>> + *
>>>>> + * @param rx_offloads
>>>>> + *   offloads variable
>>>>> + *
>>>>> + * @return
>>>>> + *   Return positive if keeping CRC is requested,
>>>>> + *   zero if stripping CRC is requested
>>>>> + */
>>>>> +static inline int
>>>>> +rte_eth_dev_is_keep_crc(uint64_t rx_offloads)
>>>>> +{
>>>>> +	if (rx_offloads & DEV_RX_OFFLOAD_CRC_STRIP)
>>>>> +		return 0;
>>>>> +
>>>>> +	/* no KEEP_CRC or CRC_STRIP offload flags means keep CRC */
>>>>> +	return 1;
>>>>> +}
>>>>> +
>>>>>   #ifdef __cplusplus
>>>>>   }
>>>>>   #endif
>>>> A couple of control questions about the function:
>>>>   - shouldn't __rte_experimental be used?
>>> This is an internal function, not API, so I think doesn't require to be
>>> experimental.
>> Just to make my thoughts clear: description does not say that it is an internal.
>> So, nothing prevents external entities to use it. Changes will be API breakage.
> rte_ethdev_driver.h is not public header, it is just for PMDs.

I see. So, it will not be a problem to remove it. OK.

>>>>   - if the function remains in the future, it will be a bit asymmetric vs other
>>>>     offload flags. Right now it is clear why the function is introduced, but
>>>>     it is the question if the function should remain or go away in the future
>>>>     (as far as I know no other offload flag has similar function to check).
>>> No other offloads don't have similar functions, this is kind special.
>>>
>>> There will be more changes related CRC next release, CRC_STRIP will be removed
>>> and no flag will mean strip CRC. So the conditions to is_keep_crc will be changed.
>>> This function is to manage this change easier, localize the information in to
>>> single function to make it easy to update later.
>> It is perfectly clear why it is required right now and introduced (as I said
>> from the very beginning).
>> Yes, it is will be the history which explains why it is so, but if we make
>> a step forward and discard the history it will look asymmetric -
>> it will be a function which checks single bit. It is really minor and
>> 100% up to you.
> I see, right it will be just a wrapper to bit check. In this patch it helps to
> revert to logic, from strip_crc to keep_crc. In next release I am OK to remove
> function and return back to bit check in PMDs, will this be more reasonable?

Just for consistency I'd say yes.
  
Shahaf Shuler June 21, 2018, 7:53 a.m. UTC | #9
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"
  

Patch

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 1ce692eac..cd128ae23 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -67,16 +67,6 @@  Deprecation Notices
   - removal of ``txq_flags`` field from ``rte_eth_txconf`` struct.
   - removal of the offloads bit-field from ``rte_eth_rxmode`` struct.
 
-* ethdev: A new offloading flag ``DEV_RX_OFFLOAD_KEEP_CRC`` will be added in v18.08,
-  This will replace the usage of not setting ``DEV_RX_OFFLOAD_CRC_STRIP`` flag
-  and will be implemented in PMDs accordingly.
-  In v18.08 both ``DEV_RX_OFFLOAD_KEEP_CRC`` and ``DEV_RX_OFFLOAD_CRC_STRIP`` flags
-  will be available, usage will be:
-  ``CRC_STRIP``: Strip CRC from packet
-  ``KEEP_CRC``: Keep CRC in packet
-  Both ``CRC_STRIP`` & ``KEEP_CRC``: Invalid
-  No flag: Keep CRC in packet
-
 * ethdev: In v18.11 ``DEV_RX_OFFLOAD_CRC_STRIP`` offload flag will be removed, default
   behavior without any flag will be changed to CRC strip.
   To keep CRC ``DEV_RX_OFFLOAD_KEEP_CRC`` flag is required.
diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c
index ea47abbf8..8cfb7ada4 100644
--- a/drivers/net/af_packet/rte_eth_af_packet.c
+++ b/drivers/net/af_packet/rte_eth_af_packet.c
@@ -305,6 +305,7 @@  eth_dev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->max_rx_queues = (uint16_t)internals->nb_queues;
 	dev_info->max_tx_queues = (uint16_t)internals->nb_queues;
 	dev_info->min_rx_bufsize = 0;
+	dev_info->rx_offload_capa = DEV_RX_OFFLOAD_CRC_STRIP;
 }
 
 static int
diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c
index dc97e60e6..9cc8fbcdf 100644
--- a/drivers/net/avp/avp_ethdev.c
+++ b/drivers/net/avp/avp_ethdev.c
@@ -2170,6 +2170,7 @@  avp_dev_info_get(struct rte_eth_dev *eth_dev,
 		dev_info->rx_offload_capa = DEV_RX_OFFLOAD_VLAN_STRIP;
 		dev_info->tx_offload_capa = DEV_TX_OFFLOAD_VLAN_INSERT;
 	}
+	dev_info->rx_offload_capa |= DEV_RX_OFFLOAD_CRC_STRIP;
 }
 
 static int
diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
index 7a3ba2e7b..e3773f4a2 100644
--- a/drivers/net/axgbe/axgbe_ethdev.c
+++ b/drivers/net/axgbe/axgbe_ethdev.c
@@ -363,7 +363,9 @@  axgbe_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->rx_offload_capa =
 		DEV_RX_OFFLOAD_IPV4_CKSUM |
 		DEV_RX_OFFLOAD_UDP_CKSUM  |
-		DEV_RX_OFFLOAD_TCP_CKSUM;
+		DEV_RX_OFFLOAD_TCP_CKSUM  |
+		DEV_RX_OFFLOAD_CRC_STRIP  |
+		DEV_RX_OFFLOAD_KEEP_CRC;
 
 	dev_info->tx_offload_capa =
 		DEV_TX_OFFLOAD_IPV4_CKSUM  |
diff --git a/drivers/net/axgbe/axgbe_rxtx.c b/drivers/net/axgbe/axgbe_rxtx.c
index b302bdd1f..e665ef939 100644
--- a/drivers/net/axgbe/axgbe_rxtx.c
+++ b/drivers/net/axgbe/axgbe_rxtx.c
@@ -74,8 +74,10 @@  int axgbe_dev_rx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
 		(DMA_CH_INC * rxq->queue_id));
 	rxq->dma_tail_reg = (volatile uint32_t *)((uint8_t *)rxq->dma_regs +
 						  DMA_CH_RDTR_LO);
-	rxq->crc_len = (uint8_t)((dev->data->dev_conf.rxmode.offloads &
-			DEV_RX_OFFLOAD_CRC_STRIP) ? 0 : ETHER_CRC_LEN);
+	if (rte_eth_dev_is_keep_crc(dev->data->dev_conf.rxmode.offloads))
+		rxq->crc_len = ETHER_CRC_LEN;
+	else
+		rxq->crc_len = 0;
 
 	/* CRC strip in AXGBE supports per port not per queue */
 	pdata->crc_strip_enable = (rxq->crc_len == 0) ? 1 : 0;
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 6e56bfd36..f26461c42 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -147,6 +147,7 @@  static const struct rte_pci_id bnxt_pci_id_map[] = {
 				     DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM | \
 				     DEV_RX_OFFLOAD_JUMBO_FRAME | \
 				     DEV_RX_OFFLOAD_CRC_STRIP | \
+				     DEV_RX_OFFLOAD_KEEP_CRC | \
 				     DEV_RX_OFFLOAD_TCP_LRO)
 
 static int bnxt_vlan_offload_set_op(struct rte_eth_dev *dev, int mask);
diff --git a/drivers/net/bnxt/bnxt_rxq.c b/drivers/net/bnxt/bnxt_rxq.c
index c55ddec4b..265901e63 100644
--- a/drivers/net/bnxt/bnxt_rxq.c
+++ b/drivers/net/bnxt/bnxt_rxq.c
@@ -326,8 +326,7 @@  int bnxt_rx_queue_setup_op(struct rte_eth_dev *eth_dev,
 
 	rxq->queue_id = queue_idx;
 	rxq->port_id = eth_dev->data->port_id;
-	rxq->crc_len = rx_offloads & DEV_RX_OFFLOAD_CRC_STRIP ?
-		0 : ETHER_CRC_LEN;
+	rxq->crc_len = rte_eth_dev_is_keep_crc(rx_offloads) ? ETHER_CRC_LEN : 0;
 
 	eth_dev->data->rx_queues[queue_idx] = rxq;
 	/* Allocate RX ring hardware descriptors */
diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
index 713dc8fae..b0156ec77 100644
--- a/drivers/net/cxgbe/cxgbe_ethdev.c
+++ b/drivers/net/cxgbe/cxgbe_ethdev.c
@@ -354,7 +354,11 @@  int cxgbe_dev_configure(struct rte_eth_dev *eth_dev)
 
 	CXGBE_FUNC_TRACE();
 	configured_offloads = eth_dev->data->dev_conf.rxmode.offloads;
-	if (!(configured_offloads & DEV_RX_OFFLOAD_CRC_STRIP)) {
+
+	/* KEEP_CRC offload flag is not supported by PMD
+	 * can remove the below block when DEV_RX_OFFLOAD_CRC_STRIP removed
+	 */
+	if (rte_eth_dev_is_keep_crc(configured_offloads)) {
 		dev_info(adapter, "can't disable hw crc strip\n");
 		eth_dev->data->dev_conf.rxmode.offloads |=
 			DEV_RX_OFFLOAD_CRC_STRIP;
diff --git a/drivers/net/e1000/em_rxtx.c b/drivers/net/e1000/em_rxtx.c
index a6b3e92a6..944118baf 100644
--- a/drivers/net/e1000/em_rxtx.c
+++ b/drivers/net/e1000/em_rxtx.c
@@ -1364,6 +1364,7 @@  em_get_rx_port_offloads_capa(struct rte_eth_dev *dev)
 		DEV_RX_OFFLOAD_UDP_CKSUM   |
 		DEV_RX_OFFLOAD_TCP_CKSUM   |
 		DEV_RX_OFFLOAD_CRC_STRIP   |
+		DEV_RX_OFFLOAD_KEEP_CRC    |
 		DEV_RX_OFFLOAD_SCATTER;
 	if (max_rx_pktlen > ETHER_MAX_LEN)
 		rx_offload_capa |= DEV_RX_OFFLOAD_JUMBO_FRAME;
@@ -1458,8 +1459,10 @@  eth_em_rx_queue_setup(struct rte_eth_dev *dev,
 	rxq->rx_free_thresh = rx_conf->rx_free_thresh;
 	rxq->queue_id = queue_idx;
 	rxq->port_id = dev->data->port_id;
-	rxq->crc_len = (uint8_t)((dev->data->dev_conf.rxmode.offloads &
-		DEV_RX_OFFLOAD_CRC_STRIP) ? 0 : ETHER_CRC_LEN);
+	if (rte_eth_dev_is_keep_crc(dev->data->dev_conf.rxmode.offloads))
+		rxq->crc_len = ETHER_CRC_LEN;
+	else
+		rxq->crc_len = 0;
 
 	rxq->rdt_reg_addr = E1000_PCI_REG_ADDR(hw, E1000_RDT(queue_idx));
 	rxq->rdh_reg_addr = E1000_PCI_REG_ADDR(hw, E1000_RDH(queue_idx));
@@ -1792,9 +1795,10 @@  eth_em_rx_init(struct rte_eth_dev *dev)
 		 * Reset crc_len in case it was changed after queue setup by a
 		 *  call to configure
 		 */
-		rxq->crc_len =
-			(uint8_t)(dev->data->dev_conf.rxmode.offloads &
-				DEV_RX_OFFLOAD_CRC_STRIP ? 0 : ETHER_CRC_LEN);
+		if (rte_eth_dev_is_keep_crc(dev->data->dev_conf.rxmode.offloads))
+			rxq->crc_len = ETHER_CRC_LEN;
+		else
+			rxq->crc_len = 0;
 
 		bus_addr = rxq->rx_ring_phys_addr;
 		E1000_WRITE_REG(hw, E1000_RDLEN(i),
@@ -1873,10 +1877,10 @@  eth_em_rx_init(struct rte_eth_dev *dev)
 	}
 
 	/* Setup the Receive Control Register. */
-	if (dev->data->dev_conf.rxmode.offloads & DEV_RX_OFFLOAD_CRC_STRIP)
-		rctl |= E1000_RCTL_SECRC; /* Strip Ethernet CRC. */
-	else
+	if (rte_eth_dev_is_keep_crc(dev->data->dev_conf.rxmode.offloads))
 		rctl &= ~E1000_RCTL_SECRC; /* Do not Strip Ethernet CRC. */
+	else
+		rctl |= E1000_RCTL_SECRC; /* Strip Ethernet CRC. */
 
 	rctl &= ~(3 << E1000_RCTL_MO_SHIFT);
 	rctl |= E1000_RCTL_EN | E1000_RCTL_BAM | E1000_RCTL_LBM_NO |
diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index edc7be319..5c62445fe 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -3194,12 +3194,12 @@  igbvf_dev_configure(struct rte_eth_dev *dev)
 	 * Keep the persistent behavior the same as Host PF
 	 */
 #ifndef RTE_LIBRTE_E1000_PF_DISABLE_STRIP_CRC
-	if (!(conf->rxmode.offloads & DEV_RX_OFFLOAD_CRC_STRIP)) {
+	if (rte_eth_dev_is_keep_crc(conf->rxmode.offloads)) {
 		PMD_INIT_LOG(NOTICE, "VF can't disable HW CRC Strip");
 		conf->rxmode.offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
 	}
 #else
-	if (conf->rxmode.offloads & DEV_RX_OFFLOAD_CRC_STRIP) {
+	if (!rte_eth_dev_is_keep_crc(conf->rxmode.offloads)) {
 		PMD_INIT_LOG(NOTICE, "VF can't enable HW CRC Strip");
 		conf->rxmode.offloads &= ~DEV_RX_OFFLOAD_CRC_STRIP;
 	}
diff --git a/drivers/net/e1000/igb_rxtx.c b/drivers/net/e1000/igb_rxtx.c
index 5f729f271..db594c1a3 100644
--- a/drivers/net/e1000/igb_rxtx.c
+++ b/drivers/net/e1000/igb_rxtx.c
@@ -1639,6 +1639,7 @@  igb_get_rx_port_offloads_capa(struct rte_eth_dev *dev)
 			  DEV_RX_OFFLOAD_TCP_CKSUM   |
 			  DEV_RX_OFFLOAD_JUMBO_FRAME |
 			  DEV_RX_OFFLOAD_CRC_STRIP   |
+			  DEV_RX_OFFLOAD_KEEP_CRC    |
 			  DEV_RX_OFFLOAD_SCATTER;
 
 	return rx_offload_capa;
@@ -1720,8 +1721,10 @@  eth_igb_rx_queue_setup(struct rte_eth_dev *dev,
 	rxq->reg_idx = (uint16_t)((RTE_ETH_DEV_SRIOV(dev).active == 0) ?
 		queue_idx : RTE_ETH_DEV_SRIOV(dev).def_pool_q_idx + queue_idx);
 	rxq->port_id = dev->data->port_id;
-	rxq->crc_len = (uint8_t)((dev->data->dev_conf.rxmode.offloads &
-			DEV_RX_OFFLOAD_CRC_STRIP) ? 0 : ETHER_CRC_LEN);
+	if (rte_eth_dev_is_keep_crc(dev->data->dev_conf.rxmode.offloads))
+		rxq->crc_len = ETHER_CRC_LEN;
+	else
+		rxq->crc_len = 0;
 
 	/*
 	 *  Allocate RX ring hardware descriptors. A memzone large enough to
@@ -2371,8 +2374,10 @@  eth_igb_rx_init(struct rte_eth_dev *dev)
 		 * Reset crc_len in case it was changed after queue setup by a
 		 *  call to configure
 		 */
-		rxq->crc_len = (uint8_t)(dev->data->dev_conf.rxmode.offloads &
-				DEV_RX_OFFLOAD_CRC_STRIP ? 0 : ETHER_CRC_LEN);
+		if (rte_eth_dev_is_keep_crc(dev->data->dev_conf.rxmode.offloads))
+			rxq->crc_len = ETHER_CRC_LEN;
+		else
+			rxq->crc_len = 0;
 
 		bus_addr = rxq->rx_ring_phys_addr;
 		E1000_WRITE_REG(hw, E1000_RDLEN(rxq->reg_idx),
@@ -2501,10 +2506,10 @@  eth_igb_rx_init(struct rte_eth_dev *dev)
 	E1000_WRITE_REG(hw, E1000_RXCSUM, rxcsum);
 
 	/* Setup the Receive Control Register. */
-	if (dev->data->dev_conf.rxmode.offloads & DEV_RX_OFFLOAD_CRC_STRIP) {
-		rctl |= E1000_RCTL_SECRC; /* Strip Ethernet CRC. */
+	if (rte_eth_dev_is_keep_crc(dev->data->dev_conf.rxmode.offloads)) {
+		rctl &= ~E1000_RCTL_SECRC; /* Do not Strip Ethernet CRC. */
 
-		/* set STRCRC bit in all queues */
+		/* clear STRCRC bit in all queues */
 		if (hw->mac.type == e1000_i350 ||
 		    hw->mac.type == e1000_i210 ||
 		    hw->mac.type == e1000_i211 ||
@@ -2513,14 +2518,14 @@  eth_igb_rx_init(struct rte_eth_dev *dev)
 				rxq = dev->data->rx_queues[i];
 				uint32_t dvmolr = E1000_READ_REG(hw,
 					E1000_DVMOLR(rxq->reg_idx));
-				dvmolr |= E1000_DVMOLR_STRCRC;
+				dvmolr &= ~E1000_DVMOLR_STRCRC;
 				E1000_WRITE_REG(hw, E1000_DVMOLR(rxq->reg_idx), dvmolr);
 			}
 		}
 	} else {
-		rctl &= ~E1000_RCTL_SECRC; /* Do not Strip Ethernet CRC. */
+		rctl |= E1000_RCTL_SECRC; /* Strip Ethernet CRC. */
 
-		/* clear STRCRC bit in all queues */
+		/* set STRCRC bit in all queues */
 		if (hw->mac.type == e1000_i350 ||
 		    hw->mac.type == e1000_i210 ||
 		    hw->mac.type == e1000_i211 ||
@@ -2529,7 +2534,7 @@  eth_igb_rx_init(struct rte_eth_dev *dev)
 				rxq = dev->data->rx_queues[i];
 				uint32_t dvmolr = E1000_READ_REG(hw,
 					E1000_DVMOLR(rxq->reg_idx));
-				dvmolr &= ~E1000_DVMOLR_STRCRC;
+				dvmolr |= E1000_DVMOLR_STRCRC;
 				E1000_WRITE_REG(hw, E1000_DVMOLR(rxq->reg_idx), dvmolr);
 			}
 		}
diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
index 3ff1b0e0f..2b354dc15 100644
--- a/drivers/net/fm10k/fm10k_ethdev.c
+++ b/drivers/net/fm10k/fm10k_ethdev.c
@@ -451,8 +451,10 @@  fm10k_dev_configure(struct rte_eth_dev *dev)
 
 	PMD_INIT_FUNC_TRACE();
 
-	if ((dev->data->dev_conf.rxmode.offloads &
-	     DEV_RX_OFFLOAD_CRC_STRIP) == 0)
+	/* KEEP_CRC offload flag is not supported by PMD
+	 * can remove the below block when DEV_RX_OFFLOAD_CRC_STRIP removed
+	 */
+	if (rte_eth_dev_is_keep_crc(dev->data->dev_conf.rxmode.offloads))
 		PMD_INIT_LOG(WARNING, "fm10k always strip CRC");
 
 	/* multipe queue mode checking */
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 13c5d3296..1276bb207 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3327,6 +3327,7 @@  i40e_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 		DEV_RX_OFFLOAD_TCP_CKSUM |
 		DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM |
 		DEV_RX_OFFLOAD_CRC_STRIP |
+		DEV_RX_OFFLOAD_KEEP_CRC |
 		DEV_RX_OFFLOAD_VLAN_EXTEND |
 		DEV_RX_OFFLOAD_VLAN_FILTER |
 		DEV_RX_OFFLOAD_JUMBO_FRAME;
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index 804e44530..520655bbe 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1536,7 +1536,7 @@  i40evf_dev_configure(struct rte_eth_dev *dev)
 	/* For non-DPDK PF drivers, VF has no ability to disable HW
 	 * CRC strip, and is implicitly enabled by the PF.
 	 */
-	if (!(conf->rxmode.offloads & DEV_RX_OFFLOAD_CRC_STRIP)) {
+	if (rte_eth_dev_is_keep_crc(conf->rxmode.offloads)) {
 		vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
 		if ((vf->version_major == VIRTCHNL_VERSION_MAJOR) &&
 		    (vf->version_minor <= VIRTCHNL_VERSION_MINOR)) {
@@ -2200,6 +2200,7 @@  i40evf_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 		DEV_RX_OFFLOAD_TCP_CKSUM |
 		DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM |
 		DEV_RX_OFFLOAD_CRC_STRIP |
+		DEV_RX_OFFLOAD_KEEP_CRC |
 		DEV_RX_OFFLOAD_SCATTER |
 		DEV_RX_OFFLOAD_JUMBO_FRAME |
 		DEV_RX_OFFLOAD_VLAN_FILTER;
diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 6032d5541..4c3a2924f 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -1829,8 +1829,10 @@  i40e_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	rxq->queue_id = queue_idx;
 	rxq->reg_idx = reg_idx;
 	rxq->port_id = dev->data->port_id;
-	rxq->crc_len = (uint8_t)((dev->data->dev_conf.rxmode.offloads &
-			DEV_RX_OFFLOAD_CRC_STRIP) ? 0 : ETHER_CRC_LEN);
+	if (rte_eth_dev_is_keep_crc(dev->data->dev_conf.rxmode.offloads))
+		rxq->crc_len = ETHER_CRC_LEN;
+	else
+		rxq->crc_len = 0;
 	rxq->drop_en = rx_conf->rx_drop_en;
 	rxq->vsi = vsi;
 	rxq->rx_deferred_start = rx_conf->rx_deferred_start;
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 87d2ad090..60b7f038f 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -4991,12 +4991,12 @@  ixgbevf_dev_configure(struct rte_eth_dev *dev)
 	 * Keep the persistent behavior the same as Host PF
 	 */
 #ifndef RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC
-	if (!(conf->rxmode.offloads & DEV_RX_OFFLOAD_CRC_STRIP)) {
+	if (rte_eth_dev_is_keep_crc(conf->rxmode.offloads)) {
 		PMD_INIT_LOG(NOTICE, "VF can't disable HW CRC Strip");
 		conf->rxmode.offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
 	}
 #else
-	if (conf->rxmode.offloads & DEV_RX_OFFLOAD_CRC_STRIP) {
+	if (!rte_eth_dev_is_keep_crc(conf->rxmode.offloads)) {
 		PMD_INIT_LOG(NOTICE, "VF can't enable HW CRC Strip");
 		conf->rxmode.offloads &= ~DEV_RX_OFFLOAD_CRC_STRIP;
 	}
diff --git a/drivers/net/ixgbe/ixgbe_ipsec.c b/drivers/net/ixgbe/ixgbe_ipsec.c
index de7ed3676..da861accf 100644
--- a/drivers/net/ixgbe/ixgbe_ipsec.c
+++ b/drivers/net/ixgbe/ixgbe_ipsec.c
@@ -609,7 +609,7 @@  ixgbe_crypto_enable_ipsec(struct rte_eth_dev *dev)
 		PMD_DRV_LOG(ERR, "RSC and IPsec not supported");
 		return -1;
 	}
-	if (!(rx_offloads & DEV_RX_OFFLOAD_CRC_STRIP)) {
+	if (rte_eth_dev_is_keep_crc(rx_offloads)) {
 		PMD_DRV_LOG(ERR, "HW CRC strip needs to be enabled for IPsec");
 		return -1;
 	}
diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 3e13d26ae..b7b1d6be4 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -2849,6 +2849,7 @@  ixgbe_get_rx_port_offloads(struct rte_eth_dev *dev)
 		   DEV_RX_OFFLOAD_UDP_CKSUM   |
 		   DEV_RX_OFFLOAD_TCP_CKSUM   |
 		   DEV_RX_OFFLOAD_CRC_STRIP   |
+		   DEV_RX_OFFLOAD_KEEP_CRC    |
 		   DEV_RX_OFFLOAD_JUMBO_FRAME |
 		   DEV_RX_OFFLOAD_SCATTER;
 
@@ -2935,8 +2936,10 @@  ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	rxq->reg_idx = (uint16_t)((RTE_ETH_DEV_SRIOV(dev).active == 0) ?
 		queue_idx : RTE_ETH_DEV_SRIOV(dev).def_pool_q_idx + queue_idx);
 	rxq->port_id = dev->data->port_id;
-	rxq->crc_len = (uint8_t)((dev->data->dev_conf.rxmode.offloads &
-		DEV_RX_OFFLOAD_CRC_STRIP) ? 0 : ETHER_CRC_LEN);
+	if (rte_eth_dev_is_keep_crc(dev->data->dev_conf.rxmode.offloads))
+		rxq->crc_len = ETHER_CRC_LEN;
+	else
+		rxq->crc_len = 0;
 	rxq->drop_en = rx_conf->rx_drop_en;
 	rxq->rx_deferred_start = rx_conf->rx_deferred_start;
 	rxq->offloads = offloads;
@@ -4702,7 +4705,7 @@  ixgbe_set_rsc(struct rte_eth_dev *dev)
 
 	/* RSC global configuration (chapter 4.6.7.2.1 of 82599 Spec) */
 
-	if (!(rx_conf->offloads & DEV_RX_OFFLOAD_CRC_STRIP) &&
+	if (rte_eth_dev_is_keep_crc(rx_conf->offloads) &&
 	     (rx_conf->offloads & DEV_RX_OFFLOAD_TCP_LRO)) {
 		/*
 		 * According to chapter of 4.6.7.2.1 of the Spec Rev.
@@ -4851,10 +4854,10 @@  ixgbe_dev_rx_init(struct rte_eth_dev *dev)
 	 * Configure CRC stripping, if any.
 	 */
 	hlreg0 = IXGBE_READ_REG(hw, IXGBE_HLREG0);
-	if (rx_conf->offloads & DEV_RX_OFFLOAD_CRC_STRIP)
-		hlreg0 |= IXGBE_HLREG0_RXCRCSTRP;
-	else
+	if (rte_eth_dev_is_keep_crc(rx_conf->offloads))
 		hlreg0 &= ~IXGBE_HLREG0_RXCRCSTRP;
+	else
+		hlreg0 |= IXGBE_HLREG0_RXCRCSTRP;
 
 	/*
 	 * Configure jumbo frame support, if any.
@@ -4892,8 +4895,8 @@  ixgbe_dev_rx_init(struct rte_eth_dev *dev)
 		 * Reset crc_len in case it was changed after queue setup by a
 		 * call to configure.
 		 */
-		rxq->crc_len = (rx_conf->offloads & DEV_RX_OFFLOAD_CRC_STRIP) ?
-				0 : ETHER_CRC_LEN;
+		rxq->crc_len = rte_eth_dev_is_keep_crc(rx_conf->offloads) ?
+				ETHER_CRC_LEN : 0;
 
 		/* Setup the Base and Length of the Rx Descriptor Rings */
 		bus_addr = rxq->rx_ring_phys_addr;
@@ -4962,10 +4965,10 @@  ixgbe_dev_rx_init(struct rte_eth_dev *dev)
 	if (hw->mac.type == ixgbe_mac_82599EB ||
 	    hw->mac.type == ixgbe_mac_X540) {
 		rdrxctl = IXGBE_READ_REG(hw, IXGBE_RDRXCTL);
-		if (rx_conf->offloads & DEV_RX_OFFLOAD_CRC_STRIP)
-			rdrxctl |= IXGBE_RDRXCTL_CRCSTRIP;
-		else
+		if (rte_eth_dev_is_keep_crc(rx_conf->offloads))
 			rdrxctl &= ~IXGBE_RDRXCTL_CRCSTRIP;
+		else
+			rdrxctl |= IXGBE_RDRXCTL_CRCSTRIP;
 		rdrxctl &= ~IXGBE_RDRXCTL_RSCFRSTSIZE;
 		IXGBE_WRITE_REG(hw, IXGBE_RDRXCTL, rdrxctl);
 	}
diff --git a/drivers/net/kni/rte_eth_kni.c b/drivers/net/kni/rte_eth_kni.c
index ab63ea427..04cab248e 100644
--- a/drivers/net/kni/rte_eth_kni.c
+++ b/drivers/net/kni/rte_eth_kni.c
@@ -207,6 +207,7 @@  eth_kni_dev_info(struct rte_eth_dev *dev __rte_unused,
 	dev_info->max_rx_queues = KNI_MAX_QUEUE_PER_PORT;
 	dev_info->max_tx_queues = KNI_MAX_QUEUE_PER_PORT;
 	dev_info->min_rx_bufsize = 0;
+	dev_info->rx_offload_capa = DEV_RX_OFFLOAD_CRC_STRIP;
 }
 
 static int
diff --git a/drivers/net/mlx4/mlx4_rxq.c b/drivers/net/mlx4/mlx4_rxq.c
index 87688c1c7..d223e15cb 100644
--- a/drivers/net/mlx4/mlx4_rxq.c
+++ b/drivers/net/mlx4/mlx4_rxq.c
@@ -672,7 +672,8 @@  uint64_t
 mlx4_get_rx_queue_offloads(struct priv *priv)
 {
 	uint64_t offloads = DEV_RX_OFFLOAD_SCATTER |
-			    DEV_RX_OFFLOAD_CRC_STRIP;
+			    DEV_RX_OFFLOAD_CRC_STRIP |
+			    DEV_RX_OFFLOAD_KEEP_CRC;
 
 	if (priv->hw_csum)
 		offloads |= DEV_RX_OFFLOAD_CHECKSUM;
@@ -771,16 +772,16 @@  mlx4_rx_queue_setup(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc,
 		     (void *)dev, idx, desc);
 	}
 	/* By default, FCS (CRC) is stripped by hardware. */
-	if (offloads & DEV_RX_OFFLOAD_CRC_STRIP) {
-		crc_present = 0;
-	} else if (priv->hw_fcs_strip) {
-		crc_present = 1;
-	} else {
-		WARN("%p: CRC stripping has been disabled but will still"
-		     " be performed by hardware, make sure MLNX_OFED and"
-		     " firmware are up to date",
-		     (void *)dev);
-		crc_present = 0;
+	crc_present = 0;
+	if (rte_eth_dev_is_keep_crc(offloads)) {
+		if (priv->hw_fcs_strip) {
+			crc_present = 1;
+		} else {
+			WARN("%p: CRC stripping has been disabled but will still"
+			     " be performed by hardware, make sure MLNX_OFED and"
+			     " firmware are up to date",
+			     (void *)dev);
+		}
 	}
 	DEBUG("%p: CRC stripping is %s, %u bytes will be subtracted from"
 	      " incoming frames to hide it",
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;
+
 	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"
diff --git a/drivers/net/mvpp2/mrvl_ethdev.c b/drivers/net/mvpp2/mrvl_ethdev.c
index d5eb1fe69..74d067ac2 100644
--- a/drivers/net/mvpp2/mrvl_ethdev.c
+++ b/drivers/net/mvpp2/mrvl_ethdev.c
@@ -314,9 +314,11 @@  mrvl_dev_configure(struct rte_eth_dev *dev)
 		return -EINVAL;
 	}
 
-	if (!(dev->data->dev_conf.rxmode.offloads & DEV_RX_OFFLOAD_CRC_STRIP)) {
-		MRVL_LOG(INFO,
-			"L2 CRC stripping is always enabled in hw");
+	/* KEEP_CRC offload flag is not supported by PMD
+	 * can remove the below block when DEV_RX_OFFLOAD_CRC_STRIP removed
+	 */
+	if (rte_eth_dev_is_keep_crc(dev->data->dev_conf.rxmode.offloads)) {
+		MRVL_LOG(INFO, "L2 CRC stripping is always enabled in hw");
 		dev->data->dev_conf.rxmode.offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
 	}
 
diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index 36586969f..f8c65c1c6 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -411,8 +411,10 @@  nfp_net_configure(struct rte_eth_dev *dev)
 		return -EINVAL;
 	}
 
-	/* Checking RX offloads */
-	if (!(rxmode->offloads & DEV_RX_OFFLOAD_CRC_STRIP))
+	/* KEEP_CRC offload flag is not supported by PMD
+	 * can remove the below block when DEV_RX_OFFLOAD_CRC_STRIP removed
+	 */
+	if (rte_eth_dev_is_keep_crc(rxmode->offloads))
 		PMD_INIT_LOG(INFO, "HW does strip CRC. No configurable!");
 
 	return 0;
@@ -1166,7 +1168,8 @@  nfp_net_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 					     DEV_RX_OFFLOAD_UDP_CKSUM |
 					     DEV_RX_OFFLOAD_TCP_CKSUM;
 
-	dev_info->rx_offload_capa |= DEV_RX_OFFLOAD_JUMBO_FRAME;
+	dev_info->rx_offload_capa |= DEV_RX_OFFLOAD_JUMBO_FRAME |
+				     DEV_RX_OFFLOAD_KEEP_CRC;
 
 	if (hw->cap & NFP_NET_CFG_CTRL_TXVLAN)
 		dev_info->tx_offload_capa = DEV_TX_OFFLOAD_VLAN_INSERT;
diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
index 1d2e6b9e9..38c8c42bc 100644
--- a/drivers/net/null/rte_eth_null.c
+++ b/drivers/net/null/rte_eth_null.c
@@ -305,6 +305,7 @@  eth_dev_info(struct rte_eth_dev *dev,
 	dev_info->min_rx_bufsize = 0;
 	dev_info->reta_size = internals->reta_size;
 	dev_info->flow_type_rss_offloads = internals->flow_type_rss_offloads;
+	dev_info->rx_offload_capa = DEV_RX_OFFLOAD_CRC_STRIP;
 }
 
 static int
diff --git a/drivers/net/octeontx/octeontx_ethdev.c b/drivers/net/octeontx/octeontx_ethdev.c
index 1eb453b21..b0541f115 100644
--- a/drivers/net/octeontx/octeontx_ethdev.c
+++ b/drivers/net/octeontx/octeontx_ethdev.c
@@ -283,7 +283,10 @@  octeontx_dev_configure(struct rte_eth_dev *dev)
 		return -EINVAL;
 	}
 
-	if (!(rxmode->offloads & DEV_RX_OFFLOAD_CRC_STRIP)) {
+	/* KEEP_CRC offload flag is not supported by PMD
+	 * can remove the below block when DEV_RX_OFFLOAD_CRC_STRIP removed
+	 */
+	if (rte_eth_dev_is_keep_crc(rxmode->offloads)) {
 		PMD_INIT_LOG(NOTICE, "can't disable hw crc strip");
 		rxmode->offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
 	}
diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index 6bd4a7d79..626e89833 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -538,6 +538,7 @@  eth_dev_info(struct rte_eth_dev *dev,
 	dev_info->max_rx_queues = dev->data->nb_rx_queues;
 	dev_info->max_tx_queues = dev->data->nb_tx_queues;
 	dev_info->min_rx_bufsize = 0;
+	dev_info->rx_offload_capa = DEV_RX_OFFLOAD_CRC_STRIP;
 }
 
 static int
diff --git a/drivers/net/qede/qede_ethdev.c b/drivers/net/qede/qede_ethdev.c
index 7a63d0564..e5e8ea4a3 100644
--- a/drivers/net/qede/qede_ethdev.c
+++ b/drivers/net/qede/qede_ethdev.c
@@ -1532,6 +1532,7 @@  qede_dev_info_get(struct rte_eth_dev *eth_dev,
 				     DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM |
 				     DEV_RX_OFFLOAD_TCP_LRO	|
 				     DEV_RX_OFFLOAD_CRC_STRIP	|
+				     DEV_RX_OFFLOAD_KEEP_CRC    |
 				     DEV_RX_OFFLOAD_SCATTER	|
 				     DEV_RX_OFFLOAD_JUMBO_FRAME |
 				     DEV_RX_OFFLOAD_VLAN_FILTER |
@@ -1562,6 +1563,7 @@  qede_dev_info_get(struct rte_eth_dev *eth_dev,
 		.rx_drop_en = 1,
 		/* The below RX offloads are always enabled */
 		.offloads = (DEV_RX_OFFLOAD_CRC_STRIP  |
+			     DEV_RX_OFFLOAD_KEEP_CRC   |
 			     DEV_RX_OFFLOAD_IPV4_CKSUM |
 			     DEV_RX_OFFLOAD_TCP_CKSUM  |
 			     DEV_RX_OFFLOAD_UDP_CKSUM),
diff --git a/drivers/net/ring/rte_eth_ring.c b/drivers/net/ring/rte_eth_ring.c
index 35b837c3f..5aa48da42 100644
--- a/drivers/net/ring/rte_eth_ring.c
+++ b/drivers/net/ring/rte_eth_ring.c
@@ -164,6 +164,7 @@  eth_dev_info(struct rte_eth_dev *dev,
 	dev_info->max_rx_queues = (uint16_t)internals->max_rx_queues;
 	dev_info->max_tx_queues = (uint16_t)internals->max_tx_queues;
 	dev_info->min_rx_bufsize = 0;
+	dev_info->rx_offload_capa = DEV_RX_OFFLOAD_CRC_STRIP;
 }
 
 static int
diff --git a/drivers/net/sfc/sfc_rx.c b/drivers/net/sfc/sfc_rx.c
index cc76a5b15..cdcc70b26 100644
--- a/drivers/net/sfc/sfc_rx.c
+++ b/drivers/net/sfc/sfc_rx.c
@@ -1443,7 +1443,10 @@  sfc_rx_check_mode(struct sfc_adapter *sa, struct rte_eth_rxmode *rxmode)
 		rc = EINVAL;
 	}
 
-	if (~rxmode->offloads & DEV_RX_OFFLOAD_CRC_STRIP) {
+	/* KEEP_CRC offload flag is not supported by PMD
+	 * can remove the below block when DEV_RX_OFFLOAD_CRC_STRIP removed
+	 */
+	if (rte_eth_dev_is_keep_crc(rxmode->offloads)) {
 		sfc_warn(sa, "FCS stripping cannot be disabled - always on");
 		rxmode->offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
 		rxmode->hw_strip_crc = 1;
diff --git a/drivers/net/softnic/rte_eth_softnic.c b/drivers/net/softnic/rte_eth_softnic.c
index 6b3c13e5c..9360cf6c3 100644
--- a/drivers/net/softnic/rte_eth_softnic.c
+++ b/drivers/net/softnic/rte_eth_softnic.c
@@ -65,6 +65,7 @@  static const struct rte_eth_dev_info pmd_dev_info = {
 		.nb_min = 0,
 		.nb_align = 1,
 	},
+	.rx_offload_capa = DEV_RX_OFFLOAD_CRC_STRIP,
 };
 
 static int pmd_softnic_logtype;
diff --git a/drivers/net/szedata2/rte_eth_szedata2.c b/drivers/net/szedata2/rte_eth_szedata2.c
index 910c64d04..829ad13fa 100644
--- a/drivers/net/szedata2/rte_eth_szedata2.c
+++ b/drivers/net/szedata2/rte_eth_szedata2.c
@@ -1056,7 +1056,8 @@  eth_dev_info(struct rte_eth_dev *dev,
 	dev_info->max_rx_queues = internals->max_rx_queues;
 	dev_info->max_tx_queues = internals->max_tx_queues;
 	dev_info->min_rx_bufsize = 0;
-	dev_info->rx_offload_capa = DEV_RX_OFFLOAD_SCATTER;
+	dev_info->rx_offload_capa = DEV_RX_OFFLOAD_SCATTER |
+				    DEV_RX_OFFLOAD_CRC_STRIP;
 	dev_info->tx_offload_capa = 0;
 	dev_info->rx_queue_offload_capa = 0;
 	dev_info->tx_queue_offload_capa = 0;
diff --git a/drivers/net/thunderx/nicvf_ethdev.c b/drivers/net/thunderx/nicvf_ethdev.c
index 99fcd516b..f92f38f42 100644
--- a/drivers/net/thunderx/nicvf_ethdev.c
+++ b/drivers/net/thunderx/nicvf_ethdev.c
@@ -1887,7 +1887,10 @@  nicvf_dev_configure(struct rte_eth_dev *dev)
 		return -EINVAL;
 	}
 
-	if ((rxmode->offloads & DEV_RX_OFFLOAD_CRC_STRIP) == 0) {
+	/* KEEP_CRC offload flag is not supported by PMD
+	 * can remove the below block when DEV_RX_OFFLOAD_CRC_STRIP removed
+	 */
+	if (rte_eth_dev_is_keep_crc(rxmode->offloads)) {
 		PMD_INIT_LOG(NOTICE, "Can't disable hw crc strip");
 		rxmode->offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
 	}
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index ba9d768a0..eb1afe691 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -1070,7 +1070,8 @@  eth_dev_info(struct rte_eth_dev *dev,
 
 	dev_info->tx_offload_capa = DEV_TX_OFFLOAD_MULTI_SEGS |
 				DEV_TX_OFFLOAD_VLAN_INSERT;
-	dev_info->rx_offload_capa = DEV_RX_OFFLOAD_VLAN_STRIP;
+	dev_info->rx_offload_capa = DEV_RX_OFFLOAD_VLAN_STRIP |
+				    DEV_RX_OFFLOAD_CRC_STRIP;
 }
 
 static int
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index df50a571a..b629cd45d 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -2127,7 +2127,8 @@  virtio_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	};
 
 	host_features = VTPCI_OPS(hw)->get_features(hw);
-	dev_info->rx_offload_capa = DEV_RX_OFFLOAD_VLAN_STRIP;
+	dev_info->rx_offload_capa = DEV_RX_OFFLOAD_VLAN_STRIP |
+				    DEV_RX_OFFLOAD_CRC_STRIP;
 	if (host_features & (1ULL << VIRTIO_NET_F_GUEST_CSUM)) {
 		dev_info->rx_offload_capa |=
 			DEV_RX_OFFLOAD_TCP_CKSUM |
diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
index ba932ff27..95b465952 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
@@ -57,7 +57,8 @@ 
 	 DEV_RX_OFFLOAD_UDP_CKSUM |	\
 	 DEV_RX_OFFLOAD_TCP_CKSUM |	\
 	 DEV_RX_OFFLOAD_TCP_LRO |	\
-	 DEV_RX_OFFLOAD_JUMBO_FRAME)
+	 DEV_RX_OFFLOAD_JUMBO_FRAME |   \
+	 DEV_RX_OFFLOAD_CRC_STRIP)
 
 static int eth_vmxnet3_dev_init(struct rte_eth_dev *eth_dev);
 static int eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev);
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index a9977df97..5d0132223 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -129,6 +129,7 @@  static const struct {
 	RTE_RX_OFFLOAD_BIT2STR(SCATTER),
 	RTE_RX_OFFLOAD_BIT2STR(TIMESTAMP),
 	RTE_RX_OFFLOAD_BIT2STR(SECURITY),
+	RTE_RX_OFFLOAD_BIT2STR(KEEP_CRC),
 };
 
 #undef RTE_RX_OFFLOAD_BIT2STR
@@ -1185,6 +1186,14 @@  rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		return -EINVAL;
 	}
 
+	if ((local_conf.rxmode.offloads & DEV_RX_OFFLOAD_CRC_STRIP) &&
+			(local_conf.rxmode.offloads & DEV_RX_OFFLOAD_KEEP_CRC)) {
+		ethdev_log(ERR,
+			"Port id=%u not allowed to set both CRC STRIP and KEEP CRC offload flags\n",
+			port_id);
+		return -EINVAL;
+	}
+
 	/* Check that device supports requested rss hash functions. */
 	if ((dev_info.flow_type_rss_offloads |
 	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 36e3984ea..1f5652e85 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -939,6 +939,11 @@  struct rte_eth_conf {
 #define DEV_RX_OFFLOAD_SCATTER		0x00002000
 #define DEV_RX_OFFLOAD_TIMESTAMP	0x00004000
 #define DEV_RX_OFFLOAD_SECURITY         0x00008000
+
+/* Invalid to set both DEV_RX_OFFLOAD_CRC_STRIP and DEV_RX_OFFLOAD_KEEP_CRC
+ * No DEV_RX_OFFLOAD_CRC_STRIP flag means keep CRC
+ */
+#define DEV_RX_OFFLOAD_KEEP_CRC		0x00010000
 #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
 				 DEV_RX_OFFLOAD_UDP_CKSUM | \
 				 DEV_RX_OFFLOAD_TCP_CKSUM)
diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
index c9c825e3f..09a42f8c2 100644
--- a/lib/librte_ethdev/rte_ethdev_driver.h
+++ b/lib/librte_ethdev/rte_ethdev_driver.h
@@ -325,6 +325,26 @@  typedef int (*ethdev_uninit_t)(struct rte_eth_dev *ethdev);
 int __rte_experimental
 rte_eth_dev_destroy(struct rte_eth_dev *ethdev, ethdev_uninit_t ethdev_uninit);
 
+/**
+ * PMD helper function to check if keeping CRC is requested
+ *
+ * @param rx_offloads
+ *   offloads variable
+ *
+ * @return
+ *   Return positive if keeping CRC is requested,
+ *   zero if stripping CRC is requested
+ */
+static inline int
+rte_eth_dev_is_keep_crc(uint64_t rx_offloads)
+{
+	if (rx_offloads & DEV_RX_OFFLOAD_CRC_STRIP)
+		return 0;
+
+	/* no KEEP_CRC or CRC_STRIP offload flags means keep CRC */
+	return 1;
+}
+
 #ifdef __cplusplus
 }
 #endif