Re: [v4,net-next,06/11] net/nebula-matrix: add common resource implementation

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says "This is an automated AI code reviewer, please treat what
it says with a grain of salt."

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net/nebula-matrix: add common resource implementation

This patch adds the common resource layer for the nebula-matrix network
driver, implementing conversion relationships among vsi_id, func_id,
eth_id, and pf_id.

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_resource_leonis.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_resource_leonis.c
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_resource_leonis.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_resource_leonis.c

[ ... ]

> +static void nbl_res_get_eth_id(struct nbl_resource_mgt *res_mgt, u16 vsi_id,
> +			       u8 *eth_mode, u8 *eth_id, u8 *logic_eth_id)
> +{
> +	struct nbl_eth_info *eth_info = NBL_RES_MGT_TO_ETH_INFO(res_mgt);
> +	u16 pf_id = nbl_res_vsi_id_to_pf_id(res_mgt, vsi_id);
> +
> +	*eth_mode = eth_info->eth_num;
> +	if (pf_id < eth_info->eth_num) {
> +		*eth_id = eth_info->eth_id[pf_id];
> +		*logic_eth_id = pf_id;
> +		/* if pf_id > eth_num, use eth_id 0 */
> +	} else {

This isn't a bug, but the comment appears to be misplaced. It describes
what happens in the else branch but is located inside the if branch. Should
it be moved above the else?

[ ... ]

> +static int nbl_res_ctrl_dev_setup_eth_info(struct nbl_resource_mgt *res_mgt)
> +{

[ ... ]

> +	for (i = 0; i < NBL_RES_MGT_TO_PF_NUM(res_mgt); i++) {
> +		/* if pf_id <= eth_num, the pf relate corresponding eth_id*/
> +		if (i < eth_num) {

This isn't a bug, but the comment says "pf_id <= eth_num" while the code
checks "i < eth_num". Should the comment say "<" instead of "<="?

> +			eth_id = eth_info->eth_id[i];
> +			eth_info->pf_bitmap[eth_id] |= BIT(i);
> +		}
> +		/* if pf_id > eth_num, the pf relate eth 0*/
> +		else

Similarly, the comment says "pf_id > eth_num" but the else handles the case
where "i >= eth_num". Should the comment say ">=" instead of ">"?

> +			eth_info->pf_bitmap[0] |= BIT(i);
> +	}

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_resource.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_resource.c
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_resource.c

[ ... ]

> +static int vsi_id_to_pf_id(struct nbl_resource_mgt *res_mgt, u16 vsi_id)
> +{
> +	struct nbl_vsi_info *vsi_info = NBL_RES_MGT_TO_VSI_INFO(res_mgt);
> +	struct nbl_common_info *common = res_mgt->common;
> +	bool vsi_find = false;
> +	u32 pf_id = U32_MAX;
> +	int i, j;
> +
> +	for (i = 0; i < vsi_info->num; i++) {
> +		for (j = 0; j < NBL_VSI_SERV_MAX_TYPE; j++)
> +			if (vsi_id >= vsi_info->serv_info[i][j].base_id &&
> +			    (vsi_id < vsi_info->serv_info[i][j].base_id +
> +					      vsi_info->serv_info[i][j].num)) {
> +				vsi_find = true;
> +				break;
> +			}
> +
> +		if (vsi_find)
> +			break;
> +	}
> +
> +	if (vsi_find) {
> +		/* if pf_id < eth_num */
> +		if (j >= NBL_VSI_SERV_PF_DATA_TYPE &&
> +		    j <= NBL_VSI_SERV_PF_DATA_TYPE)

This isn't a bug, but the comment says "if pf_id < eth_num" while the code
checks if j equals NBL_VSI_SERV_PF_DATA_TYPE (which is 0). The comment does
not seem to describe the actual condition being tested. Could it be updated
to reflect what the code is checking?

> +			pf_id = i + common->mgt_pf;
> +	}
> +
> +	return pf_id;
> +}