Re: [PATCH v22 net-next 09/12] net/nebula-matrix: dispatch: add cross-version channel message framework
flat view
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-07-31 01:30:35
Also in:
linux-doc, lkml
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. ---
--- Patch [9]: [PATCH v22 net-next 09/12] net/nebula-matrix: dispatch: add cross-version channel message framework ---commit d32035d9a9bd3114eb253cf0283172847c233d7d
Author: illusion wang [off-list ref]
net/nebula-matrix: dispatch: add cross-version channel message framework
Implement bidirectional channel request/response RPC handlers for 5 dispatch
resource operations: get_vsi_id, get_eth_id, configure_msix_map,
destroy_msix_map, set_mailbox_irq.
quoted hunk ↗ jump to hunk
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 8116643859c79..c82c336f50e5e 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
[ ... ]
quoted hunk ↗ jump to hunk
+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; + struct nbl_common_info *common = disp_mgt->common; + + return NBL_OPS_CALL_RET(res_ops->configure_msix_map, (p, + common->mgt_pf, num_net_msix, + num_others_msix, net_msix_mask_en)); +}
[Severity: High] Does nbl_disp_configure_msix_map() need to take a mutex lock before invoking res_ops->configure_msix_map()? Calling this resource operation without synchronization might lead to data races and hardware state corruption if multiple requests arrive simultaneously. [ ... ]
quoted hunk ↗ jump to hunk
+static void nbl_disp_chan_configure_msix_map_resp(void *priv, u16 src_id, + u16 msg_id, void *data, + u32 data_len) +{
[ ... ]
quoted hunk ↗ jump to hunk
+ if (!res_ops->configure_msix_map) { + err = NBL_CHAN_RESP_UNIMPLEMENTED; + } else { + ret = NBL_OPS_CALL_RET(res_ops->configure_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))); + if (ret) + err = NBL_CHAN_RESP_ERR; + }
[Severity: High] Should nbl_disp_chan_configure_msix_map_resp() also use locking when invoking res_ops->configure_msix_map()? Since this response handler can run from a workqueue context concurrently with other operations, this might also lead to hardware state corruption. [ ... ]
quoted hunk ↗ jump to hunk
+static void nbl_disp_chan_set_mailbox_irq_resp(void *priv, u16 src_id, + u16 msg_id, void *data, + u32 data_len) +{
[ ... ]
quoted hunk ↗ jump to hunk
+ if (!res_ops->set_mailbox_irq) { + err = NBL_CHAN_RESP_UNIMPLEMENTED; + } else { + ret = NBL_OPS_CALL_RET(res_ops->set_mailbox_irq, + (p, src_id, vector_id, enable_msix)); + if (ret)
[Severity: High] Similarly, should nbl_disp_chan_set_mailbox_irq_resp() be protected by a lock when modifying the mailbox IRQ hardware state? [ ... ]
quoted hunk ↗ jump to hunk
@@ -126,6 +562,10 @@ 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; + if (common->has_ctrl) nbl_disp_setup_ctrl_lvl(disp_mgt, NBL_DISP_CTRL_LVL_MGT);
[Severity: High] Does the dispatch operations table remain uninitialized for non-control PFs here? Since nbl_disp_setup_ctrl_lvl() is responsible for populating the disp_ops table (including setting up the remote message request handlers as fallbacks when testing control levels), skipping this step when common->has_ctrl is false might leave the function pointers as NULL. Could this lead to a NULL pointer dereference when a regular non-control PF subsequently attempts to invoke a dispatch operation like get_vsi_id or get_eth_id?