Re: [PATCH v14 net-next 09/11] net/nebula-matrix: add Dispatch layer implementation

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/nebula-matrix: add Dispatch layer implementation

This patch introduces a control-level routing mechanism for the Dispatch layer.
Routing is controlled by interface-declared levels and upper-layer configured
control levels.

> Current state:
> - Regular PF: configures NET_LVL at Dispatch layer.
>   Since NBL_DISP_CTRL_LVL_NET is not yet wired in nbl_disp_init(),
>   all operations currently fall through to the CTRL_LVL_MGT branch
>   and use the channel path. The direct path for NET_LVL is a
>   framework extension point reserved for future use.
>
> - Management PF: configures both NET_LVL and CTRL_LVL.
>   Same as above — currently all ops use channel path.
>
> Future work:
> Implement the has_net / NBL_DISP_CTRL_LVL_NET branch in nbl_disp_init()
> to enable direct path routing for network operations.

The commit message states that NBL_DISP_CTRL_LVL_NET is not yet wired in
nbl_disp_init(), and lists implementing the has_net branch as future work.

However, the patch includes this exact logic in nbl_disp_init() below.

Could the commit message be updated to match the implemented code?

[ ... ]
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dispatch.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dispatch.c
[ ... ]
> +static void nbl_disp_chan_get_vsi_id_resp(void *priv, u16 src_id, u16 msg_id,
> +					  void *data, u32 data_len)
> +{
[ ... ]
> +	NBL_CHAN_ACK(chan_ack, src_id, NBL_CHAN_MSG_GET_VSI_ID, msg_id, err,
> +		     &result, sizeof(result));
> +	ret = chan_ops->send_ack(disp_mgt->chan_ops_tbl->priv, &chan_ack);

Could this lead to a self-deadlock on txq_lock during mailbox polling?

If a message is sent while polling, nbl_chan_send_msg() acquires txq_lock
and processes incoming messages in its polling loop:

nbl_chan_send_msg() [acquires txq_lock]
  -> nbl_chan_clean_queue()
    -> nbl_chan_recv_msg()
      -> nbl_disp_chan_get_vsi_id_resp()

If nbl_disp_chan_get_vsi_id_resp() processes a request and calls
chan_ops->send_ack(), this will eventually call back into nbl_chan_send_msg():

nbl_disp_chan_get_vsi_id_resp()
  -> chan_ops->send_ack()
    -> nbl_chan_send_ack()
      -> nbl_chan_send_msg() [attempts to acquire txq_lock again]

Will this cause an immediate deadlock since the non-recursive txq_lock is
already held by the outer nbl_chan_send_msg() call?

[ ... ]
> @@ -71,9 +477,24 @@ int nbl_disp_init(struct nbl_adapter *adapter)
[ ... ]
> +	if (common->has_ctrl)
> +		nbl_disp_setup_ctrl_lvl(disp_mgt, NBL_DISP_CTRL_LVL_MGT);
> +
> +	if (common->has_net)
> +		nbl_disp_setup_ctrl_lvl(disp_mgt, NBL_DISP_CTRL_LVL_NET);

This appears to wire up the NBL_DISP_CTRL_LVL_NET branch, which contradicts the
commit message stating it is not yet wired.

> +
> +	nbl_disp_setup_ctrl_lvl(disp_mgt, NBL_DISP_CTRL_LVL_ALWAYS);
> +
> +	return ret;
>  }