Re: [PATCH v31 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 says this patch will "Add wire ABI structures
  with static_assert for upcoming mailbox RPC", but the patch adds…
- [Low] At this commit, the kernel-doc on struct nbl_dispatch_ops in
  nbl_def_dispatch.h (lines 18-43) describes things that are not in the…

--

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

This commit allocates the dispatch management state and ops table, adds a
ctrl_lvl bitmap to track control privileges, and wires init_module and
deinit_module wrappers to the resource ops. The MGT level is enabled only
on the control PF.

> 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]
This patch doesn't seem to add any wire ABI structures or static_assert
checks. The diff only touches the Makefile, nbl_core.h, nbl_dispatch.c,
nbl_dispatch.h, nbl_def_dispatch.h and nbl_main.c. None of those files adds
a mailbox wire structure or a static_assert.

At this commit, enum nbl_chan_msg_type in nbl_def_channel.h still contains
only:

enum nbl_chan_msg_type {
	NBL_CHAN_MSG_ACK = 0,
	/* mailbox msg end */
	NBL_CHAN_MSG_MAILBOX_MAX,
};

The only static_assert under drivers/net/ethernet/nebula-matrix/ is the
existing one on struct nbl_board_port_info, which an earlier patch added.

The param structures and their asserts (nbl_chan_param_cfg_msix_map,
nbl_chan_param_set_mailbox_irq, nbl_chan_param_get_vsi_id and
nbl_chan_param_get_eth_id) first appear in the next patch, "dispatch:
implement channel RPC framework and serialize hardware ops".

Could this sentence be removed from this commit message, or moved to the
patch that actually adds the structures?

> 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 0000000000000..08fef0f679268
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_def_dispatch.h
> @@ -0,0 +1,56 @@

[ ... ]

> +enum {
> +	NBL_DISP_CTRL_LVL_MGT,
> +	NBL_DISP_CTRL_LVL_MAX,
> +};
> +
> +/**
> + * struct nbl_dispatch_ops - dispatch control plane operation callbacks

[ ... ]

> + * @deinit_module: chip-wide teardown, forwarded the same way; on leonis
> + *               it clears driver_status, which starts an asynchronous
> + *               firmware per-PF cleanup pass with no completion status
> + *               bit, waited for best-effort only.  The caller must have
> + *               stopped the mailbox channel and drained all in-flight
> + *               mailbox DMA before calling it, see nbl_dev_remove().
> + *               Returns void.  Control-PF exclusive, same NULL rule as
> + *               @init_module.
> + *
> + * 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]
This isn't a bug, but at this commit the kernel-doc for struct
nbl_dispatch_ops describes code that isn't in the tree yet.

It refers callers to nbl_dev_remove(). That function doesn't exist yet,
because nbl_dev.c hasn't been added at this point.

It also describes five PF-only ops (cfg_msix_map, destroy_msix_map,
set_mailbox_irq, get_vsi_id, get_eth_id), has_net routing over mailbox
RPC, and -EPERM responders. The struct itself has only init_module and
deinit_module, and the enum has only NBL_DISP_CTRL_LVL_MGT and
NBL_DISP_CTRL_LVL_MAX. nbl_disp_refresh_ctrl_ops() also installs only the
two module ops, and only under the MGT level.

Later patches in the series fill this in. The next patch, "dispatch:
implement channel RPC framework and serialize hardware ops", adds
NBL_DISP_CTRL_LVL_NET, the five ops and the responders. The patch after
that, "add common/ctrl dev init/remove operation", adds nbl_dev_remove()
in nbl_dev.c. At the end of the series the comment matches the code; it
only runs ahead of the code at the intermediate commits.

Would it make sense to split this comment so each part lands with the
code it documents?

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