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

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.

[ ... ]

[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?

[ ... ]

[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?

[ ... ]

[ ... ]

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914123429.56596-1-illusion.wang%40nebula-matrix.com