Re: [PATCH net v3 2/3] pds_core: don't release PCI regions for VFs on reset

[netdev-bot+sashiko@kernel.org]

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

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

- [High] pdsc_reset_prepare()
  (drivers/net/ethernet/amd/pds_core/main.c:516-521) now gates…

--

pds_core: don't release PCI regions for VFs on reset

This moves pci_release_regions() in pdsc_reset_prepare() inside the
existing !pdev->is_virtfn block, since only the PF path calls
pci_request_regions() in pdsc_init_pf(). The stated goal is to stop the
"Trying to free nonexistent resource" warning on a VF FLR.

Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804235946.177762-1-nikhil.rao%40amd.com

> diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
> index 6e1079f9ba0f5..a971c66d36f98 100644
> --- a/drivers/net/ethernet/amd/pds_core/main.c
> +++ b/drivers/net/ethernet/amd/pds_core/main.c
> @@ -517,8 +517,8 @@ static void pdsc_reset_prepare(struct pci_dev *pdev)
>  		mutex_lock(&pdsc->devcmd_lock);
>  		pdsc_unmap_bars(pdsc);
>  		mutex_unlock(&pdsc->devcmd_lock);
> +		pci_release_regions(pdev);
>  	}
> -	pci_release_regions(pdev);

[Severity: High]

The new condition keys the release off the device type rather than off
whether the driver currently holds the regions.  Can the PF still hit the
same "Trying to free nonexistent resource" warning from __release_region()
because pdsc_reset_prepare() runs twice per AER frozen-channel recovery
with no pdsc_reset_done() in between?

The first call comes from the error_detected callback:

drivers/net/ethernet/amd/pds_core/main.c:pdsc_pci_error_detected() {
	if (error == pci_channel_io_frozen) {
		...
		pdsc_reset_prepare(pdev);
		if (!pdev->is_virtfn)
			cancel_work_sync(&pdsc->pci_reset_work);
		return PCI_ERS_RESULT_NEED_RESET;
	}
}

For a PF this unmaps the bars, releases the regions and disables the
device, and pdsc_fw_down() leaves PDSC_S_FW_DEAD set.

pcie_do_recovery() then does reset_subordinates(), which for AER goes
pci_bus_error_reset() -> pci_reset_bridge() -> pci_bus_reset() or
pci_slot_reset().  None of those call pci_dev_save_and_disable(), so no
reset_prepare/reset_done pair happens there, and pdsc_err_handler has no
.slot_reset, so report_slot_reset() is a no-op for this driver.  Recovery
proceeds to report_resume():

drivers/net/ethernet/amd/pds_core/main.c:pdsc_pci_error_resume() {
	if (test_bit(PDSC_S_FW_DEAD, &pdsc->state))
		pci_reset_function_locked(pdev);
}

PDSC_S_FW_DEAD is still set here, since pdsc_fw_up() only runs from
pdsc_reset_done(), which has not run yet.  pci_reset_function_locked()
calls pci_dev_save_and_disable():

drivers/pci/pci.c:pci_dev_save_and_disable() {
	...
	if (err_handler && err_handler->reset_prepare)
		err_handler->reset_prepare(dev);
}

so pdsc_reset_prepare() is entered a second time on a PF whose regions are
already released, and pci_release_regions() -> release_mem_region() ->
__release_region() prints the same message the commit sets out to remove.

Every other teardown step in pdsc_reset_prepare() already tolerates a
double invocation, which suggests re-entry is an expected condition:

drivers/net/ethernet/amd/pds_core/core.c:pdsc_fw_down() {
	if (test_and_set_bit(PDSC_S_FW_DEAD, &pdsc->state)) {
		dev_warn(pdsc->dev, "%s: already happening\n", __func__);
		return;
	}
}

pdsc_unmap_bars() skips NULL vaddrs, and the disable is guarded:

	if (pci_is_enabled(pdev))
		pci_disable_device(pdev);

Only pci_release_regions() has no equivalent ownership tracking.

There also seems to be a second unbalanced-release path on the PF.
pdsc_reset_done() returns early when pci_enable_device(),
pci_request_regions() or pdsc_map_bars() fail:

drivers/net/ethernet/amd/pds_core/main.c:pdsc_reset_done() {
	...
		err = pci_request_regions(pdsc->pdev, PDS_CORE_DRV_NAME);
		if (err) {
			dev_err(pdsc->dev, "Cannot request PCI regions: %pe\n",
				ERR_PTR(err));
			return;
		}
	...
}

After such a bail-out the PF holds no regions, but pdsc_remove() still
does:

		pdsc_unmap_bars(pdsc);
		pci_release_regions(pdev);

as does the next pdsc_reset_prepare().  Would tracking region ownership in
struct pdsc, for example a flag set on a successful pci_request_regions()
and cleared on release, cover both the VF case and these PF cases instead
of keying the release off pdev->is_virtfn?

>  	if (pci_is_enabled(pdev))
>  		pci_disable_device(pdev);
>  	pdsc_deferred_dma_free(pdsc);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901044219.1361466-1-nikhil.rao%40amd.com