Re: [PATCH net-next v09 4/5] hinic3: Add ethtool rss ops

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
hinic3: Add ethtool rss ops

This commit implements ethtool callback functions for the hinic3 driver.
These callbacks allow users to utilize ethtool for detailed RSS parameters
configuration and monitoring.

> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c

[ ... ]

> +int hinic3_set_channels(struct net_device *netdev,
> +			struct ethtool_channels *channels)
> +{
[ ... ]
> +	if (netif_running(netdev)) {
> +		q_params = nic_dev->q_params;
> +		q_params.num_qps = (u16)count;
> +		q_params.txqs_res = NULL;
> +		q_params.rxqs_res = NULL;
> +		q_params.irq_cfg = NULL;
> +
> +		err = hinic3_change_channel_settings(netdev, &q_params);
> +		if (err) {
> +			netdev_err(netdev, "Failed to change channel settings\n");
> +			return err;
> +		}
> +	} else {
> +		nic_dev->q_params.num_qps = (u16)count;
> +	}
> +
> +	err = hinic3_rss_update_num_qps_and_reprogram(netdev);
> +	if (err)
> +		return err;

[Severity: Medium]
If hinic3_rss_update_num_qps_and_reprogram() fails, the error code is
returned to the caller, but the queue changes made earlier by
hinic3_change_channel_settings() remain active. Is it expected that a failure
leaves the newly requested channel configuration running?

[ ... ]

> +static int hinic3_set_rss_rxfh(struct net_device *netdev,
> +			       const u32 *indir, u8 *key)
> +{
[ ... ]
> +	if (indir) {
> +		for (i = 0; i < L2NIC_RSS_INDIR_SIZE; i++)
> +			temp_indir[i] = (u16)indir[i];
> +
> +		err = hinic3_rss_set_indir_tbl(nic_dev->hwdev, temp_indir);
> +		if (err) {
> +			netdev_err(netdev, "Failed to set rss indir table\n");
> +			return err;
> +		}
> +
> +		memcpy(nic_dev->rss_indir, temp_indir, L2NIC_RSS_INDIR_SIZE);

[Severity: High]
Does this copy the entire indirection table? Since temp_indir is an array
of u16 elements, copying L2NIC_RSS_INDIR_SIZE bytes instead of
L2NIC_RSS_INDIR_SIZE * sizeof(u16) will only update the first half of the
nic_dev->rss_indir array.

[ ... ]

> +static int hinic3_rss_get_indir_tbl(struct hinic3_hwdev *hwdev,
> +				    u32 *indir_table)
> +{
[ ... ]
> +	err = hinic3_cmdq_detail_resp(hwdev, MGMT_MOD_L2NIC,
> +				      L2NIC_UCODE_CMD_GET_RSS_INDIR_TBL,
> +				      pair.in, pair.out, NULL);
> +	if (err) {
> +		dev_err(hwdev->dev, "Failed to get rss indir table\n");
> +		goto err_get_indir_tbl;
> +	}
> +
> +	indir_tbl = (__force __le16 *)pair.out->buf;
> +	for (i = 0; i < L2NIC_RSS_INDIR_SIZE; i++)
> +		indir_table[i] = le16_to_cpu(*(indir_tbl + i));

[Severity: High]
Should the response buffer words be swabbed using hinic3_cmdq_buf_swab32()
before reading, similar to how it is handled in the set operation? Also,
does the data start immediately at offset 0, or is there a 16-byte
reserved prefix like the one defined in struct l2nic_cmd_rss_set_indir_tbl
that needs to be skipped?

[ ... ]

> +static int hinic3_update_hash_func_type(struct net_device *netdev, u8 hfunc)
> +{
> +	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
> +	enum hinic3_rss_hash_type new_rss_hash_type;
> +	int err;
[ ... ]
> +	if (new_rss_hash_type == nic_dev->rss_hash_type)
> +		return 0;
> +
> +	err = hinic3_rss_set_hash_type(nic_dev->hwdev, nic_dev->rss_hash_type);

[Severity: Medium]
Will this push the old hash type to the hardware instead of the newly
requested one? It passes nic_dev->rss_hash_type rather than
new_rss_hash_type.

> +	if (err) {
> +		netdev_err(netdev, "Failed to set RSS hash type to HW\n");
> +		return err;
> +	}
> +
> +	nic_dev->rss_hash_type = new_rss_hash_type;
> +
> +	return 0;
> +}