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

[Zhu Qiyu <qiyuzhu2@amd.com>]

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