octeon_pci_flr() calls __pci_reset_function_locked() from probe-failure and
remove paths that already hold the endpoint device lock. Its explicit
config-space lock covers only the endpoint. If the reset uses the bus-reset
method, the PCI core writes the upstream bridge's Bridge Control register.
Without the bridge lock, that access can race with other
configuration access and emit the "unlocked secondary bus reset" warning.

Take the upstream bridge configuration access lock before the endpoint
lock, and hold both locks through pci_restore_state(). This serializes the
complete reset and state-restore sequence with PCI configuration access.

The PatchProof static-analysis tool identified this issue; manual source
inspection confirmed it in v7.1.5 and current mainline.

A source-level check found that the original reset path takes the endpoint
configuration lock without first taking the upstream bridge lock. The
patched source was checked for bridge-first acquisition, restoration while
both locks are held, and reverse-order release. A user-space POSIX-thread
model held the bridge lock in a concurrent configuration accessor. The
original reset proceeded anyway; the fixed reset waited until the accessor
released it. No live Liquidio hardware or PCI lockdep test was run.

Fixes: 70535350e26f ("liquidio: with embedded f/w, don't reload f/w, issue pf flr at exit")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/net/ethernet/cavium/liquidio/lio_main.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cavium/liquidio/lio_main.c b/drivers/net/ethernet/cavium/liquidio/lio_main.c
index 32dd9b25760e..1d566aec2d75 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_main.c
@@ -914,12 +914,15 @@ static bool fw_type_is_auto(void)
  */
 static void octeon_pci_flr(struct octeon_device *oct)
 {
+	struct pci_dev *bridge = pci_upstream_bridge(oct->pci_dev);
 	int rc;
 
-	pci_save_state(oct->pci_dev);
-
+	if (bridge)
+		pci_cfg_access_lock(bridge);
 	pci_cfg_access_lock(oct->pci_dev);
 
+	pci_save_state(oct->pci_dev);
+
 	/* Quiesce the device completely */
 	pci_write_config_word(oct->pci_dev, PCI_COMMAND,
 			      PCI_COMMAND_INTX_DISABLE);
@@ -931,6 +934,7 @@ static void octeon_pci_flr(struct octeon_device *oct)
 			rc, oct->pf_num);
 
+	pci_restore_state(oct->pci_dev);
 	pci_cfg_access_unlock(oct->pci_dev);
+	if (bridge)
+		pci_cfg_access_unlock(bridge);
-
-	pci_restore_state(oct->pci_dev);
 }
-- 
2.34.1

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260809105715.3669436-1-runyu.xiao@seu.edu.cn?part=1
---
--- Patch [1]: [PATCH net] net: liquidio: lock upstream bridge for function reset ---
commit 2860fd8a604e5a859143abde28004c138ba9f2fa
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

net: liquidio: lock upstream bridge for function reset

This adds a pci_cfg_access_lock() on the upstream bridge around the
__pci_reset_function_locked() call in octeon_pci_flr(), and moves
pci_save_state()/pci_restore_state() inside the locked region so both
locks are held across the reset and the state restore.

> diff --git a/drivers/net/ethernet/cavium/liquidio/lio_main.c b/drivers/net/ethernet/cavium/liquidio/lio_main.c
> index e303956b4bf1..9aab310ba229 100644
> --- a/drivers/net/ethernet/cavium/liquidio/lio_main.c
> +++ b/drivers/net/ethernet/cavium/liquidio/lio_main.c
> @@ -914,12 +914,15 @@ static bool fw_type_is_auto(void)
>   */
>  static void octeon_pci_flr(struct octeon_device *oct)
>  {

[Severity: Low]
[Finding: gpt-5-6-sol-2-3]
[Sources: gpt-5-6-sol, opus-5]
This isn't a bug introduced by this patch, but the kernel-doc directly
above octeon_pci_flr() still says "PCI FLR for each Octeon device" while
the body calls the generic helper, which can pick any of the methods in
dev->reset_methods:

drivers/pci/pci.c:__pci_reset_function_locked() {
	for (i = 0; i < PCI_NUM_RESET_METHODS; i++) {
		m = dev->reset_methods[i];
		...
		rc = method->reset_fn(dev, PCI_RESET_DO_RESET);

and one of those methods ends in a secondary bus reset:

drivers/pci/pci.c:pci_reset_bus_function() {
	rc = pci_dev_reset_slot_function(dev, probe);
	if (rc != -ENOTTY)
		goto done;

	rc = pci_parent_bus_reset(dev, probe);

Since the changelog's premise is exactly that the bus-reset method may be
selected, would it make sense to update the doc string in the same patch?

> +	struct pci_dev *bridge = pci_upstream_bridge(oct->pci_dev);
>  	int rc;
>  
> -	pci_save_state(oct->pci_dev);
> -
> +	if (bridge)
> +		pci_cfg_access_lock(bridge);
>  	pci_cfg_access_lock(oct->pci_dev);

[Severity: High]
[Finding: gpt-5-6-sol-5-8]
[Sources: gpt-5-6-sol, opus-5]
Can this acquisition order deadlock against the PCI core?

octeon_pci_flr() is only reached from octeon_destroy_resources():

	if (atomic_read(oct->adapter_fw_state) == FW_IS_PRELOADED)
		octeon_pci_flr(oct);

which runs from liquidio_remove() and from the liquidio_remove(pdev)
calls on the probe-failure paths in liquidio_probe(). Both of those run
with device_lock(&pdev->dev) already held by the driver core, which is
why __pci_reset_function_locked() can be used at all:

drivers/pci/pci.c:__pci_reset_function_locked() {
	might_sleep();
	device_lock_assert(&dev->dev);

So after this change the driver holds device_lock(endpoint) and then takes
the bridge config-access lock. The core takes those two objects in the
opposite order:

drivers/pci/pci.c:pci_reset_function() {
	bridge = pci_upstream_bridge(dev);
	if (bridge)
		pci_dev_lock(bridge);

	pci_dev_lock(dev);

drivers/pci/pci.c:pci_dev_lock() {
	/* block PM suspend, driver probe, etc. */
	device_lock(&dev->dev);
	pci_cfg_access_lock(dev);

__pci_bus_lock() does the same bridge-first walk before locking each
child.

That gives an AB-BA pattern:

CPU0 (liquidio remove or probe failure)
  device_lock(endpoint)             [held by driver core]
  octeon_pci_flr()
    pci_cfg_access_lock(bridge)     -> blocks in pci_wait_cfg()

CPU1 (echo 1 > /sys/bus/pci/devices/.../reset, or AER/DPC recovery via
      pci_bus_error_reset() -> pci_bus_reset() -> pci_bus_lock())
  pci_dev_lock(bridge)              [sets bridge->block_cfg_access]
  pci_dev_lock(endpoint)
    device_lock(endpoint)           -> blocks, held by CPU0

Both waits look uninterruptible and untimed:

drivers/pci/access.c:pci_cfg_access_lock() {
	might_sleep();

	raw_spin_lock_irq(&pci_lock);
	if (dev->block_cfg_access)
		pci_wait_cfg(dev);

Since block_cfg_access is not lockdep-tracked, would this show up only as
a hung task, with the bridge's config-access flag left blocked?

Before this change the driver took only the endpoint's own config-access
lock, which the core also acquires after device_lock(endpoint), so there
was no inversion. Would deferring the bridge lock to a path that does not
already hold the endpoint device lock, or using the core's
pci_reset_function() style bridge-first sequence, avoid the cycle?

>  
> +	pci_save_state(oct->pci_dev);
> +
>  	/* Quiesce the device completely */
>  	pci_write_config_word(oct->pci_dev, PCI_COMMAND,
>  			      PCI_COMMAND_INTX_DISABLE);
> @@ -930,9 +933,10 @@ static void octeon_pci_flr(struct octeon_device *oct)
>  		dev_err(&oct->pci_dev->dev, "Error %d resetting PCI function %d\n",
>  			rc, oct->pf_num);
>  
> -	pci_cfg_access_unlock(oct->pci_dev);
> -
>  	pci_restore_state(oct->pci_dev);
> +	pci_cfg_access_unlock(oct->pci_dev);
> +	if (bridge)
> +		pci_cfg_access_unlock(bridge);
>  }

[ ... ]

Hi,

Thanks for the detailed review.

> This isn't a bug introduced by this patch, but the kernel-doc directly
> above octeon_pci_flr() still says "PCI FLR" while the body calls the
> generic reset helper. Would it make sense to update the doc string?

The documentation issue is valid and predates this patch. However, it is
secondary to the locking problem. Since the current locking approach is
unsafe, I will not update the comment in this patch.

> Can this acquisition order deadlock against the PCI core?

Yes, this concern is valid. octeon_pci_flr() is called from the remove
and probe-failure paths while the endpoint device lock is already held.
Taking the upstream bridge configuration lock from there can conflict
with the bridge-first locking order used by the PCI core.

> Since block_cfg_access is not lockdep-tracked, would this show up only
> as a hung task, with the bridge's configuration access flag left
> blocked?

That is possible. The wait in pci_cfg_access_lock() is not represented
as a normal lockdep dependency, so this ordering problem may appear as a
hung task rather than a lockdep report.

> Before this change the driver took only the endpoint's own
> configuration lock, which the core also acquires after
> device_lock(endpoint), so there was no inversion.

I agree that the new bridge-lock acquisition introduces this additional
inversion. The original missing bridge serialization is a separate
problem, but adding the bridge lock at this point is not a safe fix for
it.

> Would deferring the bridge lock to a path that does not already hold
> the endpoint device lock, or using the core's pci_reset_function()
> style bridge-first sequence, avoid the cycle?

Yes, the reset must be redesigned around the PCI core locking order.
Taking the bridge lock from the current callback is unsafe because the
endpoint device lock is already held. Simply switching to
pci_reset_function() would not solve the problem because that helper
also tries to acquire the endpoint device lock.

There is also a more direct issue with the current approach. If
__pci_reset_function_locked() selects the bus-reset method,
pci_bus_lock() locks the upstream bridge and then tries to lock the
endpoint again. The endpoint lock is already held by the caller in this
path.

The existing user-space model did not model the endpoint device lock
being held before the reset. It therefore does not validate the
submitted patch and should be discarded.

I will withdraw this driver-only approach and will not send a revised
version using the same locking sequence.

Regards,
Runyu Xiao