Re: [PATCH v28 net-next 04/10] net/nebula-matrix: add common resource implementation

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] Misleading comment and (at this commit) write-only shared state
  in nbl_res_ctrl_dev_sriov_info_init(): the comment asserts that…
- [Low] Dead state in struct nbl_eth_info:
  nbl_res_ctrl_dev_setup_eth_info() (drivers/net/ethernet/nebula-…

--

net/nebula-matrix: add common resource implementation

This patch adds the chip-independent resource layer for the nbl driver,
including PF-count validation from the firmware k_pf_mask, per-PF BDF and
VSI base-id tables, eth port discovery, and the VSI/func/eth/PF conversion
helpers.  All of the table setup runs only on the control PF during probe.

Two small observations below, both about state that has no reader at this
point in the series.

[ ... ]

+	/*
+	 * Any subset of valid bitmap bits is accepted (e.g. 0/1, 0/2,
+	 * 1/3, etc.).  Firmware only needs to report the correct count
+	 * of active ports; no hard-coded fixed bit positions required.
+	 */
+	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++;
+		}
+	}
+
+	for (i = 0; i < res_mgt->common->max_pf; i++) {
+		eth_id = eth_info->eth_id[i];
+		eth_info->pf_bitmap[eth_id] |= BIT(i);
+	}

[Severity: Low]
This isn't a bug, but are eth_info->eth_bitmap, eth_info->logic_eth_id[]
and eth_info->pf_bitmap[] ever read?

Grepping the driver with the whole series applied, the only appearances of
these three members are the writes above plus the declaration in struct
nbl_eth_info.  eth_info->eth_id[] is the only member of the three tables
that gets consumed, via nbl_res_get_eth_id().

The commit message describes this as building "the per-PF eth_id /
logic_eth_id lookup tables", but nbl_res_get_eth_id() recomputes the
logical id instead of reading the table it just built:

	*eth_num = eth_info->eth_num;
	*eth_id = eth_info->eth_id[rel_pf_id];
	*logic_eth_id = rel_pf_id;

The two derivations agree for every layout the code accepts, because
max_pf == eth_num is enforced earlier, so the values are identical today.
Would it be clearer to either return eth_info->logic_eth_id[*eth_id] here,
or drop the unused members and the loops that fill them until a consumer
shows up?

+static int nbl_res_ctrl_dev_sriov_info_init(struct nbl_resource_mgt *res_mgt)
+{

[ ... ]

+	res_mgt->resource_info->sriov_info = sriov_info;
+	/*
+	 * common->hw_bus supplies bus number for channel mailbox QINFO mapping.
+	 * Execution order guarantee: this assignment runs before
+	 * cfg_chan_qinfo_map_table(), only executed
+	 * on control PF path.
+	 */
+	common->hw_bus = hw_bus;

[Severity: Low]
Can this comment be verified at this point in the series?

At this commit nothing reads common->hw_bus outside of the loop just below,
and cfg_chan_qinfo_map_table() has no caller yet, so the ordering contract
the comment documents cannot be checked against the tree here.  The readers
do arrive in the later patches of the series (nbl_res_intr_cfg_msix_map()
passes common->hw_bus to cfg_msix_map(), and nbl_dev_setup_chan_qinfo()
passes it to cfg_chan_qinfo_map_table()), and nbl_core_init() does call
nbl_res_init_leonis() before nbl_disp_init() and nbl_dev_init(), so the
ordering claim holds once the series is complete.

Would it make sense to move this comment to the patch that adds the
cfg_chan_qinfo_map_table() call, so the referenced consumer exists in the
same commit?

[ ... ]

+struct nbl_eth_info {
+	DECLARE_BITMAP(eth_bitmap, NBL_MAX_ETHERNET);
+	u8 pf_bitmap[NBL_MAX_ETHERNET];
+	u8 eth_num;
+	u8 resv[3];
+	u8 eth_id[NBL_MAX_ETHERNET];
+	u8 logic_eth_id[NBL_MAX_ETHERNET];
+};

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914123429.56596-1-illusion.wang%40nebula-matrix.com