Re: [PATCH] cxgb4: fix FW SGE queue cleanup on remove

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

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

- [Medium] Incomplete fix: the CXGB4_FULL_INIT_DONE-gated release of the
  probe-time FW SGE queues is corrected only in remove_one(); the…

--

cxgb4: fix FW SGE queue cleanup on remove

The patch adds a branch to remove_one() so that the FW SGE queues
allocated by setup_fw_sge_queues() during init_one() are released when
CXGB4_FULL_INIT_DONE was never set, for example when no interface of the
adapter was ever opened.

> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> index 1ced6df6eac8c..146419c663488 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> @@ -7116,6 +7116,9 @@ static void remove_one(struct pci_dev *pdev)
>  
>  		if (adapter->flags & CXGB4_FULL_INIT_DONE)
>  			cxgb_down(adapter);
> +		else if (adapter->sge.fw_evtq.desc ||
> +			 adapter->sge.intrq.desc)
> +			t4_free_sge_resources(adapter);

[Severity: Medium]
The same CXGB4_FULL_INIT_DONE assumption that this patch corrects in
remove_one() also exists in the PCI reset pair eeh_reset_prepare() /
eeh_reset_done().  Should that path be fixed too?

eeh_reset_prepare() only releases SGE resources through the same gate:

	for_each_port(adapter, i)
		if (adapter->port[i]->reg_state == NETREG_REGISTERED)
			cxgb_close(adapter->port[i]);
	...
	if (adapter->flags & CXGB4_FULL_INIT_DONE)
		cxgb_down(adapter);

cxgb_close() does not free SGE resources and does not clear
CXGB4_FULL_INIT_DONE, so on an adapter that was probed but never opened
the FW queues stay allocated across the reset.

eeh_reset_done() then re-runs the allocation unconditionally:

	err = setup_fw_sge_queues(adapter);
	if (err) {
		dev_err(adapter->pdev_dev,
			"FW sge queue allocation failed, err %d", err);
		return;
	}

Does this leak the previous fw_evtq (and intrq in the MSI/INTx case)
ring?  setup_fw_sge_queues() overwrites s->fwevtq_msix_idx after taking a
fresh index from cxgb4_get_msix_idx_from_bmap(), and t4_sge_alloc_rxq()
overwrites the ring pointer without looking at or releasing an existing
one:

drivers/net/ethernet/chelsio/cxgb4/sge.c:t4_sge_alloc_rxq() {
	...
	iq->desc = alloc_ring(adap->pdev_dev, iq->size, iq->iqe_len, 0,
			      &iq->phys_addr, NULL, 0,
			      dev_to_node(adap->pdev_dev));
	if (!iq->desc)
		return -ENOMEM;
	...
}

So the old dma_alloc_coherent() ingress ring becomes unreachable, the
previously reserved bit in adapter->msix_bmap is never returned via
cxgb4_free_msix_idx_in_bmap(), and the old firmware ingress queue context
is never released with t4_iq_free().  This repeats on every reset of the
pf == 4 function (sysfs reset, FLR, VFIO reset), and once the msix bitmap
is exhausted setup_fw_sge_queues() returns -ENOMEM and eeh_reset_done()
bails out, leaving the adapter unrecovered.

The new branch in remove_one() cannot recover those allocations, since
their only pointers were already overwritten during the reset.

Would an unconditional t4_free_sge_resources() at both teardown sites be
simpler?  t4_free_sge_resources() already performs the same checks
internally:

drivers/net/ethernet/chelsio/cxgb4/sge.c:t4_free_sge_resources() {
	...
	if (adap->sge.fw_evtq.desc) {
		free_rspq_fl(adap, &adap->sge.fw_evtq, NULL);
		...
	}
	...
	if (adap->sge.intrq.desc)
		free_rspq_fl(adap, &adap->sge.intrq, NULL);
	...
}

and init_one()'s out_free_dev label already calls it unconditionally:

 out_free_dev:
	t4_free_sge_resources(adapter);

Could the changelog also mention the reset path, so that a backporter
knows whether the identical lifecycle mismatch there was considered?

>  
>  		if (adapter->flags & CXGB4_USING_MSIX)
>  			free_msix_info(adapter);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916025215.2780658-1-lgs201920130244%40gmail.com