[PATCH v3 0/2] Don't make noise about disconnected USB4 devices

[PATCH v3 0/2] Don't make noise about disconnected USB4 devices

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

When a USB4 or TBT3 dock is disconnected a lot of warnings and errors
are emitted related to the PCIe tunnels in the dock.

The messages are loud, but it's mostly because the functions that
emit the messages don't check whether the device is actually alive.
The PCIe hotplug services mark the device disconnected, so that
can be used to avoid an unnecessary wakeup on a disconnected device.

There is also a case of a runtime PM imbalance from this situation that
is fixed.

v2: https://lore.kernel.org/linux-pci/20250616192813.3829124-1-superm1@kernel.org/

Mario Limonciello (2):
  PCI/PM: Skip resuming to D0 if disconnected
  PCI: Fix runtime PM usage count underflow on device unplug

 drivers/pci/pci-driver.c | 3 ---
 drivers/pci/pci.c        | 5 +++++
 drivers/pci/pci.h        | 1 +
 3 files changed, 6 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH v3 1/2] PCI/PM: Skip resuming to D0 if disconnected

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

When a USB4 dock is unplugged the PCIe bridge it's connected to will
remove issue a "Link Down" and "Card not detected event". The PCI core
will treat this as a surprise hotplug event and unconfigure all downstream
devices. This involves setting the device error state to
`pci_channel_io_perm_failure` which pci_dev_is_disconnected() will check.

It doesn't make sense to runtime resume disconnected devices to D0 and
report the (expected) error to do so.  It is still useful information
that a device is disconnected though, so add an info message for that
purpose.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v3:
 * Adjust text and subject
 * Add an info message instead
v2:
 * Use pci_dev_is_disconnected()
v1: https://lore.kernel.org/linux-usb/20250609020223.269407-1-superm1@kernel.org/T/#mf95c947990d016fbfccfd11afe60b8ae08aafa0b
---
 drivers/pci/pci.c | 5 +++++
 drivers/pci/pci.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9e42090fb1089..160a9a482c732 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1374,6 +1374,11 @@ int pci_power_up(struct pci_dev *dev)
 		return -EIO;
 	}
 
+	if (pci_dev_is_disconnected(dev)) {
+		dev->current_state = PCI_D3cold;
+		return -EIO;
+	}
+
 	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
 	if (PCI_POSSIBLE_ERROR(pmcsr)) {
 		pci_err(dev, "Unable to change power state from %s to D0, device inaccessible\n",
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 12215ee72afb6..57aaf9c63e2e8 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -550,6 +550,7 @@ static inline bool pci_dev_set_io_state(struct pci_dev *dev,
 
 static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
 {
+	pci_info(dev, "Device disconnected\n");
 	pci_dev_set_io_state(dev, pci_channel_io_perm_failure);
 	pci_doe_disconnected(dev);
 
-- 
2.43.0

Re: [PATCH v3 1/2] PCI/PM: Skip resuming to D0 if disconnected

[Lukas Wunner]

On Thu, Jun 19, 2025 at 09:55:34PM -0500, Mario Limonciello wrote:
> When a USB4 dock is unplugged the PCIe bridge it's connected to will
> remove issue a "Link Down" and "Card not detected event". The PCI core

Small nit here, the word "remove" seems superfluous.


> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1374,6 +1374,11 @@ int pci_power_up(struct pci_dev *dev)
>  		return -EIO;
>  	}
>  
> +	if (pci_dev_is_disconnected(dev)) {
> +		dev->current_state = PCI_D3cold;
> +		return -EIO;
> +	}
> +
>  	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
>  	if (PCI_POSSIBLE_ERROR(pmcsr)) {
>  		pci_err(dev, "Unable to change power state from %s to D0, device inaccessible\n",

This hunk looks good to me.


> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -550,6 +550,7 @@ static inline bool pci_dev_set_io_state(struct pci_dev *dev,
>  
>  static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
>  {
> +	pci_info(dev, "Device disconnected\n");
>  	pci_dev_set_io_state(dev, pci_channel_io_perm_failure);
>  	pci_doe_disconnected(dev);

However this one (which I think was newly added in v3) seems problematic:

pci_dev_set_disconnected() needs to be fast so that devices are marked
unplugged as quickly as possible.  We want to minimize the time window
where MMIO and Config Space reads already return "all ones" and writes
go to nirvana, but pci_dev_is_disconnected() still returns false.
However printk() isn't a cheap operation, so it may unduly delay
marking the removed devices disconnected.

Second, the object of the patch is to make hot-removal less noisy,
but this change *increases* noise.  It will emit an extra message
for every removed device.

Third, the change is somewhat orthogonal to the first hunk in the patch,
so I would have put this in a separate patch.

Bottom line is, at the very least I'd recommend making this a pci_dbg(),
but I'm not even sure the extra message adds sufficient value to justify
the change.

Thanks,

Lukas

[PATCH v3 2/2] PCI: Fix runtime PM usage count underflow on device unplug

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

When a USB4 dock is unplugged the PCIe bridge it's connected to will
remove issue a "Link Down" and "Card not detected event". The PCI core
will treat this as a surprise hotplug event and unconfigure all downstream
devices.

pci_stop_bus_device() will call device_release_driver(). As part of device
release sequence pm_runtime_put_sync() is called for the device which will
decrement the runtime counter to 0. After this, the device remove callback
(pci_device_remove()) will be called which again calls pm_runtime_put_sync()
but as the counter is already 0 will cause an underflow.

This behavior was introduced in commit 967577b062417 ("PCI/PM: Keep runtime
PM enabled for unbound PCI devices") to prevent asymmetrical get/put from
probe/remove, but this misses out on the point that when releasing a driver
the usage count is decremented from the device core.

Drop the extra call from pci_device_remove().

Fixes: 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI devices")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v3:
 * git archeaology
 * Drop call alltogether, not just for the device disconnected case
v2:
 * Use pci_dev_is_disconnected()
v1: https://lore.kernel.org/linux-usb/20250609020223.269407-1-superm1@kernel.org/T/#mf95c947990d016fbfccfd11afe60b8ae08aafa0b
---
 drivers/pci/pci-driver.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index b78b98133e7df..63f1cb11906ad 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -478,9 +478,6 @@ static void pci_device_remove(struct device *dev)
 	pci_dev->driver = NULL;
 	pci_iov_remove(pci_dev);
 
-	/* Undo the runtime PM settings in local_pci_probe() */
-	pm_runtime_put_sync(dev);
-
 	/*
 	 * If the device is still on, set the power state as "unknown",
 	 * since it might change by the next time we load the driver.
-- 
2.43.0

Re: [PATCH v3 2/2] PCI: Fix runtime PM usage count underflow on device unplug

[Lukas Wunner]

On Thu, Jun 19, 2025 at 09:55:35PM -0500, Mario Limonciello wrote:
> When a USB4 dock is unplugged the PCIe bridge it's connected to will
> remove issue a "Link Down" and "Card not detected event". The PCI core
> will treat this as a surprise hotplug event and unconfigure all downstream
> devices.
> 
> pci_stop_bus_device() will call device_release_driver(). As part of device
> release sequence pm_runtime_put_sync() is called for the device which will
> decrement the runtime counter to 0. After this, the device remove callback
> (pci_device_remove()) will be called which again calls pm_runtime_put_sync()
> but as the counter is already 0 will cause an underflow.
> 
> This behavior was introduced in commit 967577b062417 ("PCI/PM: Keep runtime
> PM enabled for unbound PCI devices") to prevent asymmetrical get/put from
> probe/remove, but this misses out on the point that when releasing a driver
> the usage count is decremented from the device core.
> 
> Drop the extra call from pci_device_remove().
> 
> Fixes: 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI devices")

This doesn't look right.  The refcount underflow issue seems new,
we surely haven't been doing the wrong thing since 2012.


> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -478,9 +478,6 @@ static void pci_device_remove(struct device *dev)
>  	pci_dev->driver = NULL;
>  	pci_iov_remove(pci_dev);
>  
> -	/* Undo the runtime PM settings in local_pci_probe() */
> -	pm_runtime_put_sync(dev);
> -

local_pci_probe() increases the refcount to keep the device in D0.
If the driver wants to use runtime suspend, it needs to decrement
the refcount on ->probe() and re-increment on ->remove().

In the dmesg output attached to...

https://bugzilla.kernel.org/show_bug.cgi?id=220216

... the device exhibiting the refcount underflow is a PCIe port.
Are you also seeing this on a PCIe port or is it a different device?

So the refcount decrement happens in pcie_portdrv_probe() and
the refcount increment happens in pcie_portdrv_remove().
Both times it's conditional on pci_bridge_d3_possible().
Does that return a different value on probe versus remove?

Does any of the port service drivers decrement the refcount
once too often?  I've just looked through pciehp but cannot
find anything out of the ordinary.

Looking through recent changes, 002bf2fbc00e and bca84a7b93fd
look like potential candidates causing a regression, but the
former is for AER (which isn't used in the dmesg attached to
the bugzilla) and the latter touches suspend on system sleep,
not runtime suspend.

Can you maybe instrument the pm_runtime_{get,put}*() functions
with a printk() and/or dump_stack() to see where a gratuitous
refcount decrement occurs?

Alternatively, is there a known-good kernel version which does
not exhibit the issue and which could serve as anchor for
git bisect?

Thanks,

Lukas

Re: [PATCH v3 2/2] PCI: Fix runtime PM usage count underflow on device unplug

[Mario Limonciello]

On 6/21/25 2:05 PM, Lukas Wunner wrote:
> On Thu, Jun 19, 2025 at 09:55:35PM -0500, Mario Limonciello wrote:
>> When a USB4 dock is unplugged the PCIe bridge it's connected to will
>> remove issue a "Link Down" and "Card not detected event". The PCI core
>> will treat this as a surprise hotplug event and unconfigure all downstream
>> devices.
>>
>> pci_stop_bus_device() will call device_release_driver(). As part of device
>> release sequence pm_runtime_put_sync() is called for the device which will
>> decrement the runtime counter to 0. After this, the device remove callback
>> (pci_device_remove()) will be called which again calls pm_runtime_put_sync()
>> but as the counter is already 0 will cause an underflow.
>>
>> This behavior was introduced in commit 967577b062417 ("PCI/PM: Keep runtime
>> PM enabled for unbound PCI devices") to prevent asymmetrical get/put from
>> probe/remove, but this misses out on the point that when releasing a driver
>> the usage count is decremented from the device core.
>>
>> Drop the extra call from pci_device_remove().
>>
>> Fixes: 967577b062417 ("PCI/PM: Keep runtime PM enabled for unbound PCI devices")
> 
> This doesn't look right.  The refcount underflow issue seems new,
> we surely haven't been doing the wrong thing since 2012.
> 
> 
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>> @@ -478,9 +478,6 @@ static void pci_device_remove(struct device *dev)
>>   	pci_dev->driver = NULL;
>>   	pci_iov_remove(pci_dev);
>>   
>> -	/* Undo the runtime PM settings in local_pci_probe() */
>> -	pm_runtime_put_sync(dev);
>> -
> 
> local_pci_probe() increases the refcount to keep the device in D0.
> If the driver wants to use runtime suspend, it needs to decrement
> the refcount on ->probe() and re-increment on ->remove().
> 
> In the dmesg output attached to...
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=220216
> 
> ... the device exhibiting the refcount underflow is a PCIe port.
> Are you also seeing this on a PCIe port or is it a different device?

The device with the underflow is the disconnected PCIe bridge.

In my case it was this bridge that was downstream.

pci 0000:02:04.0: [8086:15ef] type 01 class 0x060400 PCIe Switch 
Downstream Port
pci 0000:02:04.0: PCI bridge to [bus 04]
pci 0000:02:04.0: enabling Extended Tags
pci 0000:02:04.0: supports D1 D2
pci 0000:02:04.0: PME# supported from D0 D1 D2 D3hot D3cold

> 
> So the refcount decrement happens in pcie_portdrv_probe() and
> the refcount increment happens in pcie_portdrv_remove().
> Both times it's conditional on pci_bridge_d3_possible().
> Does that return a different value on probe versus remove?
> 
> Does any of the port service drivers decrement the refcount
> once too often?  I've just looked through pciehp but cannot
> find anything out of the ordinary.
> 
> Looking through recent changes, 002bf2fbc00e and bca84a7b93fd
> look like potential candidates causing a regression, but the
> former is for AER (which isn't used in the dmesg attached to
> the bugzilla) and the latter touches suspend on system sleep,
> not runtime suspend.
> 
> Can you maybe instrument the pm_runtime_{get,put}*() functions
> with a printk() and/or dump_stack() to see where a gratuitous
> refcount decrement occurs?
> 
That's exactly what I did to conclude this call was an extra one.


Here's the drop to 0:

pm_runtime: 0000:02:04.0 usage count is now 0 from 
__pm_runtime_idle+0x6f/0xd0
CPU: 7 UID: 0 PID: 196 Comm: irq/36-pciehp Not tainted 
6.16.0-rc2-00086-g4156cebf8a54 #281 PREEMPT(full) 
48d9dd361274f6fbfa98cd17f346d017a60ec738
Hardware name: Framework Laptop 13 (AMD Ryzen AI 300 Series)/FRANMGCP09, 
BIOS 03.03 03/10/2025
Call Trace:
  <TASK>
  dump_stack_lvl+0x53/0x70
  rpm_drop_usage_count+0x50/0x90
  __pm_runtime_idle+0x6f/0xd0
  device_release_driver_internal+0x197/0x200
  pci_stop_bus_device+0x6d/0x90
  pci_stop_bus_device+0x2c/0x90
  pci_stop_and_remove_bus_device+0x12/0x20
  pciehp_unconfigure_device+0x97/0x160
  pciehp_disable_slot+0x66/0x140
  pciehp_handle_presence_or_link_change+0x86/0x4b0
  pciehp_ist+0x196/0x280
  irq_thread_fn+0x20/0x60
  irq_thread+0x1ae/0x290
  ? __pfx_irq_thread_fn+0x10/0x10
  ? __pfx_irq_thread_dtor+0x10/0x10
  ? __pfx_irq_thread+0x10/0x10
  kthread+0xfb/0x260
  ? __pfx_kthread+0x10/0x10
  ret_from_fork+0x14a/0x180
  ? __pfx_kthread+0x10/0x10
  ret_from_fork_asm+0x1a/0x30
  </TASK>

And then here is a dev_WARN at the underflow case.

------------[ cut here ]------------
pcieport 0000:02:04.0: Runtime PM usage count underflow!
WARNING: CPU: 7 PID: 196 at drivers/base/power/runtime.c:1084 
rpm_drop_usage_count+0x82/0x90
Modules linked in: vivaldi_fmap aesni_intel thunderbolt(+) ccp nvme_core 
i8042 tpm_tis i2c_hid_acpi serio tpm_tis_core i2c_hid tpm libaescfb 
ecdh_generic
CPU: 7 UID: 0 PID: 196 Comm: irq/36-pciehp Not tainted 
6.16.0-rc2-00086-g4156cebf8a54 #281 PREEMPT(full) 
48d9dd361274f6fbfa98cd17f346d017a60ec738
Hardware name: Framework Laptop 13 (AMD Ryzen AI 300 Series)/FRANMGCP09, 
BIOS 03.03 03/10/2025
RIP: 0010:rpm_drop_usage_count+0x82/0x90
Code: f0 ff 87 b0 01 00 00 48 8b 5f 50 48 85 db 75 03 48 8b 1f e8 20 44 
fe ff 48 89 da 48 c7 c7 80 52 c8 a0 48 89 c6 e8 8e 3f 6e ff <0f> 0b bb 
ea ff ff ff eb 9b>
RSP: 0018:ffffd240c09e7c28 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffff8997c213f430 RCX: 0000000000000027
RDX: ffff899f1dbdc288 RSI: 0000000000000001 RDI: ffff899f1dbdc280
RBP: ffff8997c29150c8 R08: 0000000000000000 R09: 0000000000000003
R10: ffffd240c09e7ab0 R11: ffff899f1e129368 R12: ffffffffa1161ca0
R13: ffff8997c2915148 R14: 0000000000000080 R15: ffff8997c1613914
FS:  0000000000000000(0000) GS:ffff899f7c175000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fbbc1357750 CR3: 0000000111245000 CR4: 0000000000b50ef0
Call Trace:
  <TASK>
  __pm_runtime_idle+0x6f/0xd0
  pci_device_remove+0x7e/0xb0
  device_release_driver_internal+0x19f/0x200
  pci_stop_bus_device+0x6d/0x90
  pci_stop_bus_device+0x2c/0x90
  pci_stop_and_remove_bus_device+0x12/0x20
  pciehp_unconfigure_device+0x97/0x160
  pciehp_disable_slot+0x66/0x140
  pciehp_handle_presence_or_link_change+0x86/0x4b0
  pciehp_ist+0x196/0x280
  irq_thread_fn+0x20/0x60
  irq_thread+0x1ae/0x290
  ? __pfx_irq_thread_fn+0x10/0x10
  ? __pfx_irq_thread_dtor+0x10/0x10
  ? __pfx_irq_thread+0x10/0x10
  kthread+0xfb/0x260
  ? __pfx_kthread+0x10/0x10
  ret_from_fork+0x14a/0x180
  ? __pfx_kthread+0x10/0x10
  ret_from_fork_asm+0x1a/0x30
  </TASK>
---[ end trace 0000000000000000 ]---

Re: [PATCH v3 2/2] PCI: Fix runtime PM usage count underflow on device unplug

[Lukas Wunner]

On Sat, Jun 21, 2025 at 02:56:08PM -0500, Mario Limonciello wrote:
> On 6/21/25 2:05 PM, Lukas Wunner wrote:
> > In the dmesg output attached to...
> > 
> > https://bugzilla.kernel.org/show_bug.cgi?id=220216
> > 
> > ... the device exhibiting the refcount underflow is a PCIe port.
> > Are you also seeing this on a PCIe port or is it a different device?
> 
> The device with the underflow is the disconnected PCIe bridge.
> 
> In my case it was this bridge that was downstream.

Okay, in both cases the refcount underflow occurs on a PCIe port.
So it seems likely the gratuitous refcount decrement is in portdrv.c
or one of the port services drivers.

Your patch changes the code path for *all* PCI devices.
Not just PCIe ports.  Hence it's likely not the right fix.

It may fix the issue on this particular PCIe port but
I strongly suspect it'll leak a runtime PM ref on all other devices.


> > So the refcount decrement happens in pcie_portdrv_probe() and
> > the refcount increment happens in pcie_portdrv_remove().
> > Both times it's conditional on pci_bridge_d3_possible().
> > Does that return a different value on probe versus remove?

Could you please answer this?


> > Does any of the port service drivers decrement the refcount
> > once too often?  I've just looked through pciehp but cannot
> > find anything out of the ordinary.
> > 
> > Looking through recent changes, 002bf2fbc00e and bca84a7b93fd
> > look like potential candidates causing a regression, but the
> > former is for AER (which isn't used in the dmesg attached to
> > the bugzilla) and the latter touches suspend on system sleep,
> > not runtime suspend.
> > 
> > Can you maybe instrument the pm_runtime_{get,put}*() functions
> > with a printk() and/or dump_stack() to see where a gratuitous
> > refcount decrement occurs?
> 
> That's exactly what I did to conclude this call was an extra one.
> 
> Here's the drop to 0:

The drop to 0 is uninteresting.  You need to record *all*
refcount increments/decrements so that we can see where the
gratuitous one occurs.  It happens earlier than the drop to 0.

However, please first check whether the pci_bridge_d3_possible()
return value changes on probe versus remove of the PCIe port
in question.  If it does, then that's the root cause and there's
no need to look any further.

Thanks,

Lukas

Re: [PATCH v3 2/2] PCI: Fix runtime PM usage count underflow on device unplug

[Mario Limonciello]

On 6/21/2025 11:43 PM, Lukas Wunner wrote:
> On Sat, Jun 21, 2025 at 02:56:08PM -0500, Mario Limonciello wrote:
>> On 6/21/25 2:05 PM, Lukas Wunner wrote:
>>> In the dmesg output attached to...
>>>
>>> https://bugzilla.kernel.org/show_bug.cgi?id=220216
>>>
>>> ... the device exhibiting the refcount underflow is a PCIe port.
>>> Are you also seeing this on a PCIe port or is it a different device?
>>
>> The device with the underflow is the disconnected PCIe bridge.
>>
>> In my case it was this bridge that was downstream.
> 
> Okay, in both cases the refcount underflow occurs on a PCIe port.
> So it seems likely the gratuitous refcount decrement is in portdrv.c
> or one of the port services drivers.
> 
> Your patch changes the code path for *all* PCI devices.
> Not just PCIe ports.  Hence it's likely not the right fix.
> 
> It may fix the issue on this particular PCIe port but
> I strongly suspect it'll leak a runtime PM ref on all other devices.
> 

Thanks, I see your point.

> 
>>> So the refcount decrement happens in pcie_portdrv_probe() and
>>> the refcount increment happens in pcie_portdrv_remove().
>>> Both times it's conditional on pci_bridge_d3_possible().
>>> Does that return a different value on probe versus remove?
> 
> Could you please answer this?

I did this check and yes specifically on this PCIe port with the 
underflow the d3 possible lookup returns false during 
pcie_portdrv_remove().  It returns true during pcie_portdrv_probe().

> 
> 
>>> Does any of the port service drivers decrement the refcount
>>> once too often?  I've just looked through pciehp but cannot
>>> find anything out of the ordinary.
>>>
>>> Looking through recent changes, 002bf2fbc00e and bca84a7b93fd
>>> look like potential candidates causing a regression, but the
>>> former is for AER (which isn't used in the dmesg attached to
>>> the bugzilla) and the latter touches suspend on system sleep,
>>> not runtime suspend.
>>>
>>> Can you maybe instrument the pm_runtime_{get,put}*() functions
>>> with a printk() and/or dump_stack() to see where a gratuitous
>>> refcount decrement occurs?
>>
>> That's exactly what I did to conclude this call was an extra one.
>>
>> Here's the drop to 0:
> 
> The drop to 0 is uninteresting.  You need to record *all*
> refcount increments/decrements so that we can see where the
> gratuitous one occurs.  It happens earlier than the drop to 0.
> 
> However, please first check whether the pci_bridge_d3_possible()
> return value changes on probe versus remove of the PCIe port
> in question.  If it does, then that's the root cause and there's
> no need to look any further.
> 

That was a great hypothesis that's spot on.

Just for posterity this was all the increment/decrement calls that I saw 
happen.

pci 0000:02:04.0: inc usage cnt from 0, caller: pci_pm_init+0x84/0x2d0
pci 0000:02:04.0: inc usage cnt from 1, caller: 
pci_scan_bridge_extend+0x6d/0x710
pci 0000:02:04.0: dec usage cnt from 2, caller: 
pci_scan_bridge_extend+0x19e/0x710
pci 0000:02:04.0: inc usage cnt from 1, caller: 
pci_scan_bridge_extend+0x6d/0x710
pci 0000:02:04.0: dec usage cnt from 2, caller: 
pci_scan_bridge_extend+0x19e/0x710
pcieport 0000:02:04.0: inc usage cnt from 1, caller: 
local_pci_probe+0x2d/0xa0
pcieport 0000:02:04.0: inc usage cnt from 2, caller: 
__device_attach+0x9c/0x1b0
pcieport 0000:02:04.0: inc usage cnt from 3, caller: 
__driver_probe_device+0x5c/0x150
pcieport 0000:02:04.0: dec usage cnt from 4, caller: 
__driver_probe_device+0x9a/0x150
pcieport 0000:02:04.0: dec usage cnt from 3, caller: 
__device_attach+0x145/0x1b0
pcieport 0000:02:04.0: dec usage cnt from 2, caller: 
pcie_portdrv_probe+0x19d/0x6d0
pcieport 0000:02:04.0: dec usage cnt from 1, caller: 
pcie_portdrv_probe+0x1a5/0x6d0
pcieport 0000:02:04.0: inc usage cnt from 0, caller: 
device_release_driver_internal+0xac/0x200
pcieport 0000:02:04.0: dec usage cnt from 1, caller: 
device_release_driver_internal+0x197/0x200
pcieport 0000:02:04.0: inc usage cnt from 0, caller: 
pci_device_remove+0x2d/0xb0
pcieport 0000:02:04.0: dec usage cnt from 0, caller: 
pci_device_remove+0x7e/0xb0
pcieport 0000:02:04.0: Runtime PM usage cnt underflow!

What's your suggestion on what to actually do here then?

Re: [PATCH v3 2/2] PCI: Fix runtime PM usage count underflow on device unplug

[Mario Limonciello]

On 6/22/25 1:39 PM, Mario Limonciello wrote:
> On 6/21/2025 11:43 PM, Lukas Wunner wrote:
>> On Sat, Jun 21, 2025 at 02:56:08PM -0500, Mario Limonciello wrote:
>>> On 6/21/25 2:05 PM, Lukas Wunner wrote:
>>>> In the dmesg output attached to...
>>>>
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=220216
>>>>
>>>> ... the device exhibiting the refcount underflow is a PCIe port.
>>>> Are you also seeing this on a PCIe port or is it a different device?
>>>
>>> The device with the underflow is the disconnected PCIe bridge.
>>>
>>> In my case it was this bridge that was downstream.
>>
>> Okay, in both cases the refcount underflow occurs on a PCIe port.
>> So it seems likely the gratuitous refcount decrement is in portdrv.c
>> or one of the port services drivers.
>>
>> Your patch changes the code path for *all* PCI devices.
>> Not just PCIe ports.  Hence it's likely not the right fix.
>>
>> It may fix the issue on this particular PCIe port but
>> I strongly suspect it'll leak a runtime PM ref on all other devices.
>>
> 
> Thanks, I see your point.
> 
>>
>>>> So the refcount decrement happens in pcie_portdrv_probe() and
>>>> the refcount increment happens in pcie_portdrv_remove().
>>>> Both times it's conditional on pci_bridge_d3_possible().
>>>> Does that return a different value on probe versus remove?
>>
>> Could you please answer this?
> 
> I did this check and yes specifically on this PCIe port with the 
> underflow the d3 possible lookup returns false during 
> pcie_portdrv_remove().  It returns true during pcie_portdrv_probe().
> 
>>
>>
>>>> Does any of the port service drivers decrement the refcount
>>>> once too often?  I've just looked through pciehp but cannot
>>>> find anything out of the ordinary.
>>>>
>>>> Looking through recent changes, 002bf2fbc00e and bca84a7b93fd
>>>> look like potential candidates causing a regression, but the
>>>> former is for AER (which isn't used in the dmesg attached to
>>>> the bugzilla) and the latter touches suspend on system sleep,
>>>> not runtime suspend.
>>>>
>>>> Can you maybe instrument the pm_runtime_{get,put}*() functions
>>>> with a printk() and/or dump_stack() to see where a gratuitous
>>>> refcount decrement occurs?
>>>
>>> That's exactly what I did to conclude this call was an extra one.
>>>
>>> Here's the drop to 0:
>>
>> The drop to 0 is uninteresting.  You need to record *all*
>> refcount increments/decrements so that we can see where the
>> gratuitous one occurs.  It happens earlier than the drop to 0.
>>
>> However, please first check whether the pci_bridge_d3_possible()
>> return value changes on probe versus remove of the PCIe port
>> in question.  If it does, then that's the root cause and there's
>> no need to look any further.
>>
> 
> That was a great hypothesis that's spot on.
> 
> Just for posterity this was all the increment/decrement calls that I saw 
> happen.
> 
> pci 0000:02:04.0: inc usage cnt from 0, caller: pci_pm_init+0x84/0x2d0
> pci 0000:02:04.0: inc usage cnt from 1, caller: 
> pci_scan_bridge_extend+0x6d/0x710
> pci 0000:02:04.0: dec usage cnt from 2, caller: 
> pci_scan_bridge_extend+0x19e/0x710
> pci 0000:02:04.0: inc usage cnt from 1, caller: 
> pci_scan_bridge_extend+0x6d/0x710
> pci 0000:02:04.0: dec usage cnt from 2, caller: 
> pci_scan_bridge_extend+0x19e/0x710
> pcieport 0000:02:04.0: inc usage cnt from 1, caller: 
> local_pci_probe+0x2d/0xa0
> pcieport 0000:02:04.0: inc usage cnt from 2, caller: 
> __device_attach+0x9c/0x1b0
> pcieport 0000:02:04.0: inc usage cnt from 3, caller: 
> __driver_probe_device+0x5c/0x150
> pcieport 0000:02:04.0: dec usage cnt from 4, caller: 
> __driver_probe_device+0x9a/0x150
> pcieport 0000:02:04.0: dec usage cnt from 3, caller: 
> __device_attach+0x145/0x1b0
> pcieport 0000:02:04.0: dec usage cnt from 2, caller: 
> pcie_portdrv_probe+0x19d/0x6d0
> pcieport 0000:02:04.0: dec usage cnt from 1, caller: 
> pcie_portdrv_probe+0x1a5/0x6d0
> pcieport 0000:02:04.0: inc usage cnt from 0, caller: 
> device_release_driver_internal+0xac/0x200
> pcieport 0000:02:04.0: dec usage cnt from 1, caller: 
> device_release_driver_internal+0x197/0x200
> pcieport 0000:02:04.0: inc usage cnt from 0, caller: 
> pci_device_remove+0x2d/0xb0
> pcieport 0000:02:04.0: dec usage cnt from 0, caller: 
> pci_device_remove+0x7e/0xb0
> pcieport 0000:02:04.0: Runtime PM usage cnt underflow!
> 
> What's your suggestion on what to actually do here then?
> 
> 

Actually I came up with the idea to forbid runtime PM on the service 
when it doesn't allow d3 at probe which I believe means no need to check 
again on remove.

This works cleanly for me.  LMK what you think of this.

diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index e8318fd5f6ed5..a85cd7412cf4d 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -717,6 +717,8 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
                 pm_runtime_mark_last_busy(&dev->dev);
                 pm_runtime_put_autosuspend(&dev->dev);
                 pm_runtime_allow(&dev->dev);
+       } else {
+               pm_runtime_forbid(&dev->dev);
         }

         return 0;
@@ -724,11 +726,9 @@ static int pcie_portdrv_probe(struct pci_dev *dev,

  static void pcie_portdrv_remove(struct pci_dev *dev)
  {
-       if (pci_bridge_d3_possible(dev)) {
-               pm_runtime_forbid(&dev->dev);
-               pm_runtime_get_noresume(&dev->dev);
-               pm_runtime_dont_use_autosuspend(&dev->dev);
-       }
+       pm_runtime_forbid(&dev->dev);
+       pm_runtime_get_noresume(&dev->dev);
+       pm_runtime_dont_use_autosuspend(&dev->dev);

         pcie_port_device_remove(dev);

@@ -737,11 +737,9 @@ static void pcie_portdrv_remove(struct pci_dev *dev)

  static void pcie_portdrv_shutdown(struct pci_dev *dev)
  {
-       if (pci_bridge_d3_possible(dev)) {
-               pm_runtime_forbid(&dev->dev);
-               pm_runtime_get_noresume(&dev->dev);
-               pm_runtime_dont_use_autosuspend(&dev->dev);
-       }
+       pm_runtime_forbid(&dev->dev);
+       pm_runtime_get_noresume(&dev->dev);
+       pm_runtime_dont_use_autosuspend(&dev->dev);

         pcie_port_device_remove(dev);
  }

Re: [PATCH v3 2/2] PCI: Fix runtime PM usage count underflow on device unplug

[Lukas Wunner]

On Sun, Jun 22, 2025 at 08:47:03PM -0500, Mario Limonciello wrote:
> Actually I came up with the idea to forbid runtime PM on the service when it
> doesn't allow d3 at probe which I believe means no need to check again on
> remove.
> 
> This works cleanly for me.  LMK what you think of this.

User space can override the allow/forbid setting through the
sysfs "control" attribute, hence this doesn't seem like a
suitable way to somehow cache the return value of
pci_bridge_d3_possible() and thus avoid its re-execution
on remove.

However we shouldn't have to cache the return value anyway
as the underlying assumption is that the function always
returns the same value.

Thanks,

Lukas

Re: [PATCH v3 2/2] PCI: Fix runtime PM usage count underflow on device unplug

[Lukas Wunner]

On Sun, Jun 22, 2025 at 01:39:26PM -0500, Mario Limonciello wrote:
> > > On 6/21/25 2:05 PM, Lukas Wunner wrote:
> > > > So the refcount decrement happens in pcie_portdrv_probe() and
> > > > the refcount increment happens in pcie_portdrv_remove().
> > > > Both times it's conditional on pci_bridge_d3_possible().
> > > > Does that return a different value on probe versus remove?
> 
> I did this check and yes specifically on this PCIe port with the underflow
> the d3 possible lookup returns false during pcie_portdrv_remove().  It
> returns true during pcie_portdrv_probe().

That's not supposed to happen.  The expectation is that
pci_bridge_d3_possible() always returns the same value.

For this reason the return value on ->probe() isn't cached.

So which of the conditions changes in pci_bridge_d3_possible()
on probe versus remove?  Could you instrument each with a printk()
so that we can understand what's going wrong there?

Thanks,

Lukas

Re: [PATCH v3 2/2] PCI: Fix runtime PM usage count underflow on device unplug

[Lukas Wunner]

On Mon, Jun 23, 2025 at 08:43:25AM +0200, Lukas Wunner wrote:
> On Sun, Jun 22, 2025 at 01:39:26PM -0500, Mario Limonciello wrote:
> > > > On 6/21/25 2:05 PM, Lukas Wunner wrote:
> > > > > So the refcount decrement happens in pcie_portdrv_probe() and
> > > > > the refcount increment happens in pcie_portdrv_remove().
> > > > > Both times it's conditional on pci_bridge_d3_possible().
> > > > > Does that return a different value on probe versus remove?
> > 
> > I did this check and yes specifically on this PCIe port with the underflow
> > the d3 possible lookup returns false during pcie_portdrv_remove().  It
> > returns true during pcie_portdrv_probe().
> 
> That's not supposed to happen.  The expectation is that
> pci_bridge_d3_possible() always returns the same value.

I'm wondering if the patch below fixes the issue?

Normally the "is_thunderbolt" check in pci_bridge_d3_possible()
should return true for this particular device.

But there's a check for pciehp_is_native() before that and
it accesses config space to check if the Hot-Plug Capable bit
is set.  The device is inaccessible, so the expectation is that
"all ones" is returned, which implies a set HPC bit.

But maybe for some reason "all ones" isn't returned anymore?
If the patch below does help, could you check what the read
of the Slot Capabilities register returns?

Thanks!

-- >8 --

From: Lukas Wunner <lukas@wunner.de>
Subject: [PATCH] PCI/ACPI: Use cached is_hotplug_bridge value in
 pciehp_is_native()

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pci-acpi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index b78e0e4..909ca4a 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -821,8 +821,7 @@ bool pciehp_is_native(struct pci_dev *bridge)
 	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
 		return false;
 
-	pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
-	if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
+	if (!bridge->is_hotplug_bridge)
 		return false;
 
 	if (pcie_ports_native)
-- 
2.47.2

Re: [PATCH v3 2/2] PCI: Fix runtime PM usage count underflow on device unplug

[Lukas Wunner]

On Mon, Jun 23, 2025 at 09:37:07AM +0200, Lukas Wunner wrote:
> On Mon, Jun 23, 2025 at 08:43:25AM +0200, Lukas Wunner wrote:
> > On Sun, Jun 22, 2025 at 01:39:26PM -0500, Mario Limonciello wrote:
> > > > > On 6/21/25 2:05 PM, Lukas Wunner wrote:
> > > > > > So the refcount decrement happens in pcie_portdrv_probe() and
> > > > > > the refcount increment happens in pcie_portdrv_remove().
> > > > > > Both times it's conditional on pci_bridge_d3_possible().
> > > > > > Does that return a different value on probe versus remove?
> > > 
> > > I did this check and yes specifically on this PCIe port with the underflow
> > > the d3 possible lookup returns false during pcie_portdrv_remove().  It
> > > returns true during pcie_portdrv_probe().
> > 
> > That's not supposed to happen.  The expectation is that
> > pci_bridge_d3_possible() always returns the same value.
> 
> I'm wondering if the patch below fixes the issue?

Refined patch below with proper commit message,
also avoids a compiler warning caused by an unused variable.

-- >8 --

From: Lukas Wunner <lukas@wunner.de>
Subject: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable
 ports

pcie_portdrv_probe() and pcie_portdrv_remove() both call
pci_bridge_d3_possible() to determine whether to use runtime power
management.  The underlying assumption is that pci_bridge_d3_possible()
always returns the same value because otherwise a runtime PM reference
imbalance occurs.

That assumption falls apart if the device is inaccessible on ->remove()
due to hot-unplug:  pci_bridge_d3_possible() calls pciehp_is_native(),
which accesses Config Space to determine whether the device is Hot-Plug
Capable.   An inaccessible device generally returns "all ones" for such
Config Read Requests.  Hence the device may seem Hot-Plug Capable on
->remove() even though it wasn't on ->probe().

Use the cached copy of the Hot-Plug Capable bit to avoid the Config Space
access and the resulting runtime PM ref imbalance.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pci-acpi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index b78e0e4..8859cce 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -816,13 +816,11 @@ int pci_acpi_program_hp_params(struct pci_dev *dev)
 bool pciehp_is_native(struct pci_dev *bridge)
 {
 	const struct pci_host_bridge *host;
-	u32 slot_cap;
 
 	if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
 		return false;
 
-	pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
-	if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
+	if (!bridge->is_hotplug_bridge)
 		return false;
 
 	if (pcie_ports_native)
-- 
2.47.2

Re: [PATCH v3 2/2] PCI: Fix runtime PM usage count underflow on device unplug

[Rafael J. Wysocki]

On Mon, Jun 23, 2025 at 12:05 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Mon, Jun 23, 2025 at 09:37:07AM +0200, Lukas Wunner wrote:
> > On Mon, Jun 23, 2025 at 08:43:25AM +0200, Lukas Wunner wrote:
> > > On Sun, Jun 22, 2025 at 01:39:26PM -0500, Mario Limonciello wrote:
> > > > > > On 6/21/25 2:05 PM, Lukas Wunner wrote:
> > > > > > > So the refcount decrement happens in pcie_portdrv_probe() and
> > > > > > > the refcount increment happens in pcie_portdrv_remove().
> > > > > > > Both times it's conditional on pci_bridge_d3_possible().
> > > > > > > Does that return a different value on probe versus remove?
> > > >
> > > > I did this check and yes specifically on this PCIe port with the underflow
> > > > the d3 possible lookup returns false during pcie_portdrv_remove().  It
> > > > returns true during pcie_portdrv_probe().
> > >
> > > That's not supposed to happen.  The expectation is that
> > > pci_bridge_d3_possible() always returns the same value.
> >
> > I'm wondering if the patch below fixes the issue?
>
> Refined patch below with proper commit message,
> also avoids a compiler warning caused by an unused variable.
>
> -- >8 --
>
> From: Lukas Wunner <lukas@wunner.de>
> Subject: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable
>  ports
>
> pcie_portdrv_probe() and pcie_portdrv_remove() both call
> pci_bridge_d3_possible() to determine whether to use runtime power
> management.  The underlying assumption is that pci_bridge_d3_possible()
> always returns the same value because otherwise a runtime PM reference
> imbalance occurs.
>
> That assumption falls apart if the device is inaccessible on ->remove()
> due to hot-unplug:  pci_bridge_d3_possible() calls pciehp_is_native(),
> which accesses Config Space to determine whether the device is Hot-Plug
> Capable.   An inaccessible device generally returns "all ones" for such
> Config Read Requests.  Hence the device may seem Hot-Plug Capable on
> ->remove() even though it wasn't on ->probe().
>
> Use the cached copy of the Hot-Plug Capable bit to avoid the Config Space
> access and the resulting runtime PM ref imbalance.
>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

LGTM

Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>

> ---
>  drivers/pci/pci-acpi.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index b78e0e4..8859cce 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -816,13 +816,11 @@ int pci_acpi_program_hp_params(struct pci_dev *dev)
>  bool pciehp_is_native(struct pci_dev *bridge)
>  {
>         const struct pci_host_bridge *host;
> -       u32 slot_cap;
>
>         if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
>                 return false;
>
> -       pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
> -       if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
> +       if (!bridge->is_hotplug_bridge)
>                 return false;
>
>         if (pcie_ports_native)
> --
> 2.47.2
>
>

Re: [PATCH v3 2/2] PCI: Fix runtime PM usage count underflow on device unplug

[Mario Limonciello]

On 6/23/25 5:11 AM, Rafael J. Wysocki wrote:
> On Mon, Jun 23, 2025 at 12:05 PM Lukas Wunner <lukas@wunner.de> wrote:
>>
>> On Mon, Jun 23, 2025 at 09:37:07AM +0200, Lukas Wunner wrote:
>>> On Mon, Jun 23, 2025 at 08:43:25AM +0200, Lukas Wunner wrote:
>>>> On Sun, Jun 22, 2025 at 01:39:26PM -0500, Mario Limonciello wrote:
>>>>>>> On 6/21/25 2:05 PM, Lukas Wunner wrote:
>>>>>>>> So the refcount decrement happens in pcie_portdrv_probe() and
>>>>>>>> the refcount increment happens in pcie_portdrv_remove().
>>>>>>>> Both times it's conditional on pci_bridge_d3_possible().
>>>>>>>> Does that return a different value on probe versus remove?
>>>>>
>>>>> I did this check and yes specifically on this PCIe port with the underflow
>>>>> the d3 possible lookup returns false during pcie_portdrv_remove().  It
>>>>> returns true during pcie_portdrv_probe().
>>>>
>>>> That's not supposed to happen.  The expectation is that
>>>> pci_bridge_d3_possible() always returns the same value.
>>>
>>> I'm wondering if the patch below fixes the issue?
>>
>> Refined patch below with proper commit message,
>> also avoids a compiler warning caused by an unused variable.

Yes this works, thanks!

>>
>> -- >8 --
>>
>> From: Lukas Wunner <lukas@wunner.de>
>> Subject: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable
>>   ports
>>
>> pcie_portdrv_probe() and pcie_portdrv_remove() both call
>> pci_bridge_d3_possible() to determine whether to use runtime power
>> management.  The underlying assumption is that pci_bridge_d3_possible()
>> always returns the same value because otherwise a runtime PM reference
>> imbalance occurs.
>>
>> That assumption falls apart if the device is inaccessible on ->remove()
>> due to hot-unplug:  pci_bridge_d3_possible() calls pciehp_is_native(),
>> which accesses Config Space to determine whether the device is Hot-Plug
>> Capable.   An inaccessible device generally returns "all ones" for such
>> Config Read Requests.  Hence the device may seem Hot-Plug Capable on
>> ->remove() even though it wasn't on ->probe().
>>
>> Use the cached copy of the Hot-Plug Capable bit to avoid the Config Space
>> access and the resulting runtime PM ref imbalance.
>>
>> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> 
> LGTM
> 
> Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>

Tested-by: Mario Limonciello <mario.limonciello@amd.com>>
>> ---
>>   drivers/pci/pci-acpi.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index b78e0e4..8859cce 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -816,13 +816,11 @@ int pci_acpi_program_hp_params(struct pci_dev *dev)
>>   bool pciehp_is_native(struct pci_dev *bridge)
>>   {
>>          const struct pci_host_bridge *host;
>> -       u32 slot_cap;
>>
>>          if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
>>                  return false;
>>
>> -       pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
>> -       if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
>> +       if (!bridge->is_hotplug_bridge)
>>                  return false;
>>
>>          if (pcie_ports_native)
>> --
>> 2.47.2
>>
>>
> 

Re: [PATCH v3 2/2] PCI: Fix runtime PM usage count underflow on device unplug

[Lukas Wunner]

On Mon, Jun 23, 2025 at 06:37:33AM -0500, Mario Limonciello wrote:
> On 6/23/25 5:11 AM, Rafael J. Wysocki wrote:
> > On Mon, Jun 23, 2025 at 12:05???PM Lukas Wunner <lukas@wunner.de> wrote:
> > > On Mon, Jun 23, 2025 at 09:37:07AM +0200, Lukas Wunner wrote:
> > > > On Mon, Jun 23, 2025 at 08:43:25AM +0200, Lukas Wunner wrote:
> > > > > On Sun, Jun 22, 2025 at 01:39:26PM -0500, Mario Limonciello wrote:
> > > > > > I did this check and yes specifically on this PCIe port with
> > > > > > the underflow the d3 possible lookup returns false during
> > > > > > pcie_portdrv_remove().  It returns true during
> > > > > > pcie_portdrv_probe().
> > > > > 
> > > > > That's not supposed to happen.  The expectation is that
> > > > > pci_bridge_d3_possible() always returns the same value.
> > > > 
> > > > I'm wondering if the patch below fixes the issue?
> 
> Yes this works, thanks!

Could you still check what the value read from the Slot Capabilities
register is in pciehp_is_native() (if the patch is not applied)?

I guess it must be something else than "all ones" and I'd like to
understand why.

Thanks,

Lukas

Re: [PATCH v3 2/2] PCI: Fix runtime PM usage count underflow on device unplug

[Mario Limonciello]

On 6/23/25 7:19 AM, Lukas Wunner wrote:
> On Mon, Jun 23, 2025 at 06:37:33AM -0500, Mario Limonciello wrote:
>> On 6/23/25 5:11 AM, Rafael J. Wysocki wrote:
>>> On Mon, Jun 23, 2025 at 12:05???PM Lukas Wunner <lukas@wunner.de> wrote:
>>>> On Mon, Jun 23, 2025 at 09:37:07AM +0200, Lukas Wunner wrote:
>>>>> On Mon, Jun 23, 2025 at 08:43:25AM +0200, Lukas Wunner wrote:
>>>>>> On Sun, Jun 22, 2025 at 01:39:26PM -0500, Mario Limonciello wrote:
>>>>>>> I did this check and yes specifically on this PCIe port with
>>>>>>> the underflow the d3 possible lookup returns false during
>>>>>>> pcie_portdrv_remove().  It returns true during
>>>>>>> pcie_portdrv_probe().
>>>>>>
>>>>>> That's not supposed to happen.  The expectation is that
>>>>>> pci_bridge_d3_possible() always returns the same value.
>>>>>
>>>>> I'm wondering if the patch below fixes the issue?
>>
>> Yes this works, thanks!
> 
> Could you still check what the value read from the Slot Capabilities
> register is in pciehp_is_native() (if the patch is not applied)?
> 
> I guess it must be something else than "all ones" and I'd like to
> understand why.
> 
> Thanks,
> 
> Lukas

At probe:
0x00240060

At remove:
0x00000000

Re: [PATCH v3 2/2] PCI: Fix runtime PM usage count underflow on device unplug

[Lukas Wunner]

[cc += Mika]

On Mon, Jun 23, 2025 at 06:37:33AM -0500, Mario Limonciello wrote:
> On 6/23/25 5:11 AM, Rafael J. Wysocki wrote:
> > On Mon, Jun 23, 2025 at 12:05 PM Lukas Wunner <lukas@wunner.de> wrote:
> > > pcie_portdrv_probe() and pcie_portdrv_remove() both call
> > > pci_bridge_d3_possible() to determine whether to use runtime power
> > > management.  The underlying assumption is that pci_bridge_d3_possible()
> > > always returns the same value because otherwise a runtime PM reference
> > > imbalance occurs.
> > > 
> > > That assumption falls apart if the device is inaccessible on ->remove()
> > > due to hot-unplug:  pci_bridge_d3_possible() calls pciehp_is_native(),
> > > which accesses Config Space to determine whether the device is Hot-Plug
> > > Capable.   An inaccessible device generally returns "all ones" for such
> > > Config Read Requests.  Hence the device may seem Hot-Plug Capable on
> > > ->remove() even though it wasn't on ->probe().
> > > 
> > > Use the cached copy of the Hot-Plug Capable bit to avoid the Config Space
> > > access and the resulting runtime PM ref imbalance.
> > > 
> > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > 
> > Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>
> 
> Tested-by: Mario Limonciello <mario.limonciello@amd.com>>

I ended up changing the patch significantly, so I did not include
Rafael's Reviewed-by and Mario's Tested-by in the final patch.
My apologies for this!

Looking at the commit which introduced the Config Space read,
5352a44a561d, I got the impression that Mika may have deliberately
avoided using the is_hotplug_bridge flag.  Notably, is_hotplug_bridge
is also set by check_hotplug_bridge() in acpiphp_glue.c, and his
intention was probably to avoid matching those bridges in
pciehp_is_native().

So I decided to err on the side of caution and keep the Config Space
read if pciehp_is_native() is called from hotplug_is_native().
Just to avoid any potential regressions since the fix is tagged for
stable.

I also searched lore for occurrences of the keywords...

  pcieport Runtime PM usage count underflow

...and did find quite a few reports, but this error message was just
a side effect and the reports were about completely different issues.
It does prove though that this bug has existed for a while!

Thanks Laurent for the report and Mario for root-causing this!

Lukas

Re: [PATCH v3 2/2] PCI: Fix runtime PM usage count underflow on device unplug

[Mario Limonciello]

On 6/23/25 12:23 PM, Lukas Wunner wrote:
> [cc += Mika]
> 
> On Mon, Jun 23, 2025 at 06:37:33AM -0500, Mario Limonciello wrote:
>> On 6/23/25 5:11 AM, Rafael J. Wysocki wrote:
>>> On Mon, Jun 23, 2025 at 12:05 PM Lukas Wunner <lukas@wunner.de> wrote:
>>>> pcie_portdrv_probe() and pcie_portdrv_remove() both call
>>>> pci_bridge_d3_possible() to determine whether to use runtime power
>>>> management.  The underlying assumption is that pci_bridge_d3_possible()
>>>> always returns the same value because otherwise a runtime PM reference
>>>> imbalance occurs.
>>>>
>>>> That assumption falls apart if the device is inaccessible on ->remove()
>>>> due to hot-unplug:  pci_bridge_d3_possible() calls pciehp_is_native(),
>>>> which accesses Config Space to determine whether the device is Hot-Plug
>>>> Capable.   An inaccessible device generally returns "all ones" for such
>>>> Config Read Requests.  Hence the device may seem Hot-Plug Capable on
>>>> ->remove() even though it wasn't on ->probe().
>>>>
>>>> Use the cached copy of the Hot-Plug Capable bit to avoid the Config Space
>>>> access and the resulting runtime PM ref imbalance.
>>>>
>>>> Signed-off-by: Lukas Wunner <lukas@wunner.de>
>>>
>>> Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>
>>
>> Tested-by: Mario Limonciello <mario.limonciello@amd.com>>
> 
> I ended up changing the patch significantly, so I did not include
> Rafael's Reviewed-by and Mario's Tested-by in the final patch.
> My apologies for this!
> 
> Looking at the commit which introduced the Config Space read,
> 5352a44a561d, I got the impression that Mika may have deliberately
> avoided using the is_hotplug_bridge flag.  Notably, is_hotplug_bridge
> is also set by check_hotplug_bridge() in acpiphp_glue.c, and his
> intention was probably to avoid matching those bridges in
> pciehp_is_native().
> 
> So I decided to err on the side of caution and keep the Config Space
> read if pciehp_is_native() is called from hotplug_is_native().
> Just to avoid any potential regressions since the fix is tagged for
> stable.
> 
> I also searched lore for occurrences of the keywords...
> 
>    pcieport Runtime PM usage count underflow
> 
> ...and did find quite a few reports, but this error message was just
> a side effect and the reports were about completely different issues.
> It does prove though that this bug has existed for a while!
> 
> Thanks Laurent for the report and Mario for root-causing this!
> 
> Lukas

Thanks Lukas!

I still do think my patch 1/2 in this series makes sense though, can you 
review that separately?