[PATCH] PCI: pciehp: Re-read Slot Control when cached HPIE is clear

[PATCH] PCI: pciehp: Re-read Slot Control when cached HPIE is clear

[Zhu Qiyu]

pciehp_isr() dismisses an interrupt as not ours (IRQ_NONE) when the
Hot-Plug Interrupt Enable (HPIE) bit is clear in ctrl->slot_ctrl, the
driver's cached copy of the Slot Control register.

This can drop hotplug events during PCIe hot-add.  On an AMD EPYC
platform, sometimes hot-adding an PCIe device delivered only the
Presence Detect Changed (PDC) event while the Data Link Layer State
Changed (DLSC) event was lost. Afterwards lspci still showed the
Link State Changed bit latched in Slot Status, i.e. the DLSC interrupt
was never serviced:

  pcieport 0000:c0:03.4: pciehp: pending interrupts 0x0008 from Slot Status
  pcieport 0000:c0:03.4: pciehp: Slot(71): Card present

  lspci -vvv after hot-add:
    SltCtl: Enable: ... PresDet+ HPIrq+ LinkChg+
    SltSta: Changed: MRL- PresDet- LinkState+

Only PDC (Slot Status 0x0008) was reported; LinkState+ (DLSC) stayed
latched and unhandled. This will affect the device's next hot-plug
operation, causing the state machine to become inconsistent.

The root cause is a stale cache.  Platform firmware transiently clears
HPIE in hardware and restores it shortly afterwards.  While handling the
PDC event the driver updates the slot indicators via
pciehp_set_indicators(), which issues a Slot Control read-modify-write
(pcie_do_write_cmd()).  If that read occurs while firmware has HPIE
cleared, HPIE=0 is read back and stored into ctrl->slot_ctrl.  Firmware
then restores HPIE=1 in hardware, but the cached HPIE=0 remains, so the
following DLSC interrupt is misjudged as not ours and dropped.

The hardware HPIE bit is correct in this case, so consult hardware rather
than the possibly stale cache.  To keep the fast path cheap and avoid any
behavioural change for low-power ports, only re-read Slot Control when the
cache says HPIE is disabled and the port is in D0 (accessible and
enabled).  In a lower-power state the port may be inaccessible, so the
cached value remains authoritative and the existing early return is
preserved.  Surprise removal is handled via PCI_POSSIBLE_ERROR().

This changes behaviour only in the previously broken case (D0, cached
HPIE clear but hardware HPIE set).  Poll mode, the shared-INTx early
return while suspended, and the D3hot deferral to the IRQ thread are
unchanged.

Signed-off-by: Zhu Qiyu <qiyuzhu2@amd.com>
---
 drivers/pci/hotplug/pciehp_hpc.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 4c62140a3cb44..df977d4af4056 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -628,13 +628,34 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 	u16 status, events = 0;
 
 	/*
-	 * Interrupts only occur in D3hot or shallower and only if enabled
-	 * in the Slot Control register (PCIe r4.0, sec 6.7.3.4).
+	 * Interrupts only occur in D3hot or shallower (PCIe r4.0, sec 6.7.3.4).
 	 */
-	if (pdev->current_state == PCI_D3cold ||
-	    (!(ctrl->slot_ctrl & PCI_EXP_SLTCTL_HPIE) && !pciehp_poll_mode))
+	if (pdev->current_state == PCI_D3cold)
 		return IRQ_NONE;
 
+	/*
+	 * Interrupts are only sent if enabled in the Slot Control register
+	 * (PCIe r4.0, sec 6.7.3.4).  ctrl->slot_ctrl caches that register, but
+	 * the cached Hot-Plug Interrupt Enable bit can fall transiently out of
+	 * sync with the hardware if platform firmware clears it behind the
+	 * driver's back (e.g. concurrently with a Slot Control read-modify-
+	 * write).  While the port is in D0 it is accessible, so re-read the
+	 * register from hardware before dismissing the interrupt as not ours.
+	 * In a low-power state the port may be inaccessible and the cached
+	 * value is authoritative.
+	 */
+	if (!pciehp_poll_mode && !(ctrl->slot_ctrl & PCI_EXP_SLTCTL_HPIE)) {
+		u16 slot_ctrl;
+
+		if (pdev->current_state != PCI_D0)
+			return IRQ_NONE;
+
+		pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
+		if (PCI_POSSIBLE_ERROR(slot_ctrl) ||
+		    !(slot_ctrl & PCI_EXP_SLTCTL_HPIE))
+			return IRQ_NONE;
+	}
+
 	/*
 	 * Keep the port accessible by holding a runtime PM ref on its parent.
 	 * Defer resume of the parent to the IRQ thread if it's suspended.
-- 
2.43.0

[PATCH v2] PCI: pciehp: Avoid dropping hotplug interrupts due to stale HPIE

[Zhu Qiyu]

pciehp_isr() uses the Hot-Plug Interrupt Enable (HPIE) bit in
ctrl->slot_ctrl to reject interrupts when hotplug interrupts are
disabled.  This check is needed because hotplug can share an interrupt
with other sources, including native PME.

However, ctrl->slot_ctrl is updated from hardware on every Slot Control
Register read-modify-write.  A firmware change to HPIE can race with an
unrelated driver operation, such as an LED indicator update:

  1. The driver enables hotplug interrupts, setting both hardware HPIE
	  and the cached HPIE bit to 1.
  2. Firmware temporarily clears hardware HPIE; the cached bit remains 1.
  3. pcie_do_write_cmd() reads Slot Control for an indicator update and
	  observes HPIE=0.  Since the command's mask excludes HPIE, it
	  preserves the value read from hardware.
  4. The driver updates ctrl->slot_ctrl and writes Slot Control.  Both
	  hardware and cached HPIE are now 0, although the driver did not
	  explicitly disable hotplug interrupts.
  5. Firmware restores hardware HPIE to 1 after the driver's write,
	  but the cached bit remains 0.
  6. A subsequent hotplug interrupt reaches pciehp_isr(), which sees
	  cached HPIE=0 and returns IRQ_NONE without reading or clearing
	  Slot Status, leaving the event pending.

ctrl_lock serializes driver commands, but cannot prevent this race
because firmware does not acquire it.

Track the driver's HPIE setting separately.  Update it under ctrl_lock
only when the command's mask includes HPIE, before writing Slot Control.
Use this setting in the ISR so unrelated hardware readbacks cannot
override the driver's enable/disable state.  This adds no configuration
space accesses to the ISR and leaves the register writes and runtime PM
flow unchanged.

Wait for command completion interrupts only when the software HPIE
setting and the cached HPIE and CCIE bits are all set; otherwise poll.
Also check cmd_busy when polling to recognize completions already
consumed by the ISR.

This prevents stale-cache rejection of delivered interrupts, but does
not restore hardware interrupt delivery.  If firmware restores HPIE
before the driver's write, that write may clear it again.

Signed-off-by: Zhu Qiyu <qiyuzhu2@amd.com>
---
Changes in v2:
- Replace the Slot Control re-read in the ISR with a separate software
  HPIE setting.  A parent runtime PM reference does not prevent the port
  itself from entering D3cold, so it cannot guarantee that the added
  configuration access is safe.
- Update the setting only for explicit HPIE commands, under ctrl_lock,
  using READ_ONCE()/WRITE_ONCE() for concurrent access.  Keep the early
  interrupt-disable check and the existing ISR runtime PM flow.
- Require the software setting as well as cached HPIE/CCIE for command
  completion interrupt waits.  Check cmd_busy when polling to recognize
  completions already consumed by the ISR.
- Explain the race step by step and clarify that the fix does not
  restore hardware interrupt delivery.
- Retitle the patch to describe the failure rather than the solution.

 drivers/pci/hotplug/pciehp.h     |  3 +++
 drivers/pci/hotplug/pciehp_hpc.c | 18 ++++++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index debc79b0adfb2..819899d04a418 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -54,6 +54,8 @@ extern int pciehp_poll_time;
  *	controller and disabled per spec recommendation (PCIe r5.0, appendix I
  *	implementation note)
  * @slot_ctrl: cached copy of the Slot Control register
+ * @hpie_enabled: driver's Hot-Plug Interrupt Enable setting, updated only
+ *	by commands that explicitly change HPIE, not by hardware readback
  * @ctrl_lock: serializes writes to the Slot Control register
  * @cmd_started: jiffies when the Slot Control register was last written;
  *	the next write is allowed 1 second later, absent a Command Completed
@@ -96,6 +98,7 @@ struct controller {
 	unsigned int inband_presence_disabled:1;
 
 	u16 slot_ctrl;				/* control register access */
+	bool hpie_enabled;
 	struct mutex ctrl_lock;
 	unsigned long cmd_started;
 	unsigned int cmd_busy:1;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 4c62140a3cb44..ab1c6da9054e1 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -89,6 +89,10 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
 	u16 slot_status;
 
 	do {
+		/* The IRQ handler may have consumed the completion event. */
+		if (!ctrl->cmd_busy)
+			return 1;
+
 		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
 		if (PCI_POSSIBLE_ERROR(slot_status)) {
 			ctrl_info(ctrl, "%s: no response from device\n",
@@ -106,7 +110,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout)
 		msleep(10);
 		timeout -= 10;
 	} while (timeout >= 0);
-	return 0;	/* timeout */
+	return !ctrl->cmd_busy;
 }
 
 static void pcie_wait_cmd(struct controller *ctrl)
@@ -137,7 +141,8 @@ static void pcie_wait_cmd(struct controller *ctrl)
 	else
 		timeout = cmd_timeout - now;
 
-	if (ctrl->slot_ctrl & PCI_EXP_SLTCTL_HPIE &&
+	if (READ_ONCE(ctrl->hpie_enabled) &&
+	    ctrl->slot_ctrl & PCI_EXP_SLTCTL_HPIE &&
 	    ctrl->slot_ctrl & PCI_EXP_SLTCTL_CCIE)
 		rc = wait_event_timeout(ctrl->queue, !ctrl->cmd_busy, timeout);
 	else
@@ -179,6 +184,9 @@ static void pcie_do_write_cmd(struct controller *ctrl, u16 cmd,
 	ctrl->cmd_busy = 1;
 	smp_mb();
 	ctrl->slot_ctrl = slot_ctrl;
+	/* Do not let unrelated read-modify-writes change the IRQ setting. */
+	if (mask & PCI_EXP_SLTCTL_HPIE)
+		WRITE_ONCE(ctrl->hpie_enabled, !!(cmd & PCI_EXP_SLTCTL_HPIE));
 	pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl);
 	ctrl->cmd_started = jiffies;
 
@@ -629,10 +637,12 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 
 	/*
 	 * Interrupts only occur in D3hot or shallower and only if enabled
-	 * in the Slot Control register (PCIe r4.0, sec 6.7.3.4).
+	 * in the Slot Control register (PCIe r4.0, sec 6.7.3.4).  Use the
+	 * driver's HPIE setting, not a cached hardware value which may be
+	 * affected by firmware changes to Slot Control.
 	 */
 	if (pdev->current_state == PCI_D3cold ||
-	    (!(ctrl->slot_ctrl & PCI_EXP_SLTCTL_HPIE) && !pciehp_poll_mode))
+	    (!READ_ONCE(ctrl->hpie_enabled) && !pciehp_poll_mode))
 		return IRQ_NONE;
 
 	/*
-- 
2.43.0

Re: [PATCH v2] PCI: pciehp: Avoid dropping hotplug interrupts due to stale HPIE

[Lukas Wunner]

On Fri, Sep 25, 2026 at 11:46:35AM +0000, Zhu Qiyu wrote:
> pciehp_isr() uses the Hot-Plug Interrupt Enable (HPIE) bit in
> ctrl->slot_ctrl to reject interrupts when hotplug interrupts are
> disabled.  This check is needed because hotplug can share an interrupt
> with other sources, including native PME.
> 
> However, ctrl->slot_ctrl is updated from hardware on every Slot Control
> Register read-modify-write.  A firmware change to HPIE can race with an
> unrelated driver operation, such as an LED indicator update:

Firmware is not allowed to change the Slot Control Register once it has
granted control of PCI Express Native Hot Plug Control to the operating
system.

From PCIe r3.3 table 4-6 bit 0:

   "Additionally, after control is transferred to the operating system,
    firmware must not update the state of hot plug slots, including the
    state of the indicators and power controller."

Are you seeing this on a real-world system or is it an LLM-generated
hallucination?

If the former, please consider fixing this through a firmware update.

Thanks,

Lukas

Re: [PATCH v2] PCI: pciehp: Avoid dropping hotplug interrupts due to stale HPIE

[Lukas Wunner]

On Fri, Sep 25, 2026 at 08:49:26PM +0200, Lukas Wunner wrote:
> From PCIe r3.3 table 4-6 bit 0:

Sorry, this should have been PCIe *Firmware* r3.3.