[PATCH net v3 0/3] pds_core: fixes for the PCI reset path

[PATCH net v3 0/3] pds_core: fixes for the PCI reset path

[Nikhil P. Rao]

The v2 review raised several issues beyond the cmd_regs race, so v3 is a
series rather than a single patch.

Patch 1 is the v2 patch with the pdsc_core_init() and
pdsc_identify_ver() checks removed. Those checks are dead code. commit
cd09971dcc1c ("pds_core: keep the health thread stopped during reset")
disables health_work across the reset, so the health thread can no
longer reach pdsc_setup() with the BARs unmapped. The only other callers
are probe and pdsc_reset_done(), and both map the BARs earlier in the
same call, so cmd_regs cannot be NULL by the time they get there.

The pdsc_core_init() check is also worse than what it replaces. Its
bail-out jumps to err_out_uninit, which ends up in pdsc_intr_free() and
writes to pdsc->intr_ctrl, also NULL at that point.

On v2 I said I would convert pdsc_identify() and pdsc_core_init() to
pdsc_devcmd_with_data() once the PLDM series landed. Dropping that: the
helper has no read-back path and both callers need one, and giving them
an -ENXIO return means hardening pdsc_intr_free() against a NULL
intr_ctrl on the err_out_uninit path. That is a lot of churn to
deduplicate two call sites.

Patch 2 is the VF pci_release_regions() fix, older than the cmd_regs
race, so it carries its own Fixes tag.

Patch 3 is the identity_show() NULL deref.

The v2 changelog claim that pdsc_unmap_bars() clears db_pages was wrong.
It clears info_regs, cmd_regs, intr_status and intr_ctrl; db_pages is
never mapped.

v2: https://lore.kernel.org/netdev/20260804235946.177762-1-nikhil.rao@amd.com/
v1: https://lore.kernel.org/netdev/20260729055258.1416225-1-nikhil.rao@amd.com/

Nikhil P. Rao (3):
  pds_core: fix cmd_regs access racing BAR unmap on reset
  pds_core: don't release PCI regions for VFs on reset
  pds_core: check info_regs in the identity debugfs reader

 drivers/net/ethernet/amd/pds_core/debugfs.c |  5 +++--
 drivers/net/ethernet/amd/pds_core/fw.c      | 10 +++++++++-
 drivers/net/ethernet/amd/pds_core/main.c    | 10 ++++++++--
 3 files changed, 20 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH net v3 1/3] pds_core: fix cmd_regs access racing BAR unmap on reset

[Nikhil P. Rao]

pdsc_reset_prepare() and pdsc_reset_done()'s pdsc_map_bars() error path
clear/iounmap cmd_regs without devcmd_lock, and
pdsc_legacy_firmware_update()'s download loop derefs cmd_regs after
dropping and retaking the lock without re-checking. An FLR concurrent
with a devlink flash can unmap cmd_regs under an in-flight devcmd,
causing a NULL deref or a write to unmapped MMIO.

Take devcmd_lock across the BAR unmap/remap, and re-check cmd_regs in
the download loop. Only the PF maps cmd_regs and runs devcmd, so skip
the unmap on a VF, as pdsc_remove() and pdsc_reset_done() already do.

A reset that completes entirely within the unlocked window is not a
correctness problem for the image: the device clears its update session,
so a resumed download is rejected, and it verifies the staged image
before writing a flash slot, reporting PDS_RC_BAD_FW rather than
activating it.

pdsc_unmap_bars() also clears info_regs, intr_status and intr_ctrl. The
interrupt and start/stop readers of those are quiesced before the unmap
by pdsc_fw_down(), which frees the interrupts and tears down the queues.
The debugfs readers are not, since those files outlive a reset; that is
pre-existing and out of scope here.

Fixes: e96094c1d11c ("pds_core: Clear BARs on reset")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260708212222.296202-1-nikhil.rao%40amd.com?part=3
Assisted-by: Claude:claude-opus-4.8
Signed-off-by: Nikhil P. Rao <nikhil.rao@amd.com>
---
 drivers/net/ethernet/amd/pds_core/fw.c   | 10 +++++++++-
 drivers/net/ethernet/amd/pds_core/main.c |  8 +++++++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/fw.c b/drivers/net/ethernet/amd/pds_core/fw.c
index 5ccf017f6af4..19899bf38d40 100644
--- a/drivers/net/ethernet/amd/pds_core/fw.c
+++ b/drivers/net/ethernet/amd/pds_core/fw.c
@@ -171,8 +171,10 @@ pdsc_legacy_firmware_update(struct pdsc *pdsc,
 
 	dev_info(pdsc->dev, "Installing firmware\n");
 
-	if (!pdsc->cmd_regs)
+	if (!pdsc->cmd_regs) {
+		NL_SET_ERR_MSG_MOD(extack, "BARs not mapped");
 		return -ENXIO;
+	}
 
 	dl = priv_to_devlink(pdsc);
 	devlink_flash_update_status_notify(dl, "Preparing to flash",
@@ -198,6 +200,12 @@ pdsc_legacy_firmware_update(struct pdsc *pdsc,
 
 		copy_sz = min_t(unsigned int, buf_sz, fw->size - offset);
 		mutex_lock(&pdsc->devcmd_lock);
+		if (!pdsc->cmd_regs) {
+			mutex_unlock(&pdsc->devcmd_lock);
+			err = -ENXIO;
+			NL_SET_ERR_MSG_MOD(extack, "Device reset during flash");
+			goto err_out;
+		}
 		memcpy_toio(&pdsc->cmd_regs->data, fw->data + offset, copy_sz);
 		err = pdsc_devcmd_fw_download_locked(pdsc, data_addr,
 						     offset, copy_sz);
diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
index bb79e7476370..6e1079f9ba0f 100644
--- a/drivers/net/ethernet/amd/pds_core/main.c
+++ b/drivers/net/ethernet/amd/pds_core/main.c
@@ -513,7 +513,11 @@ static void pdsc_reset_prepare(struct pci_dev *pdev)
 		pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
 	}
 
-	pdsc_unmap_bars(pdsc);
+	if (!pdev->is_virtfn) {
+		mutex_lock(&pdsc->devcmd_lock);
+		pdsc_unmap_bars(pdsc);
+		mutex_unlock(&pdsc->devcmd_lock);
+	}
 	pci_release_regions(pdev);
 	if (pci_is_enabled(pdev))
 		pci_disable_device(pdev);
@@ -543,7 +547,9 @@ static void pdsc_reset_done(struct pci_dev *pdev)
 			return;
 		}
 
+		mutex_lock(&pdsc->devcmd_lock);
 		err = pdsc_map_bars(pdsc);
+		mutex_unlock(&pdsc->devcmd_lock);
 		if (err)
 			return;
 	}
-- 
2.43.0

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

[Nikhil P. Rao]

pdsc_reset_prepare() called pci_release_regions() unconditionally, but
only PFs call pci_request_regions() (pdsc_init_pf). On a VF FLR this
makes the kernel warn "Trying to free nonexistent resource".

Fixes: ffa55858330f ("pds_core: implement pci reset handlers")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804235946.177762-1-nikhil.rao%40amd.com
Assisted-by: Claude:claude-opus-4.8
Signed-off-by: Nikhil P. Rao <nikhil.rao@amd.com>
---
 drivers/net/ethernet/amd/pds_core/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
index 6e1079f9ba0f..a971c66d36f9 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);
 	if (pci_is_enabled(pdev))
 		pci_disable_device(pdev);
 	pdsc_deferred_dma_free(pdsc);
-- 
2.43.0

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

[netdev-bot+sashiko]

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

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

[Jakub Kicinski]

On Sat, 05 Sep 2026 01:06:50 +0000 netdev-bot+sashiko@kernel.org wrote:
> 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?

seemed a bit orthogonal so I applied this patch as is

[PATCH net v3 3/3] pds_core: check info_regs in the identity debugfs reader

[Nikhil P. Rao]

identity_show() reads fw_heartbeat from info_regs with no NULL check,
unlike every other reader of that register. pdsc_unmap_bars() sets
info_regs to NULL on reset and on remove, and the identity file survives
a reset.

Fixes: e96094c1d11c ("pds_core: Clear BARs on reset")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804235946.177762-1-nikhil.rao%40amd.com
Assisted-by: Claude:claude-opus-4.8
Signed-off-by: Nikhil P. Rao <nikhil.rao@amd.com>
---
 drivers/net/ethernet/amd/pds_core/debugfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/debugfs.c b/drivers/net/ethernet/amd/pds_core/debugfs.c
index ef0a1b7d159b..c458e6758959 100644
--- a/drivers/net/ethernet/amd/pds_core/debugfs.c
+++ b/drivers/net/ethernet/amd/pds_core/debugfs.c
@@ -38,8 +38,9 @@ static int identity_show(struct seq_file *seq, void *v)
 
 	ident = &pdsc->dev_ident;
 
-	seq_printf(seq, "fw_heartbeat:     0x%x\n",
-		   ioread32(&pdsc->info_regs->fw_heartbeat));
+	if (pdsc->info_regs)
+		seq_printf(seq, "fw_heartbeat:     0x%x\n",
+			   ioread32(&pdsc->info_regs->fw_heartbeat));
 
 	seq_printf(seq, "nlifs:            %d\n",
 		   le32_to_cpu(ident->nlifs));
-- 
2.43.0

Re: [PATCH net v3 3/3] pds_core: check info_regs in the identity debugfs reader

[netdev-bot+sashiko]

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

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

- [Medium] The newly added guard in `identity_show()` is an
  unsynchronized check-then-use of an `__iomem` pointer that a…

Pre-existing issues:
- [High] NULL-pointer dereference in the sibling debugfs reader
  `viftype_show()` (drivers/net/ethernet/amd/pds_core/debugfs.c:81-96).

--

pds_core: check info_regs in the identity debugfs reader

This adds a NULL test around the fw_heartbeat read in identity_show() so
the identity debugfs file does not dereference pdsc->info_regs after
pdsc_unmap_bars() has cleared it. The identity file is deliberately kept
across a reset, so the reader can run while the BARs are unmapped.

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/debugfs.c b/drivers/net/ethernet/amd/pds_core/debugfs.c
> index ef0a1b7d159b5..c458e6758959a 100644
> --- a/drivers/net/ethernet/amd/pds_core/debugfs.c
> +++ b/drivers/net/ethernet/amd/pds_core/debugfs.c
> @@ -38,8 +38,9 @@ static int identity_show(struct seq_file *seq, void *v)
>  
>  	ident = &pdsc->dev_ident;
>  
> -	seq_printf(seq, "fw_heartbeat:     0x%x\n",
> -		   ioread32(&pdsc->info_regs->fw_heartbeat));
> +	if (pdsc->info_regs)
> +		seq_printf(seq, "fw_heartbeat:     0x%x\n",
> +			   ioread32(&pdsc->info_regs->fw_heartbeat));

[Severity: Medium]
Is the guard enough when the unmap happens concurrently with the read?
The test and the use are two separate plain loads of pdsc->info_regs,
and identity_show() holds none of the driver's locks:

	if (pdsc->info_regs)
		seq_printf(seq, "fw_heartbeat:     0x%x\n",
			   ioread32(&pdsc->info_regs->fw_heartbeat));

Meanwhile pdsc_unmap_bars() in main.c clears the pointer and then drops
the mapping:

	pdsc->info_regs = NULL;
	pdsc->cmd_regs = NULL;
	...
		if (bars[i].vaddr)
			pci_iounmap(pdsc->pdev, bars[i].vaddr);

So can the second load still observe NULL and make ioread32() fault at
offsetof(struct pds_core_dev_info_regs, fw_heartbeat)?  And even if the
compiler keeps the first value, can pci_iounmap() have already torn the
mapping down, leaving the ioread32() touching an unmapped ioremap range?

The other info_regs readers appear to be serialized against the unmap:
pdsc_check_pci_health() and pdsc_is_fw_running() run from the health work
under config_lock, which pdsc_reset_prepare() stops first, and the adminq
readers are gated by PDSC_S_FW_DEAD plus the adminq_refcnt drain in
pdsc_fw_down().  pdsc_reset_prepare() serializes the unmap only with
devcmd_lock:

	mutex_lock(&pdsc->devcmd_lock);
	pdsc_unmap_bars(pdsc);
	mutex_unlock(&pdsc->devcmd_lock);
	pci_release_regions(pdev);

which identity_show() never takes.  In pdsc_remove() the ordering looks
worse, since the lock is gone before the unmap and the debugfs consumer of
the mapping is only removed afterwards:

	mutex_destroy(&pdsc->devcmd_lock);
	...
	pdsc_unmap_bars(pdsc);
	pci_release_regions(pdev);
	...
	pdsc_debugfs_del_dev(pdsc);

Would snapshotting the pointer into a local plus serializing against
map/unmap (or removing the debugfs entries before unmapping the BARs) close
this properly?

[Severity: High]
This isn't a bug introduced by this patch, but the sibling reader in the
same file, viftype_show(), has the same shape of problem and no NULL test
at all:

	for (vt = 0; vt < PDS_DEV_TYPE_MAX; vt++) {
		if (!pdsc->viftype_status[vt].name)
			continue;

pdsc_teardown() with PDSC_TEARDOWN_REMOVING frees and clears the base
pointer:

	if (removing) {
		kfree(pdsc->viftype_status);
		pdsc->viftype_status = NULL;
	}

In pdsc_remove() that teardown runs well before pdsc_debugfs_del_dev(),
with destroy_workqueue(), two mutex_destroy() calls, pdsc_unmap_bars(),
pci_release_regions(), pci_disable_device(), pdsc_deferred_dma_free() and
ida_free() in between, so can a read of the still-registered viftypes file
dereference a NULL pdsc->viftype_status?  The same window looks present on
the pdsc_init_pf() error path, where err_out_teardown runs long before
pdsc_debugfs_del_dev().

The other readers of that member treat NULL as reachable, for example
pdsc_dl_find_viftype_by_id() in devlink.c:

	if (!pdsc->viftype_status)
		return NULL;

and host_mem_show() in this same file NULL-checks pdsc->host_mem_reqs.
Should viftype_show() get the same treatment while this area is being
touched?

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

Re: [PATCH net v3 3/3] pds_core: check info_regs in the identity debugfs reader

[Jakub Kicinski]

On Sat, 05 Sep 2026 01:06:51 +0000 netdev-bot+sashiko@kernel.org wrote:
> > -	seq_printf(seq, "fw_heartbeat:     0x%x\n",
> > -		   ioread32(&pdsc->info_regs->fw_heartbeat));
> > +	if (pdsc->info_regs)
> > +		seq_printf(seq, "fw_heartbeat:     0x%x\n",
> > +			   ioread32(&pdsc->info_regs->fw_heartbeat));  
> 
> [Severity: Medium]
> Is the guard enough when the unmap happens concurrently with the read?
> The test and the use are two separate plain loads of pdsc->info_regs,
> and identity_show() holds none of the driver's locks:

If the comment is correct the patch is pointless in its current form..

Re: [PATCH net v3 0/3] pds_core: fixes for the PCI reset path

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 1 Sep 2026 04:42:16 +0000 you wrote:
> The v2 review raised several issues beyond the cmd_regs race, so v3 is a
> series rather than a single patch.
> 
> Patch 1 is the v2 patch with the pdsc_core_init() and
> pdsc_identify_ver() checks removed. Those checks are dead code. commit
> cd09971dcc1c ("pds_core: keep the health thread stopped during reset")
> disables health_work across the reset, so the health thread can no
> longer reach pdsc_setup() with the BARs unmapped. The only other callers
> are probe and pdsc_reset_done(), and both map the BARs earlier in the
> same call, so cmd_regs cannot be NULL by the time they get there.
> 
> [...]

Here is the summary with links:
  - [net,v3,1/3] pds_core: fix cmd_regs access racing BAR unmap on reset
    https://git.kernel.org/netdev/net/c/7980325b2f71
  - [net,v3,2/3] pds_core: don't release PCI regions for VFs on reset
    https://git.kernel.org/netdev/net/c/73608de7e592
  - [net,v3,3/3] pds_core: check info_regs in the identity debugfs reader
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html