Re: [PATCH net-next 1/2] nfp: add ethtool flow steering callbacks

[Simon Horman <horms@kernel.org>]

On Fri, Nov 17, 2023 at 09:11:13AM +0200, Louis Peens wrote:
> From: Yinjun Zhang <yinjun.zhang@corigine.com>
> 
> This is the first part to implement flow steering. The communication
> between ethtool and driver is done. User can use following commands
> to display and set flows:
> 
> ethtool -n <netdev>
> ethtool -N <netdev> flow-type ...
> 
> Signed-off-by: Yinjun Zhang <yinjun.zhang@corigine.com>
> Signed-off-by: Louis Peens <louis.peens@corigine.com>

Thanks Yinjun and Louis,

The minor suggestion provided inline not withstanding this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

...

> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> index e75cbb287625..d7896391b8ba 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> @@ -1317,6 +1317,116 @@ static int nfp_net_get_rss_hash_opts(struct nfp_net *nn,
>  	return 0;
>  }
>  
> +#define NFP_FS_MAX_ENTRY	1024
> +
> +static int nfp_net_fs_to_ethtool(struct nfp_fs_entry *entry, struct ethtool_rxnfc *cmd)
> +{
> +	struct ethtool_rx_flow_spec *fs = &cmd->fs;
> +	unsigned int i;
> +
> +	switch (entry->flow_type & ~FLOW_RSS) {
> +	case TCP_V4_FLOW:
> +	case UDP_V4_FLOW:
> +	case SCTP_V4_FLOW:
> +		fs->h_u.tcp_ip4_spec.ip4src = entry->key.sip4;
> +		fs->h_u.tcp_ip4_spec.ip4dst = entry->key.dip4;
> +		fs->h_u.tcp_ip4_spec.psrc   = entry->key.sport;
> +		fs->h_u.tcp_ip4_spec.pdst   = entry->key.dport;
> +		fs->m_u.tcp_ip4_spec.ip4src = entry->msk.sip4;
> +		fs->m_u.tcp_ip4_spec.ip4dst = entry->msk.dip4;
> +		fs->m_u.tcp_ip4_spec.psrc   = entry->msk.sport;
> +		fs->m_u.tcp_ip4_spec.pdst   = entry->msk.dport;
> +		break;
> +	case TCP_V6_FLOW:
> +	case UDP_V6_FLOW:
> +	case SCTP_V6_FLOW:
> +		for (i = 0; i < 4; i++) {
> +			fs->h_u.tcp_ip6_spec.ip6src[i] = entry->key.sip6[i];
> +			fs->h_u.tcp_ip6_spec.ip6dst[i] = entry->key.dip6[i];
> +			fs->m_u.tcp_ip6_spec.ip6src[i] = entry->msk.sip6[i];
> +			fs->m_u.tcp_ip6_spec.ip6dst[i] = entry->msk.dip6[i];
> +		}

I think the above loop can be more succinctly be expressed using a single
memcpy(). For which I do see precedence in Intel drivers. Likewise
elsewhere in this patch-set.

I don't feel strongly about this, so feel free to take this suggestion,
defer it to later, or dismiss it entirely.

> +		fs->h_u.tcp_ip6_spec.psrc = entry->key.sport;
> +		fs->h_u.tcp_ip6_spec.pdst = entry->key.dport;
> +		fs->m_u.tcp_ip6_spec.psrc = entry->msk.sport;
> +		fs->m_u.tcp_ip6_spec.pdst = entry->msk.dport;
> +		break;

...