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

[Jakub Kicinski <kuba@kernel.org>]

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 adds the dispatch layer, which either routes an op directly to
the resource layer or sends a channel message, selected by a per-op control
level bitmap populated at init based on has_ctrl / has_net flags. A set of
channel request/response handlers is introduced for init/deinit of chip
modules, MSIX map configuration, mailbox IRQ enable, and VSI/ETH id
lookup.

A few questions below.

> 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 281d33051185..caf99dd93338 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
> @@ -6,6 +6,396 @@
> +static u16 nbl_disp_chan_get_vsi_id_req(struct nbl_dispatch_mgt *disp_mgt,
> +					u16 type)
> +{
> +	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;
> +
> +	param.type = type;
> +
> +	NBL_CHAN_SEND(chan_send, common->mgt_pf, NBL_CHAN_MSG_GET_VSI_ID,
> +		      &param, sizeof(param), &result, sizeof(result), 1);
> +	chan_ops->send_msg(disp_mgt->chan_ops_tbl->priv, &chan_send);
> +
> +	return result.vsi_id;
> +}

The return value of chan_ops->send_msg() is discarded here, and the
zero-initialized result is returned on failure.

Combined with NBL_OPS_CALL_RET returning 0 when the op pointer is NULL,
can callers tell a mailbox timeout (e.g. -ETIMEDOUT) apart from a valid
vsi_id of 0 or from an op that was not implemented?

The same pattern appears in nbl_disp_chan_get_eth_id_req() below, where
send_msg() failure yields eth_mode=0, eth_id=0, logic_eth_id=0, which are
then fed into subsequent MSIX/VSI configuration.

> +static void nbl_disp_chan_get_vsi_id_resp(void *priv, u16 src_id, u16 msg_id,
> +					  void *data, u32 data_len)
> +{
> +	struct nbl_dispatch_mgt *disp_mgt = (struct nbl_dispatch_mgt *)priv;
> +	struct nbl_channel_ops *chan_ops = disp_mgt->chan_ops_tbl->ops;
> +	struct nbl_resource_ops *res_ops = disp_mgt->res_ops_tbl->ops;
> +	struct nbl_resource_mgt *p = disp_mgt->res_ops_tbl->priv;
> +	struct device *dev = disp_mgt->common->dev;
> +	struct nbl_chan_param_get_vsi_id *param;
> +	struct nbl_chan_param_get_vsi_id result = { 0 };
> +	struct nbl_chan_ack_info chan_ack;
> +	int err = NBL_CHAN_RESP_OK;
> +	int ret;
> +
> +	param = (struct nbl_chan_param_get_vsi_id *)data;
> +
> +	result.vsi_id =
> +		NBL_OPS_CALL_RET(res_ops->get_vsi_id, (p, src_id, param->type));

The incoming data buffer is cast to the parameter struct and param->type is
dereferenced without checking data_len against sizeof(*param).

If a peer sends a truncated NBL_CHAN_MSG_GET_VSI_ID payload, can this read
past the end of the buffer?

The same question applies to nbl_disp_chan_get_eth_id_resp(),
nbl_disp_chan_configure_msix_map_resp(),
nbl_disp_chan_destroy_msix_map_resp() (which at least has no param deref),
and nbl_disp_chan_enable_mailbox_irq_resp() below, which all do the same
unchecked cast on the peer-supplied payload before driving privileged
resource ops such as configure_msix_map and enable_mailbox_irq.

[ ... ]

> +static int nbl_disp_configure_msix_map(struct nbl_dispatch_mgt *disp_mgt,
> +				       u16 num_net_msix, u16 num_others_msix,
> +				       bool net_msix_mask_en)
> +{
> +	struct nbl_resource_ops *res_ops = disp_mgt->res_ops_tbl->ops;
> +	struct nbl_resource_mgt *p = disp_mgt->res_ops_tbl->priv;
> +
> +	return NBL_OPS_CALL_LOCK_RET(disp_mgt, res_ops->configure_msix_map, p,
> +				     0, num_net_msix, num_others_msix,
> +				     net_msix_mask_en);
> +}

configure_msix_map, destroy_msix_map and enable_mailbox_irq call into
res_ops under disp_mgt->ops_mutex_lock, but init_chip_module,
deinit_chip_module, get_vsi_id and get_eth_id (and their _resp twins) call
into the same res_ops without taking the mutex.

Is the mutex meant to serialize shared state in the resource layer?  If
yes, are the unlocked call sites racy against the locked ones; if no, is
the lock needed at all?

[ ... ]

> +static int
> +nbl_disp_chan_configure_msix_map_req(struct nbl_dispatch_mgt *disp_mgt,
> +				     u16 num_net_msix, u16 num_others_msix,
> +				     bool net_msix_mask_en)
> +{
> +	struct nbl_channel_ops *chan_ops = disp_mgt->chan_ops_tbl->ops;
> +	struct nbl_common_info *common = disp_mgt->common;
> +	struct nbl_chan_param_cfg_msix_map param = { 0 };
> +	struct nbl_chan_send_info chan_send;
> +
> +	param.num_net_msix = num_net_msix;
> +	param.num_others_msix = num_others_msix;
> +	param.msix_mask_en = net_msix_mask_en;

The dispatch API and the resource op take net_msix_mask_en as bool, but
nbl_chan_param_cfg_msix_map.msix_mask_en is u16 on the wire:

    struct nbl_chan_param_cfg_msix_map {
            u16 num_net_msix;
            u16 num_others_msix;
            u16 msix_mask_en;
    };

Any non-zero u16 (including garbage in the high bits from a misbehaving
peer) is then treated as true on the receiving side without a !! before
it is handed to res_ops->configure_msix_map.  Is that intentional?

[ ... ]

> +/* NBL_DISP_SET_OPS(disp_op_name, func, ctrl_lvl, msg_type, msg_req, msg_resp)
> + * ctrl_lvl is to define when this disp_op should go directly to res_op,
> + * not sending a channel msg.
> + * Use X Macros to reduce codes in channel_op and disp_op setup/remove
> + */
> +#define NBL_DISP_OPS_TBL						\
> +do {									\
> +	NBL_DISP_SET_OPS(init_chip_module, nbl_disp_init_chip_module,	\
> +			 NBL_DISP_CTRL_LVL_MGT, -1, NULL, NULL);	\
> +	NBL_DISP_SET_OPS(deinit_chip_module,				\
> +			 nbl_disp_deinit_chip_module,			\
> +			 NBL_DISP_CTRL_LVL_MGT, -1, NULL, NULL);	\
> +	NBL_DISP_SET_OPS(configure_msix_map,				\
> +			 nbl_disp_configure_msix_map,			\
> +			 NBL_DISP_CTRL_LVL_MGT,				\
> +			 NBL_CHAN_MSG_CONFIGURE_MSIX_MAP,		\
> +			 nbl_disp_chan_configure_msix_map_req,		\
> +			 nbl_disp_chan_configure_msix_map_resp);	\
> +	NBL_DISP_SET_OPS(destroy_msix_map, nbl_disp_destroy_msix_map,	\
> +			 NBL_DISP_CTRL_LVL_MGT,				\
> +			 NBL_CHAN_MSG_DESTROY_MSIX_MAP,			\
> +			 nbl_disp_chan_destroy_msix_map_req,		\
> +			 nbl_disp_chan_destroy_msix_map_resp);		\
> +	NBL_DISP_SET_OPS(enable_mailbox_irq,				\
> +			 nbl_disp_enable_mailbox_irq,			\
> +			 NBL_DISP_CTRL_LVL_MGT,				\
> +			 NBL_CHAN_MSG_MAILBOX_ENABLE_IRQ,		\
> +			 nbl_disp_chan_enable_mailbox_irq_req,		\
> +			 nbl_disp_chan_enable_mailbox_irq_resp);	\
> +	NBL_DISP_SET_OPS(get_vsi_id, nbl_disp_get_vsi_id,		\
> +			 NBL_DISP_CTRL_LVL_MGT, NBL_CHAN_MSG_GET_VSI_ID,\
> +			 nbl_disp_chan_get_vsi_id_req,			\
> +			 nbl_disp_chan_get_vsi_id_resp);		\
> +	NBL_DISP_SET_OPS(get_eth_id, nbl_disp_get_eth_id,		\
> +			 NBL_DISP_CTRL_LVL_MGT, NBL_CHAN_MSG_GET_ETH_ID,\
> +			 nbl_disp_chan_get_eth_id_req,			\
> +			 nbl_disp_chan_get_eth_id_resp);		\
> +} while (0)

The commit message describes the routing mechanism using a "Regular PF"
example that direct-dispatches NET_LVL ops while channel-dispatching
CTRL_LVL (management) ops:

    Regular PF
    Configures 'NET_LVL' at Dispatch layer
      All 'NET_LVL' operations use direct path; 'CTRL_LVL' operations go via channel.

Every entry in NBL_DISP_OPS_TBL, however, declares NBL_DISP_CTRL_LVL_MGT,
and no entry declares NBL_DISP_CTRL_LVL_NET.  Is the has_net /
NBL_DISP_CTRL_LVL_NET branch in nbl_disp_init() reachable by any op as
submitted, or is the NET_LVL half of the feature not yet wired up?

> +/* Structure starts here, adding an op should not modify anything below */
> +static int nbl_disp_setup_msg(struct nbl_dispatch_mgt *disp_mgt)
> +{
> +	struct nbl_dispatch_ops *disp_ops = disp_mgt->disp_ops_tbl->ops;
> +	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;
> +
> +	mutex_init(&disp_mgt->ops_mutex_lock);
> +
> +#define NBL_DISP_SET_OPS(disp_op, func, ctrl, msg_type, msg_req, resp) \
> +do {									\
> +	typeof(msg_type) _msg_type = (msg_type);			\
> +	typeof(ctrl) _ctrl_lvl = (ctrl);				\
> +	(void)(disp_ops->NBL_NAME(disp_op));				\
> +	(void)(func);							\
> +	(void)(msg_req);						\
> +	(void)_ctrl_lvl;						\
> +	if (_msg_type >= 0)						\
> +		ret += chan_ops->register_msg(p, _msg_type, resp, disp_mgt);\
> +} while (0)
> +	NBL_DISP_OPS_TBL;
> +#undef  NBL_DISP_SET_OPS
> +
> +	return ret;
> +}

Two questions about this helper.

First, the return codes of the individual register_msg() calls are summed
with "ret += chan_ops->register_msg(...)".  If two registrations return
-ENOMEM, the summed result is -24 (which is -EMFILE), and in general a
sum of negative errnos does not map back to any single defined error.  Is
an early-return-on-first-error pattern intended here instead?

Second, mutex_init() and every successful chan_ops->register_msg() install
state into the channel layer with disp_mgt as priv, but nbl_disp_remove()
is currently an empty stub:

    void nbl_disp_remove(struct nbl_adapter *adapter)
    {
    }

On driver unload or on any later probe failure path that tears the
adapter down, is there a matching unregister_msg() for each handler
installed here, and a matching mutex_destroy()?

If nbl_disp_setup_msg() partially succeeds and then fails mid-way, is the
partially installed handler table also rolled back before nbl_disp_init()
returns an error to its caller?  Handlers left registered would retain
disp_mgt as priv and would be invoked on the next incoming message.

> +/* Ctrl lvl means that if a certain level is set, then all disp_ops that
> + * decleared this lvl will go directly to res_ops, rather than send a
                ^^^^^^^^^
Minor: typo, should be "declared".

> + * channel msg, and vice versa.
> + */
> +static int nbl_disp_setup_ctrl_lvl(struct nbl_dispatch_mgt *disp_mgt, u32 lvl)
> +{
> +	struct nbl_dispatch_ops *disp_ops = disp_mgt->disp_ops_tbl->ops;
> +
> +	set_bit(lvl, disp_mgt->ctrl_lvl);
> +
> +#define NBL_DISP_SET_OPS(disp_op, func, ctrl, msg_type, msg_req, msg_resp) \
> +do {									\
> +	typeof(msg_type) _msg_type = (msg_type);			\
> +	(void)(_msg_type);						\
> +	(void)(msg_resp);						\
> +	disp_ops->NBL_NAME(disp_op) =					\
> +		test_bit(ctrl, disp_mgt->ctrl_lvl) ? func : msg_req;	\
> +} while (0)
> +	NBL_DISP_OPS_TBL;
> +#undef  NBL_DISP_SET_OPS
> +
> +	return 0;
> +}

nbl_disp_setup_ctrl_lvl() unconditionally returns 0, but all three call
sites in nbl_disp_init() below check "if (ret) return ret;" on the
return value:

> @@ -71,7 +461,25 @@ int nbl_disp_init(struct nbl_adapter *adapter)
> +	if (common->has_ctrl) {
> +		ret = nbl_disp_setup_ctrl_lvl(disp_mgt, NBL_DISP_CTRL_LVL_MGT);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (common->has_net) {
> +		ret = nbl_disp_setup_ctrl_lvl(disp_mgt, NBL_DISP_CTRL_LVL_NET);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = nbl_disp_setup_ctrl_lvl(disp_mgt, NBL_DISP_CTRL_LVL_ALWAYS);
> +
> +	return ret;
>  }

Is the fallible signature intentional, or should nbl_disp_setup_ctrl_lvl()
return void so these dead "if (ret)" checks can be dropped?