Re: [PATCH v28 net-next 10/10] net/nebula-matrix: add common dev start/stop operation
From: netdev-bot+sashiko
Date: Thu Sep 17 2026 - 20:28:21 EST
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 2 · Low: 0
- [Medium] A non-control PF's nbl_dev_start() (nbl_dev.c:457-492) issues
three mailbox RPCs to the manager PF (cfg_msix_map at line 467,…
- [Medium] nbl_dev_setup_common_dev() (nbl_dev.c:300-332) calls
cancel_work_sync(&common_dev->clean_mbx_task) at its err_cleanup label…
--
net/nebula-matrix: add common dev start/stop operation
This adds nbl_dev_start()/nbl_dev_stop() to configure the hardware MSI-X
map, fetch VSI/ETH identity, allocate MSI-X vectors and request/release the
mailbox interrupt. Thin wrappers nbl_core_start()/nbl_core_stop() are hooked
into nbl_probe() and nbl_remove().
A couple of questions below.
> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c
> index 75c0d83663b9c..e9b0576de0f30 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c
[ ... ]
> @@ -77,6 +288,14 @@ static void nbl_dev_clean_mailbox_task(struct work_struct *work)
> NBL_CHAN_TYPE_MAILBOX);
> }
>
> +static void nbl_dev_clean_mailbox_schedule(struct nbl_dev_mgt *dev_mgt)
> +{
> + struct nbl_dev_common *common_dev = dev_mgt->common_dev;
> + struct nbl_common_info *common = dev_mgt->common;
> +
> + queue_work(common->wq, &common_dev->clean_mbx_task);
> +}
> +
> /* ---------- Dev init process ---------- */
> static int nbl_dev_setup_common_dev(struct nbl_adapter *adapter)
[Severity: Medium]
Now that clean_mbx_task is driven from the mailbox hardirq handler, can we
also take a look at the error path in nbl_dev_setup_common_dev()? The work
is cancelled there before it is ever initialized:
common_dev = devm_kzalloc(&adapter->pdev->dev, sizeof(*common_dev),
GFP_KERNEL);
...
ret = nbl_dev_setup_chan_queue(dev_mgt, NBL_CHAN_TYPE_MAILBOX);
if (ret)
goto err_cleanup;
INIT_WORK(&common_dev->clean_mbx_task, nbl_dev_clean_mailbox_task);
...
err_cleanup:
cancel_work_sync(&common_dev->clean_mbx_task);
If nbl_dev_setup_chan_queue() fails (its ring allocations can return
-ENOMEM), clean_mbx_task is still the all-zero devm_kzalloc content, so
work->func is NULL when cancel_work_sync() runs:
kernel/workqueue.c:__flush_work() {
...
if (WARN_ON(!work->func))
return false;
...
}
Does this produce a workqueue warning backtrace on that path? The zeroed
work->data also decodes to pool_id 0 rather than WORK_OFFQ_POOL_NONE, so
start_flush_work() would look at an unrelated worker pool. Would moving
INIT_WORK() up to right after the devm_kzalloc(), or dropping the cancel
from a path where the work was never initialized, be preferable?
[ ... ]
> @@ -233,3 +452,120 @@ void nbl_dev_remove(struct nbl_adapter *adapter)
> if (common->has_ctrl)
> nbl_dev_remove_ctrl_dev(adapter);
> }
> +
> +/* ---------- Dev start process ---------- */
> +int nbl_dev_start(struct nbl_adapter *adapter)
> +{
> + struct nbl_dev_mgt *dev_mgt = adapter->core.dev_mgt;
> + struct nbl_dispatch_ops *disp_ops = dev_mgt->disp_ops_tbl->ops;
> + struct nbl_dispatch_mgt *priv = dev_mgt->disp_ops_tbl->priv;
> + struct nbl_dev_common *common_dev = dev_mgt->common_dev;
> + struct nbl_common_info *common = dev_mgt->common;
> + int cleanup_ret;
> + int ret;
> +
> + ret = nbl_dev_cfg_msix_map(dev_mgt);
> + if (ret)
> + goto err_destroy_map;
> +
> + /* Fetch VSI/ETH identity after cfg_msix_map */
> + ret = disp_ops->get_vsi_id(priv, NBL_VSI_DATA, &common->vsi_id);
> + if (ret)
> + goto err_destroy_map;
> + ret = disp_ops->get_eth_id(priv, common->vsi_id, &common->eth_num,
> + &common->eth_id, &common->logic_eth_id);
> + if (ret)
> + goto err_destroy_map;
[Severity: Medium]
On a non-control PF, all three of these calls are mailbox RPCs to the
manager PF, since nbl_disp_refresh_ctrl_ops() installs the
NBL_DISP_CTRL_LVL_NET request variants:
nbl_dispatch.c:nbl_disp_refresh_ctrl_ops() {
...
} else if (test_bit(NBL_DISP_CTRL_LVL_NET, disp_mgt->ctrl_lvl)) {
disp_ops->cfg_msix_map =
...
}
What happens if PF0 has not finished its own init/start yet? The send goes
through the polling path and comes back as a bounded failure:
nbl_channel.c:nbl_chan_send_msg() {
...
dev_err_ratelimited(dev,
"Channel polling ack failed, message type: %d msg id: %u\n",
chan_send->msg_type, msgid);
ret = -ETIMEDOUT;
...
}
nbl_dev_start() then returns -ETIMEDOUT to nbl_probe(), which turns it into
a plain probe failure rather than -EPROBE_DEFER, so the PF stays unbound
with no retry once PF0 does come up.
This looks reachable with asynchronous probing enabled
(driver_async_probe=nbl or nbl.async_probe=1), where PF1..PF3 can run these
RPCs before PF0 has programmed the mailbox QINFO routing and registered the
responders, and also when an administrator binds a non-control PF via
/sys/bus/pci/drivers/nbl/bind while PF0 is unbound.
The commit message notes the dependency:
Non-control PFs rely on the control PF's mailbox responder
being fully operational when this RPC runs.
Should the code enforce that, either by checking control-PF readiness or by
returning -EPROBE_DEFER so the bind is retried?
[ ... ]
> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> index 9896c1b49be2b..02cb35d32012f 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_main.c
[ ... ]
> @@ -127,7 +137,13 @@ static int nbl_probe(struct pci_dev *pdev,
> goto adapter_init_err;
> }
> pci_set_drvdata(pdev, adapter);
> + err = nbl_core_start(adapter);
> + if (err)
> + goto core_start_err;
> +
> return 0;
> +core_start_err:
> + nbl_core_remove(adapter);
> adapter_init_err:
> pci_clear_master(pdev);
> return err;
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914123429.56596-1-illusion.wang%40nebula-matrix.com