Re: [PATCH v14 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 has considered the AI review valid, or at least plausible.
---
net/nebula-matrix: add common resource implementation

The Resource layer processes the entries/data of various modules within
the processing chip to accomplish specific entry management operations,
this describes the module business capabilities of the chip and the data
it manages.
This patch provides the common part, including the 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_hw_leonis.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c
> index b63c91e559af4..55d6ed70a9e60 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c
[ ... ]
> @@ -148,6 +169,36 @@ static void nbl_hw_cfg_mailbox_qinfo(struct nbl_hw_mgt *hw_mgt, u16 func_id,
> +static void nbl_hw_cfg_mailbox_qinfo(struct nbl_hw_mgt *hw_mgt, u16 func_id,
> +				     u16 bus, u16 devid, u16 function)
> +{
> +	union nbl_mailbox_qinfo_map_table_u mb_qinfo_map;
> +
> +	memset(&mb_qinfo_map, 0, sizeof(mb_qinfo_map));
> +	mb_qinfo_map.info.function = function;
> +	mb_qinfo_map.info.devid = devid;
> +	mb_qinfo_map.info.bus = bus;

Since the memory layout of C bitfields within an integer is compiler-defined
and depends on the CPU's endianness, will assigning to bitfields in
mb_qinfo_map.info result in incorrect bits being written to the hardware
register on big-endian architectures?

Also, if the caller passes a function value that exceeds the width of the
function bitfield (which appears to be 3 bits based on standard PCIe), will
the value be silently truncated, causing multiple distinct PFs to map to the
same PCIe function?

> +	mb_qinfo_map.info.msix_idx_valid = 0;
> +	nbl_hw_wr_regs(hw_mgt, NBL_MAILBOX_QINFO_MAP_REG_ARR(func_id),
> +		       &mb_qinfo_map.data, sizeof(mb_qinfo_map));
> +}
> +
> +static void nbl_hw_get_board_info(struct nbl_hw_mgt *hw_mgt,
> +				  struct nbl_board_port_info *board_info)
> +{
> +	union nbl_fw_board_cfg_dw3 dw3 = { .info = { 0 } };
> +
> +	nbl_hw_read_mbx_regs(hw_mgt, NBL_FW_BOARD_DW3_OFFSET, &dw3.data,
> +			     sizeof(dw3));
> +	board_info->eth_num = dw3.info.port_num;

Similarly, does reading from the dw3.info bitfields cause incorrect values
to be extracted on big-endian architectures?

[ ... ]
> 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
> index 6885195c96ca8..7c366a1d5e489 100644
> --- 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
[ ... ]
> @@ -45,8 +48,164 @@ nbl_res_setup_ops(struct device *dev, struct nbl_resource_mgt *res_mgt)
> +static int nbl_res_ctrl_dev_setup_eth_info(struct nbl_resource_mgt *res_mgt)
> +{
[ ... ]
> +	eth_info->eth_num =
> +		(u8)hw_ops->get_fw_eth_num(res_mgt->hw_ops_tbl->priv);
> +	if (eth_info->eth_num > NBL_MAX_ETHERNET) {
> +		dev_warn(dev, "FW reports %u Ethernet ports, but only %u are supported\n",
> +			 eth_info->eth_num, NBL_MAX_ETHERNET);
> +		eth_info->eth_num = NBL_MAX_ETHERNET;
> +	}
> +	eth_bitmap = hw_ops->get_fw_eth_map(res_mgt->hw_ops_tbl->priv);
> +	/* for 2 eth port board, the eth_id is 0, 2 */
> +	for (i = 0; i < NBL_MAX_ETHERNET; i++) {
> +		if ((1 << i) & eth_bitmap) {
> +			set_bit(i, eth_info->eth_bitmap);
> +			eth_info->eth_id[eth_num] = i;
> +			eth_info->logic_eth_id[i] = eth_num;
> +			eth_num++;
> +		}
> +	}

If the firmware returns an eth_info->eth_num that is greater than the
number of set bits in eth_bitmap, could this lead to uninitialized elements
in the eth_id array being accessed later?

> +
> +	for (i = 0; i < res_mgt->resource_info->max_pf; i++) {
> +		/* Map PF index i to eth_id from eth_info->eth_id[i]
> +		 * if i < eth_num, otherwise map to eth_id 0
> +		 */
> +		if (i < eth_num) {
> +			eth_id = eth_info->eth_id[i];
> +			eth_info->pf_bitmap[eth_id] |= BIT(i);
> +		} else {
> +			eth_info->pf_bitmap[0] |= BIT(i);
> +		}
> +	}

If the eth_bitmap from the firmware does not have bit 0 set (meaning
physical port 0 is inactive), will excess PFs be mapped to a dead port here
instead of the first active physical port?

> +
> +	return 0;
> +}
> +
> +static int nbl_res_ctrl_dev_sriov_info_init(struct nbl_resource_mgt *res_mgt)
> +{
[ ... ]
> +	common->hw_bus = hw_ops->get_real_bus(p);
> +	for (func_id = 0; func_id < res_mgt->resource_info->max_pf; func_id++) {
> +		sriov_info = res_mgt->resource_info->sriov_info + func_id;
> +		sriov_info->bdf = PCI_DEVID(common->hw_bus,
> +					    PCI_DEVFN(common->devid, func_id));
> +	}
> +
> +	return 0;
> +}

Is there a potential mismatch in BDF tracking here if the base device function
(common->function) is not 0?

If the mailbox configuration layer expects the function to include
common->function, could this lead to inconsistent state between the
software and the hardware?