[PATCH v3 53/73] net/ntnic: enable RSS feature

[Stephen Hemminger]

On Wed, 23 Oct 2024 19:00:01 +0200
Serhii Iliushyk <sil-plv at napatech.com> wrote:

> +
> +		rte_memcpy(&tmp_rss_conf.rss_key, rss_conf->rss_key, rss_conf->rss_key_len);

Avoid use of rte_memcpy(), it has less checking that memcpy().
The only place it can make sense is in the hot data path where compiler is more
conservative about alignment.

> +		rte_memcpy(&ndev->rss_conf, &tmp_rss_conf, sizeof(struct nt_eth_rss_conf));
>

Use structure assignment instead of memcpy() to keep type checking.

> +	if (rss_conf->rss_key != NULL) {
> +		int key_len = rss_conf->rss_key_len < MAX_RSS_KEY_LEN ? rss_conf->rss_key_len
> +			: MAX_RSS_KEY_LEN;

Use RTE_MIN() and the key_len variable should not be signed.

> +		memset(rss_conf->rss_key, 0, rss_conf->rss_key_len);
> +		rte_memcpy(rss_conf->rss_key, &ndev->rss_conf.rss_key, key_len);

> +static int eth_dev_rss_hash_update(struct rte_eth_dev *eth_dev, struct rte_eth_rss_conf *rss_conf)
> +{
> +	const struct flow_filter_ops *flow_filter_ops = get_flow_filter_ops();
> +
> +	if (flow_filter_ops == NULL) {
> +		NT_LOG_DBGX(ERR, NTNIC, "flow_filter module uninitialized");
> +		return -1;
> +	}
> +
> +	struct pmd_internals *internals = (struct pmd_internals *)eth_dev->data->dev_private;

Since dev_private is void *, no need for the cast here (in C code).

> +
> +	struct flow_nic_dev *ndev = internals->flw_dev->ndev;
> +	struct nt_eth_rss_conf tmp_rss_conf = { 0 };
> +	const int hsh_idx = 0;	/* hsh index 0 means the default receipt in HSH module */
> +	int res = 0;
> +
> +	if (rss_conf->rss_key != NULL) {
> +		if (rss_conf->rss_key_len > MAX_RSS_KEY_LEN) {
> +			NT_LOG(ERR, NTNIC,
> +				"ERROR: - RSS hash key length %u exceeds maximum value %u",
> +				rss_conf->rss_key_len, MAX_RSS_KEY_LEN);
> +			return -1;
> +		}
> +
> +		rte_memcpy(&tmp_rss_conf.rss_key, rss_conf->rss_key, rss_conf->rss_key_len);
> +	}
> +
> +	tmp_rss_conf.algorithm = rss_conf->algorithm;
> +
> +	tmp_rss_conf.rss_hf = rss_conf->rss_hf;
> +	res = flow_filter_ops->flow_nic_set_hasher_fields(ndev, hsh_idx, tmp_rss_conf);

In general, this code is good about moving declarations next to first use.
But here res is initialized to 0 but then set again from hasher_fields.
Why not just move the declaration there.