[PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable ports

[PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable ports

[Lukas Wunner]

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 returns "all ones", which is converted
to "all zeroes" by pcie_capability_read_dword().  Hence the device no
longer seems Hot-Plug Capable on ->remove() even though it was on
->probe().

The resulting runtime PM ref imbalance causes errors such as:

  pcieport 0000:02:04.0: Runtime PM usage count underflow!

The Hot-Plug Capable bit is cached in pci_dev->is_hotplug_bridge.
pci_bridge_d3_possible() only calls pciehp_is_native() if that flag is
set.  Re-checking the bit in pciehp_is_native() is thus unnecessary.

However pciehp_is_native() is also called from hotplug_is_native().  Move
the Config Space access to that function.  The function is only invoked
from acpiphp_glue.c, so move it there instead of keeping it in a publicly
visible header.

Fixes: 5352a44a561d ("PCI: pciehp: Make pciehp_is_native() stricter")
Reported-by: Laurent Bigonville <bigon@bigon.be>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220216
Reported-by: Mario Limonciello <mario.limonciello@amd.com>
Closes: https://lore.kernel.org/r/20250609020223.269407-3-superm1@kernel.org/
Link: https://lore.kernel.org/all/20250620025535.3425049-3-superm1@kernel.org/T/#u
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v4.18+
---
 drivers/pci/hotplug/acpiphp_glue.c | 15 +++++++++++++++
 drivers/pci/pci-acpi.c             |  5 -----
 include/linux/pci_hotplug.h        |  4 ----
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 5b1f271c6034..ae2bb8970f63 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -50,6 +50,21 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus);
 static void hotplug_event(u32 type, struct acpiphp_context *context);
 static void free_bridge(struct kref *kref);
 
+static bool hotplug_is_native(struct pci_dev *bridge)
+{
+	u32 slot_cap;
+
+	pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
+
+	if (slot_cap & PCI_EXP_SLTCAP_HPC && pciehp_is_native(bridge))
+		return true;
+
+	if (shpchp_is_native(bridge))
+		return true;
+
+	return false;
+}
+
 /**
  * acpiphp_init_context - Create hotplug context and grab a reference to it.
  * @adev: ACPI device object to create the context for.
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index b78e0e417324..57bce9cc8a38 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -816,15 +816,10 @@ 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))
-		return false;
-
 	if (pcie_ports_native)
 		return true;
 
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index ec77ccf1fc4d..02efeea62b25 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -102,8 +102,4 @@ static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
 static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; }
 #endif
 
-static inline bool hotplug_is_native(struct pci_dev *bridge)
-{
-	return pciehp_is_native(bridge) || shpchp_is_native(bridge);
-}
 #endif
-- 
2.47.2

Re: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable ports

[Rafael J. Wysocki]

On Mon, Jun 23, 2025 at 7:08 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 returns "all ones", which is converted
> to "all zeroes" by pcie_capability_read_dword().  Hence the device no
> longer seems Hot-Plug Capable on ->remove() even though it was on
> ->probe().
>
> The resulting runtime PM ref imbalance causes errors such as:
>
>   pcieport 0000:02:04.0: Runtime PM usage count underflow!
>
> The Hot-Plug Capable bit is cached in pci_dev->is_hotplug_bridge.
> pci_bridge_d3_possible() only calls pciehp_is_native() if that flag is
> set.  Re-checking the bit in pciehp_is_native() is thus unnecessary.
>
> However pciehp_is_native() is also called from hotplug_is_native().  Move
> the Config Space access to that function.  The function is only invoked
> from acpiphp_glue.c, so move it there instead of keeping it in a publicly
> visible header.
>
> Fixes: 5352a44a561d ("PCI: pciehp: Make pciehp_is_native() stricter")
> Reported-by: Laurent Bigonville <bigon@bigon.be>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220216
> Reported-by: Mario Limonciello <mario.limonciello@amd.com>
> Closes: https://lore.kernel.org/r/20250609020223.269407-3-superm1@kernel.org/
> Link: https://lore.kernel.org/all/20250620025535.3425049-3-superm1@kernel.org/T/#u
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v4.18+

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

Thanks!

> ---
>  drivers/pci/hotplug/acpiphp_glue.c | 15 +++++++++++++++
>  drivers/pci/pci-acpi.c             |  5 -----
>  include/linux/pci_hotplug.h        |  4 ----
>  3 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 5b1f271c6034..ae2bb8970f63 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -50,6 +50,21 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus);
>  static void hotplug_event(u32 type, struct acpiphp_context *context);
>  static void free_bridge(struct kref *kref);
>
> +static bool hotplug_is_native(struct pci_dev *bridge)
> +{
> +       u32 slot_cap;
> +
> +       pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
> +
> +       if (slot_cap & PCI_EXP_SLTCAP_HPC && pciehp_is_native(bridge))
> +               return true;
> +
> +       if (shpchp_is_native(bridge))
> +               return true;
> +
> +       return false;
> +}
> +
>  /**
>   * acpiphp_init_context - Create hotplug context and grab a reference to it.
>   * @adev: ACPI device object to create the context for.
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index b78e0e417324..57bce9cc8a38 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -816,15 +816,10 @@ 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))
> -               return false;
> -
>         if (pcie_ports_native)
>                 return true;
>
> diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
> index ec77ccf1fc4d..02efeea62b25 100644
> --- a/include/linux/pci_hotplug.h
> +++ b/include/linux/pci_hotplug.h
> @@ -102,8 +102,4 @@ static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
>  static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; }
>  #endif
>
> -static inline bool hotplug_is_native(struct pci_dev *bridge)
> -{
> -       return pciehp_is_native(bridge) || shpchp_is_native(bridge);
> -}
>  #endif
> --
> 2.47.2
>

Re: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable ports

[Mario Limonciello]

On 6/23/2025 12:08 PM, Lukas Wunner 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 returns "all ones", which is converted
> to "all zeroes" by pcie_capability_read_dword().  Hence the device no
> longer seems Hot-Plug Capable on ->remove() even though it was on
> ->probe().
> 
> The resulting runtime PM ref imbalance causes errors such as:
> 
>    pcieport 0000:02:04.0: Runtime PM usage count underflow!
> 
> The Hot-Plug Capable bit is cached in pci_dev->is_hotplug_bridge.
> pci_bridge_d3_possible() only calls pciehp_is_native() if that flag is
> set.  Re-checking the bit in pciehp_is_native() is thus unnecessary.
> 
> However pciehp_is_native() is also called from hotplug_is_native().  Move
> the Config Space access to that function.  The function is only invoked
> from acpiphp_glue.c, so move it there instead of keeping it in a publicly
> visible header.
> 
> Fixes: 5352a44a561d ("PCI: pciehp: Make pciehp_is_native() stricter")
> Reported-by: Laurent Bigonville <bigon@bigon.be>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220216
> Reported-by: Mario Limonciello <mario.limonciello@amd.com>
> Closes: https://lore.kernel.org/r/20250609020223.269407-3-superm1@kernel.org/
> Link: https://lore.kernel.org/all/20250620025535.3425049-3-superm1@kernel.org/T/#u
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v4.18+

Tested-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   drivers/pci/hotplug/acpiphp_glue.c | 15 +++++++++++++++
>   drivers/pci/pci-acpi.c             |  5 -----
>   include/linux/pci_hotplug.h        |  4 ----
>   3 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 5b1f271c6034..ae2bb8970f63 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -50,6 +50,21 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus);
>   static void hotplug_event(u32 type, struct acpiphp_context *context);
>   static void free_bridge(struct kref *kref);
>   
> +static bool hotplug_is_native(struct pci_dev *bridge)
> +{
> +	u32 slot_cap;
> +
> +	pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
> +
> +	if (slot_cap & PCI_EXP_SLTCAP_HPC && pciehp_is_native(bridge))
> +		return true;
> +
> +	if (shpchp_is_native(bridge))
> +		return true;
> +
> +	return false;
> +}
> +
>   /**
>    * acpiphp_init_context - Create hotplug context and grab a reference to it.
>    * @adev: ACPI device object to create the context for.
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index b78e0e417324..57bce9cc8a38 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -816,15 +816,10 @@ 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))
> -		return false;
> -
>   	if (pcie_ports_native)
>   		return true;
>   
> diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
> index ec77ccf1fc4d..02efeea62b25 100644
> --- a/include/linux/pci_hotplug.h
> +++ b/include/linux/pci_hotplug.h
> @@ -102,8 +102,4 @@ static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
>   static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; }
>   #endif
>   
> -static inline bool hotplug_is_native(struct pci_dev *bridge)
> -{
> -	return pciehp_is_native(bridge) || shpchp_is_native(bridge);
> -}
>   #endif

Re: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable ports

[Mika Westerberg]

On Mon, Jun 23, 2025 at 07:08:20PM +0200, Lukas Wunner 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 returns "all ones", which is converted
> to "all zeroes" by pcie_capability_read_dword().  Hence the device no
> longer seems Hot-Plug Capable on ->remove() even though it was on
> ->probe().
> 
> The resulting runtime PM ref imbalance causes errors such as:
> 
>   pcieport 0000:02:04.0: Runtime PM usage count underflow!
> 
> The Hot-Plug Capable bit is cached in pci_dev->is_hotplug_bridge.
> pci_bridge_d3_possible() only calls pciehp_is_native() if that flag is
> set.  Re-checking the bit in pciehp_is_native() is thus unnecessary.
> 
> However pciehp_is_native() is also called from hotplug_is_native().  Move
> the Config Space access to that function.  The function is only invoked
> from acpiphp_glue.c, so move it there instead of keeping it in a publicly
> visible header.
> 
> Fixes: 5352a44a561d ("PCI: pciehp: Make pciehp_is_native() stricter")
> Reported-by: Laurent Bigonville <bigon@bigon.be>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220216
> Reported-by: Mario Limonciello <mario.limonciello@amd.com>
> Closes: https://lore.kernel.org/r/20250609020223.269407-3-superm1@kernel.org/
> Link: https://lore.kernel.org/all/20250620025535.3425049-3-superm1@kernel.org/T/#u
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v4.18+

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable ports

[Bjorn Helgaas]

On Mon, Jun 23, 2025 at 07:08:20PM +0200, Lukas Wunner 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 returns "all ones", which is converted
> to "all zeroes" by pcie_capability_read_dword().  Hence the device no
> longer seems Hot-Plug Capable on ->remove() even though it was on
> ->probe().

This is pretty subtle; thanks for chasing it down.

It doesn't look like anything in pci_bridge_d3_possible() should 
change over the life of the device, although acpi_pci_bridge_d3() is
non-trivial.

Should we consider calling pci_bridge_d3_possible() only once and
caching the result?  We already call it in pci_pm_init() and save the
result in dev->bridge_d3.  That member can be changed by
pci_bridge_d3_update(), but we could add another copy that we never
update after pci_pm_init().

I worry a little that the fix is equally subtle and we could easily
reintroduce this issue with future code reorganization.

> The resulting runtime PM ref imbalance causes errors such as:
> 
>   pcieport 0000:02:04.0: Runtime PM usage count underflow!
> 
> The Hot-Plug Capable bit is cached in pci_dev->is_hotplug_bridge.
> pci_bridge_d3_possible() only calls pciehp_is_native() if that flag is
> set.  Re-checking the bit in pciehp_is_native() is thus unnecessary.
> 
> However pciehp_is_native() is also called from hotplug_is_native().  Move
> the Config Space access to that function.  The function is only invoked
> from acpiphp_glue.c, so move it there instead of keeping it in a publicly
> visible header.
> 
> Fixes: 5352a44a561d ("PCI: pciehp: Make pciehp_is_native() stricter")
> Reported-by: Laurent Bigonville <bigon@bigon.be>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220216
> Reported-by: Mario Limonciello <mario.limonciello@amd.com>
> Closes: https://lore.kernel.org/r/20250609020223.269407-3-superm1@kernel.org/
> Link: https://lore.kernel.org/all/20250620025535.3425049-3-superm1@kernel.org/T/#u
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v4.18+
> ---
>  drivers/pci/hotplug/acpiphp_glue.c | 15 +++++++++++++++
>  drivers/pci/pci-acpi.c             |  5 -----
>  include/linux/pci_hotplug.h        |  4 ----
>  3 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 5b1f271c6034..ae2bb8970f63 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -50,6 +50,21 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus);
>  static void hotplug_event(u32 type, struct acpiphp_context *context);
>  static void free_bridge(struct kref *kref);
>  
> +static bool hotplug_is_native(struct pci_dev *bridge)
> +{
> +	u32 slot_cap;
> +
> +	pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
> +
> +	if (slot_cap & PCI_EXP_SLTCAP_HPC && pciehp_is_native(bridge))
> +		return true;
> +
> +	if (shpchp_is_native(bridge))
> +		return true;
> +
> +	return false;
> +}
> +
>  /**
>   * acpiphp_init_context - Create hotplug context and grab a reference to it.
>   * @adev: ACPI device object to create the context for.
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index b78e0e417324..57bce9cc8a38 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -816,15 +816,10 @@ 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))
> -		return false;
> -
>  	if (pcie_ports_native)
>  		return true;
>  
> diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
> index ec77ccf1fc4d..02efeea62b25 100644
> --- a/include/linux/pci_hotplug.h
> +++ b/include/linux/pci_hotplug.h
> @@ -102,8 +102,4 @@ static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
>  static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; }
>  #endif
>  
> -static inline bool hotplug_is_native(struct pci_dev *bridge)
> -{
> -	return pciehp_is_native(bridge) || shpchp_is_native(bridge);
> -}
>  #endif
> -- 
> 2.47.2
> 

Re: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable ports

[Lukas Wunner]

On Tue, Jun 24, 2025 at 09:24:07AM -0500, Bjorn Helgaas wrote:
> On Mon, Jun 23, 2025 at 07:08:20PM +0200, Lukas Wunner 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 returns "all ones", which is converted
> > to "all zeroes" by pcie_capability_read_dword().  Hence the device no
> > longer seems Hot-Plug Capable on ->remove() even though it was on
> > ->probe().
> 
> This is pretty subtle; thanks for chasing it down.
> 
> It doesn't look like anything in pci_bridge_d3_possible() should 
> change over the life of the device, although acpi_pci_bridge_d3() is
> non-trivial.
> 
> Should we consider calling pci_bridge_d3_possible() only once and
> caching the result?  We already call it in pci_pm_init() and save the
> result in dev->bridge_d3.  That member can be changed by
> pci_bridge_d3_update(), but we could add another copy that we never
> update after pci_pm_init().

If we did that, I think we'd still want to have a WARN_ON() like this in
pcie_portdrv_remove():

+	WARN_ON(dev->bridge_d3_orig != pci_bridge_d3_possible(dev));
+
+	if (dev->bridge_d3_orig) {
-	if (pci_bridge_d3_possible(dev)) {

Because without the WARN_ON(), such bugs would fly under the radar.

However currently we get the WARN_ON() for free because of the runtime PM
refcount underflow.

So caching the original return value of pci_bridge_d3_possible(dev)
wouldn't be a net positive.

Also note that the bug isn't catastrophic:  The struct device is about
to be free()'d anyway because it's been hot-removed.  It's just the
annoying warning message that we want to get rid of.

But maybe we should amend the kernel-doc of pci_bridge_d3_possible()
to clearly state that the return value must be constant across the
entire lifetime of the device.  For me that's obvious because I was
involved when the code was originally conceived, but I realized upon
seeing Mario's attempts to solve this that it may not be obvious at all
for anyone else.

Thanks,

Lukas

Re: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable ports

[Rafael J. Wysocki]

On Wed, Jun 25, 2025 at 9:37 AM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Tue, Jun 24, 2025 at 09:24:07AM -0500, Bjorn Helgaas wrote:
> > On Mon, Jun 23, 2025 at 07:08:20PM +0200, Lukas Wunner 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 returns "all ones", which is converted
> > > to "all zeroes" by pcie_capability_read_dword().  Hence the device no
> > > longer seems Hot-Plug Capable on ->remove() even though it was on
> > > ->probe().
> >
> > This is pretty subtle; thanks for chasing it down.
> >
> > It doesn't look like anything in pci_bridge_d3_possible() should
> > change over the life of the device, although acpi_pci_bridge_d3() is
> > non-trivial.
> >
> > Should we consider calling pci_bridge_d3_possible() only once and
> > caching the result?  We already call it in pci_pm_init() and save the
> > result in dev->bridge_d3.  That member can be changed by
> > pci_bridge_d3_update(), but we could add another copy that we never
> > update after pci_pm_init().
>
> If we did that, I think we'd still want to have a WARN_ON() like this in
> pcie_portdrv_remove():
>
> +       WARN_ON(dev->bridge_d3_orig != pci_bridge_d3_possible(dev));
> +
> +       if (dev->bridge_d3_orig) {
> -       if (pci_bridge_d3_possible(dev)) {
>
> Because without the WARN_ON(), such bugs would fly under the radar.
>
> However currently we get the WARN_ON() for free because of the runtime PM
> refcount underflow.
>
> So caching the original return value of pci_bridge_d3_possible(dev)
> wouldn't be a net positive.
>
> Also note that the bug isn't catastrophic:  The struct device is about
> to be free()'d anyway because it's been hot-removed.  It's just the
> annoying warning message that we want to get rid of.
>
> But maybe we should amend the kernel-doc of pci_bridge_d3_possible()
> to clearly state that the return value must be constant across the
> entire lifetime of the device.

Yes, please!

Re: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable ports

[Bjorn Helgaas]

On Wed, Jun 25, 2025 at 09:37:38AM +0200, Lukas Wunner wrote:
> On Tue, Jun 24, 2025 at 09:24:07AM -0500, Bjorn Helgaas wrote:
> > On Mon, Jun 23, 2025 at 07:08:20PM +0200, Lukas Wunner 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 returns "all ones", which is converted
> > > to "all zeroes" by pcie_capability_read_dword().  Hence the device no
> > > longer seems Hot-Plug Capable on ->remove() even though it was on
> > > ->probe().
> > 
> > This is pretty subtle; thanks for chasing it down.
> > 
> > It doesn't look like anything in pci_bridge_d3_possible() should 
> > change over the life of the device, although acpi_pci_bridge_d3() is
> > non-trivial.
> > 
> > Should we consider calling pci_bridge_d3_possible() only once and
> > caching the result?  We already call it in pci_pm_init() and save the
> > result in dev->bridge_d3.  That member can be changed by
> > pci_bridge_d3_update(), but we could add another copy that we never
> > update after pci_pm_init().
> 
> If we did that, I think we'd still want to have a WARN_ON() like this in
> pcie_portdrv_remove():
> 
> +	WARN_ON(dev->bridge_d3_orig != pci_bridge_d3_possible(dev));
> +
> +	if (dev->bridge_d3_orig) {
> -	if (pci_bridge_d3_possible(dev)) {
> 
> Because without the WARN_ON(), such bugs would fly under the radar.
> 
> However currently we get the WARN_ON() for free because of the runtime PM
> refcount underflow.
> 
> So caching the original return value of pci_bridge_d3_possible(dev)
> wouldn't be a net positive.

Fair point.  pci_bridge_d3_possible() is mainly used by portdrv, and 
keeping another copy in the pci_dev does seem like overkill.

If the point is to ensure that the runtime PM setup done by
pcie_portdrv_probe() is undone by pcie_portdrv_remove() and
pcie_portdrv_shutdown(), maybe portdrv should remember what it did,
e.g., call pci_bridge_d3_possible() once in .probe() and save the
result for use in .remove() and .shutdown().

That's what I expect drivers to do in general for cleaning up things
in .remove(): it's the driver's problem to remember what needs to be
cleaned up.

But I feel like I'm missing your point about bugs flying under the
radar.  Having portdrv keep track of whether it did runtime PM setup
(i.e., the pci_bridge_d3_possible() state at .probe()-time) is
functionally the same as having struct pci_dev keep track of it, so
the bugs you're referring to could still fly under the radar.

Bjorn

Re: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable ports

[Lukas Wunner]

On Wed, Jun 25, 2025 at 02:32:31PM -0500, Bjorn Helgaas wrote:
> On Wed, Jun 25, 2025 at 09:37:38AM +0200, Lukas Wunner wrote:
> > On Tue, Jun 24, 2025 at 09:24:07AM -0500, Bjorn Helgaas wrote:
> > > It doesn't look like anything in pci_bridge_d3_possible() should 
> > > change over the life of the device, although acpi_pci_bridge_d3() is
> > > non-trivial.
> > > 
> > > Should we consider calling pci_bridge_d3_possible() only once and
> > > caching the result?  We already call it in pci_pm_init() and save the
> > > result in dev->bridge_d3.  That member can be changed by
> > > pci_bridge_d3_update(), but we could add another copy that we never
> > > update after pci_pm_init().
> > 
> > If we did that, I think we'd still want to have a WARN_ON() like this in
> > pcie_portdrv_remove():
> > 
> > +	WARN_ON(dev->bridge_d3_orig != pci_bridge_d3_possible(dev));
> > +
> > +	if (dev->bridge_d3_orig) {
> > -	if (pci_bridge_d3_possible(dev)) {
> > 
> > Because without the WARN_ON(), such bugs would fly under the radar.
> > 
> > However currently we get the WARN_ON() for free because of the runtime PM
> > refcount underflow.
> > 
> > So caching the original return value of pci_bridge_d3_possible(dev)
> > wouldn't be a net positive.
[...]
> But I feel like I'm missing your point about bugs flying under the
> radar.  Having portdrv keep track of whether it did runtime PM setup
> (i.e., the pci_bridge_d3_possible() state at .probe()-time) is
> functionally the same as having struct pci_dev keep track of it, so
> the bugs you're referring to could still fly under the radar.

So the return value of pci_bridge_d3_possible() should never change
over the lifetime of a device.  We're also invoking that function
from pci_bridge_d3_update() and the logic would no longer work
if the return value changed.

My point is that we're currently verifying that the return value
hasn't changed by regenerating it in pcie_portdrv_remove().
If it *has* changed, the runtime PM ref imbalance occurs and we get
a warning message.
If we instead cached the value in pcie_portdrv_probe(), we wouldn't
have found this bug.

Does that make sense?

Thanks,

Lukas

Re: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable ports

[Lukas Wunner]

On Tue, Jun 24, 2025 at 09:24:07AM -0500, Bjorn Helgaas wrote:
> On Mon, Jun 23, 2025 at 07:08:20PM +0200, Lukas Wunner 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 returns "all ones", which is converted
> > to "all zeroes" by pcie_capability_read_dword().  Hence the device no
> > longer seems Hot-Plug Capable on ->remove() even though it was on
> > ->probe().
> 
> This is pretty subtle; thanks for chasing it down.
> 
> It doesn't look like anything in pci_bridge_d3_possible() should 
> change over the life of the device, although acpi_pci_bridge_d3() is
> non-trivial.
> 
> Should we consider calling pci_bridge_d3_possible() only once and
> caching the result?  We already call it in pci_pm_init() and save the
> result in dev->bridge_d3.  That member can be changed by
> pci_bridge_d3_update(), but we could add another copy that we never
> update after pci_pm_init().
> 
> I worry a little that the fix is equally subtle and we could easily
> reintroduce this issue with future code reorganization.

I think this fix makes sense regardless of whether or not
the return value of pci_bridge_d3_possible() is cached:

Right now pciehp_is_native() reads the Hot-Plug	Capable	bit from
the register even though the bit is cached in pci_dev->is_hotplug_bridge
and pci_bridge_d3_possible() only calls pciehp_is_native() if that flag
is set.  In other words, pciehp_is_native() is re-checking the condition
under which it was called.  That's just nonsensical and superfluous.

There's only one other caller of pciehp_is_native() and that's
hotplug_is_native().  Only that other caller needs the register read,
so it should be moved there.

So I think the question of whether the pci_bridge_d3_possible() return
value should be cached is orthogonal to this patch.

Thanks,

Lukas

Re: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable ports

[Rafael J. Wysocki]

On Thu, Jun 26, 2025 at 7:30 AM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Tue, Jun 24, 2025 at 09:24:07AM -0500, Bjorn Helgaas wrote:
> > On Mon, Jun 23, 2025 at 07:08:20PM +0200, Lukas Wunner 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 returns "all ones", which is converted
> > > to "all zeroes" by pcie_capability_read_dword().  Hence the device no
> > > longer seems Hot-Plug Capable on ->remove() even though it was on
> > > ->probe().
> >
> > This is pretty subtle; thanks for chasing it down.
> >
> > It doesn't look like anything in pci_bridge_d3_possible() should
> > change over the life of the device, although acpi_pci_bridge_d3() is
> > non-trivial.
> >
> > Should we consider calling pci_bridge_d3_possible() only once and
> > caching the result?  We already call it in pci_pm_init() and save the
> > result in dev->bridge_d3.  That member can be changed by
> > pci_bridge_d3_update(), but we could add another copy that we never
> > update after pci_pm_init().
> >
> > I worry a little that the fix is equally subtle and we could easily
> > reintroduce this issue with future code reorganization.
>
> I think this fix makes sense regardless of whether or not
> the return value of pci_bridge_d3_possible() is cached:
>
> Right now pciehp_is_native() reads the Hot-Plug Capable bit from
> the register even though the bit is cached in pci_dev->is_hotplug_bridge
> and pci_bridge_d3_possible() only calls pciehp_is_native() if that flag
> is set.  In other words, pciehp_is_native() is re-checking the condition
> under which it was called.  That's just nonsensical and superfluous.

Agreed.

> There's only one other caller of pciehp_is_native() and that's
> hotplug_is_native().  Only that other caller needs the register read,
> so it should be moved there.
>
> So I think the question of whether the pci_bridge_d3_possible() return
> value should be cached is orthogonal to this patch.

+1

Re: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable ports

[Ilpo Järvinen]

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

On Mon, 23 Jun 2025, Lukas Wunner 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 returns "all ones", which is converted
> to "all zeroes" by pcie_capability_read_dword().  Hence the device no
> longer seems Hot-Plug Capable on ->remove() even though it was on
> ->probe().
> 
> The resulting runtime PM ref imbalance causes errors such as:
> 
>   pcieport 0000:02:04.0: Runtime PM usage count underflow!
> 
> The Hot-Plug Capable bit is cached in pci_dev->is_hotplug_bridge.
> pci_bridge_d3_possible() only calls pciehp_is_native() if that flag is
> set.  Re-checking the bit in pciehp_is_native() is thus unnecessary.
> 
> However pciehp_is_native() is also called from hotplug_is_native().  Move
> the Config Space access to that function.  The function is only invoked
> from acpiphp_glue.c, so move it there instead of keeping it in a publicly
> visible header.
> 
> Fixes: 5352a44a561d ("PCI: pciehp: Make pciehp_is_native() stricter")
> Reported-by: Laurent Bigonville <bigon@bigon.be>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220216
> Reported-by: Mario Limonciello <mario.limonciello@amd.com>
> Closes: https://lore.kernel.org/r/20250609020223.269407-3-superm1@kernel.org/
> Link: https://lore.kernel.org/all/20250620025535.3425049-3-superm1@kernel.org/T/#u
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v4.18+

Hmm, I suspect this fixes the root cause for TB unplug while suspended 
which did have these RPM underflows. I saw them in logs while we were 
looking into LBMS interactions with the Target Speed quirk (that most 
visibly caused extra 60s delay, which was solved).

Back then, I briefly tried find solution to the underflow problem too but 
couldn't come up anything useful and as it looked relatively harmless I 
ended up postponing looking deeper into it.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

> ---
>  drivers/pci/hotplug/acpiphp_glue.c | 15 +++++++++++++++
>  drivers/pci/pci-acpi.c             |  5 -----
>  include/linux/pci_hotplug.h        |  4 ----
>  3 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 5b1f271c6034..ae2bb8970f63 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -50,6 +50,21 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus);
>  static void hotplug_event(u32 type, struct acpiphp_context *context);
>  static void free_bridge(struct kref *kref);
>  
> +static bool hotplug_is_native(struct pci_dev *bridge)
> +{
> +	u32 slot_cap;
> +
> +	pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
> +
> +	if (slot_cap & PCI_EXP_SLTCAP_HPC && pciehp_is_native(bridge))
> +		return true;
> +
> +	if (shpchp_is_native(bridge))
> +		return true;
> +
> +	return false;
> +}
> +
>  /**
>   * acpiphp_init_context - Create hotplug context and grab a reference to it.
>   * @adev: ACPI device object to create the context for.
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index b78e0e417324..57bce9cc8a38 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -816,15 +816,10 @@ 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))
> -		return false;
> -
>  	if (pcie_ports_native)
>  		return true;
>  
> diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
> index ec77ccf1fc4d..02efeea62b25 100644
> --- a/include/linux/pci_hotplug.h
> +++ b/include/linux/pci_hotplug.h
> @@ -102,8 +102,4 @@ static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
>  static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; }
>  #endif
>  
> -static inline bool hotplug_is_native(struct pci_dev *bridge)
> -{
> -	return pciehp_is_native(bridge) || shpchp_is_native(bridge);
> -}
>  #endif
> 

Re: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable ports

[Bjorn Helgaas]

[+cc Ilpo]

On Mon, Jun 23, 2025 at 07:08:20PM +0200, Lukas Wunner 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 returns "all ones", which is converted
> to "all zeroes" by pcie_capability_read_dword().  Hence the device no
> longer seems Hot-Plug Capable on ->remove() even though it was on
> ->probe().
> 
> The resulting runtime PM ref imbalance causes errors such as:
> 
>   pcieport 0000:02:04.0: Runtime PM usage count underflow!
> 
> The Hot-Plug Capable bit is cached in pci_dev->is_hotplug_bridge.

pci_dev->is_hotplug_bridge is not just a cache of PCI_EXP_SLTCAP_HPC;
it can also be set in two other cases.  Example below.

> pci_bridge_d3_possible() only calls pciehp_is_native() if that flag is
> set.  Re-checking the bit in pciehp_is_native() is thus unnecessary.
> 
> However pciehp_is_native() is also called from hotplug_is_native().  Move
> the Config Space access to that function.  The function is only invoked
> from acpiphp_glue.c, so move it there instead of keeping it in a publicly
> visible header.
> 
> Fixes: 5352a44a561d ("PCI: pciehp: Make pciehp_is_native() stricter")
> Reported-by: Laurent Bigonville <bigon@bigon.be>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220216
> Reported-by: Mario Limonciello <mario.limonciello@amd.com>
> Closes: https://lore.kernel.org/r/20250609020223.269407-3-superm1@kernel.org/
> Link: https://lore.kernel.org/all/20250620025535.3425049-3-superm1@kernel.org/T/#u
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v4.18+
> ---
>  drivers/pci/hotplug/acpiphp_glue.c | 15 +++++++++++++++
>  drivers/pci/pci-acpi.c             |  5 -----
>  include/linux/pci_hotplug.h        |  4 ----
>  3 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 5b1f271c6034..ae2bb8970f63 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -50,6 +50,21 @@ static void acpiphp_sanitize_bus(struct pci_bus *bus);
>  static void hotplug_event(u32 type, struct acpiphp_context *context);
>  static void free_bridge(struct kref *kref);
>  
> +static bool hotplug_is_native(struct pci_dev *bridge)
> +{
> +	u32 slot_cap;
> +
> +	pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
> +
> +	if (slot_cap & PCI_EXP_SLTCAP_HPC && pciehp_is_native(bridge))
> +		return true;
> +
> +	if (shpchp_is_native(bridge))
> +		return true;
> +
> +	return false;
> +}
> +
>  /**
>   * acpiphp_init_context - Create hotplug context and grab a reference to it.
>   * @adev: ACPI device object to create the context for.
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index b78e0e417324..57bce9cc8a38 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -816,15 +816,10 @@ 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))
> -		return false;

If we have a bridge where bridge->is_hotplug_bridge=1 from the acpiphp
check_hotplug_bridge() or quirk_hotplug_bridge(), and the bridge has
no Slot or a Slot with PCI_EXP_SLTCAP_HPC=0, we previously returned
false here but may now return true.  That seems like a problem, but
maybe I'm missing the reason why it is not an issue.  

Previously pciehp_is_native() depended on the bridge, but now it will
only depend on the negotiation with the platform for native PCIe
hotplug control, so it definitely changes the semantics.

If the new semantics are what we want, that would be great because I
think we could probably set host->native_pcie_hotplug up front based
on CONFIG_HOTPLUG_PCI_PCIE and pcie_ports_native, and
pciehp_is_native() would collapse to just an accessor for
host->native_pcie_hotplug.

>  	if (pcie_ports_native)
>  		return true;
>  
> diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
> index ec77ccf1fc4d..02efeea62b25 100644
> --- a/include/linux/pci_hotplug.h
> +++ b/include/linux/pci_hotplug.h
> @@ -102,8 +102,4 @@ static inline bool pciehp_is_native(struct pci_dev *bridge) { return true; }
>  static inline bool shpchp_is_native(struct pci_dev *bridge) { return true; }
>  #endif
>  
> -static inline bool hotplug_is_native(struct pci_dev *bridge)
> -{
> -	return pciehp_is_native(bridge) || shpchp_is_native(bridge);
> -}
>  #endif
> -- 
> 2.47.2
> 

Re: [PATCH] PCI/ACPI: Fix runtime PM ref imbalance on hot-plug capable ports

[Lukas Wunner]

On Thu, Jun 26, 2025 at 09:56:07PM -0500, Bjorn Helgaas wrote:
> On Mon, Jun 23, 2025 at 07:08:20PM +0200, Lukas Wunner 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 returns "all ones", which is converted
> > to "all zeroes" by pcie_capability_read_dword().  Hence the device no
> > longer seems Hot-Plug Capable on ->remove() even though it was on
> > ->probe().
> > 
> > The resulting runtime PM ref imbalance causes errors such as:
> > 
> >   pcieport 0000:02:04.0: Runtime PM usage count underflow!
> > 
> > The Hot-Plug Capable bit is cached in pci_dev->is_hotplug_bridge.
> 
> pci_dev->is_hotplug_bridge is not just a cache of PCI_EXP_SLTCAP_HPC;
> it can also be set in two other cases.  Example below.
[...]
> If we have a bridge where bridge->is_hotplug_bridge=1 from the acpiphp
> check_hotplug_bridge() or quirk_hotplug_bridge(), and the bridge has
> no Slot or a Slot with PCI_EXP_SLTCAP_HPC=0, we previously returned
> false here but may now return true.  That seems like a problem, but
> maybe I'm missing the reason why it is not an issue.

You're absolutely right, thanks for spotting this.
I'm respinning the patch as part of a larger series:

https://lore.kernel.org/r/cover.1752390101.git.lukas@wunner.de

It seems to me we should cache PCI_EXP_SLTCAP_HPC, rather than
the result of pci_bridge_d3_possible().  That allows cleaning up
is_hotplug_bridge usage and fix a few bugs in that area.

(As an aside, quirk_hotplug_bridge() is for a Conventional PCI hotplug
bridge.  Hence I'm mentioning Conventional PCI in addition to ACPI slots
in a number of code comments and commit messages in the above-linked
series.)

> If the new semantics are what we want, that would be great because I
> think we could probably set host->native_pcie_hotplug up front based
> on CONFIG_HOTPLUG_PCI_PCIE and pcie_ports_native, and
> pciehp_is_native() would collapse to just an accessor for
> host->native_pcie_hotplug.

I've looked into it, see patch [5/5] of the series.  It turns out,
we can get rid of the pcie_ports_native check in pciehp_is_native(),
but not the CONFIG_HOTPLUG_PCI_PCIE check.  So it's only a partial
solution.  And it has the downside that the pcie_ports_native
variable is no longer confined to the PCI core, it now leaks into
ACPI PCI code.  Which means future canges in that area will involve
ACPI and require an ack from Rafael.  Your decision whether that's a
net-positive.  Feel free to ignore the patch if you think it's not!

Thanks!

Lukas