Re: [v26,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 3 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 1 · Low: 1

- [High] This patch adds the first live callers of
  cfg_msix_map()/destroy_msix_map(), so the control PF (function 0) now…
- [Medium] In nbl_dev_stop() (nbl_core/nbl_dev.c:489-495) and in the
  identical unwind inside nbl_dev_start() (nbl_dev.c:465-472), a failure…
- [Low] Claim (assessed FALSE POSITIVE): nbl_dev_disable_mailbox_irq()
  clears NBL_CHAN_IRQ_RDY (nbl_core/nbl_dev.c:137-139) without waking…

--

net/nebula-matrix: add common dev start/stop operation

This adds nbl_dev_start() and nbl_dev_stop() to configure the hardware
MSI-X map, allocate MSI-X vectors, request the mailbox interrupt and tear
all of that down again.  Thin nbl_core_start() / nbl_core_stop() wrappers
are hooked into the PCI probe and remove paths.

A few questions on the new start/stop paths 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 4fc52cadf60f..26b785c9a05f 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

[ ... ]

> @@ -14,6 +25,189 @@ static void nbl_dev_init_msix_cnt(struct nbl_dev_mgt *dev_mgt)
>  	msix_info->serv_info[NBL_MSIX_MAILBOX_TYPE].num = 1;
>  }
>  

[ ... ]

> +static int nbl_dev_disable_mailbox_irq(struct nbl_dev_mgt *dev_mgt)
> +{
> +	struct nbl_dispatch_ops *disp_ops = dev_mgt->disp_ops_tbl->ops;
> +	struct nbl_channel_ops *chan_ops = dev_mgt->chan_ops_tbl->ops;
> +	struct nbl_dev_common *dev_common = dev_mgt->common_dev;
> +	struct nbl_msix_info *msix_info = &dev_common->msix_info;
> +	u16 local_vec_id;
> +
> +	if (!msix_info->serv_info[NBL_MSIX_MAILBOX_TYPE].num)
> +		return 0;
> +
> +	local_vec_id =
> +		msix_info->serv_info[NBL_MSIX_MAILBOX_TYPE].base_vector_id;

[ ... comment block snipped ... ]

> +	chan_ops->set_queue_state(dev_mgt->chan_ops_tbl->priv,
> +				  NBL_CHAN_IRQ_RDY,
> +				  NBL_CHAN_TYPE_MAILBOX, false);
> +
> +	return disp_ops->set_mailbox_irq(dev_mgt->disp_ops_tbl->priv,
> +					    local_vec_id, false);
> +}

[Severity: Low]
This isn't a bug in the tree as it stands, but clearing NBL_CHAN_IRQ_RDY
here does not wake the per-message ACK wait queues that test that same
bit.

nbl_chan_send_msg() sleeps on it:

	ret = wait_event_timeout(wait_head->wait_queue,
				 READ_ONCE(wait_head->acked) ||
				 READ_ONCE(chan_info->shutdn) ||
				 !test_bit(NBL_CHAN_IRQ_RDY,
					   chan_info->state),
				 NBL_CHAN_ACK_WAIT_TIME);

At this point in the series every ack-requesting sender is one of the
nbl_disp_chan_*_req() helpers, all reached only from
nbl_dev_setup_common_dev(), nbl_dev_start() and nbl_dev_stop(), which the
driver core serializes against this path, and nbl_chan_send_ack() sends
with ack = 0 and never sleeps.  So no waiter can be stranded today.

Once netdev or ethtool paths start sending mailbox messages, would this
also need the WRITE_ONCE(wait_head->acked, 1) plus wake_up() sequence that
nbl_chan_teardown_queue() already does?

[ ... ]

> @@ -234,3 +436,69 @@ void nbl_dev_remove(struct nbl_adapter *adapter)
>  		nbl_dev_remove_ctrl_dev(adapter);
>  	nbl_dev_remove_common_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_dev_common *common_dev = dev_mgt->common_dev;
> +	int cleanup_ret;
> +	int ret;
> +
> +	ret = nbl_dev_cfg_msix_map(dev_mgt);
> +	if (ret)
> +		goto cfg_msix_map_err;

[Severity: High]
This is the first live caller of cfg_msix_map() / destroy_msix_map(), so
the control PF now allocates per-function MSI-X state on behalf of the
other PFs.  Is there anything on the control PF side that ever releases
that state?

On the control PF, nbl_res_intr_cfg_msix_map() allocates:

	tmp_msix_tbl->base_addr = dmam_alloc_coherent(dev, tmp_msix_tbl->size,
						      &tmp_msix_tbl->dma,
						      GFP_KERNEL);
	...
	tmp_interrupts = kcalloc(requested, sizeof(tmp_interrupts[0]), GFP_KERNEL);

stores both in intr_mgt->func_intr_res[func_id] and programs the hardware
map with VALID=1.  The only release path is
nbl_res_intr_destroy_msix_map(), which for a peer function runs only when
that peer PF executes its own nbl_dev_stop() and its DESTROY_MSIX_MAP
mailbox request succeeds.

On the control PF side, nbl_intr_mgt_start() has no stop counterpart,
nbl_res_remove_leonis() is a stub:

void nbl_res_remove_leonis(struct nbl_adapter *adap)
{
	/*
	 * No resource release here because all memory uses devm managed
	 * allocation
	 */
}

and nbl_dev_remove_ctrl_dev() only calls disp_ops->deinit_module().

Does unbinding or hot-removing the control PF while another PF is still
bound leak the kcalloc()'d func_intr_res[].interrupts arrays?  intr_mgt
itself is devm_kzalloc()'d, so the pointers to those arrays look
unreachable once devres frees it, and the comment above claims everything
is devm managed.

In the same scenario, can the peer function be left with
NBL_PCOMPLETER_FUNCTION_MSIX_MAP still VALID=1 pointing at the coherent
table that devres just reclaimed together with the control PF, so a later
interrupt from that function makes the device DMA-read freed memory?  The
peer's own DESTROY_MSIX_MAP request has no manager left at that point, and
nbl_dev_stop() only logs the failure.

> +
> +	ret = nbl_dev_init_interrupt_scheme(dev_mgt);
> +	if (ret)
> +		goto init_interrupt_scheme_err;
> +	ret = nbl_dev_request_mailbox_irq(dev_mgt);
> +	if (ret)
> +		goto mailbox_request_irq_err;
> +	ret = nbl_dev_enable_mailbox_irq(dev_mgt);
> +	if (ret)
> +		goto enable_mailbox_irq_err;
> +
> +	return 0;
> +
> +enable_mailbox_irq_err:
> +	cleanup_ret = nbl_dev_disable_mailbox_irq(dev_mgt);
> +	if (cleanup_ret)
> +		dev_err(dev_mgt->common->dev,
> +			"Failed to disable mailbox IRQ: %d\n", cleanup_ret);
> +	nbl_dev_free_mailbox_irq(dev_mgt);
> +	cancel_work_sync(&common_dev->clean_mbx_task);
> +mailbox_request_irq_err:
> +	nbl_dev_clear_interrupt_scheme(dev_mgt);
> +init_interrupt_scheme_err:
> +cfg_msix_map_err:
> +	cleanup_ret = nbl_dev_destroy_msix_map(dev_mgt);
> +	if (cleanup_ret)
> +		dev_err(dev_mgt->common->dev,
> +			"Failed to destroy MSI-X map: %d\n", cleanup_ret);
> +	cancel_work_sync(&common_dev->clean_mbx_task);
> +	return ret;
> +}
> +
> +void nbl_dev_stop(struct nbl_adapter *adapter)
> +{
> +	struct nbl_dev_mgt *dev_mgt = adapter->core.dev_mgt;
> +	struct nbl_dev_common *common_dev = dev_mgt->common_dev;
> +	int ret;
> +
> +	ret = nbl_dev_disable_mailbox_irq(dev_mgt);
> +	if (ret)
> +		dev_err(dev_mgt->common->dev,
> +			"Failed to disable mailbox IRQ: %d\n", ret);
> +	nbl_dev_free_mailbox_irq(dev_mgt);
> +	nbl_dev_clear_interrupt_scheme(dev_mgt);
> +	ret = nbl_dev_destroy_msix_map(dev_mgt);
> +	if (ret)
> +		dev_err(dev_mgt->common->dev,
> +			"Failed to destroy MSI-X map: %d\n", ret);

[Severity: Medium]
Here a failure of nbl_dev_disable_mailbox_irq() is only logged and
teardown carries on.  On a non-control PF that helper is a mailbox RPC,
nbl_disp_chan_set_mailbox_irq_req(), which can return -EREMOTEIO,
-ETIMEDOUT or -ESHUTDOWN when the manager PF is unresponsive or already
unbound.

It is also the only operation that clears MSIX_IDX and MSIX_IDX_VALID in
NBL_MAILBOX_QINFO_MAP_REG_ARR(func_id):

nbl_hw_leonis.c:nbl_hw_set_mailbox_irq() {
	data &= ~(NBL_MAILBOX_QINFO_MAP_MSIX_IDX_MASK |
		  NBL_MAILBOX_QINFO_MAP_MSIX_IDX_VALID_MASK);
}

So if that RPC fails, the mailbox queue stays armed to emit an MSI-X
message while nbl_dev_free_mailbox_irq() and then pci_free_irq_vectors()
run.  Can that leave the device sending a vector with no handler behind
it?

The remaining masking path is reached over the same mailbox channel:

nbl_interrupt.c:nbl_res_intr_destroy_msix_map() {
	for (i = 0; i < intr_num; i++) {
		hw_ops->cfg_msix_info(res_mgt->hw_ops_tbl->priv,
				      func_id, false, interrupts[i],
				      0, 0, 0, false);
	}
}

and its failure is likewise only logged, so when the channel is wedged
nothing quiesces the interrupt source.  Should stop fail loudly or use a
local register write instead of the RPC for the masking step?

Separately, even on the success path nbl_dev_clear_interrupt_scheme()
runs before nbl_dev_destroy_msix_map(), i.e. the kernel-side vectors are
released while the device-side mapping is still valid.  Was that ordering
intended, given that only the (fallible) disable makes it safe?

The identical unwind in nbl_dev_start() under enable_mailbox_irq_err
ignores both failures in the same way, so probe can fail with the MSI-X
map and mailbox routing still armed on the device.

> +	/*
> +	 * destroy_msix_map() sends ack-requested messages which may requeue
> +	 * clean_mbx_task via polling send path. Drain work after the operation.
> +	 */
> +	cancel_work_sync(&common_dev->clean_mbx_task);
> +}

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