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

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.

[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[]:

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