Re: [PATCH v29 net-next 2/8] net/nebula-matrix: add common resource implementation

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Commit message statements are not backed by the submitted code:
  (a) it says `nbl_res_ctrl_dev_setup_eth_info()` builds "the per-PF…

--

net/nebula-matrix: add common resource implementation

This adds the chip-independent resource layer for the nbl driver: PF mask
parsing, SR-IOV BDF table construction, eth info and VSI base id setup on
the control PF, plus VSI/func/eth id conversion helpers.

> - nbl_res_ctrl_dev_setup_eth_info(): validates that firmware port
>   count (get_board_info()->eth_num) matches max_pf and the eth
>   bitmap reported by get_fw_eth_map(), then builds the per-PF
>   eth_id / logic_eth_id lookup tables.

[Severity: Low]
Is there really a logic_eth_id lookup table?  struct nbl_eth_info in
nbl_hw/nbl_resource.h only carries eth_num and eth_id[]:

> +struct nbl_eth_info {
> +	u8 eth_num;
> +	u8 resv[3];
> +	u8 eth_id[NBL_MAX_ETHERNET];
> +};

and nbl_res_get_eth_id() computes the logical id on the fly from the
relative PF id rather than reading it from a table:

> +	*logic_eth_id = rel_pf_id;

Would it be more accurate to say setup_eth_info() builds the per-PF eth_id
table, and that logic_eth_id is derived from the relative PF id?

> The resource conversion helpers themselves do not include locking;
> all resource operations must be serialized by upper dispatch layer via
> ops_mutex_lock. Non-control PF requests are routed to control PF via
> mailbox RPC, and these helpers are only executed on control PF.

[Severity: Low]
Do ops_mutex_lock and the mailbox RPC routing exist at this commit?  A grep
over drivers/net/ethernet/nebula-matrix/ at this revision finds no
ops_mutex_lock at all, and enum nbl_chan_msg_type in
nbl_include/nbl_def_channel.h only defines the ack opcode:

enum nbl_chan_msg_type {
	NBL_CHAN_MSG_ACK = 0,
	/* mailbox msg end */
	NBL_CHAN_MSG_MAILBOX_MAX,
};

The newly registered res_ops (get_vsi_id, get_eth_id) also have no caller
yet, and non-control callers are rejected locally instead of being
forwarded:

> +	if (!common->has_ctrl || !eth_num || !eth_id || !logic_eth_id)
> +		return -EINVAL;

Both the ops_mutex_lock serialization and the mailbox RPC routing appear
only in the later patches "dispatch: add control-level routing core
infrastructure" and "dispatch: implement channel RPC framework and
serialize hardware ops".  Could the message be reworded to describe those
as coming later in the series, so the "lookup helpers require no internal
locking" claim is justified by code that is actually present?

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