Re: [PATCH v28 net-next 10/10] net/nebula-matrix: add common dev start/stop operation

[netdev-bot+sashiko@kernel.org]

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