Re: [PATCH v31 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] Baseline decision is false: in the final state of the series this
  is not a bug.
- [Low] The kernel-doc this patch adds for struct nbl_dispatch_ops in
  nbl_def_dispatch.h says the new callbacks 'forward chip/firmware…

--

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

This patch adds mailbox RPC requests and responders for five dispatch ops:
cfg_msix_map, destroy_msix_map, set_mailbox_irq, get_vsi_id and get_eth_id.
Each op is chosen by capability level. A has_ctrl function calls the
resource layer locally, and a has_net function forwards the request to the
management PF over the channel. nbl_disp_remove() gains a comment that
explains why the registered responders are never unregistered.

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

[ ... ]

>  void nbl_disp_remove(struct nbl_adapter *adapter)
>  {
> -	/* Dispatch structures are allocated via devm */
> +	/*
> +	 * Dispatch structures are devm-allocated and freed at detach.
> +	 *
> +	 * The five responders registered by nbl_disp_setup_msg() are
> +	 * owned by the channel layer (xarray of handlers) and are never
> +	 * unregistered here. This is safe because the teardown order
> +	 * guarantees no responder can run after this point:
> +	 *
> +	 *   mailbox teardown
> +	 *     -> cancel_work_sync(clean_mbx_task)   // drain RX work
> +	 *     -> nbl_chan_teardown_queue()          // stop HW queue,
> +	 *                                                 // join clean task,
> +	 *                                                 // active=false
> +	 *     -> nbl_chan_remove_common()
> +	 *          -> destroy_wq()                       // no new work
> +	 *          -> nbl_chan_remove_msg_handler()      // free handler nodes

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

clean_mbx_task doesn't appear anywhere in the tree yet. Nothing calls
chan_ops->setup_queue at this point either, so the mailbox RX path that
this sequence quiesces is never active.

The ordering the comment relies on is nbl_dev_remove() doing the mailbox
teardown before nbl_disp_remove(), nbl_res_remove_leonis() and
nbl_chan_remove_common(). That ordering only arrives with the later
"net/nebula-matrix: add common/ctrl dev init/remove operation" patch.

Also, destroy_wq() looks like shorthand for nbl_common_destroy_wq().

Could this comment move to the patch that introduces that teardown
ordering, and use the real function names?

> +	 *
> +	 * By the time devres frees disp_mgt, the mailbox queue is stopped
> +	 * and the handler xarray is empty, so no responder can touch
> +	 * res_mgt->intr_mgt after nbl_intr_mgt_stop() has cleared it.
> +	 */
>  }

[ ... ]

> 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 08fef0f679268..bcd74b9df8275 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,6 +33,16 @@ 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
> + * @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 to the upper layers instead.

[Severity: Low]
Is "upper layers" the right direction here? The new callbacks forward
downward, for example:

nbl_disp_cfg_msix_map()
    return res_ops->cfg_msix_map(p, common->mgt_pf, num_net_msix,
                                 num_others_msix, net_msix_mask_en);

The nbl_disp_chan_*_req() variants call chan_ops->send_msg() instead.

The resource and channel layers both sit below dispatch, and nbl_dev is
the caller. Later patches in the series don't change this text, so the
wording stays as it is.

>   *
>   * Warning: init_module/deinit_module are control-PF exclusive. The five
>   * resource ops (cfg_msix_map, destroy_msix_map, set_mailbox_irq,

[ ... ]

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