Re: [PATCH net-next v01 10/12] hinic3: Add Rss function

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 26/08/2025 10:05, Fan Gong wrote:
> Initialize rss functions. Configure rss hash data and HW resources.
> 
> Co-developed-by: Xin Guo <guoxin09@huawei.com>
> Signed-off-by: Xin Guo <guoxin09@huawei.com>
> Co-developed-by: Zhu Yikai <zhuyikai1@h-partners.com>
> Signed-off-by: Zhu Yikai <zhuyikai1@h-partners.com>
> Signed-off-by: Fan Gong <gongfan1@huawei.com>
> ---
>   drivers/net/ethernet/huawei/hinic3/Makefile   |   1 +
>   .../net/ethernet/huawei/hinic3/hinic3_main.c  |   9 +-
>   .../huawei/hinic3/hinic3_mgmt_interface.h     |  55 +++
>   .../huawei/hinic3/hinic3_netdev_ops.c         |  18 +
>   .../ethernet/huawei/hinic3/hinic3_nic_dev.h   |   5 +
>   .../net/ethernet/huawei/hinic3/hinic3_rss.c   | 359 ++++++++++++++++++
>   .../net/ethernet/huawei/hinic3/hinic3_rss.h   |  14 +
>   7 files changed, 460 insertions(+), 1 deletion(-)
[...]

> +static int alloc_rss_resource(struct net_device *netdev)
> +{
> +	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
> +	static const u8 default_rss_key[L2NIC_RSS_KEY_SIZE] = {
> +		0x6d, 0x5a, 0x56, 0xda, 0x25, 0x5b, 0x0e, 0xc2,
> +		0x41, 0x67, 0x25, 0x3d, 0x43, 0xa3, 0x8f, 0xb0,
> +		0xd0, 0xca, 0x2b, 0xcb, 0xae, 0x7b, 0x30, 0xb4,
> +		0x77, 0xcb, 0x2d, 0xa3, 0x80, 0x30, 0xf2, 0x0c,
> +		0x6a, 0x42, 0xb7, 0x3b, 0xbe, 0xac, 0x01, 0xfa};
> +
> +	nic_dev->rss_hkey = kzalloc(L2NIC_RSS_KEY_SIZE, GFP_KERNEL);

no need to request zero'ed allocation if you are going to overwrite it
completely on the very next line.

> +	if (!nic_dev->rss_hkey)
> +		return -ENOMEM;
> +
> +	memcpy(nic_dev->rss_hkey, default_rss_key, L2NIC_RSS_KEY_SIZE);

I would better move this line after both allocations when the code flow
has no way to fail.

> +	nic_dev->rss_indir = kcalloc(L2NIC_RSS_INDIR_SIZE, sizeof(u32),
> +				     GFP_KERNEL);

why do you allocate L2NIC_RSS_INDIR_SIZE of u32 when the HW table has
le16 type for the entry?

> +	if (!nic_dev->rss_indir) {
> +		kfree(nic_dev->rss_hkey);
> +		nic_dev->rss_hkey = NULL;
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int hinic3_rss_set_indir_tbl(struct hinic3_hwdev *hwdev,
> +				    const u32 *indir_table)
> +{
> +	struct l2nic_cmd_rss_set_indir_tbl *indir_tbl;
> +	struct hinic3_cmd_buf *cmd_buf;
> +	__le64 out_param;
> +	int err;
> +	u32 i;
> +
> +	cmd_buf = hinic3_alloc_cmd_buf(hwdev);
> +	if (!cmd_buf) {
> +		dev_err(hwdev->dev, "Failed to allocate cmd buf\n");
> +		return -ENOMEM;
> +	}
> +
> +	cmd_buf->size = cpu_to_le16(sizeof(struct l2nic_cmd_rss_set_indir_tbl));
> +	indir_tbl = cmd_buf->buf;
> +	memset(indir_tbl, 0, sizeof(*indir_tbl));
> +
> +	for (i = 0; i < L2NIC_RSS_INDIR_SIZE; i++)
> +		indir_tbl->entry[i] = cpu_to_le16((u16)indir_table[i]);
> +
> +	hinic3_cmdq_buf_swab32(indir_tbl, sizeof(*indir_tbl));
> +
> +	err = hinic3_cmdq_direct_resp(hwdev, MGMT_MOD_L2NIC,
> +				      L2NIC_UCODE_CMD_SET_RSS_INDIR_TBL,
> +				      cmd_buf, &out_param);
> +	if (err || out_param != 0) {

no need for "!= 0"

> +		dev_err(hwdev->dev, "Failed to set rss indir table\n");
> +		err = -EFAULT;
> +	}
> +
> +	hinic3_free_cmd_buf(hwdev, cmd_buf);
> +
> +	return err;
> +}

[...]

> +static int hinic3_rss_cfg_hash_key(struct hinic3_hwdev *hwdev, u8 opcode,
> +				   u8 *key)
> +{
> +	struct l2nic_cmd_cfg_rss_hash_key hash_key = {};
> +	struct mgmt_msg_params msg_params = {};
> +	int err;
> +
> +	hash_key.func_id = hinic3_global_func_id(hwdev);
> +	hash_key.opcode = opcode;
> +
> +	if (opcode == MGMT_MSG_CMD_OP_SET)
> +		memcpy(hash_key.key, key, L2NIC_RSS_KEY_SIZE);

here you copy hash key to a stack allocated structure ...

> +
> +	mgmt_msg_params_init_default(&msg_params, &hash_key, sizeof(hash_key));


... which is copied to another stack allocated structure ...

> +
> +	err = hinic3_send_mbox_to_mgmt(hwdev, MGMT_MOD_L2NIC,
> +				       L2NIC_CMD_CFG_RSS_HASH_KEY, &msg_params);
> +	if (err || hash_key.msg_head.status) {
> +		dev_err(hwdev->dev, "Failed to %s hash key, err: %d, status: 0x%x\n",
> +			opcode == MGMT_MSG_CMD_OP_SET ? "set" : "get",
> +			err, hash_key.msg_head.status);
> +		return -EINVAL;
> +	}
> +
> +	if (opcode == MGMT_MSG_CMD_OP_GET)
> +		memcpy(key, hash_key.key, L2NIC_RSS_KEY_SIZE);
> +
> +	return 0;
> +}
> +
> +static int hinic3_rss_set_hash_key(struct hinic3_hwdev *hwdev, const u8 *key)
> +{
> +	u8 hash_key[L2NIC_RSS_KEY_SIZE];
> +
> +	memcpy(hash_key, key, L2NIC_RSS_KEY_SIZE);

... but it was already copied to stack allocated buffer ...

> +
> +	return hinic3_rss_cfg_hash_key(hwdev, MGMT_MSG_CMD_OP_SET, hash_key);
> +}
> +
> +static int hinic3_set_hw_rss_parameters(struct net_device *netdev, u8 rss_en)
> +{
> +	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
> +	int err;
> +
> +	err = hinic3_rss_set_hash_key(nic_dev->hwdev, nic_dev->rss_hkey);

... which is previously copied from static array.

It's 4 copies in total to configure one simple thing. Looks like too
much of copying with no good reason


> +	if (err)
> +		return err;
> +