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

[netdev-bot+sashiko@kernel.org]

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.

> 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
> new file mode 100644
> index 000000000000..a719cc8fbf8a
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_resource_leonis.c
> @@ -0,0 +1,334 @@

[ ... ]

> +	eth_info->eth_num = fw_port_num;
> +	/* Intentional design constraint: each PF maps to exactly one
> +	 * Ethernet port. This couples PF identity to port identity
> +	 * and is required by nbl_res_get_eth_id() which indexes
> +	 * eth_info->eth_id[] by relative PF id.
> +	 */
> +	if (res_mgt->resource_info->max_pf != eth_info->eth_num) {
> +		dev_err(dev, "Invalid PF-to-port topology: max_pf=%u, eth_num=%u\n",
> +			res_mgt->resource_info->max_pf, eth_info->eth_num);
> +		return -EINVAL;
> +	}

[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?

> +
> +	/*
> +	 * Original comment said dual-port board eth_id fixed to 0,2;
> +	 * Code accepts any contiguous valid bitmap bits (0/1 or 0/2 etc).
> +	 * Firmware only needs to report 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++;
> +		}
> +	}

[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?

[ ... ]

> +	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() in nbl_dev_start(), only executed
> +	 * on control PF path.
> +	 */
> +	common->hw_bus = hw_bus;

[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?

[ ... ]

> +static int nbl_res_init_pf_num(struct nbl_resource_mgt *res_mgt)
> +{
> +	struct nbl_hw_ops *hw_ops = res_mgt->hw_ops_tbl->ops;
> +	u32 exp_contiguous_mask = 0;
> +	u32 pf_mask = 0;
> +	u32 pf_num = 0;
> +	int i;
> +
> +	hw_ops->get_host_pf_mask(res_mgt->hw_ops_tbl->priv, &pf_mask);
> +
> +	/*
> +	 * k_pf_mask register rule:
> +	 * bit N == 0  -> PF#N enabled; bit N == 1 -> PF#N masked out.
> +	 * Hardware constraint: bit0 is reserved, PF0 cannot be masked.
> +	 * All-zero pf_mask means all PF0~PF7 are enabled.
> +	 *
> +	 * Product firmware constraint: only 3 valid configurations supported:
> +	 * 1 PF  (PF0 only): pf_num = 1, mask = 0xfe
> +	 * 2 PFs (PF0,PF1):  pf_num = 2, mask = 0xfc
> +	 * 4 PFs (PF0~PF3): pf_num = 4, mask = 0xf0
> +	 * No other PF count or sparse/non-contiguous PF layout is allowed.
> +	 */
> +	for (i = 0; i < NBL_MAX_PF; i++) {
> +		if (!(pf_mask & (1 << i)))
> +			pf_num++;
> +	}
> +
> +	/*
> +	 * Sanity check: enabled PFs must be contiguous starting from PF0.
> +	 * Current resource framework uses relative PF id, sparse PF layout
> +	 * will cause mismatch between resource layer and hardware func_id.
> +	 */
> +	for (i = 0; i < pf_num; i++)
> +		exp_contiguous_mask |= BIT(i);
> +	if ((pf_mask & exp_contiguous_mask) != 0) {
> +		dev_err(res_mgt->common->dev,
> +			"pf_mask 0x%08x: non-contiguous enabled PF, unsupported\n",
> +			pf_mask);
> +		return -EINVAL;
> +	}

[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?

> +
> +	/* Only allow product-specified PF count: 1 / 2 / 4 */
> +	if (pf_num != 1 && pf_num != 2 && pf_num != 4) {
> +		dev_err(res_mgt->common->dev,
> +			"Invalid pf_num=%u (mask=0x%08x), only 1/2/4 PFs supported\n",
> +			pf_num, pf_mask);
> +		return -EINVAL;
> +	}

[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?

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_def_channel.h b/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_def_channel.h
> index 4fc987c0e56d..61dd97c779ef 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_def_channel.h
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_def_channel.h
> @@ -39,6 +39,16 @@ enum nbl_chan_state {
>  	NBL_CHAN_STATE_NBITS
>  };
>  
> +struct nbl_board_port_info {
> +	u8 eth_num;
> +	u8 eth_speed;
> +	u8 p4_version;
> +	u8 rsv[5];
> +};
> +
> +static_assert(sizeof(struct nbl_board_port_info) == 8,
> +	      "nbl_board_port_info size must be 8 bytes");
> +

[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.

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_def_hw.h b/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_def_hw.h
> index 587ac0c58c24..ee53f9e10a8e 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_def_hw.h
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_def_hw.h
> @@ -34,9 +35,13 @@ struct nbl_hw_ops {
>  	 * or other PF counts are unsupported by driver resource management.
>  	 */
>  	void (*get_host_pf_mask)(struct nbl_hw_mgt *hw_mgt, u32 *pf_mask);
> +	void (*get_real_bus)(struct nbl_hw_mgt *hw_mgt, u8 *bus);

[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?

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> index b7c80ea54c8d..1aafed2d46d7 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> @@ -42,7 +44,13 @@ struct nbl_adapter *nbl_core_init(struct pci_dev *pdev,
>  	ret = nbl_chan_init_common(adapter);
>  	if (ret)
>  		goto chan_init_fail;
> +
> +	ret = nbl_res_init_leonis(adapter);
> +	if (ret)
> +		goto res_init_fail;
>  	return adapter;

[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