Re: [PATCH] net: ionic: free VF resources on remove

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The newly added `ionic_vf_dealloc()` call in `ionic_remove()`
  (ionic_bus_pci.c:427) reaches `ionic_vf_dealloc_locked()`…

Pre-existing issues:
- [Medium] `ionic_vf_start()`
  (drivers/net/ethernet/pensando/ionic/ionic_dev.c:804-816) issues a…

--

net: ionic: free VF resources on remove

The patch adds a call to ionic_vf_dealloc() in ionic_remove() so that the
VF tracking array and the per-VF statistics DMA mappings created by
ionic_vf_alloc() during probe are released when the PF driver is removed.
Previously only the probe failure path did this.

> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> index 05f19489ec5c8..666efe972de78 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> @@ -424,6 +424,7 @@ static void ionic_remove(struct pci_dev *pdev)
>  		ionic_auxbus_unregister(ionic->lif);
>  		ionic_lif_unregister(ionic->lif);
>  		ionic_devlink_unregister(ionic);
> +		ionic_vf_dealloc(ionic);

[Severity: Medium]
Can the device still be DMAing into the VF statistics buffers at this
point?  ionic_vf_dealloc() -> ionic_vf_dealloc_locked() asks the firmware
to un-arm each buffer but drops the result:

drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c:ionic_vf_dealloc_locked() {
	...
		if (v->stats_pa) {
			vfc.stats_pa = 0;
			ionic_set_vf_config(ionic, i, &vfc);
			dma_unmap_single(ionic->dev, v->stats_pa,
					 sizeof(v->stats), DMA_FROM_DEVICE);
			v->stats_pa = 0;
		}
	...
	kfree(ionic->vfs);
}

ionic_set_vf_config() is a synchronous devcmd that can fail.
__ionic_dev_cmd_wait() returns -ENXIO when ionic_is_fw_running() reports
the firmware down, and on a stalled firmware it returns -ETIMEDOUT after
calling ionic_dev_cmd_clean(), which zeroes the doorbell and command area:

drivers/net/ethernet/pensando/ionic/ionic_main.c:__ionic_dev_cmd_wait() {
	...
		ionic_dev_cmd_clean(ionic);
		dev_warn(ionic->dev, "DEVCMD %s (%d) timeout after %ld secs\n",
			 ionic_opcode_to_str(opcode), opcode, max_seconds);
		return -ETIMEDOUT;
	...
}

In that case the firmware never sees the clear request, yet the mapping is
unmapped and ionic->vfs is freed anyway.  Does anything stop DMA before
this?  SR-IOV is still enabled here (ionic_remove() never calls
pci_disable_sriov(), and pci_iov_remove() only prints "driver left SR-IOV
enabled after remove"), and ionic_port_reset(), ionic_reset() and
ionic_clear_pci() run afterwards, each of the preceding devcmds able to
block for DEVCMD_TIMEOUT first.  Can a later stats DMA then land on an
unmapped IOVA, or on recycled memory when no IOMMU is present?

The FW-down case looks anticipated by ionic_remove() itself, which turns
IONIC_LIF_F_FW_RESET into IONIC_LIF_F_FW_STOPPING just above, and
ionic_lif_reset() guards its devcmd with ionic_is_fw_running() for the same
reason.  Would it make sense to check the ionic_set_vf_config() return
value, or to skip the clear when the firmware is not running, before
unmapping?

The commit message says the call is made:

    "so the VF statistics addresses can be cleared"

On the failure paths above they are not cleared, although the mapping and
the tracking array are released regardless.  Could the message mention
that?

>  		ionic_lif_deinit(ionic->lif);
>  		ionic_lif_free(ionic->lif);
>  		ionic->lif = NULL;

[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at the VF
devcmd paths: ionic_vf_start() in ionic_dev.c issues a devcmd without
holding ionic->dev_cmd_lock, unlike ionic_set_vf_config() right above it
in the same file:

drivers/net/ethernet/pensando/ionic/ionic_dev.c:ionic_set_vf_config() {
	...
	mutex_lock(&ionic->dev_cmd_lock);
	ionic_dev_cmd_go(&ionic->idev, &cmd);
	err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
	mutex_unlock(&ionic->dev_cmd_lock);
	...
}

drivers/net/ethernet/pensando/ionic/ionic_dev.c:ionic_vf_start() {
	...
	ionic_dev_cmd_go(&ionic->idev, &cmd);
	ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
}

The only caller, ionic_vf_attr_replay() in ionic_lif.c, drops vf_op_lock
and then calls it with no lock held:

	up_read(&ionic->vf_op_lock);

	ionic_vf_start(ionic);

Since idev->dev_cmd_regs->cmd/done/comp and idev->opcode are a single
shared window, can two unserialized sequences overwrite each other's
command and consume each other's completion?  The replay runs from
ionic_lif_deferred_work() -> ionic_lif_handle_fw_up() ->
ionic_restart_lif() -> ionic_vf_attr_replay(), before
clear_bit(IONIC_LIF_F_FW_RESET, ...), and ionic_firmware_update() in
ionic_fw.c takes dev_cmd_lock for its own commands without testing
IONIC_LIF_F_FW_RESET, so a concurrent devlink flash seems able to overlap.
This behaviour predates this patch (it goes back to the VF start control
change), and this patch does not touch ionic_vf_start().

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