Re: [PATCH v30 net-next 5/8] net/nebula-matrix: dispatch: add control-level routing core infrastructure

[netdev-bot+sashiko@kernel.org]

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 commit message of "net/nebula-matrix: dispatch: add control-
  level routing core infrastructure" says the patch will "Add wire ABI…
- [Low] At this commit, the kernel-doc Warning paragraph for struct
  nbl_dispatch_ops (nbl_def_dispatch.h lines 25-31) describes five…
- [Low] The kernel-doc for struct nbl_dispatch_ops (nbl_def_dispatch.h)
  calls @init_module "dispatch layer initialization" and @deinit_module…

--

net/nebula-matrix: dispatch: add control-level routing core infrastructure

This commit adds the nbl dispatch layer. It allocates a devm-managed
nbl_dispatch_mgt, an ops table and a ctrl_lvl bitmap. When has_ctrl is
set, it enables the MGT level and installs init_module/deinit_module
wrappers that forward to the resource ops.

> MGT level is only enabled for Control PF. Add kerneldoc for caller
> permission constraints. Add wire ABI structures with static_assert for
> upcoming mailbox RPC.

[Severity: Low]
Is the last sentence accurate for this patch? The diff touches the
Makefile, nbl_core.h, nbl_dispatch.c, nbl_dispatch.h, nbl_def_dispatch.h
and nbl_main.c. None of these files gets a mailbox wire-format struct or
a static_assert.

At this commit, the driver has one static_assert, for struct
nbl_board_port_info in nbl_def_channel.h. It was already there, and this
patch does not change it.

The RPC payload structs and their size checks (nbl_chan_param_cfg_msix_map,
nbl_chan_param_set_mailbox_irq, nbl_chan_param_get_vsi_id,
nbl_chan_param_get_eth_id) first appear in the next patch,
"net/nebula-matrix: dispatch: implement channel RPC framework and
serialize hardware ops".

Could this sentence be dropped here, or moved to that patch's commit
message?

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_def_dispatch.h b/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_def_dispatch.h
> new file mode 100644
> index 000000000000..b3398591035b
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_def_dispatch.h
> @@ -0,0 +1,45 @@

[ ... ]

> +/**
> + * struct nbl_dispatch_ops - dispatch control plane operation callbacks
> + * @init_module: dispatch layer initialization, control-PF exclusive,
> + *               caller must check has_ctrl guard
> + * @deinit_module: dispatch layer cleanup, control-PF exclusive,
> + *                 caller must check has_ctrl guard

[Severity: Low]
Do "dispatch layer initialization" and "dispatch layer cleanup" describe
these callbacks correctly? All dispatch-layer setup happens in
nbl_disp_init(), and nbl_disp_remove() is empty. The installed callbacks
only forward to the resource ops:

nbl_disp_init_module()
    res_ops->init_module(p)          /* nbl_res_chip_init_module() */
        hw_ops->init_module(p, eth_speed, eth_num)

nbl_disp_deinit_module()
    res_ops->deinit_module(p)        /* nbl_res_chip_deinit_module() */
        hw_ops->deinit_module(res_mgt->hw_ops_tbl->priv)

On leonis, these calls program the chip-wide datapath and set or clear
driver_status. Clearing driver_status starts asynchronous firmware
cleanup. nbl_dev.c notes that mailbox DMA must be drained before
deinit_module is called.

Would it be clearer to call these chip/firmware init and deinit, and to
mention the ordering requirement? That way later callers won't treat
deinit_module as software-only cleanup. The same wording is still there
at the end of the series.

> + *
> + * Warning: init_module/deinit_module are control-PF exclusive. The five
> + * resource ops (cfg_msix_map, destroy_msix_map, set_mailbox_irq,
> + * get_vsi_id, get_eth_id) are PF-only and resolve to either a local
> + * resource call (control PF) or a mailbox RPC (non-control PF with
> + * has_net). A function with neither has_ctrl nor has_net leaves these
> + * pointers NULL; callers must not invoke them on such functions. VFs are
> + * rejected by the responders with -EPERM.
> + */
> +struct nbl_dispatch_ops {
> +	int (*init_module)(struct nbl_dispatch_mgt *disp_mgt);
> +	void (*deinit_module)(struct nbl_dispatch_mgt *disp_mgt);
> +};

[Severity: Low]
Is this paragraph describing code that isn't in this patch yet? Here,
struct nbl_dispatch_ops has only init_module and deinit_module, and the
enum has only NBL_DISP_CTRL_LVL_MGT. nbl_disp_init() never checks
has_net, and nbl_disp_refresh_ctrl_ops() installs only the MGT pointers:

	if (test_bit(NBL_DISP_CTRL_LVL_MGT, disp_mgt->ctrl_lvl)) {
		disp_ops->init_module = nbl_disp_init_module;
		disp_ops->deinit_module = nbl_disp_deinit_module;
	}

The five ops, NBL_DISP_CTRL_LVL_NET, the has_net routing and the -EPERM
responders all come in the next patch, "net/nebula-matrix: dispatch:
implement channel RPC framework and serialize hardware ops". By the end
of the series, the doc matches the code.

Could this paragraph be added in that patch instead, so the documentation
in each commit matches its code?

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