Re: [v26,net-next,08/10] net/nebula-matrix: dispatch: implement channel RPC framework and serialize hardware ops

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 4 · Low: 4

- [Medium] All five new *_req() helpers translate the return value of
  chan_ops->send_msg() with a switch that only enumerates the four wire…
- [Medium] nbl_disp_setup_msg()'s rollback calls
  chan_ops->unregister_all_msg(), which is…
- [Medium] nbl_disp_chan_get_vsi_id_req() and
  nbl_disp_chan_get_eth_id_req() consume the ACK payload without…
- [Medium] The newly exposed destroy_msix_map op (local
  nbl_disp_destroy_msix_map(), remote…
- [Low] The commit message describes work the diff does not contain.
- [Low] nbl_disp_init() sets NBL_DISP_CTRL_LVL_NET whenever
  common->has_net, with a comment claiming this enables the 'net control…
- [Low] The kernel-doc block that this patch extends states 'Warning: All
  ops except init_module/deinit_module can be safely called on PF/VF',…
- [Low] The newly exposed NBL_CHAN_MSG_CONFIGURE_MSIX_MAP handler
  forwards peer-supplied vector counts straight to the global allocator…

--

net/nebula-matrix: dispatch: implement channel RPC framework and serialize
hardware ops

This adds mailbox RPC request/response handlers for configure_msix_map,
destroy_msix_map, set_mailbox_irq, get_vsi_id and get_eth_id, resolving the
dispatch ops to either local hardware calls (control PF) or remote requests
to the manager PF. It also adds a per-dispatch mutex to serialize the
mutable hardware ops, plus new wire message types and a permission-denied
status code.

A few questions below about the commit message and about the new handlers.

The commit message says:

    Improve message payload validation for forward compatibility:
    reject requests with insufficient payload length, enforce bounds
    checks before parsing incoming RPC parameters. Fix unimplemented
    operation check ordering to avoid potential NULL pointer
    dereferences.

and:

    Add helper to register channel response callbacks; extend wire
    protocol with new message types and NBL_CHAN_RESP_PERM_DENY error
    code.

[Severity: Low]
Which pre-existing code do the words "Improve", "Fix ... ordering" and
"Add helper" refer to?

At the parent commit the only helpers in nbl_dispatch.c are
nbl_disp_init_module() and nbl_disp_deinit_module(), and both already had
the guard:

	if (res_ops->init_module)
		return res_ops->init_module(p);
	return -EOPNOTSUPP;

Neither is touched by this patch, so no check ordering appears to change.
Likewise, every handler containing a data_len < sizeof(param) test is
introduced by this patch, so there is no earlier validation being improved.

register_msg() and unregister_all_msg() also already exist in
struct nbl_channel_ops, and nbl_chan_setup_ops() already registers
NBL_CHAN_MSG_ACK; what this patch adds is a dispatch-layer setup function,
nbl_disp_setup_msg(), not a channel helper.

Could the wording be changed to describe the new code as new, so readers do
not look for a Fixes: tag and a previously broken commit?

> 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
> index 3da5f8351fa4..239d8317a9c5 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dispatch.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dispatch.c
> @@ -3,9 +3,170 @@
>   * Copyright (c) 2026 Nebula Matrix Limited.
>   */
>  #include <linux/device.h>
> +#include <linux/mutex.h>
>  #include <linux/pci.h>
>  #include "nbl_dispatch.h"
>  
> +static int nbl_disp_chan_get_vsi_id_req(struct nbl_dispatch_mgt *disp_mgt,
> +					u16 type, u16 *vsi_id)
> +{
> +	struct nbl_channel_ops *chan_ops = disp_mgt->chan_ops_tbl->ops;
> +	struct nbl_common_info *common = disp_mgt->common;
> +	struct nbl_chan_param_get_vsi_id result = { 0 };
> +	struct nbl_chan_param_get_vsi_id param = { 0 };
> +	struct nbl_chan_send_info chan_send = {0};
> +	int ret;
> +
> +	param.type = cpu_to_le16(type);
> +
> +	nbl_chan_fill_send_info(&chan_send, common->mgt_pf,
> +				NBL_CHAN_MSG_GET_VSI_ID,
> +				&param, sizeof(param), &result,
> +				sizeof(result), 1);
> +	ret = chan_ops->send_msg(disp_mgt->chan_ops_tbl->priv, &chan_send);
> +	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;
> +	default:
> +		return -EREMOTEIO;
> +	}

[Severity: Medium]
Does this switch turn every local transport error into -EREMOTEIO?

nbl_chan_send_msg() only returns a wire status code when an ACK actually
arrives.  Otherwise it returns a negative Linux errno:

nbl_channel/nbl_channel.c:nbl_chan_send_msg() {
	...
	if (READ_ONCE(chan_info->shutdn)) { ... return -ESHUTDOWN; }
	...
	ret = -ETIMEDOUT;
	...
	ret = READ_ONCE(wait_head->ack_err);
}

plus -EINVAL for oversized arg_len/resp_len, -EAGAIN when all TX wait slots
are busy, and -EIO when the mailbox IRQ is disabled mid-wait.

All of those land in the default arm, so a caller cannot tell a retryable
or teardown condition from a genuine remote hardware failure, and
nbl_dev_stop() ends up logging a remote I/O error for what was really
-ESHUTDOWN or -ETIMEDOUT.  The same switch is repeated in
nbl_disp_chan_get_eth_id_req(), nbl_disp_chan_cfg_msix_map_req(),
nbl_disp_chan_destroy_msix_map_req() and
nbl_disp_chan_set_mailbox_irq_req().

Would an early "if (ret < 0) return ret;" before the switch preserve the
error semantics the changelog aims for?

> +	*vsi_id = le16_to_cpu(result.vsi_id);
> +	return 0;
> +}

[ ... ]

> +	ret = chan_ops->send_msg(disp_mgt->chan_ops_tbl->priv, &chan_send);
> +	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;
> +	default:
> +		return -EREMOTEIO;
> +	}
> +	*eth_num = result.eth_num;
> +	*eth_id = result.eth_id;
> +	*logic_eth_id = result.logic_eth_id;
> +
> +	return 0;
> +}

[Severity: Medium]
Can a short ACK make these two helpers report success with zeroed
identifiers?

nbl_chan_recv_ack_msg() takes the status straight from the wire and only
clamps the payload copy, reporting the effective length back through
chan_send->ack_len:

nbl_channel/nbl_channel.c:nbl_chan_recv_ack_msg() {
	...
	copy_len = min_t(u32, READ_ONCE(wait_head->ack_data_len), ack_datalen);
	if (READ_ONCE(wait_head->ack_err) >= 0 && copy_len > 0) {
		...
		WRITE_ONCE(wait_head->ack_data_len, (u16)copy_len);
	}
}

nbl_disp_chan_get_vsi_id_req() and nbl_disp_chan_get_eth_id_req() never look
at chan_send.ack_len, so a peer answering status OK with a truncated or
empty payload leaves the "= { 0 }" initialiser values in place and the
caller gets vsi_id 0, eth_id 0, logic_eth_id 0, eth_num 0 and a 0 return.
Per nbl_res_ctrl_dev_vsi_info_init() ("pf0,pf1,pf2,pf3 vsi is 0,256,512,768")
vsi_id 0 and eth_id 0 are valid identifiers belonging to PF0, so a
non-control PF would silently adopt another function's VSI/port identity.

There is also no range check on the returned values, unlike the local
producer:

nbl_hw/nbl_resource.c:nbl_res_get_eth_id() {
	...
	if (rel_pf_id >= eth_info->eth_num) {
		dev_err(dev, "rel_pf_id %d out of range [0, %u)\n", ...);
		return -ERANGE;
	}
}

Should these helpers check chan_send.ack_len against sizeof(result) and
bound eth_num/eth_id/logic_eth_id against NBL_MAX_ETHERNET before handing
the values to the caller?

> @@ -25,6 +186,346 @@ static int nbl_disp_init_module(struct nbl_dispatch_mgt *disp_mgt)
>  	return -EOPNOTSUPP;
>  }
>  

[ ... ]

> +	if (res_ops->cfg_msix_map) {
> +		mutex_lock(&disp_mgt->ops_mutex_lock);
> +		ret = res_ops->cfg_msix_map(p, src_id,
> +					    le16_to_cpu(param.num_net_msix),
> +					    le16_to_cpu(param.num_others_msix),
> +					    !!le16_to_cpu(param.msix_mask_en));
> +		mutex_unlock(&disp_mgt->ops_mutex_lock);
> +		if (ret)
> +			err = NBL_CHAN_RESP_ERR;
> +	} else {
> +		err = NBL_CHAN_RESP_UNIMPLEMENTED;
> +	}

[Severity: Low]
Should there be a per-function limit on the requested vector counts here?

After the coarse src_id check the peer-supplied counts go straight to the
global allocator, which only enforces device-wide maxima:

nbl_hw/nbl_interrupt.c:nbl_res_intr_cfg_msix_map() {
	...
	if (num_net_msix > NBL_MSIX_MAP_TABLE_MAX_ENTRIES ||
	    num_others_msix > NBL_MSIX_MAP_TABLE_MAX_ENTRIES) { ... }
}

The accepted maximum for num_others_msix equals the whole shared pool
(NBL_MAX_OTHER_INTERRUPT is 1024), so a single request for 1024 drains
intr_other_bmap and every later allocation for any function, including the
control PF's own mailbox and misc vectors, then fails with -EAGAIN.

Each request also does a full 1024-entry coherent DMA table allocation and
the usleep_range(100, 120) quiesce inside nbl_res_intr_destroy_msix_map()
while ops_mutex_lock is held, which stalls the control PF's mailbox work
item.

> +	if (src_id > NBL_MAX_PF_SRC_ID) {
> +		err = NBL_CHAN_RESP_PERM_DENY;
> +		goto ack_out;
> +	}
> +	if (res_ops->destroy_msix_map) {
> +		mutex_lock(&disp_mgt->ops_mutex_lock);
> +		ret = res_ops->destroy_msix_map(p, src_id);
> +		mutex_unlock(&disp_mgt->ops_mutex_lock);
> +		if (ret)
> +			err = NBL_CHAN_RESP_ERR;
> +	} else {
> +		err = NBL_CHAN_RESP_UNIMPLEMENTED;
> +	}

[Severity: Medium]
Does making destroy_msix_map remotely invokable leave the requester's
mailbox interrupt routing armed on a recycled vector?

nbl_res_intr_destroy_msix_map() masks the per-vector entries, returns the
global vector numbers to the bitmaps and frees the per-function array:

nbl_hw/nbl_interrupt.c:nbl_res_intr_destroy_msix_map() {
	...
	nbl_intr_release_bitmap(res_mgt, interrupts, intr_num);
	...
	kfree(intr_mgt->func_intr_res[func_id].interrupts);
	intr_mgt->func_intr_res[func_id].interrupts = NULL;
}

but MSIX_IDX/MSIX_IDX_VALID in NBL_MAILBOX_QINFO_MAP_REG_ARR(func_id),
written only by nbl_hw_set_mailbox_irq(), is left pointing at the vector
that was just released.  It cannot be cleared afterwards either:

nbl_hw/nbl_interrupt.c:nbl_res_intr_set_mailbox_irq() {
	if (!intr_mgt->func_intr_res[func_id].interrupts) {
		dev_err(dev, "func %u MSIX map not configured\n", func_id);
		return -ENODEV;
	}
}

A later nbl_res_intr_cfg_msix_map() for another function re-validates the
same global vector with the new owner's BDF, so a mailbox event for the old
function would raise an interrupt owned by a different PF.
nbl_res_intr_cfg_msix_map() also calls the same teardown internally, so a
peer sending CONFIGURE_MSIX_MAP twice, or DESTROY_MSIX_MAP while its
mailbox IRQ is still armed, reaches this from the wire.

Would it be better to clear the mailbox QINFO mapping as part of
destroy_msix_map, or at least document the required ordering against
set_mailbox_irq in nbl_def_dispatch.h?

> +static int nbl_disp_setup_msg(struct nbl_dispatch_mgt *disp_mgt)
> +{
> +	struct nbl_channel_ops *chan_ops = disp_mgt->chan_ops_tbl->ops;
> +	struct nbl_channel_mgt *p = disp_mgt->chan_ops_tbl->priv;
> +	int ret = 0;
> +	int _ret;
> +
> +	_ret = chan_ops->register_msg(p, NBL_CHAN_MSG_CONFIGURE_MSIX_MAP,
> +				      nbl_disp_chan_cfg_msix_map_resp,
> +				      disp_mgt);
> +	if (_ret < 0 && !ret)
> +		ret = _ret;

[ ... ]

> +	if (ret)
> +		chan_ops->unregister_all_msg(p);
> +	return ret;
> +}

[Severity: Medium]
Does this rollback destroy state owned by the channel layer?

unregister_all_msg is nbl_chan_remove_msg_handler():

nbl_channel/nbl_channel.c:nbl_chan_remove_msg_handler() {
	if (!chan_mgt->handle_hash_tbl)
		return;
	nbl_common_remove_hash_table(chan_mgt->handle_hash_tbl);
	chan_mgt->handle_hash_tbl = NULL;
}

That frees every handler node, including the channel's own ACK handler
registered by nbl_chan_setup_ops():

	ret = nbl_chan_register_msg(chan_mgt, NBL_CHAN_MSG_ACK,
				    nbl_chan_recv_ack_msg, chan_mgt);

and then NULLs the shared table pointer with no re-init path, since
nbl_chan_init_msg_handler() is only called from nbl_chan_setup_chan_mgt().
Neither accessor tolerates a NULL table:

nbl_common/nbl_common.c:nbl_common_get_hash_node() {
	...
	key_size = tbl_mgt->tbl_key.key_size;
}

Registration is also unpaired at this layer: nbl_disp_remove() is empty and
delegates handler teardown to nbl_chan_remove_common().  As far as I can
tell no crash is reachable today, because the only failure mode of
nbl_chan_register_msg() is -ENOMEM during nbl_disp_init(), and at that point
no mailbox queue, IRQ or clean_mbx_task exists yet, so nothing looks the
table up during the unwind.  Would it be more robust to unregister only the
five message types this function registered, and to do it from
nbl_disp_remove()?

> @@ -104,14 +621,30 @@ int nbl_disp_init(struct nbl_adapter *adapter)
>  	adapter->core.disp_mgt = disp_mgt;
>  	adapter->intf.dispatch_ops_tbl = disp_ops_tbl;
>  
> +	ret = nbl_disp_setup_msg(disp_mgt);
> +	if (ret)
> +		return ret;
> +
>  	if (common->has_ctrl)
>  		nbl_disp_set_ctrl_bit(disp_mgt, NBL_DISP_CTRL_LVL_MGT);
>  
> +	/*
> +	 * For non-control PF with network capability, enable net control
> +	 * level.
> +	 * All dispatch ops declared with NBL_DISP_CTRL_LVL_MGT fall back
> +	 * to remote mailbox msg_req handlers when MGT bit is not set.
> +	 */
> +	if (common->has_net)
> +		nbl_disp_set_ctrl_bit(disp_mgt, NBL_DISP_CTRL_LVL_NET);

[Severity: Low]
Is anything reading NBL_DISP_CTRL_LVL_NET?

Grepping the nbl driver at this commit, the symbol appears only in its enum
definition in nbl_def_dispatch.h and in this new set_bit().  The single
consumer of ctrl_lvl only tests the MGT bit:

nbl_core/nbl_dispatch.c:nbl_disp_refresh_ctrl_ops() {
	if (test_bit(NBL_DISP_CTRL_LVL_MGT, disp_mgt->ctrl_lvl)) {
	...
}

so the NET bit is write-only state.

The comment also says "For non-control PF with network capability", but the
condition is just common->has_net, and every entry in nbl_id_table sets
BIT(NBL_CAP_HAS_NET_BIT), so the control PF takes this branch too.  Should
the condition and the comment be reconciled, or the bit dropped until a
reader exists?

>  	nbl_disp_refresh_ctrl_ops(disp_mgt);
>  	return 0;
>  }

[ ... ]

> 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
> index d5e7da0fef6e..61083a750da4 100644
> --- 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
> @@ -22,12 +22,28 @@ enum {
>   *               caller must check has_ctrl guard
>   * @deinit_module: dispatch layer cleanup, ONLY valid on Control PF,
>   *                 caller must check has_ctrl guard
> + * @cfg_msix_map: configure function msix mapping table
> + * @destroy_msix_map: tear down msix mapping resource
> + * @set_mailbox_irq: bind mailbox interrupt to specified msix vector
> + * @get_vsi_id: resolve VSI ID by type
> + * @get_eth_id: resolve eth port info from VSI ID
> + *
>   * Warning: All ops except init_module/deinit_module can be safely called
>   * on PF/VF; init/deinit hooks are control-PF exclusive to prevent NULL ptr.
>   */

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_include.h b/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_include.h
> index ebf85702cef6..a53138530c54 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_include.h
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_include.h
> @@ -15,6 +15,8 @@
>  
>  #define NBL_MAX_FUNC					520
>  #define NBL_MAX_ETHERNET				4
> +/* Product firmware only supports 1/2/4 contiguous PFs (ID 0~3) */
> +#define NBL_MAX_PF_SRC_ID				3

[Severity: Low]
Does this constant contradict the documented contract just above?

The kernel-doc extended by this patch says the five new ops "can be safely
called on PF/VF", but on a non-control function they resolve to the *_req()
helpers, and every new responder starts with:

	if (src_id > NBL_MAX_PF_SRC_ID) {
		err = NBL_CHAN_RESP_PERM_DENY;
		goto ack_out;
	}

src_id is the mailbox descriptor srcid in a function-id space of
NBL_MAX_FUNC (520), so any VF requester is denied by construction and its
*_req() helper returns -EPERM.

The hardcoded 3 also disagrees with the runtime model used elsewhere:
nbl_common_func_id_to_rel_pf_id() computes pf_id - common->mgt_pf and is
documented to "Support future non-zero management PF", while
nbl_res_func_id_to_bdf() and nbl_res_get_eth_id() validate against
resource_info->max_pf.  With mgt_pf != 0 the legitimate absolute PF ids
exceed 3 and would be denied as well.

No VF can bind this driver today, since nbl_id_table lists only PF device
ids and mgt_pf is hardcoded to 0 in nbl_core_init(), so this is a
documentation and future-proofing question rather than an active failure.
Would validating src_id against mgt_pf plus max_pf be preferable to the new
constant, or should the "PF/VF" wording be adjusted?

>  
>  enum {
>  	NBL_VSI_DATA = 0,

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