Re: [PATCH v29 net-next 6/8] 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 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 kernel-doc block for struct nbl_dispatch_ops
  (drivers/net/ethernet/nebula-…
- [Low] All five new responders collapse resource-layer errors into a
  single -EREMOTEIO on the wire (`ret = res_ops->...(); if (ret) err =…
- [Low] The two "Precondition: caller must disable mailbox IRQ_RDY and
  switch send_msg to polling path ..." comments are attached to the…
- [Low] Reported concern: nbl_disp_chan_set_mailbox_irq_req() is the only
  newly added mutating RPC without the polling-path precondition…
- [Low] Reported concern: the five mailbox responders registered by
  nbl_disp_setup_msg() are never quiesced or unregistered by the…

Pre-existing issues:
- [Low] Reported concern: on the reconfiguration path cfg_msix_map
  rewrites the function's coherent MSI-X map table in place while…

--

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

This adds mailbox RPC request/response handlers for five dispatch
operations (configure_msix_map, destroy_msix_map, set_mailbox_irq,
get_vsi_id, get_eth_id), resolves disp_ops dynamically from the PF control
capability, and adds a per-dispatch mutex to serialize the mutating
hardware ops.

A few questions below, mostly about the documented contracts and about
error propagation on the wire.

On the commit message:

>     cfg_msix_map pre-allocates all coherent DMA buffers and global
>     vector indices before destroying the old configuration, then swaps
>     the new resources into the per-function entry to avoid interrupt
>     loss during reconfiguration.

[Severity: Low]
This isn't a bug introduced by this patch, but does the resource layer
actually match this description on the reconfiguration path?

In nbl_res_intr_cfg_msix_map() in nbl_hw/nbl_interrupt.c the existing
coherent map table is rewritten in place:

	msix_map_entries = official_tbl->base_addr;
	memset(msix_map_entries + requested, 0, ...);
	for (i = 0; i < requested; i++)
		msix_map_entries[i].data = ...;

while PCOMPLETER_FUNCTION_MSIX_MAP still has VALID=1 pointing at that
buffer, i.e. without the clear-VALID/quiesce sequence the teardown path
uses.

Each struct nbl_msix_map entry is a single __le16 so no torn entry is
possible, and the old global vectors have already had their
PCOMPLETER_HOST_MSIX_FID_TABLE / PADPT_HOST_MSIX_INFO entries
invalidated, so interrupts arriving in that window are suppressed rather
than misrouted. Also, no caller in this series reaches the
reconfiguration branch, since it requires
func_intr_res[func_id].interrupts != NULL and nbl_dev_start() destroys
the map on every failure path (err_destroy_map), while nbl_dev_stop()
destroys it on the normal path.

Would it be more accurate to drop or qualify the "avoid interrupt loss
during reconfiguration" claim?

> 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 966fee2dec8b0..d12ef86835283 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,176 @@

[ ... ]

> +	ret = chan_ops->send_msg(disp_mgt->chan_ops_tbl->priv, &chan_send);
> +	if (ret)
> +		return ret;
> +	if (chan_send.ack_len != sizeof(result)) {
> +		dev_err(disp_mgt->common->dev,
> +			"get_vsi_id: short ACK, ack_len=%u expected %zu\n",
> +			chan_send.ack_len, sizeof(result));
> +		return -EREMOTEIO;
> +	}

[ ... ]

> @@ -25,6 +192,347 @@ static int nbl_disp_init_module(struct nbl_dispatch_mgt *disp_mgt)
>  	return -EOPNOTSUPP;
>  }
>  
> +static int nbl_disp_cfg_msix_map(struct nbl_dispatch_mgt *disp_mgt,
> +				 u16 num_net_msix, u16 num_others_msix,
> +				 bool net_msix_mask_en)
> +{
[ ... ]
> +	mutex_lock(&disp_mgt->ops_mutex_lock);
> +	ret = res_ops->cfg_msix_map(p, common->mgt_pf, num_net_msix,
> +					  num_others_msix, net_msix_mask_en);
> +	mutex_unlock(&disp_mgt->ops_mutex_lock);
> +	return ret;
> +}

[ ... ]

> +/*
> + * Precondition: caller must disable mailbox IRQ_RDY and switch send_msg
> + * to polling path before issuing cfg_msix_map RPC.
> + * The responder will disable mailbox MSIX routing during resource ops,
> + * so ACK cannot rely on interrupt wakeup.
> + */
> +static void nbl_disp_chan_cfg_msix_map_resp(void *priv, u16 src_id, u16 msg_id,
> +					    void *data, u32 data_len)

[Severity: Low]
Is this comment attached to the right function? The precondition talks
about what the caller must do, but nbl_disp_chan_cfg_msix_map_resp() runs
on the manager PF out of the mailbox RX work item and has no such caller.

The same applies to the identical comment above
nbl_disp_chan_destroy_msix_map_resp().

The functions whose callers need to honour this are
nbl_disp_chan_cfg_msix_map_req() / nbl_disp_chan_destroy_msix_map_req(),
and the @cfg_msix_map / @destroy_msix_map kernel-doc entries in
nbl_def_dispatch.h that external callers read, none of which mention the
requirement.

A future caller reading only the ops documentation could issue the RPC
with NBL_CHAN_IRQ_RDY still set, so nbl_chan_send_msg() takes the
wait_event_timeout() branch while the responder is retargeting mailbox
MSI-X routing, and the call fails with a spurious timeout. Today
nbl_dev_start() / nbl_dev_stop() do satisfy the precondition, so nothing
misbehaves. Could the comment move to the req helpers and the ops
kernel-doc instead?

> +{
[ ... ]
> +	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 = -EREMOTEIO;

[Severity: Low]
Is the errno substitution here intentional? All five new responders
collapse every resource-layer failure into -EREMOTEIO, while the local
control-PF implementations of the same disp_ops entries return the
resource errno verbatim:

nbl_disp_cfg_msix_map()
	ret = res_ops->cfg_msix_map(p, common->mgt_pf, ...);
	return ret;

nbl_disp_chan_cfg_msix_map_resp()
	ret = res_ops->cfg_msix_map(p, src_id, ...);
	if (ret)
		err = -EREMOTEIO;

nbl_res_intr_cfg_msix_map() in nbl_hw/nbl_interrupt.c distinguishes the
cases:

		dev_err(dev, "No free control interrupt vectors left\n");
		ret = -EAGAIN;

and also produces -ESHUTDOWN while intr_mgt->stopping, -EBUSY, -EINVAL
and -ENOMEM, all of which are lost on the wire.

-EREMOTEIO is also what nbl_disp_chan_get_vsi_id_req() and
nbl_disp_chan_get_eth_id_req() return for a truncated ACK, so a caller
cannot tell a protocol error from a remote operation failure.

The commit message says:

>     Remote responders return standard Linux errnos directly on the wire.

which reads as pass-through. Could the responders forward ret instead?

> +	} else {
> +		err = -EOPNOTSUPP;
> +	}

[ ... ]

> +static int nbl_disp_chan_set_mailbox_irq_req(struct nbl_dispatch_mgt *disp_mgt,
> +					     u16 vector_id, bool en_msix)
> +{
[ ... ]
> +	nbl_chan_fill_send_info(&chan_send, common->mgt_pf,
> +				NBL_CHAN_MSG_MAILBOX_SET_IRQ,
> +				&param, sizeof(param), NULL, 0, 1);
> +	ret = chan_ops->send_msg(disp_mgt->chan_ops_tbl->priv, &chan_send);

[Severity: Low]
This is the only newly added mutating RPC without the polling-path
precondition documented for cfg_msix_map / destroy_msix_map, even though
the responder rewrites the requester's own mailbox MSI-X routing
(NBL_MAILBOX_QINFO_MAP MSIX_IDX / MSIX_IDX_VALID) before the ACK is sent.

Looking at the callers in nbl_core/nbl_dev.c, both directions do use the
polling path, so the ACK does arrive:

nbl_dev_enable_mailbox_irq()
	ret = disp_ops->set_mailbox_irq(..., lvec, true);
	if (ret)
		return ret;
	chan_ops->set_queue_state(..., NBL_CHAN_IRQ_RDY, ..., true);

nbl_dev_disable_mailbox_irq()
	chan_ops->set_queue_state(..., NBL_CHAN_IRQ_RDY, ..., false);
	return disp_ops->set_mailbox_irq(..., lvec, false);

and nbl_chan_send_msg() selects the wait path solely on
test_bit(NBL_CHAN_IRQ_RDY, chan_info->state), which is clear in both
cases.

Would it still be worth documenting the same precondition on this req
helper, so the requirement is visible to future callers?

[ ... ]

> @@ -34,9 +542,22 @@ static void nbl_disp_refresh_ctrl_ops(struct nbl_dispatch_mgt *disp_mgt)
>  {
>  	struct nbl_dispatch_ops *disp_ops = disp_mgt->disp_ops_tbl->ops;
>  
> +	memset(disp_ops, 0, sizeof(*disp_ops));
>  	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;
> +		disp_ops->cfg_msix_map = nbl_disp_cfg_msix_map;
> +		disp_ops->destroy_msix_map = nbl_disp_destroy_msix_map;
> +		disp_ops->set_mailbox_irq = nbl_disp_set_mailbox_irq;
> +		disp_ops->get_vsi_id = nbl_disp_get_vsi_id;
> +		disp_ops->get_eth_id = nbl_disp_get_eth_id;
> +	} else if (test_bit(NBL_DISP_CTRL_LVL_NET, disp_mgt->ctrl_lvl)) {
> +		disp_ops->cfg_msix_map =
> +			nbl_disp_chan_cfg_msix_map_req;
> +		disp_ops->destroy_msix_map = nbl_disp_chan_destroy_msix_map_req;
> +		disp_ops->set_mailbox_irq = nbl_disp_chan_set_mailbox_irq_req;
> +		disp_ops->get_vsi_id = nbl_disp_chan_get_vsi_id_req;
> +		disp_ops->get_eth_id = nbl_disp_chan_get_eth_id_req;
>  	}
>  }

[ ... ]

> @@ -104,14 +629,25 @@ 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;
> +
[ ... ]
>  void nbl_disp_remove(struct nbl_adapter *adapter)
>  {
> -	/* Dispatch structures are allocated via devm */
> +	/*
> +	 * Dispatch structures are allocated via devm.
> +	 * Message handlers registered by nbl_disp_setup_msg() are owned by
> +	 * channel layer; they are unregistered in nbl_chan_remove_common(),
> +	 * not here.
> +	 */
>  }

[Severity: Low]
Since the five responders registered here are never quiesced by the
dispatch layer, is the ordering on teardown guaranteed, i.e. can
nbl_chan_remove_msg_handler() free a handler node while the RX dispatch
path still holds it, or can a responder run after nbl_intr_mgt_stop() has
cleared res_mgt->intr_mgt?

Tracing the remove path, it does look safe. nbl_core_remove() reaches
nbl_dev_remove_common_dev() first:

	cancel_work_sync(&common_dev->clean_mbx_task);
	ret = nbl_dev_remove_chan_queue(dev_mgt, NBL_CHAN_TYPE_MAILBOX);

which ends in nbl_chan_teardown_queue(), stopping the hardware mailbox
queue and joining the clean task:

	if (task)
		cancel_work_sync(task);
	WRITE_ONCE(chan_info->active, false);

and only afterwards does nbl_chan_remove_common() drain the workqueue
before freeing the nodes:

	nbl_common_destroy_wq(&adap->common);
	...
	nbl_chan_remove_msg_handler(chan_mgt);

Would it be worth spelling out that ordering dependency in the comment,
since it is what makes the empty nbl_disp_remove() correct?

> 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 b0daabb05d394..61083a750da47 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
> @@ -21,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.
>   */

[Severity: Low]
This Warning now also covers the five ops being documented here, but does
it still hold for them?

On the VF part, every new responder rejects a src_id whose relative PF id
is at or above common->max_pf:

	if (rel_pf_id >= disp_mgt->common->max_pf) {
		err = -EPERM;
		goto ack_out;
	}

With mgt_pf as the base, rel_pf_id equals src_id for the usual case and
max_pf is 1, 2 or 4 from nbl_res_init_pf_num() while NBL_MAX_FUNC is 520,
so the entire VF function-id range gets -EPERM. The commit message agrees:

>     - src_id against common->max_pf at runtime; rejects VF and
>       out-of-range function IDs with -EPERM;

On the NULL part, nbl_disp_refresh_ctrl_ops() now starts with
memset(disp_ops, 0, sizeof(*disp_ops)) and fills the table only under MGT
or NET, so a function with neither capability bit leaves all five
pointers NULL, while callers such as nbl_dev_cfg_msix_map() and
nbl_dev_enable_mailbox_irq() in nbl_dev.c dereference
disp_ops->cfg_msix_map and disp_ops->set_mailbox_irq without a NULL test.
That combination is not reachable today, since every entry in
nbl_id_table[] sets BIT(NBL_CAP_HAS_NET_BIT) and no VF is probed.

Could the Warning be narrowed to say the five new ops are PF-only and
depend on the has_ctrl/has_net capability?

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