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

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

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

- [Medium] New, undisclosed fatal probe conditions derived purely from
  firmware/register content.
- [Medium] The kernel-doc contract of nbl_hw_ops::get_host_pf_mask in
  nbl_include/nbl_def_hw.h states 'PF0 cannot be masked, bit0 is…
- [Low] The commit message and a new code comment describe consumers and
  an execution-order guarantee that do not exist at this commit, and the…
- [Low] nbl_include/nbl_def_channel.h newly uses static_assert() but is
  not self-contained: its only include is <linux/types.h> (which pulls…
- [Low] The kernel-doc block for nbl_hw_ops::get_host_pf_mask documents
  '@priv: hw ops private context', but the declared first parameter is…
- [Low] Comment/implementation mismatch in
  nbl_res_ctrl_dev_setup_eth_info(): the comment states 'Code accepts…

--

net/nebula-matrix: add common resource implementation

This patch adds the common resource layer for the nbl driver, with helpers
converting between vsi_id, func_id, eth_id and pf_id, plus new hw ops to
read the board info, the FW eth bitmap and the real bus number.
nbl_res_init_leonis() is now called from nbl_core_init() and initializes
the VSI/Eth/PF data only for control devices.

A few questions below about the new firmware topology checks, the ops
documentation and some of the new comments.

nbl_res_start() initializes VSI/Eth/PF data structures only for control
devices (`common->has_ctrl == true`). Framework dispatch layer ensures
resource mapping APIs such as nbl_res_func_id_to_vsi_id() are only invoked
on control devices.

[Severity: Low]
Which dispatch layer is meant here?  At this commit nbl_core_init() returns
right after nbl_res_init_leonis():

	ret = nbl_res_init_leonis(adapter);
	if (ret)
		goto res_init_fail;
	return adapter;

adap->intf.resource_ops_tbl is published but has no consumer yet, and
nbl_res_vsi_id_to_pf_id() and nbl_res_pf_dev_vsi_type_to_hw_vsi_type() are
non-static with no users outside nbl_resource.c.

Could the commit message describe the guarantee that exists at this point in
the series, i.e. the has_ctrl checks inside the helpers themselves?

The same concern applies to the ordering comment in
nbl_res_ctrl_dev_sriov_info_init(), see below.

[ ... ]

[Severity: Medium]
Is the equality of these two values actually guaranteed by the hardware?

max_pf comes from the k_pf_mask register via nbl_res_init_pf_num(), while
eth_num comes from NBL_FW_BOARD_DW3 port_num via nbl_hw_get_board_info().
The two quantities are reported independently by firmware, and the only
statement that they must match is this comment.

The consequence is a probe failure: nbl_probe() -> nbl_core_init() ->
nbl_res_init_leonis() -> nbl_res_start() -> nbl_res_ctrl_dev_setup_eth_info()
returns -EINVAL, and nbl_core_init() turns that into ERR_PTR(ret), so the
control PF does not bind at all.

Would it make sense to document this firmware contract in nbl_def_hw.h next
to get_host_pf_mask/get_board_info, and to mention in the changelog that this
patch introduces new fatal probe conditions?

[Severity: Low]
This isn't a bug, but the word "contiguous" in the comment does not match the
loop, and the example given (0/2) is not contiguous either.

The loop imposes no contiguity rule: any subset of bits 0..3 whose population
count equals board_info.eth_num (and equals max_pf) is accepted, so
eth_bitmap = 0b1010 is mapped to PF0/PF1 as well.

Should the word be dropped, or should the bitmap actually be checked for
contiguity?

[ ... ]

[Severity: Low]
Where is nbl_dev_start()?  It does not exist at this commit, and later in the
series cfg_chan_qinfo_map_table() is reached from nbl_dev_setup_ctrl_dev() via
nbl_dev_setup_chan_qinfo():

	nbl_dev_setup_ctrl_dev()
	  nbl_dev_setup_chan_qinfo(dev_mgt, NBL_CHAN_TYPE_MAILBOX)
	    cfg_chan_qinfo_map_table()

so the function named in the ordering comment is wrong even at the end of the
series.  Could the comment name the actual caller?

[ ... ]

[Severity: Medium]
Does this treatment of bit0 agree with the ops documentation?

nbl_def_hw.h says of the same register:

	 * PF0 cannot be masked, bit0 is reserved and has no hardware effect.
	 * All-zero mask indicates all supported PFs are present and enabled.

but here bit0 is counted as a normal PF enable bit, and the contiguity test
then requires it to be clear: exp_contiguous_mask always contains BIT(0) for
any accepted pf_num >= 1, so a pf_mask with bit0 set fails.

Two examples: mask 0xff (PF0 only, with the documented-as-reserved bit0 also
set) gives pf_num == 0, and mask 0xfd gives pf_num == 1 with bit0 set and
trips the contiguity check.  Both end in -EINVAL and an aborted probe.

Should nbl_res_init_pf_num() ignore bit0, or should the header comment be
corrected to say bit0 is a real enable bit?

[Severity: Medium]
What happens on a board where the k_pf_mask register still reads its reset
value of zero?  nbl_def_hw.h describes an all-zero mask as "all supported PFs
are present and enabled", which yields pf_num == 8 here and is then rejected
with -EINVAL, failing probe.

Is the all-zero case expected to be rejected, and if so should the header
comment say so?

[ ... ]

[Severity: Low]
Should this header include linux/build_bug.h?  static_assert is defined there,
while the only include in nbl_def_channel.h is linux/types.h.

It builds today because every current translation unit pulls in linux/device.h
or linux/pci.h before the nbl headers, but nbl_hw_reg.h already includes
nbl_def_channel.h before linux/pci.h arrives via nbl_core.h, so a future .c or
.h that includes nbl_def_channel.h first would fail with an implicit
declaration of static_assert.

[ ... ]

[Severity: Low]
This isn't a bug, but the kernel-doc block just above get_host_pf_mask
documents "@priv: hw ops private context" while the declared first parameter
is struct nbl_hw_mgt *hw_mgt, so scripts/kernel-doc -Wall reports an
undescribed parameter hw_mgt plus an excess @priv description.

Since this patch extends the same ops structure and its new resource-layer
consumer relies on that comment block for the pf_mask contract, could the
parameter name be fixed here?

[ ... ]

[Severity: Medium]
This is the point where the new firmware topology validations become probe
failures: any -EINVAL from nbl_res_init_pf_num() or
nbl_res_ctrl_dev_setup_eth_info() propagates here and nbl_core_init() returns
ERR_PTR(ret), so nbl_probe() reports "Nbl adapter init fail" and the device
does not bind.

Could the changelog mention that this patch alone can turn a previously
successful probe into -EINVAL depending on register content?

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