Re: [Intel-wired-lan] [PATCH net-next 1/2] ice: Implement control of FCS/CRC stripping

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Tue, Jun 28, 2022 at 12:44:03PM +0200, Anatolii Gerasymenko wrote:
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> 
> The driver can allow the user to configure whether the CRC aka the FCS
> (Frame Check Sequence) is DMA'd to the host as part of the receive
> buffer.  The driver usually wants this feature disabled so that the
> hardware checks the FCS and strips it in order to save PCI bandwidth.
> 
> Control the reception of FCS to the host using the command:
> ethtool -K eth0 rx-fcs <on|off>
> 
> The default shown in ethtool -k eth0 | grep fcs; should be "off", as the
> hardware will drop any frame with a bad checksum, and DMA of the
> checksum is useless overhead especially for small packets.
> 
> Testing Hints:
> test the FCS/CRC arrives with received packets using
> tcpdump -nnpi eth0 -xxxx
> and it should show crc data as the last 4 bytes of the packet. Can also
> use wireshark to turn on CRC checking and check the data is correct.
> 
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Co-Developed-by: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
> Signed-off-by: Anatolii Gerasymenko <anatolii.gerasymenko@intel.com>
> Acked-by: Jesse Brandeburg <jesse.brandeburg@intel.com>

Sorry but NACK.

Could you pick
https://lore.kernel.org/bpf/20220616180609.905015-2-maciej.fijalkowski@intel.com/

and base your code on top of that? I have specially pulled out the switch
the 'changed' netdev features out of my loopback code so that we both can
take a favor of it.

Looks like you're going to be the first with landing the crc strip to the
netdev as I'm lost in fixing some stuff in libbpf/af_xdp :p

TBH the loopback toggle would also fit to your series:
https://lore.kernel.org/bpf/20220616180609.905015-3-maciej.fijalkowski@intel.com/

Thanks!

> ---
>  drivers/net/ethernet/intel/ice/ice.h         |  1 +
>  drivers/net/ethernet/intel/ice/ice_base.c    |  2 +-
>  drivers/net/ethernet/intel/ice/ice_ethtool.c |  5 +-
>  drivers/net/ethernet/intel/ice/ice_lib.c     | 21 +++++++++
>  drivers/net/ethernet/intel/ice/ice_lib.h     |  2 +
>  drivers/net/ethernet/intel/ice/ice_main.c    | 48 ++++++++++++++++++--
>  drivers/net/ethernet/intel/ice/ice_txrx.h    |  3 +-
>  7 files changed, 71 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index 60453b3b8d23..f04afce606b9 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -854,6 +854,7 @@ ice_fetch_u64_stats_per_ring(struct u64_stats_sync *syncp,
>  			     struct ice_q_stats stats, u64 *pkts, u64 *bytes);
>  int ice_up(struct ice_vsi *vsi);
>  int ice_down(struct ice_vsi *vsi);
> +int ice_down_up(struct ice_vsi *vsi);
>  int ice_vsi_cfg(struct ice_vsi *vsi);
>  struct ice_vsi *ice_lb_vsi_setup(struct ice_pf *pf, struct ice_port_info *pi);
>  int ice_vsi_determine_xdp_res(struct ice_vsi *vsi);
> diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
> index 136d7911adb4..6f092e06054e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_base.c
> +++ b/drivers/net/ethernet/intel/ice/ice_base.c
> @@ -417,7 +417,7 @@ static int ice_setup_rx_ctx(struct ice_rx_ring *ring)
>  	/* Strip the Ethernet CRC bytes before the packet is posted to host
>  	 * memory.
>  	 */
> -	rlan_ctx.crcstrip = 1;
> +	rlan_ctx.crcstrip = !(ring->flags & ICE_RX_FLAGS_CRC_STRIP_DIS);
>  
>  	/* L2TSEL flag defines the reported L2 Tags in the receive descriptor
>  	 * and it needs to remain 1 for non-DVM capable configurations to not
> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index 70335f6e8524..9489ab19c662 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -1283,10 +1283,7 @@ static int ice_set_priv_flags(struct net_device *netdev, u32 flags)
>  	}
>  	if (test_bit(ICE_FLAG_LEGACY_RX, change_flags)) {
>  		/* down and up VSI so that changes of Rx cfg are reflected. */
> -		if (!test_and_set_bit(ICE_VSI_DOWN, vsi->state)) {
> -			ice_down(vsi);
> -			ice_up(vsi);
> -		}
> +		ice_down_up(vsi);
>  	}
>  	/* don't allow modification of this flag when a single VF is in
>  	 * promiscuous mode because it's not supported
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index a6c4be5e5566..d9451a5c9106 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -1561,6 +1561,22 @@ void ice_vsi_manage_rss_lut(struct ice_vsi *vsi, bool ena)
>  	kfree(lut);
>  }
>  
> +/**
> + * ice_vsi_cfg_crc_strip - Configure CRC stripping for a VSI
> + * @vsi: VSI to be configured
> + * @disable: set to true to have FCS / CRC in the frame data
> + */
> +void ice_vsi_cfg_crc_strip(struct ice_vsi *vsi, bool disable)
> +{
> +	int i;
> +
> +	ice_for_each_rxq(vsi, i)
> +		if (disable)
> +			vsi->rx_rings[i]->flags |= ICE_RX_FLAGS_CRC_STRIP_DIS;
> +		else
> +			vsi->rx_rings[i]->flags &= ~ICE_RX_FLAGS_CRC_STRIP_DIS;
> +}
> +
>  /**
>   * ice_vsi_cfg_rss_lut_key - Configure RSS params for a VSI
>   * @vsi: VSI to be configured
> @@ -3276,6 +3292,11 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, bool init_vsi)
>  			 */
>  			if (test_bit(ICE_FLAG_RSS_ENA, pf->flags))
>  				ice_vsi_cfg_rss_lut_key(vsi);
> +
> +		/* disable or enable CRC stripping */
> +		ice_vsi_cfg_crc_strip(vsi, !!(vsi->netdev->features &
> +					      NETIF_F_RXFCS));
> +
>  		break;
>  	case ICE_VSI_VF:
>  		ret = ice_vsi_alloc_q_vectors(vsi);
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.h b/drivers/net/ethernet/intel/ice/ice_lib.h
> index 0095329949d4..d22f4b062c4f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.h
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.h
> @@ -89,6 +89,8 @@ void ice_vsi_free_tx_rings(struct ice_vsi *vsi);
>  
>  void ice_vsi_manage_rss_lut(struct ice_vsi *vsi, bool ena);
>  
> +void ice_vsi_cfg_crc_strip(struct ice_vsi *vsi, bool disable);
> +
>  void ice_update_tx_ring_stats(struct ice_tx_ring *ring, u64 pkts, u64 bytes);
>  
>  void ice_update_rx_ring_stats(struct ice_rx_ring *ring, u64 pkts, u64 bytes);
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index c1ac2f746714..6510f4910e5d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -3373,6 +3373,11 @@ static void ice_set_netdev_features(struct net_device *netdev)
>  	if (is_dvm_ena)
>  		netdev->hw_features |= NETIF_F_HW_VLAN_STAG_RX |
>  			NETIF_F_HW_VLAN_STAG_TX;
> +
> +	/* Leave CRC / FCS stripping enabled by default, but allow the value to
> +	 * be changed at runtime
> +	 */
> +	netdev->hw_features |= NETIF_F_RXFCS;
>  }
>  
>  /**
> @@ -5923,6 +5928,7 @@ ice_set_vlan_features(struct net_device *netdev, netdev_features_t features)
>  static int
>  ice_set_features(struct net_device *netdev, netdev_features_t features)
>  {
> +	netdev_features_t changed = netdev->features ^ features;
>  	struct ice_netdev_priv *np = netdev_priv(netdev);
>  	struct ice_vsi *vsi = np->vsi;
>  	struct ice_pf *pf = vsi->back;
> @@ -5943,16 +5949,23 @@ ice_set_features(struct net_device *netdev, netdev_features_t features)
>  	/* Multiple features can be changed in one call so keep features in
>  	 * separate if/else statements to guarantee each feature is checked
>  	 */
> -	if (features & NETIF_F_RXHASH && !(netdev->features & NETIF_F_RXHASH))
> -		ice_vsi_manage_rss_lut(vsi, true);
> -	else if (!(features & NETIF_F_RXHASH) &&
> -		 netdev->features & NETIF_F_RXHASH)
> -		ice_vsi_manage_rss_lut(vsi, false);
> +	if (changed & NETIF_F_RXHASH)
> +		ice_vsi_manage_rss_lut(vsi, !!(features & NETIF_F_RXHASH));
>  
>  	ret = ice_set_vlan_features(netdev, features);
>  	if (ret)
>  		return ret;
>  
> +	/* Turn on receive of FCS aka CRC, and after setting this
> +	 * flag the packet data will have the 4 byte CRC appended
> +	 */
> +	if (changed & NETIF_F_RXFCS) {
> +		ice_vsi_cfg_crc_strip(vsi, !!(features & NETIF_F_RXFCS));
> +		ret = ice_down_up(vsi);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if ((features & NETIF_F_NTUPLE) &&
>  	    !(netdev->features & NETIF_F_NTUPLE)) {
>  		ice_vsi_manage_fdir(vsi, true);
> @@ -6653,6 +6666,31 @@ int ice_down(struct ice_vsi *vsi)
>  	return 0;
>  }
>  
> +/**
> + * ice_down_up - shutdown the VSI connection and bring it up
> + * @vsi: the VSI to be reconnected
> + */
> +int ice_down_up(struct ice_vsi *vsi)
> +{
> +	int ret;
> +
> +	/* if DOWN already set, nothing to do */
> +	if (test_and_set_bit(ICE_VSI_DOWN, vsi->state))
> +		return 0;
> +
> +	ret = ice_down(vsi);
> +	if (ret)
> +		return ret;
> +
> +	ret = ice_up(vsi);
> +	if (ret) {
> +		netdev_err(vsi->netdev, "reallocating resources failed during netdev features change, may need to reload driver\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * ice_vsi_setup_tx_rings - Allocate VSI Tx queue resources
>   * @vsi: VSI having resources allocated
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
> index ca902af54bb4..932b5661ec4d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.h
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
> @@ -295,10 +295,11 @@ struct ice_rx_ring {
>  	struct xsk_buff_pool *xsk_pool;
>  	struct sk_buff *skb;
>  	dma_addr_t dma;			/* physical address of ring */
> -#define ICE_RX_FLAGS_RING_BUILD_SKB	BIT(1)
>  	u64 cached_phctime;
>  	u8 dcb_tc;			/* Traffic class of ring */
>  	u8 ptp_rx;
> +#define ICE_RX_FLAGS_RING_BUILD_SKB	BIT(1)
> +#define ICE_RX_FLAGS_CRC_STRIP_DIS	BIT(2)
>  	u8 flags;
>  } ____cacheline_internodealigned_in_smp;
>  
> -- 
> 2.25.1
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan