Re: [PATCH v28 net-next 07/10] net/nebula-matrix: dispatch: add control-level routing core infrastructure

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] New channel response enum (NBL_CHAN_RESP_OK/ERR/UNIMPLEMENTED,
  extended with PERM_DENY later in the series) uses positive wire…
- [Low] The comment in the (empty) nbl_disp_remove() bases dispatch
  teardown safety on facts that do not exist at this commit: it says…

--

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

This adds the dispatch layer skeleton: allocation of the dispatch
management structure and ops table, init_module/deinit_module wrappers
around the resource ops, and a ctrl_lvl bitmask that enables the MGT
level on the Control PF only.  It also adds forward declarations of
channel wire structures and a channel response enum for a later RPC
framework.

> 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 61dd97c779ef6..58a9fa97fbf39 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
> @@ -7,12 +7,19 @@
>  #define _NBL_DEF_CHANNEL_H_
>  
>  #include <linux/types.h>
> +#include <linux/build_bug.h>
>  
>  struct nbl_channel_mgt;
>  struct nbl_adapter;
>  
>  typedef void (*nbl_chan_resp)(void *, u16, u16, void *, u32);
>  
> +enum {
> +	NBL_CHAN_RESP_OK = 0,
> +	NBL_CHAN_RESP_ERR = 1,
> +	NBL_CHAN_RESP_UNIMPLEMENTED = 2,
> +};
> +

[Severity: Medium]
Can these positive response codes ever be observed by a sender?  The ACK
receive path in nbl_chan_recv_ack_msg() rewrites any positive remote
status:

drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c:
	raw_err = (s32)le32_to_cpu(*(__le32 *)&payload[NBL_CHAN_ACK_RET_POS]);
	if (raw_err > 0 || raw_err < -MAX_ERRNO)
		raw_err = -EREMOTEIO;

	WRITE_ONCE(wait_head->ack_err, raw_err);

nbl_chan_send_msg() returns exactly that clamped value, so a caller can
only ever see 0 or a negative errno, never NBL_CHAN_RESP_ERR (1) or
NBL_CHAN_RESP_UNIMPLEMENTED (2).

The follow-up patch in the series ("net/nebula-matrix: dispatch:
implement channel RPC framework and serialize hardware ops") both puts
these codes on the wire via nbl_disp_chan_*_resp() ->
nbl_chan_fill_ack_info(..., err, ...) and then switches on them after
send_msg(), for example in nbl_disp_chan_get_eth_id_req():

	ret = chan_ops->send_msg(disp_mgt->chan_ops_tbl->priv, &chan_send);
	if (ret < 0)
		return ret;
	switch (ret) {
	case NBL_CHAN_RESP_OK:
		break;
	case NBL_CHAN_RESP_UNIMPLEMENTED:
		return -EOPNOTSUPP;
	case NBL_CHAN_RESP_ERR:
		return -EREMOTEIO;
	case NBL_CHAN_RESP_PERM_DENY:
		return -EPERM;

Are all the non-OK arms dead code, given "if (ret < 0) return ret;"
fires first for the clamped -EREMOTEIO?  A peer answering "unimplemented"
or "permission denied" would then be reported to the requester as
-EREMOTEIO instead of -EOPNOTSUPP / -EPERM.

Would it make sense to either encode these response codes as
non-positive values (or in a dedicated payload field), or to let the
clamp in nbl_chan_recv_ack_msg() pass the defined response codes
through?

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dispatch.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dispatch.c
> new file mode 100644
> index 0000000000000..b7f404b319fac
> --- /dev/null
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dispatch.c
> @@ -0,0 +1,124 @@

[ ... ]

> +void nbl_disp_remove(struct nbl_adapter *adapter)
> +{
> +	/*
> +	 * All dispatch objects are allocated via devm.
> +	 * Dispatch message handlers are not explicitly unregistered here.
> +	 * Safety relies on dev layer shutting down mailbox channel queue
> +	 * before nbl_res_remove_leonis(), so responders cannot run after
> +	 * resource layer is torn down. Channel layer nbl_chan_remove_common()
> +	 * finalizes cleanup at device teardown.
> +	 */
> +}

[Severity: Low]
This isn't a bug, but does this comment describe code that exists at this
commit?

There is no dev layer yet (no nbl_core/nbl_dev.o in the Makefile and no
nbl_dev_init()/nbl_dev_remove() call in nbl_main.c), so nothing shuts
down the mailbox channel queue before nbl_res_remove_leonis().

This patch also registers no channel handlers at all (no
chan_ops->register_msg() call in nbl_dispatch.c), so there are no
responders that the "not explicitly unregistered here" sentence could
refer to.

Both premises do hold at the end of the series, where nbl_core_remove()
calls nbl_dev_remove() first and nbl_dev_remove_common_dev() tears down
the mailbox queue before the resource layer, and nbl_disp_setup_msg()
registers the responders.  Would it be clearer to move this comment to
the patch that actually registers the handlers?

Related to the same staging question: nbl_disp_refresh_ctrl_ops()
installs init_module/deinit_module

> +	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;
> +	}

but is there any caller of these ops at this commit?  The first callers,
nbl_dev_setup_ctrl_dev() and nbl_dev_remove_ctrl_dev(), arrive with the
later dev-layer patch, so the "dispatch management flow" the commit
message mentions appears inert here.

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> index 1aafed2d46d71..5d5c0bbf418c8 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> @@ -59,6 +66,7 @@ struct nbl_adapter *nbl_core_init(struct pci_dev *pdev,
>  
>  void nbl_core_remove(struct nbl_adapter *adapter)
>  {
> +	nbl_disp_remove(adapter);
>  	nbl_res_remove_leonis(adapter);
>  	nbl_chan_remove_common(adapter);
>  	nbl_hw_remove_leonis(adapter);

Here the channel layer is removed after the resource layer, which is the
opposite order from the one the nbl_disp_remove() comment relies on.

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