[PATCH 1/2] pci: fix slot trylock error handling

[PATCH 1/2] pci: fix slot trylock error handling

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The device lock isn't held if pci_bus_trylock() fails, so the code had
been attempting to improperly unlock it.

Fixes: a4e772898f8bf2 ("PCI: Add missing bridge lock to pci_bus_lock()")
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/pci/pci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 72d88ea95f3cc..3378221c5723a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5494,10 +5494,8 @@ static int pci_slot_trylock(struct pci_slot *slot)
 		if (!dev->slot || dev->slot != slot)
 			continue;
 		if (dev->subordinate) {
-			if (!pci_bus_trylock(dev->subordinate)) {
-				pci_dev_unlock(dev);
+			if (!pci_bus_trylock(dev->subordinate))
 				goto unlock;
-			}
 		} else if (!pci_dev_trylock(dev))
 			goto unlock;
 	}
-- 
2.47.3

[PATCH 2/2] pci: fix slot reset device locking

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Like pci_bus_lock, pci_slot_lock needs to lock the bridge device to
prevent the warning:

  pcieport 0000:e2:05.0: unlocked secondary bus reset via: pciehp_reset_slot+0x55/0xa0

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/pci/pci.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 3378221c5723a..5f8b0d06a1459 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5460,6 +5460,8 @@ static void pci_slot_lock(struct pci_slot *slot)
 {
 	struct pci_dev *dev;
 
+	if (slot->bus->self)
+		pci_dev_lock(slot->bus->self);
 	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
 		if (!dev->slot || dev->slot != slot)
 			continue;
@@ -5483,12 +5485,17 @@ static void pci_slot_unlock(struct pci_slot *slot)
 		else
 			pci_dev_unlock(dev);
 	}
+	if (slot->bus->self)
+		pci_dev_unlock(slot->bus->self);
 }
 
 /* Return 1 on successful lock, 0 on contention */
 static int pci_slot_trylock(struct pci_slot *slot)
 {
-	struct pci_dev *dev;
+	struct pci_dev *dev, *bridge = slot->bus->self;
+
+	if (bridge && !pci_dev_trylock(bridge))
+		return 0;
 
 	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
 		if (!dev->slot || dev->slot != slot)
@@ -5511,6 +5518,9 @@ static int pci_slot_trylock(struct pci_slot *slot)
 		else
 			pci_dev_unlock(dev);
 	}
+
+	if (bridge)
+		pci_dev_unlock(bridge);
 	return 0;
 }
 
-- 
2.47.3

Re: [PATCH 2/2] pci: fix slot reset device locking

[Bjorn Helgaas]

[+cc Dan, Jinhui]

On Fri, Jan 16, 2026 at 10:41:50AM -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Like pci_bus_lock, pci_slot_lock needs to lock the bridge device to
> prevent the warning:

I *think* this actually refers to pci_bus_trylock() and
pci_slot_trylock() (not pci_bus_lock() and pci_slot_lock()), since
that's what this patch changes?

It's unfortunate that pci_bus_trylock() and pci_slot_trylock() are so
similar but separate.  If there were combined, this kind of issue
where one is fixed but the other isn't wouldn't happen.

But what about pci_bus_lock() and pci_slot_lock()?  They are also
almost identical, but pci_bus_lock() locks bus->self while
pci_slot_lock() does not.  Should it?

All these almost-but-not-quite identical paths make my head hurt ;)

>   pcieport 0000:e2:05.0: unlocked secondary bus reset via: pciehp_reset_slot+0x55/0xa0
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/pci/pci.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 3378221c5723a..5f8b0d06a1459 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5460,6 +5460,8 @@ static void pci_slot_lock(struct pci_slot *slot)
>  {
>  	struct pci_dev *dev;
>  
> +	if (slot->bus->self)
> +		pci_dev_lock(slot->bus->self);
>  	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
>  		if (!dev->slot || dev->slot != slot)
>  			continue;
> @@ -5483,12 +5485,17 @@ static void pci_slot_unlock(struct pci_slot *slot)
>  		else
>  			pci_dev_unlock(dev);
>  	}
> +	if (slot->bus->self)
> +		pci_dev_unlock(slot->bus->self);
>  }
>  
>  /* Return 1 on successful lock, 0 on contention */
>  static int pci_slot_trylock(struct pci_slot *slot)
>  {
> -	struct pci_dev *dev;
> +	struct pci_dev *dev, *bridge = slot->bus->self;
> +
> +	if (bridge && !pci_dev_trylock(bridge))
> +		return 0;
>  
>  	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
>  		if (!dev->slot || dev->slot != slot)
> @@ -5511,6 +5518,9 @@ static int pci_slot_trylock(struct pci_slot *slot)
>  		else
>  			pci_dev_unlock(dev);
>  	}
> +
> +	if (bridge)
> +		pci_dev_unlock(bridge);
>  	return 0;
>  }
>  
> -- 
> 2.47.3
> 

Re: [PATCH 2/2] pci: fix slot reset device locking

[Keith Busch]

On Wed, Jan 28, 2026 at 12:03:38PM -0600, Bjorn Helgaas wrote:
> [+cc Dan, Jinhui]
> 
> On Fri, Jan 16, 2026 at 10:41:50AM -0800, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > Like pci_bus_lock, pci_slot_lock needs to lock the bridge device to
> > prevent the warning:
> 
> I *think* this actually refers to pci_bus_trylock() and
> pci_slot_trylock() (not pci_bus_lock() and pci_slot_lock()), since
> that's what this patch changes?

Oh, this patch is changing both pci_slot_trylock and pci_slot_lock since
they were both missing the equivalent locks that the "bus" version of
those functions were doing.

> It's unfortunate that pci_bus_trylock() and pci_slot_trylock() are so
> similar but separate.  If there were combined, this kind of issue
> where one is fixed but the other isn't wouldn't happen.

Honestly I think the _slot versions should go away. Those don't handle
resetting a bus with multiple device's on it: only some functions get
saved and restored even though the bus reset hits all the devices. I'm
working on a fix for that, but it's more difficult than these patches.

> But what about pci_bus_lock() and pci_slot_lock()?  They are also
> almost identical, but pci_bus_lock() locks bus->self while
> pci_slot_lock() does not.  Should it?

It should, and this patch is changing pci_slot_lock() to do that.

> All these almost-but-not-quite identical paths make my head hurt ;)

I agree! And the functions that sound almost the same but work quite
different? Looking at "pci_bus_reset" vs "pci_reset_bus" :)
 
> >   pcieport 0000:e2:05.0: unlocked secondary bus reset via: pciehp_reset_slot+0x55/0xa0
> > 
> > Signed-off-by: Keith Busch <kbusch@kernel.org>
> > ---
> >  drivers/pci/pci.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 3378221c5723a..5f8b0d06a1459 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -5460,6 +5460,8 @@ static void pci_slot_lock(struct pci_slot *slot)
> >  {
> >  	struct pci_dev *dev;
> >  
> > +	if (slot->bus->self)
> > +		pci_dev_lock(slot->bus->self);
> >  	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> >  		if (!dev->slot || dev->slot != slot)
> >  			continue;
> > @@ -5483,12 +5485,17 @@ static void pci_slot_unlock(struct pci_slot *slot)
> >  		else
> >  			pci_dev_unlock(dev);
> >  	}
> > +	if (slot->bus->self)
> > +		pci_dev_unlock(slot->bus->self);
> >  }
> >  
> >  /* Return 1 on successful lock, 0 on contention */
> >  static int pci_slot_trylock(struct pci_slot *slot)
> >  {
> > -	struct pci_dev *dev;
> > +	struct pci_dev *dev, *bridge = slot->bus->self;
> > +
> > +	if (bridge && !pci_dev_trylock(bridge))
> > +		return 0;
> >  
> >  	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> >  		if (!dev->slot || dev->slot != slot)
> > @@ -5511,6 +5518,9 @@ static int pci_slot_trylock(struct pci_slot *slot)
> >  		else
> >  			pci_dev_unlock(dev);
> >  	}
> > +
> > +	if (bridge)
> > +		pci_dev_unlock(bridge);
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.47.3
> > 

Re: [PATCH 2/2] pci: fix slot reset device locking

[Bjorn Helgaas]

On Wed, Jan 28, 2026 at 12:13:20PM -0700, Keith Busch wrote:
> On Wed, Jan 28, 2026 at 12:03:38PM -0600, Bjorn Helgaas wrote:
> > On Fri, Jan 16, 2026 at 10:41:50AM -0800, Keith Busch wrote:
> > > From: Keith Busch <kbusch@kernel.org>
> > > 
> > > Like pci_bus_lock, pci_slot_lock needs to lock the bridge device to
> > > prevent the warning:
> > 
> > I *think* this actually refers to pci_bus_trylock() and
> > pci_slot_trylock() (not pci_bus_lock() and pci_slot_lock()), since
> > that's what this patch changes?
> 
> Oh, this patch is changing both pci_slot_trylock and pci_slot_lock since
> they were both missing the equivalent locks that the "bus" version of
> those functions were doing.

Wow. The patch has hunks for:

  pci_slot_lock()               # lock bridge
  pci_slot_unlock()             # unlock bridge
  pci_slot_trylock()            # lock bridge

which makes perfect sense.

But when I apply it on either pci/main (v6.19-rc1) or pci/next, "git
am" puts those hunks in:

  pci_slot_unlock()             # unlock bridge (as expected)
  pci_slot_trylock()            # lock bridge (as expected)
  pci_slot_restore_locked()     # lock bridge (completely wrong spot)

I have no idea why the pci_slot_lock() hunk got applied in
pci_slot_restore_locked().  I do notice that the line offsets in your
patch are much different than mine, so you must have other changes in
pci.c; maybe that accounts for the confusion.

Can you regenerate this patch based on v6.19-rc1?  And maybe
incorporate Dan's "bridge = slot->bus->self" idea at the same time?

I'll attach my "git log -p" at the bottom if you want to see why I was
so confused.  What a day ;)

> > It's unfortunate that pci_bus_trylock() and pci_slot_trylock() are so
> > similar but separate.  If there were combined, this kind of issue
> > where one is fixed but the other isn't wouldn't happen.
> 
> Honestly I think the _slot versions should go away. Those don't handle
> resetting a bus with multiple device's on it: only some functions get
> saved and restored even though the bus reset hits all the devices. I'm
> working on a fix for that, but it's more difficult than these patches.
> 
> > But what about pci_bus_lock() and pci_slot_lock()?  They are also
> > almost identical, but pci_bus_lock() locks bus->self while
> > pci_slot_lock() does not.  Should it?
> 
> It should, and this patch is changing pci_slot_lock() to do that.
> 
> > All these almost-but-not-quite identical paths make my head hurt ;)
> 
> I agree! And the functions that sound almost the same but work quite
> different? Looking at "pci_bus_reset" vs "pci_reset_bus" :)
>  
> > >   pcieport 0000:e2:05.0: unlocked secondary bus reset via: pciehp_reset_slot+0x55/0xa0
> > > 
> > > Signed-off-by: Keith Busch <kbusch@kernel.org>
> > > ---
> > >  drivers/pci/pci.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 3378221c5723a..5f8b0d06a1459 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -5460,6 +5460,8 @@ static void pci_slot_lock(struct pci_slot *slot)
> > >  {
> > >  	struct pci_dev *dev;
> > >  
> > > +	if (slot->bus->self)
> > > +		pci_dev_lock(slot->bus->self);
> > >  	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> > >  		if (!dev->slot || dev->slot != slot)
> > >  			continue;
> > > @@ -5483,12 +5485,17 @@ static void pci_slot_unlock(struct pci_slot *slot)
> > >  		else
> > >  			pci_dev_unlock(dev);
> > >  	}
> > > +	if (slot->bus->self)
> > > +		pci_dev_unlock(slot->bus->self);
> > >  }
> > >  
> > >  /* Return 1 on successful lock, 0 on contention */
> > >  static int pci_slot_trylock(struct pci_slot *slot)
> > >  {
> > > -	struct pci_dev *dev;
> > > +	struct pci_dev *dev, *bridge = slot->bus->self;
> > > +
> > > +	if (bridge && !pci_dev_trylock(bridge))
> > > +		return 0;
> > >  
> > >  	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> > >  		if (!dev->slot || dev->slot != slot)
> > > @@ -5511,6 +5518,9 @@ static int pci_slot_trylock(struct pci_slot *slot)
> > >  		else
> > >  			pci_dev_unlock(dev);
> > >  	}
> > > +
> > > +	if (bridge)
> > > +		pci_dev_unlock(bridge);
> > >  	return 0;
> > >  }
> > >  
> > > -- 
> > > 2.47.3


$ git log -p v6.19-rc1.. | cat
commit 44c651ea87f9 ("pci: fix slot reset device locking")
Author: Keith Busch <kbusch@kernel.org>
Date:   Fri Jan 16 10:41:50 2026 -0800

    pci: fix slot reset device locking
    
    Like pci_bus_lock, pci_slot_lock needs to lock the bridge device to
    prevent the warning:
    
      pcieport 0000:e2:05.0: unlocked secondary bus reset via: pciehp_reset_slot+0x55/0xa0
    
    Signed-off-by: Keith Busch <kbusch@kernel.org>
    Reviewed-by: Dan Williams <dan.j.williams@intel.com>
    Link: https://patch.msgid.link/20260116184150.3013258-2-kbusch@meta.com
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>


diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 59319e08fca6..73764e66cabd 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5335,12 +5335,17 @@ static void pci_slot_unlock(struct pci_slot *slot)
 		else
 			pci_dev_unlock(dev);
 	}
+	if (slot->bus->self)
+		pci_dev_unlock(slot->bus->self);
 }
 
 /* Return 1 on successful lock, 0 on contention */
 static int pci_slot_trylock(struct pci_slot *slot)
 {
-	struct pci_dev *dev;
+	struct pci_dev *dev, *bridge = slot->bus->self;
+
+	if (bridge && !pci_dev_trylock(bridge))
+		return 0;
 
 	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
 		if (!dev->slot || dev->slot != slot)
@@ -5363,6 +5368,9 @@ static int pci_slot_trylock(struct pci_slot *slot)
 		else
 			pci_dev_unlock(dev);
 	}
+
+	if (bridge)
+		pci_dev_unlock(bridge);
 	return 0;
 }
 
@@ -5425,6 +5433,8 @@ static void pci_slot_restore_locked(struct pci_slot *slot)
 {
 	struct pci_dev *dev;
 
+	if (slot->bus->self)
+		pci_dev_lock(slot->bus->self);
 	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
 		if (!dev->slot || dev->slot != slot)
 			continue;
commit 3f2aea31058a ("pci: fix slot trylock error handling")
Author: Keith Busch <kbusch@kernel.org>
Date:   Fri Jan 16 10:41:49 2026 -0800

    pci: fix slot trylock error handling
    
    The device lock isn't held if pci_bus_trylock() fails, so the code had
    been attempting to improperly unlock it.
    
    Fixes: a4e772898f8bf2 ("PCI: Add missing bridge lock to pci_bus_lock()")
    Signed-off-by: Keith Busch <kbusch@kernel.org>
    Reviewed-by: Dan Williams <dan.j.williams@intel.com>
    Link: https://patch.msgid.link/20260116184150.3013258-1-kbusch@meta.com
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>


diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 13dbb405dc31..59319e08fca6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5346,10 +5346,8 @@ static int pci_slot_trylock(struct pci_slot *slot)
 		if (!dev->slot || dev->slot != slot)
 			continue;
 		if (dev->subordinate) {
-			if (!pci_bus_trylock(dev->subordinate)) {
-				pci_dev_unlock(dev);
+			if (!pci_bus_trylock(dev->subordinate))
 				goto unlock;
-			}
 		} else if (!pci_dev_trylock(dev))
 			goto unlock;
 	}

Re: [PATCH 2/2] pci: fix slot reset device locking

[Keith Busch]

On Wed, Jan 28, 2026 at 04:53:59PM -0600, Bjorn Helgaas wrote:
> 
> I have no idea why the pci_slot_lock() hunk got applied in
> pci_slot_restore_locked().  I do notice that the line offsets in your
> patch are much different than mine, so you must have other changes in
> pci.c; maybe that accounts for the confusion.

Oh weird. I'd hope that patch just wouldn't have applied in that case
instead of appearing successful.

I was using a newer branch that also included some other fixes I'm
working on, so not surprised the line numbers didn't match up. I'll
rebase future patches to v6.19-rc1.
 
> Can you regenerate this patch based on v6.19-rc1?  And maybe
> incorporate Dan's "bridge = slot->bus->self" idea at the same time?

I've got a new proposal about ready that just drops the slot specific
lock/unlock and save/restore instead. We kind of need to do that since
slot reset breaks devices with more than 8 functions. I'll send the
patch with the details in a few minutes.

Re: [PATCH 2/2] pci: fix slot reset device locking

[Ilpo Järvinen]

On Wed, 28 Jan 2026, Bjorn Helgaas wrote:

> [+cc Dan, Jinhui]
> 
> On Fri, Jan 16, 2026 at 10:41:50AM -0800, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > Like pci_bus_lock, pci_slot_lock needs to lock the bridge device to
> > prevent the warning:
> 
> I *think* this actually refers to pci_bus_trylock() and
> pci_slot_trylock() (not pci_bus_lock() and pci_slot_lock()), since
> that's what this patch changes?
> 
> It's unfortunate that pci_bus_trylock() and pci_slot_trylock() are so
> similar but separate.  If there were combined, this kind of issue
> where one is fixed but the other isn't wouldn't happen.
> 
> But what about pci_bus_lock() and pci_slot_lock()?  They are also
> almost identical, but pci_bus_lock() locks bus->self while
> pci_slot_lock() does not.  Should it?
>
> All these almost-but-not-quite identical paths make my head hurt ;)

My series does attempt to consolidation them (check the pending patches 
from me).

(It doesn't consider this fix patch though but the other one is taken 
into account, the series was build on top of it).

-- 
 i.

> >   pcieport 0000:e2:05.0: unlocked secondary bus reset via: pciehp_reset_slot+0x55/0xa0
> > 
> > Signed-off-by: Keith Busch <kbusch@kernel.org>
> > ---
> >  drivers/pci/pci.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 3378221c5723a..5f8b0d06a1459 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -5460,6 +5460,8 @@ static void pci_slot_lock(struct pci_slot *slot)
> >  {
> >  	struct pci_dev *dev;
> >  
> > +	if (slot->bus->self)
> > +		pci_dev_lock(slot->bus->self);
> >  	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> >  		if (!dev->slot || dev->slot != slot)
> >  			continue;
> > @@ -5483,12 +5485,17 @@ static void pci_slot_unlock(struct pci_slot *slot)
> >  		else
> >  			pci_dev_unlock(dev);
> >  	}
> > +	if (slot->bus->self)
> > +		pci_dev_unlock(slot->bus->self);
> >  }
> >  
> >  /* Return 1 on successful lock, 0 on contention */
> >  static int pci_slot_trylock(struct pci_slot *slot)
> >  {
> > -	struct pci_dev *dev;
> > +	struct pci_dev *dev, *bridge = slot->bus->self;
> > +
> > +	if (bridge && !pci_dev_trylock(bridge))
> > +		return 0;
> >  
> >  	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> >  		if (!dev->slot || dev->slot != slot)
> > @@ -5511,6 +5518,9 @@ static int pci_slot_trylock(struct pci_slot *slot)
> >  		else
> >  			pci_dev_unlock(dev);
> >  	}
> > +
> > +	if (bridge)
> > +		pci_dev_unlock(bridge);
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.47.3
> > 
> 

Re: [PATCH 2/2] pci: fix slot reset device locking

[dan.j.williams]

Ilpo Järvinen wrote:
> On Wed, 28 Jan 2026, Bjorn Helgaas wrote:
> 
> > [+cc Dan, Jinhui]
> > 
> > On Fri, Jan 16, 2026 at 10:41:50AM -0800, Keith Busch wrote:
> > > From: Keith Busch <kbusch@kernel.org>
> > > 
> > > Like pci_bus_lock, pci_slot_lock needs to lock the bridge device to
> > > prevent the warning:
> > 
> > I *think* this actually refers to pci_bus_trylock() and
> > pci_slot_trylock() (not pci_bus_lock() and pci_slot_lock()), since
> > that's what this patch changes?
> > 
> > It's unfortunate that pci_bus_trylock() and pci_slot_trylock() are so
> > similar but separate.  If there were combined, this kind of issue
> > where one is fixed but the other isn't wouldn't happen.
> > 
> > But what about pci_bus_lock() and pci_slot_lock()?  They are also
> > almost identical, but pci_bus_lock() locks bus->self while
> > pci_slot_lock() does not.  Should it?
> >
> > All these almost-but-not-quite identical paths make my head hurt ;)
> 
> My series does attempt to consolidation them (check the pending patches 
> from me).
> 
> (It doesn't consider this fix patch though but the other one is taken 
> into account, the series was build on top of it).

Lore link?

Re: [PATCH 2/2] pci: fix slot reset device locking

[Bjorn Helgaas]

On Wed, Jan 28, 2026 at 01:07:37PM -0800, dan.j.williams@intel.com wrote:
> Ilpo Järvinen wrote:
> > On Wed, 28 Jan 2026, Bjorn Helgaas wrote:
> > > On Fri, Jan 16, 2026 at 10:41:50AM -0800, Keith Busch wrote:
> > > > From: Keith Busch <kbusch@kernel.org>
> > > > 
> > > > Like pci_bus_lock, pci_slot_lock needs to lock the bridge device to
> > > > prevent the warning:
> > > 
> > > I *think* this actually refers to pci_bus_trylock() and
> > > pci_slot_trylock() (not pci_bus_lock() and pci_slot_lock()), since
> > > that's what this patch changes?
> > > 
> > > It's unfortunate that pci_bus_trylock() and pci_slot_trylock() are so
> > > similar but separate.  If there were combined, this kind of issue
> > > where one is fixed but the other isn't wouldn't happen.
> > > 
> > > But what about pci_bus_lock() and pci_slot_lock()?  They are also
> > > almost identical, but pci_bus_lock() locks bus->self while
> > > pci_slot_lock() does not.  Should it?
> > >
> > > All these almost-but-not-quite identical paths make my head hurt ;)
> > 
> > My series does attempt to consolidation them (check the pending patches 
> > from me).
> > 
> > (It doesn't consider this fix patch though but the other one is taken 
> > into account, the series was build on top of it).
> 
> Lore link?

I think it's
https://lore.kernel.org/all/20260116125742.1890-1-ilpo.jarvinen@linux.intel.com/

Re: [PATCH 2/2] pci: fix slot reset device locking

[dan.j.williams]

Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Like pci_bus_lock, pci_slot_lock needs to lock the bridge device to
> prevent the warning:
> 
>   pcieport 0000:e2:05.0: unlocked secondary bus reset via: pciehp_reset_slot+0x55/0xa0
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/pci/pci.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 3378221c5723a..5f8b0d06a1459 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5460,6 +5460,8 @@ static void pci_slot_lock(struct pci_slot *slot)
>  {
>  	struct pci_dev *dev;
>  
> +	if (slot->bus->self)
> +		pci_dev_lock(slot->bus->self);
>  	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
>  		if (!dev->slot || dev->slot != slot)
>  			continue;
> @@ -5483,12 +5485,17 @@ static void pci_slot_unlock(struct pci_slot *slot)
>  		else
>  			pci_dev_unlock(dev);
>  	}
> +	if (slot->bus->self)
> +		pci_dev_unlock(slot->bus->self);

Given how much context is needed to understand that this looks right, I
would not say no to a follow-on patch that adds kdoc for these locking
functions.

At a minimum I think a breadcrumb of a local "bridge = slot->bus->self"
variable would help.

>  }
>  
>  /* Return 1 on successful lock, 0 on contention */
>  static int pci_slot_trylock(struct pci_slot *slot)
>  {
> -	struct pci_dev *dev;
> +	struct pci_dev *dev, *bridge = slot->bus->self;
> +
> +	if (bridge && !pci_dev_trylock(bridge))
> +		return 0;
>  
>  	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
>  		if (!dev->slot || dev->slot != slot)
> @@ -5511,6 +5518,9 @@ static int pci_slot_trylock(struct pci_slot *slot)
>  		else
>  			pci_dev_unlock(dev);
>  	}
> +
> +	if (bridge)
> +		pci_dev_unlock(bridge);
>  	return 0;

Even without the above cosmetic fixup, looks good.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH 1/2] pci: fix slot trylock error handling

[Keith Busch]

On Fri, Jan 16, 2026 at 10:41:49AM -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The device lock isn't held if pci_bus_trylock() fails, so the code had
> been attempting to improperly unlock it.
> 
> Fixes: a4e772898f8bf2 ("PCI: Add missing bridge lock to pci_bus_lock()")
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Ping?

> ---
>  drivers/pci/pci.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 72d88ea95f3cc..3378221c5723a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5494,10 +5494,8 @@ static int pci_slot_trylock(struct pci_slot *slot)
>  		if (!dev->slot || dev->slot != slot)
>  			continue;
>  		if (dev->subordinate) {
> -			if (!pci_bus_trylock(dev->subordinate)) {
> -				pci_dev_unlock(dev);
> +			if (!pci_bus_trylock(dev->subordinate))
>  				goto unlock;
> -			}
>  		} else if (!pci_dev_trylock(dev))
>  			goto unlock;
>  	}
> -- 
> 2.47.3
> 

Re: [PATCH 1/2] pci: fix slot trylock error handling

[Ilpo Järvinen]

On Tue, 27 Jan 2026, Keith Busch wrote:

> On Fri, Jan 16, 2026 at 10:41:49AM -0800, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > The device lock isn't held if pci_bus_trylock() fails, so the code had
> > been attempting to improperly unlock it.
> > 
> > Fixes: a4e772898f8bf2 ("PCI: Add missing bridge lock to pci_bus_lock()")
> > Signed-off-by: Keith Busch <kbusch@kernel.org>
> 
> Ping?

There's the older patch from Jinhui for the same thing:

https://patchwork.kernel.org/project/linux-pci/patch/20251212145528.2555-1-guojinhui.liam@bytedance.com/

> > ---
> >  drivers/pci/pci.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 72d88ea95f3cc..3378221c5723a 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -5494,10 +5494,8 @@ static int pci_slot_trylock(struct pci_slot *slot)
> >  		if (!dev->slot || dev->slot != slot)
> >  			continue;
> >  		if (dev->subordinate) {
> > -			if (!pci_bus_trylock(dev->subordinate)) {
> > -				pci_dev_unlock(dev);
> > +			if (!pci_bus_trylock(dev->subordinate))
> >  				goto unlock;
> > -			}
> >  		} else if (!pci_dev_trylock(dev))
> >  			goto unlock;
> >  	}
> > -- 
> > 2.47.3
> > 
> 

-- 
 i.

Re: [PATCH 1/2] pci: fix slot trylock error handling

[Keith Busch]

On Wed, Jan 28, 2026 at 11:16:06AM +0200, Ilpo Järvinen wrote:
> On Tue, 27 Jan 2026, Keith Busch wrote:
> 
> > On Fri, Jan 16, 2026 at 10:41:49AM -0800, Keith Busch wrote:
> > > From: Keith Busch <kbusch@kernel.org>
> > > 
> > > The device lock isn't held if pci_bus_trylock() fails, so the code had
> > > been attempting to improperly unlock it.
> > > 
> > > Fixes: a4e772898f8bf2 ("PCI: Add missing bridge lock to pci_bus_lock()")
> > > Signed-off-by: Keith Busch <kbusch@kernel.org>
> > 
> > Ping?
> 
> There's the older patch from Jinhui for the same thing:
> 
> https://patchwork.kernel.org/project/linux-pci/patch/20251212145528.2555-1-guojinhui.liam@bytedance.com/

Okay, let's go with that one then. But patch 2 from this series is still
needed.

Re: [PATCH 1/2] pci: fix slot trylock error handling

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 955 bytes --]

On Wed, 28 Jan 2026, Keith Busch wrote:

> On Wed, Jan 28, 2026 at 11:16:06AM +0200, Ilpo Järvinen wrote:
> > On Tue, 27 Jan 2026, Keith Busch wrote:
> > 
> > > On Fri, Jan 16, 2026 at 10:41:49AM -0800, Keith Busch wrote:
> > > > From: Keith Busch <kbusch@kernel.org>
> > > > 
> > > > The device lock isn't held if pci_bus_trylock() fails, so the code had
> > > > been attempting to improperly unlock it.
> > > > 
> > > > Fixes: a4e772898f8bf2 ("PCI: Add missing bridge lock to pci_bus_lock()")
> > > > Signed-off-by: Keith Busch <kbusch@kernel.org>
> > > 
> > > Ping?
> > 
> > There's the older patch from Jinhui for the same thing:
> > 
> > https://patchwork.kernel.org/project/linux-pci/patch/20251212145528.2555-1-guojinhui.liam@bytedance.com/
> 
> Okay, let's go with that one then. But patch 2 from this series is still
> needed.

Yes, agreed. I meant to mention that about the patch 2 specifically but 
forgot.

-- 
 i.

Re: [PATCH 1/2] pci: fix slot trylock error handling

[Bjorn Helgaas]

[+cc Dan, author of a4e772898f8bf2]

On Fri, Jan 16, 2026 at 10:41:49AM -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The device lock isn't held if pci_bus_trylock() fails, so the code had
> been attempting to improperly unlock it.
> 
> Fixes: a4e772898f8bf2 ("PCI: Add missing bridge lock to pci_bus_lock()")
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/pci/pci.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 72d88ea95f3cc..3378221c5723a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5494,10 +5494,8 @@ static int pci_slot_trylock(struct pci_slot *slot)
>  		if (!dev->slot || dev->slot != slot)
>  			continue;
>  		if (dev->subordinate) {
> -			if (!pci_bus_trylock(dev->subordinate)) {
> -				pci_dev_unlock(dev);
> +			if (!pci_bus_trylock(dev->subordinate))
>  				goto unlock;
> -			}
>  		} else if (!pci_dev_trylock(dev))
>  			goto unlock;
>  	}
> -- 
> 2.47.3
> 

Re: [PATCH 1/2] pci: fix slot trylock error handling

[dan.j.williams]

Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The device lock isn't held if pci_bus_trylock() fails, so the code had
> been attempting to improperly unlock it.
> 
> Fixes: a4e772898f8bf2 ("PCI: Add missing bridge lock to pci_bus_lock()")
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/pci/pci.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 72d88ea95f3cc..3378221c5723a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5494,10 +5494,8 @@ static int pci_slot_trylock(struct pci_slot *slot)
>  		if (!dev->slot || dev->slot != slot)
>  			continue;
>  		if (dev->subordinate) {
> -			if (!pci_bus_trylock(dev->subordinate)) {
> -				pci_dev_unlock(dev);
> +			if (!pci_bus_trylock(dev->subordinate))
>  				goto unlock;
> -			}
>  		} else if (!pci_dev_trylock(dev))
>  			goto unlock;

This looks right and the original patch missed fixing up this inner
unlock when it moved the @dev lock to be conditional for non-bridge
devices.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>