Re: [PATCH v31 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] The `get_host_pf_mask` kernel-doc in `nbl_def_hw.h` says
  nbl_res_init_pf_num() "always runs before this op's only consumer,…

--

net/nebula-matrix: add common resource implementation

This commit adds a chip-agnostic resource layer for the Nebula Matrix
driver. On the control PF it builds the PF topology, SR-IOV BDF, Ethernet
port and VSI identity tables once at probe time. It also adds new hw ops
(flush_write, get_real_bus, get_fw_eth_map, get_board_info) and the
VSI/PF/Eth ID conversion helpers.

[ ... ]

> 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 58d1b669b8cbb..4b420db91963d 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

[ ... ]

> @@ -42,9 +55,22 @@ struct nbl_hw_ops {
>  	 * op only returns the raw register value.
>  	 */
>  	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 for get_host_pf_mask just above this
hunk now looks stale:

	 * The mask is validated in the resource layer:
	 * nbl_res_init_pf_num() rejects an all-zero mask and any
	 * non-contiguous layout with -EINVAL, and it always runs before
	 * this op's only consumer, nbl_chan_cfg_qinfo_map_table().  This
	 * op only returns the raw register value.

This patch adds nbl_res_init_pf_num() in nbl_resource_leonis.c, and that
function calls the op itself:

	hw_ops->get_host_pf_mask(res_mgt->hw_ops_tbl->priv, &pf_mask);

That means nbl_chan_cfg_qinfo_map_table() is no longer the only consumer.
The same is still true at the end of the series, where the op is called
from both nbl_resource_leonis.c and nbl_channel.c.

The ordering claim does hold by the end of the series. In "net/nebula-matrix:
add common/ctrl dev init/remove operation", nbl_dev_init() calls
nbl_dev_setup_chan_qinfo() only under common->has_ctrl, and it does so
after nbl_res_init_leonis().

Could the "only consumer" wording be changed to list both callers?

> 
>  	void (*cfg_mailbox_qinfo)(struct nbl_hw_mgt *hw_mgt, u16 func_id,
>  				  u8 bus, u8 devid, u8 function);

[ ... ]

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