Re: [PATCH v33 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 5 potential
issue(s) to consider.

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch makes it possible for a sibling PF to trigger
  runtime teardown of its own MSI-X map on the control PF, and that…
- [Low] The subject line says "dispatch: implement channel RPC framework
  and serialize hardware ops", but the patch does neither of those…
- [Low] The struct nbl_dispatch_ops kernel-doc in nbl_def_dispatch.h
  justifies the empty nbl_disp_remove() like this: "the teardown order…
- [Low] nbl_disp_chan_cfg_msix_map_req(),
  nbl_disp_chan_destroy_msix_map_req() and…
- [Low] The commit message says cfg_msix_map, destroy_msix_map and
  set_mailbox_irq "carry a caller precondition": clear mailbox IRQ_RDY…

--

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

This patch adds mailbox RPC request and response handlers for five
dispatch ops: cfg_msix_map, destroy_msix_map, set_mailbox_irq, get_vsi_id
and get_eth_id. Based on has_ctrl/has_net, each op becomes either a local
resource call or a channel RPC to the management PF. The five responders
are registered with the channel layer.

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

[Severity: Low]
This isn't a bug, but does the subject line match what the patch does?

The patch adds no lock, mutex or other serialization. The body says "No
dispatch-layer lock is taken around these ops", and the comment in
nbl_disp_cfg_msix_map() says the same thing. The only serialization is
intr_mgt->lock, which an earlier patch, "net/nebula-matrix: add intr
resource implementation", added to the resource layer.

The channel RPC framework (send_msg, send_ack, register_msg and the ACK
errno plumbing) was also added earlier, in "net/nebula-matrix: add channel
layer". This patch adds five request/response handlers on top of it.

Could the subject describe that instead, for example "dispatch: add
mailbox RPCs for msix map, mailbox irq and vsi/eth id"?

> 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..cb68544e02723 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

[ ... ]

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

[ ... ]

> +/*
> + * Precondition: caller must disable mailbox IRQ_RDY and switch send_msg
> + * to the polling path before issuing this RPC. The responder rewrites
> + * the requester's mailbox MSI-X routing during the resource op, so the
> + * ACK cannot rely on interrupt wakeup while routing is in flux.
> + */
> +static int
> +nbl_disp_chan_cfg_msix_map_req(struct nbl_dispatch_mgt *disp_mgt,
> +			       u16 num_net_msix, u16 num_others_msix,
> +			       bool net_msix_mask_en)
> +{

[ ... ]

> +	nbl_chan_fill_send_info(&chan_send, common->mgt_pf,
> +				NBL_CHAN_MSG_CONFIGURE_MSIX_MAP,
> +				&param, sizeof(param),
> +				NULL, 0, 1);
> +	ret = chan_ops->send_msg(disp_mgt->chan_ops_tbl->priv, &chan_send);

[Severity: Low]
This precondition appears only as a comment, here and above
nbl_disp_chan_destroy_msix_map_req() and
nbl_disp_chan_set_mailbox_irq_req(). Could it be enforced in code instead?

Suppose one of these RPCs were sent with NBL_CHAN_IRQ_RDY set. The
responder would rewrite the requester's mailbox MSI-X routing before it
writes the ACK. The requester would then wait in the interrupt-mode path
of nbl_chan_send_msg(), which has no RX polling fallback:

nbl_chan_send_msg() {
    ...
	if (test_bit(NBL_CHAN_IRQ_RDY, chan_info->state)) {
		while (!READ_ONCE(wait_head->acked)) {
    ...
			ret = wait_event_timeout(wait_head->wait_queue,
    ...
}

In that case the call would return -ETIMEDOUT even though the hardware
state had already changed.

No in-tree caller later in the series does this. nbl_dev_start(),
nbl_dev_enable_mailbox_irq(), nbl_dev_disable_mailbox_irq(),
nbl_dev_stop() and the err_destroy_map rollback in nbl_dev_start() all
follow the precondition. A check would only be defensive.

[ ... ]

> +static void nbl_disp_chan_destroy_msix_map_resp(void *priv, u16 src_id,
> +						u16 msg_id, void *data,
> +						u32 data_len)
> +{

[ ... ]

> +	if (res_ops->destroy_msix_map) {
> +		ret = res_ops->destroy_msix_map(p, src_id);

[Severity: Medium]
Can this let the device fetch a sibling PF's MSI-X map table after the
table has been freed?

This responder is the first way a sibling PF can trigger a runtime
teardown of its map on the control PF:

nbl_dev_destroy_msix_map()                        (sibling PF)
  -> NBL_CHAN_MSG_DESTROY_MSIX_MAP
    -> nbl_disp_chan_destroy_msix_map_resp()      (control PF)
      -> nbl_res_intr_destroy_msix_map()
        -> __nbl_res_intr_prepare_destroy_msix_map()   clears VALID
        -> nbl_intr_quiesce_wait()                     usleep_range()
        -> __nbl_res_intr_complete_destroy_msix_map()  dma_free_coherent()

The comment above nbl_intr_quiesce_wait() in nbl_interrupt.c says the
window "is not a guarantee", and continues:

 * A future caller that cannot tolerate the residual risk must keep the
 * table allocated for the lifetime of the function instead of relying on
 * this delay.
 ...
 * fetch inside this window.  It cannot help a sibling PF's map destroyed
 * at runtime, because those fetches carry the control PF's BDF and so
 * are unaffected by clearing bus mastering on the sibling.  Runtime
 * destroys of sibling maps only become reachable with the mailbox RPC
 * added later in this series.

This responder is that mailbox RPC, but the table is still freed right
after the roughly 1ms delay. The control PF stays bus master the whole
time. A pcompleter fetch that is still in flight would read freed or
reused memory. With an IOMMU that is a fault. Without one, the device
could use stale gvec routing and deliver an interrupt to the wrong
function.

nbl_disp_chan_cfg_msix_map_resp() has the same exposure when it reaches
the had_config path of nbl_res_intr_cfg_msix_map(). That path returns the
old global vectors to the shared pool via nbl_intr_release_bitmap() after
the same window.

Later in the series, the sibling frees its vectors with
pci_free_irq_vectors() (via nbl_dev_clear_interrupt_scheme()) before it
sends destroy_msix_map. That makes new fetches unlikely, but it still
gives no hardware completion guarantee.

Would it be safer to keep sibling map tables allocated for the lifetime
of the function, as the resource layer comment suggests?

[ ... ]

> 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 b5a695164cbcd..7da9e242c621e 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

[ ... ]

> @@ -32,16 +33,44 @@ enum {
>   *               mailbox DMA before calling it, see nbl_dev_remove().
>   *               Returns void.  Control-PF exclusive, same NULL rule as
>   *               @init_module.
> + * @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

[Severity: Low]
The commit message says cfg_msix_map, destroy_msix_map and
set_mailbox_irq "carry a caller precondition": IRQ_RDY must be cleared
and the polling send path used. That rule is only written above the
static *_req functions in nbl_dispatch.c.

Should the precondition also go in this kernel-doc? Callers program
against struct nbl_dispatch_ops, and from there they cannot tell whether
an op will be a local call or an RPC.

> + * @get_vsi_id: resolve VSI ID by type
> + * @get_eth_id: resolve eth port info from VSI ID
>   *
>   * These callbacks are not dispatch-layer setup/teardown: the dispatch
> - * structures are devm-allocated in nbl_disp_init(), and nbl_disp_remove()
> - * only documents why no explicit teardown is needed. They forward
> - * chip/firmware control down to the layers below instead: a control-PF
> - * function calls the resource ops directly.
> + * structures are devm-allocated in nbl_disp_init(), so nbl_disp_remove()
> + * has nothing to undo. The five responders registered by
> + * nbl_disp_setup_msg() are owned by the channel layer and go away with
> + * it, so they need no explicit unregistration either; the teardown order
> + * that makes that safe is documented with the code that performs it, in
> + * the next patch in this series. They forward chip/firmware control down

[Severity: Low]
This isn't a bug, but won't "the next patch in this series" be
meaningless once this is merged?

At this commit, the code that performs the teardown is:

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

Neither this function nor the nbl_core_init() error unwind documents the
ordering. Nothing breaks at runtime yet, because setup_queue() has no
callers, so no responder can run.

Later, "net/nebula-matrix: add common/ctrl dev init/remove operation"
makes nbl_core_remove() call nbl_dev_remove() first. That tears down the
mailbox channel queue before the resource layer is removed.

Even so, this header comment is unchanged at the end of the series. The
"Teardown order" comment in nbl_dev_remove() also never mentions the
responders or handler_xa.

Could this comment name nbl_dev_remove() and state the invariant
directly, i.e. that the channel queue is torn down before the resource
layer is removed?

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