Skip to contentSkip to conversation

PCI/DPC: Clean up DPC vs AER/EDR ownership and Kconfig

PATCH In review Bjorn Helgaas 3 participants 18 messages Feb 22, 2024 – Mar 2, 2024

Subsystems (4)
Stable branch ACPI PCI PCI enhanced error handling (eeh) for PowerPC
lore
Share & tools
lore

Bjorn Helgaas

author
#0 lore raw

v2 cover PCI/DPC: Clean up DPC vs AER/EDR ownership and Kconfig

From: Bjorn Helgaas <[email protected]>

Previously we could request control of DPC without AER, which is illegal per spec. Also, we could enable CONFIG_PCIE_DPC without CONFIG_PCIE_EDR, which is also illegal. This series addresses both.

Bjorn Helgaas (3):
PCI/DPC: Request DPC only if also requesting AER PCI/DPC: Remove CONFIG_PCIE_EDR
PCI/DPC: Encapsulate pci_acpi_add_edr_notifier()

 drivers/acpi/pci_root.c   | 22 ++++++++++++----------
 drivers/pci/pci.h         |  4 ++++
 drivers/pci/pcie/Kconfig  | 14 ++++----------
 drivers/pci/pcie/Makefile |  5 ++++-
 drivers/pci/pcie/dpc.c    | 10 ----------
 include/linux/pci-acpi.h  |  8 --------
 6 files changed, 24 insertions(+), 39 deletions(-)
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH v2 1/3 PCI/DPC: Request DPC only if also requesting AER

From: Bjorn Helgaas <[email protected]>

When booting with "pci=noaer", we don't request control of AER, but we previously *did* request control of DPC, as in the dmesg log attached at the bugzilla below:

Command line: ... pci=noaer
acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI EDR HPX-Type3] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug SHPCHotplug PME PCIeCapability LTR DPC]

That's illegal per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, which says:

If the operating system sets this bit [OSC_PCI_EXPRESS_DPC_CONTROL], it must also set bit 7 of the Support field (indicating support for Error Disconnect Recover notifications) and bits 3 and 4 of the Control field (requesting control of PCI Express Advanced Error Reporting and the PCI Express Capability Structure).

Request DPC control only if we have also requested AER control.

Fixes ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support")
Link https://bugzilla.kernel.org/show_bug.cgi?id=218491#c12
Signed-off-by Bjorn Helgaas <[email protected]>
Cc <[email protected]> # v5.7+
Cc Kuppuswamy Sathyanarayanan <[email protected]>
Cc Matthew W Carlis <[email protected]>
Cc Keith Busch <[email protected]>
Cc Lukas Wunner <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Jesse Brandeburg <[email protected]>
---
 drivers/acpi/pci_root.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)
drivers/acpi/pci_root.c +11 -9
index 58b89b8d950e..efc292b6214e 100644
@@ -518,17 +518,19 @@ static u32 calculate_control(void)
 	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
 		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
 
-	if (pci_aer_available())
+	if (pci_aer_available()) {
 		control |= OSC_PCI_EXPRESS_AER_CONTROL;
 
-	/*
-	 * Per the Downstream Port Containment Related Enhancements ECN to
-	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
-	 * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
-	 * and EDR.
-	 */
-	if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
-		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+		/*
+		 * Per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, the
+		 * OS can request DPC control only if it has advertised
+		 * OSC_PCI_EDR_SUPPORT and requested both
+		 * OSC_PCI_EXPRESS_CAPABILITY_CONTROL and
+		 * OSC_PCI_EXPRESS_AER_CONTROL.
+		 */
+		if (IS_ENABLED(CONFIG_PCIE_DPC))
+			control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+	}
 
 	return control;
 }
Signature · 1 line
2.34.1
Reviewed-by on v2 1/3
#2 lore raw

Re: [PATCH v2 1/3] PCI/DPC: Request DPC only if also requesting AER

Quoted 10 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

When booting with "pci=noaer", we don't request control of AER, but we previously *did* request control of DPC, as in the dmesg log attached at the bugzilla below:

Command line: ... pci=noaer
acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI EDR HPX-Type3] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug SHPCHotplug PME PCIeCapability LTR DPC]

Quoted 8 lines

That's illegal per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, which says:

If the operating system sets this bit [OSC_PCI_EXPRESS_DPC_CONTROL], it must also set bit 7 of the Support field (indicating support for Error Disconnect Recover notifications) and bits 3 and 4 of the Control field (requesting control of PCI Express Advanced Error Reporting and the PCI Express Capability Structure).

IIUC, this dependency is discussed in sec 4.5.2.4. "Dependencies Between _OSC Control Bits".

Because handling of Downstream Port Containment has a dependency on Advanced Error Reporting, the operating system is required to request control over Advanced Error Reporting (bit 3 of the Control field) while requesting control over Downstream Port Containment Configuration (bit 7 of the Control field). If the operating system attempts to claim control of Downstream Port Containment Configuration without also claiming control over Advanced Error Reporting, firmware is required to refuse control of the feature being illegally claimed and mask the corresponding bit. Firmware is required to maintain ownership of Advanced Error Reporting if it retains ownership of Downstream Port Containment Configuration.
If the operating system sets bit 7 of the Control field, it must set bit 7 of the Support field, indicating support for the Error Disconnect Recover event.

Quoted 13 lines

Request DPC control only if we have also requested AER control.

Fixes ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support")
Link https://bugzilla.kernel.org/show_bug.cgi?id=218491#c12
Signed-off-by Bjorn Helgaas <[email protected]>
Cc <[email protected]> # v5.7+
Cc Kuppuswamy Sathyanarayanan <[email protected]>
Cc Matthew W Carlis <[email protected]>
Cc Keith Busch <[email protected]>
Cc Lukas Wunner <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Jesse Brandeburg <[email protected]>
Quoted 1 line

---

Code wise it looks fine to me.

Reviewed-by Kuppuswamy Sathyanarayanan <[email protected]>
Quoted 36 lines
 drivers/acpi/pci_root.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)
drivers/acpi/pci_root.c +11 -9
index 58b89b8d950e..efc292b6214e 100644
@@ -518,17 +518,19 @@ static u32 calculate_control(void)
 	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
 		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
 
-	if (pci_aer_available())
+	if (pci_aer_available()) {
 		control |= OSC_PCI_EXPRESS_AER_CONTROL;
 
-	/*
-	 * Per the Downstream Port Containment Related Enhancements ECN to
-	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
-	 * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
-	 * and EDR.
-	 */
-	if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
-		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+		/*
+		 * Per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, the
+		 * OS can request DPC control only if it has advertised
+		 * OSC_PCI_EDR_SUPPORT and requested both
+		 * OSC_PCI_EXPRESS_CAPABILITY_CONTROL and
+		 * OSC_PCI_EXPRESS_AER_CONTROL.
+		 */
+		if (IS_ENABLED(CONFIG_PCIE_DPC))
+			control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+	}
 
 	return control;
 }
Signature · 2 lines
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
author
#3 lore raw

Re: [PATCH v2 1/3] PCI/DPC: Request DPC only if also requesting AER

Quoted 24 lines · Kuppuswamy Sathyanarayanan
Quoted 10 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

When booting with "pci=noaer", we don't request control of AER, but we previously *did* request control of DPC, as in the dmesg log attached at the bugzilla below:

Command line: ... pci=noaer
acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI EDR HPX-Type3] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug SHPCHotplug PME PCIeCapability LTR DPC]

Quoted 8 lines

That's illegal per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, which says:

If the operating system sets this bit [OSC_PCI_EXPRESS_DPC_CONTROL], it must also set bit 7 of the Support field (indicating support for Error Disconnect Recover notifications) and bits 3 and 4 of the Control field (requesting control of PCI Express Advanced Error Reporting and the PCI Express Capability Structure).

IIUC, this dependency is discussed in sec 4.5.2.4. "Dependencies Between _OSC Control Bits".

Quoted 14 lines

Because handling of Downstream Port Containment has a dependency on Advanced Error Reporting, the operating system is required to request control over Advanced Error Reporting (bit 3 of the Control field) while requesting control over Downstream Port Containment Configuration (bit 7 of the Control field). If the operating system attempts to claim control of Downstream Port Containment Configuration without also claiming control over Advanced Error Reporting, firmware is required to refuse control of the feature being illegally claimed and mask the corresponding bit. Firmware is required to maintain ownership of Advanced Error Reporting if it retains ownership of Downstream Port Containment Configuration. If the operating system sets bit 7 of the Control field, it must set bit 7 of the Support field, indicating support for the Error Disconnect Recover event.

So I guess you're suggesting that there are two defects here?

1) Linux requested DPC control without requesting AER control.

2) Platform granted DPC control when it shouldn't have.

I do agree with that, but obviously we can only fix 1) in Linux.

Quoted 57 lines
Quoted 12 lines

Request DPC control only if we have also requested AER control.

Fixes ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support")
Link https://bugzilla.kernel.org/show_bug.cgi?id=218491#c12
Signed-off-by Bjorn Helgaas <[email protected]>
Cc <[email protected]> # v5.7+
Cc Kuppuswamy Sathyanarayanan <[email protected]>
Cc Matthew W Carlis <[email protected]>
Cc Keith Busch <[email protected]>
Cc Lukas Wunner <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Jesse Brandeburg <[email protected]>
Quoted 1 line

---

Code wise it looks fine to me.

Reviewed-by Kuppuswamy Sathyanarayanan <[email protected]>
Quoted 36 lines
 drivers/acpi/pci_root.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)
drivers/acpi/pci_root.c +11 -9
index 58b89b8d950e..efc292b6214e 100644
@@ -518,17 +518,19 @@ static u32 calculate_control(void)
 	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
 		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
 
-	if (pci_aer_available())
+	if (pci_aer_available()) {
 		control |= OSC_PCI_EXPRESS_AER_CONTROL;
 
-	/*
-	 * Per the Downstream Port Containment Related Enhancements ECN to
-	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
-	 * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
-	 * and EDR.
-	 */
-	if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
-		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+		/*
+		 * Per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, the
+		 * OS can request DPC control only if it has advertised
+		 * OSC_PCI_EDR_SUPPORT and requested both
+		 * OSC_PCI_EXPRESS_CAPABILITY_CONTROL and
+		 * OSC_PCI_EXPRESS_AER_CONTROL.
+		 */
+		if (IS_ENABLED(CONFIG_PCIE_DPC))
+			control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+	}
 
 	return control;
 }
Signature · 2 lines
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
#4 lore raw

Re: [PATCH v2 1/3] PCI/DPC: Request DPC only if also requesting AER

Quoted 37 lines · Bjorn Helgaas
Quoted 22 lines · Kuppuswamy Sathyanarayanan
Quoted 10 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

When booting with "pci=noaer", we don't request control of AER, but we previously *did* request control of DPC, as in the dmesg log attached at the bugzilla below:

Command line: ... pci=noaer
acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI EDR HPX-Type3] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug SHPCHotplug PME PCIeCapability LTR DPC]

Quoted 8 lines

That's illegal per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, which says:

If the operating system sets this bit [OSC_PCI_EXPRESS_DPC_CONTROL], it must also set bit 7 of the Support field (indicating support for Error Disconnect Recover notifications) and bits 3 and 4 of the Control field (requesting control of PCI Express Advanced Error Reporting and the PCI Express Capability Structure).

IIUC, this dependency is discussed in sec 4.5.2.4. "Dependencies Between _OSC Control Bits".

Quoted 14 lines

Because handling of Downstream Port Containment has a dependency on Advanced Error Reporting, the operating system is required to request control over Advanced Error Reporting (bit 3 of the Control field) while requesting control over Downstream Port Containment Configuration (bit 7 of the Control field). If the operating system attempts to claim control of Downstream Port Containment Configuration without also claiming control over Advanced Error Reporting, firmware is required to refuse control of the feature being illegally claimed and mask the corresponding bit. Firmware is required to maintain ownership of Advanced Error Reporting if it retains ownership of Downstream Port Containment Configuration. If the operating system sets bit 7 of the Control field, it must set bit 7 of the Support field, indicating support for the Error Disconnect Recover event.

Quoted 7 lines

So I guess you're suggesting that there are two defects here?

1) Linux requested DPC control without requesting AER control.

2) Platform granted DPC control when it shouldn't have.

I do agree with that, but obviously we can only fix 1) in Linux.

Sorry, maybe my comment was not clear. I was just suggesting
to change the  spec reference from r3.3, sec 4.5.1, table 4-5
to  r3.3, sec 4.5.2.4 "Dependencies Between _OSC Control Bits". I agree that we cannot do much if firmware misbehaves here.

Quoted 56 lines
Quoted 56 lines
Quoted 12 lines

Request DPC control only if we have also requested AER control.

Fixes ac1c8e35a326 ("PCI/DPC: Add Error Disconnect Recover (EDR) support")
Link https://bugzilla.kernel.org/show_bug.cgi?id=218491#c12
Signed-off-by Bjorn Helgaas <[email protected]>
Cc <[email protected]> # v5.7+
Cc Kuppuswamy Sathyanarayanan <[email protected]>
Cc Matthew W Carlis <[email protected]>
Cc Keith Busch <[email protected]>
Cc Lukas Wunner <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Jesse Brandeburg <[email protected]>
Quoted 1 line

---

Code wise it looks fine to me.

Reviewed-by Kuppuswamy Sathyanarayanan <[email protected]>
Quoted 36 lines
 drivers/acpi/pci_root.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)
drivers/acpi/pci_root.c +11 -9
index 58b89b8d950e..efc292b6214e 100644
@@ -518,17 +518,19 @@ static u32 calculate_control(void)
 	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_SHPC))
 		control |= OSC_PCI_SHPC_NATIVE_HP_CONTROL;
 
-	if (pci_aer_available())
+	if (pci_aer_available()) {
 		control |= OSC_PCI_EXPRESS_AER_CONTROL;
 
-	/*
-	 * Per the Downstream Port Containment Related Enhancements ECN to
-	 * the PCI Firmware Spec, r3.2, sec 4.5.1, table 4-5,
-	 * OSC_PCI_EXPRESS_DPC_CONTROL indicates the OS supports both DPC
-	 * and EDR.
-	 */
-	if (IS_ENABLED(CONFIG_PCIE_DPC) && IS_ENABLED(CONFIG_PCIE_EDR))
-		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+		/*
+		 * Per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, the
+		 * OS can request DPC control only if it has advertised
+		 * OSC_PCI_EDR_SUPPORT and requested both
+		 * OSC_PCI_EXPRESS_CAPABILITY_CONTROL and
+		 * OSC_PCI_EXPRESS_AER_CONTROL.
+		 */
+		if (IS_ENABLED(CONFIG_PCIE_DPC))
+			control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+	}
 
 	return control;
 }
Signature · 2 lines
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
Signature · 2 lines
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
author
#5 lore raw

Re: [PATCH v2 1/3] PCI/DPC: Request DPC only if also requesting AER

Quoted 45 lines · Kuppuswamy Sathyanarayanan
Quoted 43 lines · Bjorn Helgaas
Quoted 23 lines · Kuppuswamy Sathyanarayanan
Quoted 10 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

When booting with "pci=noaer", we don't request control of AER, but we previously *did* request control of DPC, as in the dmesg log attached at the bugzilla below:

Command line: ... pci=noaer
acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI EDR HPX-Type3] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug SHPCHotplug PME PCIeCapability LTR DPC]

Quoted 8 lines

That's illegal per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, which says:

If the operating system sets this bit [OSC_PCI_EXPRESS_DPC_CONTROL], it must also set bit 7 of the Support field (indicating support for Error Disconnect Recover notifications) and bits 3 and 4 of the Control field (requesting control of PCI Express Advanced Error Reporting and the PCI Express Capability Structure).

IIUC, this dependency is discussed in sec 4.5.2.4. "Dependencies Between _OSC Control Bits".

Quoted 14 lines

Because handling of Downstream Port Containment has a dependency on Advanced Error Reporting, the operating system is required to request control over Advanced Error Reporting (bit 3 of the Control field) while requesting control over Downstream Port Containment Configuration (bit 7 of the Control field). If the operating system attempts to claim control of Downstream Port Containment Configuration without also claiming control over Advanced Error Reporting, firmware is required to refuse control of the feature being illegally claimed and mask the corresponding bit. Firmware is required to maintain ownership of Advanced Error Reporting if it retains ownership of Downstream Port Containment Configuration. If the operating system sets bit 7 of the Control field, it must set bit 7 of the Support field, indicating support for the Error Disconnect Recover event.

So I guess you're suggesting that there are two defects here?

1) Linux requested DPC control without requesting AER control.

Quoted 7 lines
Quoted 3 lines

2) Platform granted DPC control when it shouldn't have.

I do agree with that, but obviously we can only fix 1) in Linux.

Sorry, maybe my comment was not clear. I was just suggesting to change the spec reference from r3.3, sec 4.5.1, table 4-5 to r3.3, sec 4.5.2.4 "Dependencies Between _OSC Control Bits".

The requirement that the OS request AER control whenever it requests DPC control is mentioned in both sec 4.5.1 and sec 4.5.2.4. IMO sec 4.5.2.4 should not exist because the per-bit table in sec 4.5.1 is a better place for implementation guidance. 4.5.2.4 is easy to miss, mostly redundant, and hard to integrate with the 4.5.1 table.

What advantage do you see for citing 4.5.2.4 instead of 4.5.1? The only real difference I see is that it also points out a firmware problem. I don't think the extra text is worth it since it doesn't motivate the Linux change.

Bjorn

#6 lore raw

Re: [PATCH v2 1/3] PCI/DPC: Request DPC only if also requesting AER

Quoted 55 lines · Bjorn Helgaas
Quoted 40 lines · Kuppuswamy Sathyanarayanan
Quoted 39 lines · Bjorn Helgaas
Quoted 22 lines · Kuppuswamy Sathyanarayanan
Quoted 10 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

When booting with "pci=noaer", we don't request control of AER, but we previously *did* request control of DPC, as in the dmesg log attached at the bugzilla below:

Command line: ... pci=noaer
acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM ClockPM Segments MSI EDR HPX-Type3] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug SHPCHotplug PME PCIeCapability LTR DPC]

Quoted 8 lines

That's illegal per PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, which says:

If the operating system sets this bit [OSC_PCI_EXPRESS_DPC_CONTROL], it must also set bit 7 of the Support field (indicating support for Error Disconnect Recover notifications) and bits 3 and 4 of the Control field (requesting control of PCI Express Advanced Error Reporting and the PCI Express Capability Structure).

IIUC, this dependency is discussed in sec 4.5.2.4. "Dependencies Between _OSC Control Bits".

Quoted 14 lines

Because handling of Downstream Port Containment has a dependency on Advanced Error Reporting, the operating system is required to request control over Advanced Error Reporting (bit 3 of the Control field) while requesting control over Downstream Port Containment Configuration (bit 7 of the Control field). If the operating system attempts to claim control of Downstream Port Containment Configuration without also claiming control over Advanced Error Reporting, firmware is required to refuse control of the feature being illegally claimed and mask the corresponding bit. Firmware is required to maintain ownership of Advanced Error Reporting if it retains ownership of Downstream Port Containment Configuration. If the operating system sets bit 7 of the Control field, it must set bit 7 of the Support field, indicating support for the Error Disconnect Recover event.

So I guess you're suggesting that there are two defects here?

Quoted 8 lines
Quoted 5 lines

1) Linux requested DPC control without requesting AER control.

2) Platform granted DPC control when it shouldn't have.

I do agree with that, but obviously we can only fix 1) in Linux.

Sorry, maybe my comment was not clear. I was just suggesting to change the spec reference from r3.3, sec 4.5.1, table 4-5 to r3.3, sec 4.5.2.4 "Dependencies Between _OSC Control Bits".

The requirement that the OS request AER control whenever it requests DPC control is mentioned in both sec 4.5.1 and sec 4.5.2.4. IMO sec 4.5.2.4 should not exist because the per-bit table in sec 4.5.1 is a better place for implementation guidance. 4.5.2.4 is easy to miss, mostly redundant, and hard to integrate with the 4.5.1 table.

Quoted 4 lines

What advantage do you see for citing 4.5.2.4 instead of 4.5.1? The only real difference I see is that it also points out a firmware problem. I don't think the extra text is worth it since it doesn't motivate the Linux change.

My thinking is, since the fix is related to the dependency between _OSC control bits (AER & DPC), and there is a special section in the spec which discuss it, I thought it is better to  quote it.

But I get your point. I think either if fine.

Quoted 2 lines

Bjorn

Signature · 2 lines
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
author
#7 lore raw

PATCH v2 2/3 PCI/DPC: Remove CONFIG_PCIE_EDR

In reply to Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

Previous Kconfig allowed the possibility of enabling CONFIG_PCIE_DPC without CONFIG_PCIE_EDR. The PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, says an ACPI OS that requests control of DPC must also advertise support for EDR.

Remove CONFIG_PCIE_EDR and enable the EDR code with CONFIG_PCIE_DPC so that enabling DPC also enables EDR for ACPI systems. Since EDR is an ACPI feature, build edr.o only when CONFIG_ACPI is enabled. Stubs cover the case when DPC is enabled without ACPI.

Signed-off-by Bjorn Helgaas <[email protected]>
---
 drivers/acpi/pci_root.c   |  2 +-
 drivers/pci/pcie/Kconfig  | 14 ++++----------
 drivers/pci/pcie/Makefile |  5 ++++-
 drivers/pci/pcie/dpc.c    | 10 ----------
 include/linux/pci-acpi.h  |  4 ++--
 5 files changed, 11 insertions(+), 24 deletions(-)
drivers/acpi/pci_root.c +1 -1
index efc292b6214e..bcaf3d3a5e05 100644
@@ -448,7 +448,7 @@ static u32 calculate_support(void)
 		support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
 	if (pci_msi_enabled())
 		support |= OSC_PCI_MSI_SUPPORT;
-	if (IS_ENABLED(CONFIG_PCIE_EDR))
+	if (IS_ENABLED(CONFIG_PCIE_DPC))	/* DPC => EDR support */
 		support |= OSC_PCI_EDR_SUPPORT;
 
 	return support;
drivers/pci/pcie/Kconfig +4 -10
index 8999fcebde6a..21e98289fbe9 100644
@@ -137,6 +137,10 @@ config PCIE_DPC
 	  have this capability or you do not want to use this feature,
 	  it is safe to answer N.
 
+	  On ACPI systems, this includes Error Disconnect Recover support,
+	  the hybrid model that uses both firmware and OS to implement DPC,
+	  as specified in the PCI Firmware Specification r3.3.
+
 config PCIE_PTM
 	bool "PCI Express Precision Time Measurement support"
 	help

@@ -145,13 +149,3 @@ config PCIE_PTM
 
 	  This is only useful if you have devices that support PTM, but it
 	  is safe to enable even if you don't.
-
-config PCIE_EDR
-	bool "PCI Express Error Disconnect Recover support"
-	depends on PCIE_DPC && ACPI
-	help
-	  This option adds Error Disconnect Recover support as specified
-	  in the Downstream Port Containment Related Enhancements ECN to
-	  the PCI Firmware Specification r3.2.  Enable this if you want to
-	  support hybrid DPC model which uses both firmware and OS to
-	  implement DPC.
drivers/pci/pcie/Makefile +4 -1
index 8de4ed5f98f1..72657f780c33 100644
@@ -12,4 +12,7 @@ obj-$(CONFIG_PCIEAER_INJECT)	+= aer_inject.o
 obj-$(CONFIG_PCIE_PME)		+= pme.o
 obj-$(CONFIG_PCIE_DPC)		+= dpc.o
 obj-$(CONFIG_PCIE_PTM)		+= ptm.o
-obj-$(CONFIG_PCIE_EDR)		+= edr.o
+
+ifdef CONFIG_ACPI
+obj-$(CONFIG_PCIE_DPC)		+= edr.o
+endif
drivers/pci/pcie/dpc.c +0 -10
index 94111e438241..0aa79581250b 100644
@@ -102,19 +102,9 @@ static bool dpc_completed(struct pci_dev *pdev)
  */
 bool pci_dpc_recovered(struct pci_dev *pdev)
 {
-	struct pci_host_bridge *host;
-
 	if (!pdev->dpc_cap)
 		return false;
 
-	/*
-	 * Synchronization between hotplug and DPC is not supported
-	 * if DPC is owned by firmware and EDR is not enabled.
-	 */
-	host = pci_find_host_bridge(pdev->bus);
-	if (!host->native_dpc && !IS_ENABLED(CONFIG_PCIE_EDR))
-		return false;
-
 	/*
 	 * Need a timeout in case DPC never completes due to failure of
 	 * dpc_wait_rp_inactive().  The spec doesn't mandate a time limit,
include/linux/pci-acpi.h +2 -2
index 078225b514d4..92e196ba0249 100644
@@ -122,13 +122,13 @@ extern const guid_t pci_acpi_dsm_guid;
 #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
 #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
 
-#ifdef CONFIG_PCIE_EDR
+#ifdef CONFIG_PCIE_DPC
 void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
 void pci_acpi_remove_edr_notifier(struct pci_dev *pdev);
 #else
 static inline void pci_acpi_add_edr_notifier(struct pci_dev *pdev) { }
 static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
-#endif /* CONFIG_PCIE_EDR */
+#endif /* CONFIG_PCIE_DPC */
 
 int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *));
 void pci_acpi_clear_companion_lookup_hook(void);
Signature · 1 line
2.34.1
#8 lore raw

Re: [PATCH v2 2/3] PCI/DPC: Remove CONFIG_PCIE_EDR

Hi Bjorn,

Quoted 26 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

Previous Kconfig allowed the possibility of enabling CONFIG_PCIE_DPC without CONFIG_PCIE_EDR. The PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, says an ACPI OS that requests control of DPC must also advertise support for EDR.

Remove CONFIG_PCIE_EDR and enable the EDR code with CONFIG_PCIE_DPC so that enabling DPC also enables EDR for ACPI systems. Since EDR is an ACPI feature, build edr.o only when CONFIG_ACPI is enabled. Stubs cover the case when DPC is enabled without ACPI.

Signed-off-by Bjorn Helgaas <[email protected]>
---
 drivers/acpi/pci_root.c   |  2 +-
 drivers/pci/pcie/Kconfig  | 14 ++++----------
 drivers/pci/pcie/Makefile |  5 ++++-
 drivers/pci/pcie/dpc.c    | 10 ----------
 include/linux/pci-acpi.h  |  4 ++--
 5 files changed, 11 insertions(+), 24 deletions(-)
drivers/acpi/pci_root.c +0 -0
index efc292b6214e..bcaf3d3a5e05 100644

drivers/acpi/pci_root.c static u32 calculate_support(void) · line 452

Quoted 6 lines
drivers/acpi/pci_root.c +1 -1
@@ -448,7 +448,7 @@ static u32 calculate_support(void)
 		support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
 	if (pci_msi_enabled())
 		support |= OSC_PCI_MSI_SUPPORT;
-	if (IS_ENABLED(CONFIG_PCIE_EDR))
+	if (IS_ENABLED(CONFIG_PCIE_DPC))	/* DPC => EDR support */
 		support |= OSC_PCI_EDR_SUPPORT;

Since EDR internally touches AER registers, I still think we should make sure OS enables AER support before advertising the EDR support.

drivers/pci/pcie/Kconfig config PCIE_DPC · line 142

Quoted 13 lines
drivers/acpi/pci_root.c +0 -0
 	return support;
drivers/pci/pcie/Kconfig +3 -0
index 8999fcebde6a..21e98289fbe9 100644
@@ -137,6 +137,10 @@ config PCIE_DPC
 	  have this capability or you do not want to use this feature,
 	  it is safe to answer N.
 
+	  On ACPI systems, this includes Error Disconnect Recover support,
+	  the hybrid model that uses both firmware and OS to implement DPC,
+	  as specified in the PCI Firmware Specification r3.3.

Nit: Include some section reference?

Quoted 75 lines
drivers/pci/pcie/Kconfig +1 -10
+
 config PCIE_PTM
 	bool "PCI Express Precision Time Measurement support"
 	help

@@ -145,13 +149,3 @@ config PCIE_PTM
 
 	  This is only useful if you have devices that support PTM, but it
 	  is safe to enable even if you don't.
-
-config PCIE_EDR
-	bool "PCI Express Error Disconnect Recover support"
-	depends on PCIE_DPC && ACPI
-	help
-	  This option adds Error Disconnect Recover support as specified
-	  in the Downstream Port Containment Related Enhancements ECN to
-	  the PCI Firmware Specification r3.2.  Enable this if you want to
-	  support hybrid DPC model which uses both firmware and OS to
-	  implement DPC.
drivers/pci/pcie/Makefile +4 -1
index 8de4ed5f98f1..72657f780c33 100644
@@ -12,4 +12,7 @@ obj-$(CONFIG_PCIEAER_INJECT)	+= aer_inject.o
 obj-$(CONFIG_PCIE_PME)		+= pme.o
 obj-$(CONFIG_PCIE_DPC)		+= dpc.o
 obj-$(CONFIG_PCIE_PTM)		+= ptm.o
-obj-$(CONFIG_PCIE_EDR)		+= edr.o
+
+ifdef CONFIG_ACPI
+obj-$(CONFIG_PCIE_DPC)		+= edr.o
+endif
drivers/pci/pcie/dpc.c +0 -10
index 94111e438241..0aa79581250b 100644
@@ -102,19 +102,9 @@ static bool dpc_completed(struct pci_dev *pdev)
  */
 bool pci_dpc_recovered(struct pci_dev *pdev)
 {
-	struct pci_host_bridge *host;
-
 	if (!pdev->dpc_cap)
 		return false;
 
-	/*
-	 * Synchronization between hotplug and DPC is not supported
-	 * if DPC is owned by firmware and EDR is not enabled.
-	 */
-	host = pci_find_host_bridge(pdev->bus);
-	if (!host->native_dpc && !IS_ENABLED(CONFIG_PCIE_EDR))
-		return false;
-
 	/*
 	 * Need a timeout in case DPC never completes due to failure of
 	 * dpc_wait_rp_inactive().  The spec doesn't mandate a time limit,
include/linux/pci-acpi.h +2 -2
index 078225b514d4..92e196ba0249 100644
@@ -122,13 +122,13 @@ extern const guid_t pci_acpi_dsm_guid;
 #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
 #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
 
-#ifdef CONFIG_PCIE_EDR
+#ifdef CONFIG_PCIE_DPC
 void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
 void pci_acpi_remove_edr_notifier(struct pci_dev *pdev);
 #else
 static inline void pci_acpi_add_edr_notifier(struct pci_dev *pdev) { }
 static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
-#endif /* CONFIG_PCIE_EDR */
+#endif /* CONFIG_PCIE_DPC */
 
 int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *));
 void pci_acpi_clear_companion_lookup_hook(void);
Signature · 2 lines
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
author
#9 lore raw

Re: [PATCH v2 2/3] PCI/DPC: Remove CONFIG_PCIE_EDR

Quoted 22 lines · Kuppuswamy Sathyanarayanan
Quoted 21 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

Previous Kconfig allowed the possibility of enabling CONFIG_PCIE_DPC without CONFIG_PCIE_EDR. The PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, says an ACPI OS that requests control of DPC must also advertise support for EDR.

Remove CONFIG_PCIE_EDR and enable the EDR code with CONFIG_PCIE_DPC so that enabling DPC also enables EDR for ACPI systems. Since EDR is an ACPI feature, build edr.o only when CONFIG_ACPI is enabled. Stubs cover the case when DPC is enabled without ACPI.

Signed-off-by Bjorn Helgaas <[email protected]>
---
 drivers/acpi/pci_root.c   |  2 +-
 drivers/pci/pcie/Kconfig  | 14 ++++----------
 drivers/pci/pcie/Makefile |  5 ++++-
 drivers/pci/pcie/dpc.c    | 10 ----------
 include/linux/pci-acpi.h  |  4 ++--
 5 files changed, 11 insertions(+), 24 deletions(-)
Quoted 14 lines

drivers/acpi/pci_root.c static u32 calculate_support(void) · line 452

Quoted 11 lines
drivers/acpi/pci_root.c +1 -1
index efc292b6214e..bcaf3d3a5e05 100644
@@ -448,7 +448,7 @@ static u32 calculate_support(void)
 		support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
 	if (pci_msi_enabled())
 		support |= OSC_PCI_MSI_SUPPORT;
-	if (IS_ENABLED(CONFIG_PCIE_EDR))
+	if (IS_ENABLED(CONFIG_PCIE_DPC))	/* DPC => EDR support */
 		support |= OSC_PCI_EDR_SUPPORT;

Since EDR internally touches AER registers, I still think we should make sure OS enables AER support before advertising the EDR support.

I guess you're suggesting that we should make it look like this?

  if (host_bridge->native_aer && IS_ENABLED(CONFIG_PCIE_DPC))

That doesn't seem right to me because the implementation note in PCI Firmware r3.3, sec 4.6.12, shows the EDR flow when firmware maintains control of AER and DPC, i.e., "host_bridge->native_aer" would be false.

Quoted 9 lines
Quoted 9 lines
drivers/acpi/pci_root.c +0 -0
 	return support;
drivers/pci/pcie/Kconfig +0 -0
index 8999fcebde6a..21e98289fbe9 100644
@@ -137,6 +137,10 @@ config PCIE_DPC
 	  have this capability or you do not want to use this feature,
 	  it is safe to answer N.
Quoted 5 lines

drivers/pci/pcie/Kconfig config PCIE_DPC · line 142

Quoted 3 lines
drivers/pci/pcie/Kconfig +3 -0
+	  On ACPI systems, this includes Error Disconnect Recover support,
+	  the hybrid model that uses both firmware and OS to implement DPC,
+	  as specified in the PCI Firmware Specification r3.3.

Nit: Include some section reference?

I basically copied this from the PCIE_EDR help and updated the revision number. But I don't think the firmware spec is a very good reference because EDR is defined by ACPI. There's very little text in the ACPI spec about EDR, but the firmware spec does assume you know what *is* there. And the ACPI spec is available to anybody, unlike the PCI firmware spec.

+         On ACPI systems, this includes Error Disconnect Recover support,
+         the hybrid model that uses both firmware and OS to implement DPC,
+         as specified in ACPI r6.5, sec 5.6.6.
Quoted 17 lines
Quoted 17 lines
drivers/pci/pcie/Kconfig +0 -10
 config PCIE_PTM
 	bool "PCI Express Precision Time Measurement support"
 	help

@@ -145,13 +149,3 @@ config PCIE_PTM
 
 	  This is only useful if you have devices that support PTM, but it
 	  is safe to enable even if you don't.
-
-config PCIE_EDR
-	bool "PCI Express Error Disconnect Recover support"
-	depends on PCIE_DPC && ACPI
-	help
-	  This option adds Error Disconnect Recover support as specified
-	  in the Downstream Port Containment Related Enhancements ECN to
-	  the PCI Firmware Specification r3.2.  Enable this if you want to
-	  support hybrid DPC model which uses both firmware and OS to
-	  implement DPC.
#10 lore raw

Re: [PATCH v2 2/3] PCI/DPC: Remove CONFIG_PCIE_EDR

Quoted 36 lines · Bjorn Helgaas
Quoted 22 lines · Kuppuswamy Sathyanarayanan
Quoted 21 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

Previous Kconfig allowed the possibility of enabling CONFIG_PCIE_DPC without CONFIG_PCIE_EDR. The PCI Firmware Spec, r3.3, sec 4.5.1, table 4-5, says an ACPI OS that requests control of DPC must also advertise support for EDR.

Remove CONFIG_PCIE_EDR and enable the EDR code with CONFIG_PCIE_DPC so that enabling DPC also enables EDR for ACPI systems. Since EDR is an ACPI feature, build edr.o only when CONFIG_ACPI is enabled. Stubs cover the case when DPC is enabled without ACPI.

Signed-off-by Bjorn Helgaas <[email protected]>
---
 drivers/acpi/pci_root.c   |  2 +-
 drivers/pci/pcie/Kconfig  | 14 ++++----------
 drivers/pci/pcie/Makefile |  5 ++++-
 drivers/pci/pcie/dpc.c    | 10 ----------
 include/linux/pci-acpi.h  |  4 ++--
 5 files changed, 11 insertions(+), 24 deletions(-)
Quoted 13 lines

drivers/acpi/pci_root.c static u32 calculate_support(void) · line 452

Quoted 11 lines
drivers/acpi/pci_root.c +1 -1
index efc292b6214e..bcaf3d3a5e05 100644
@@ -448,7 +448,7 @@ static u32 calculate_support(void)
 		support |= OSC_PCI_ASPM_SUPPORT | OSC_PCI_CLOCK_PM_SUPPORT;
 	if (pci_msi_enabled())
 		support |= OSC_PCI_MSI_SUPPORT;
-	if (IS_ENABLED(CONFIG_PCIE_EDR))
+	if (IS_ENABLED(CONFIG_PCIE_DPC))	/* DPC => EDR support */
 		support |= OSC_PCI_EDR_SUPPORT;

Since EDR internally touches AER registers, I still think we should make sure OS enables AER support before advertising the EDR support.

Quoted 8 lines

I guess you're suggesting that we should make it look like this?

  if (host_bridge->native_aer && IS_ENABLED(CONFIG_PCIE_DPC))

That doesn't seem right to me because the implementation note in PCI Firmware r3.3, sec 4.6.12, shows the EDR flow when firmware maintains control of AER and DPC, i.e., "host_bridge->native_aer" would be false.

No, my idea is to check for something like below:

if (IS_ENABLED(CONFIG_PCIEAER) && IS_ENABLED(CONFIG_PCIE_DPC)) or if (pci_aer_available() && IS_ENABLED(CONFIG_PCIE_DPC) to ensure AER is not disabled by noaer command line option. Since EDR handler depends on AER routines (like pci_aer_raw_clear_status()) to |clear AER registers, we need to ensure AER is enabled in kernel before advertising suppor for EDR.

Quoted 42 lines
Quoted 10 lines
Quoted 10 lines
drivers/acpi/pci_root.c +0 -0
 	return support;
drivers/pci/pcie/Kconfig +0 -0
index 8999fcebde6a..21e98289fbe9 100644
@@ -137,6 +137,10 @@ config PCIE_DPC
 	  have this capability or you do not want to use this feature,
 	  it is safe to answer N.
Quoted 4 lines

drivers/pci/pcie/Kconfig config PCIE_DPC · line 142

Quoted 3 lines
drivers/pci/pcie/Kconfig +3 -0
+	  On ACPI systems, this includes Error Disconnect Recover support,
+	  the hybrid model that uses both firmware and OS to implement DPC,
+	  as specified in the PCI Firmware Specification r3.3.

Nit: Include some section reference?

I basically copied this from the PCIE_EDR help and updated the revision number. But I don't think the firmware spec is a very good reference because EDR is defined by ACPI. There's very little text in the ACPI spec about EDR, but the firmware spec does assume you know what *is* there. And the ACPI spec is available to anybody, unlike the PCI firmware spec.

drivers/pci/pcie/Kconfig +3 -0
+         On ACPI systems, this includes Error Disconnect Recover support,
+         the hybrid model that uses both firmware and OS to implement DPC,
+         as specified in ACPI r6.5, sec 5.6.6.
Quoted 18 lines
Quoted 18 lines
drivers/pci/pcie/Kconfig +0 -10
 config PCIE_PTM
 	bool "PCI Express Precision Time Measurement support"
 	help

@@ -145,13 +149,3 @@ config PCIE_PTM
 
 	  This is only useful if you have devices that support PTM, but it
 	  is safe to enable even if you don't.
-
-config PCIE_EDR
-	bool "PCI Express Error Disconnect Recover support"
-	depends on PCIE_DPC && ACPI
-	help
-	  This option adds Error Disconnect Recover support as specified
-	  in the Downstream Port Containment Related Enhancements ECN to
-	  the PCI Firmware Specification r3.2.  Enable this if you want to
-	  support hybrid DPC model which uses both firmware and OS to
-	  implement DPC.
Signature · 2 lines
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
author
#11 lore raw

PATCH v2 3/3 PCI/DPC: Encapsulate pci_acpi_add_edr_notifier()

In reply to Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

pci_acpi_add_edr_notifier() and pci_acpi_remove_edr_notifier() are only referenced inside drivers/pci/. Move their declarations from include/linux/pci-acpi.h to drivers/pci/pci.h so they're not visible outside drivers/pci/.

Signed-off-by Bjorn Helgaas <[email protected]>
---
 drivers/pci/pci.h        | 4 ++++
 include/linux/pci-acpi.h | 8 --------
 2 files changed, 4 insertions(+), 8 deletions(-)
drivers/pci/pci.h +4 -0
index 2336a8d1edab..03bf2776d73b 100644
@@ -432,11 +432,15 @@ void pci_dpc_init(struct pci_dev *pdev);
 void dpc_process_error(struct pci_dev *pdev);
 pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
 bool pci_dpc_recovered(struct pci_dev *pdev);
+void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
+void pci_acpi_remove_edr_notifier(struct pci_dev *pdev);
 #else
 static inline void pci_save_dpc_state(struct pci_dev *dev) { }
 static inline void pci_restore_dpc_state(struct pci_dev *dev) { }
 static inline void pci_dpc_init(struct pci_dev *pdev) { }
 static inline bool pci_dpc_recovered(struct pci_dev *pdev) { return false; }
+static inline void pci_acpi_add_edr_notifier(struct pci_dev *pdev) { }
+static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
 #endif
 
 #ifdef CONFIG_PCIEPORTBUS
include/linux/pci-acpi.h +0 -8
index 92e196ba0249..f447ce215adf 100644
@@ -122,14 +122,6 @@ extern const guid_t pci_acpi_dsm_guid;
 #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
 #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
 
-#ifdef CONFIG_PCIE_DPC
-void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
-void pci_acpi_remove_edr_notifier(struct pci_dev *pdev);
-#else
-static inline void pci_acpi_add_edr_notifier(struct pci_dev *pdev) { }
-static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
-#endif /* CONFIG_PCIE_DPC */
-
 int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *));
 void pci_acpi_clear_companion_lookup_hook(void);
Signature · 1 line
2.34.1
#12 lore raw

Re: [PATCH v2 3/3] PCI/DPC: Encapsulate pci_acpi_add_edr_notifier()

Quoted 18 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

pci_acpi_add_edr_notifier() and pci_acpi_remove_edr_notifier() are only referenced inside drivers/pci/. Move their declarations from include/linux/pci-acpi.h to drivers/pci/pci.h so they're not visible outside drivers/pci/.

Signed-off-by Bjorn Helgaas <[email protected]>
---
 drivers/pci/pci.h        | 4 ++++
 include/linux/pci-acpi.h | 8 --------
 2 files changed, 4 insertions(+), 8 deletions(-)
drivers/pci/pci.h +0 -0
index 2336a8d1edab..03bf2776d73b 100644

drivers/pci/pci.h void pci_dpc_init(struct pci_dev *pdev); · line 436

Quoted 5 lines
drivers/pci/pci.h +2 -0
@@ -432,11 +432,15 @@ void pci_dpc_init(struct pci_dev *pdev);
 void dpc_process_error(struct pci_dev *pdev);
 pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
 bool pci_dpc_recovered(struct pci_dev *pdev);
+void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
+void pci_acpi_remove_edr_notifier(struct pci_dev *pdev);

Protect them with CONFIG_ACPI?

Quoted 29 lines
drivers/pci/pci.h +2 -0
 #else
 static inline void pci_save_dpc_state(struct pci_dev *dev) { }
 static inline void pci_restore_dpc_state(struct pci_dev *dev) { }
 static inline void pci_dpc_init(struct pci_dev *pdev) { }
 static inline bool pci_dpc_recovered(struct pci_dev *pdev) { return false; }
+static inline void pci_acpi_add_edr_notifier(struct pci_dev *pdev) { }
+static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
 #endif
 
 #ifdef CONFIG_PCIEPORTBUS
include/linux/pci-acpi.h +0 -8
index 92e196ba0249..f447ce215adf 100644
@@ -122,14 +122,6 @@ extern const guid_t pci_acpi_dsm_guid;
 #define DSM_PCI_POWER_ON_RESET_DELAY		0x08
 #define DSM_PCI_DEVICE_READINESS_DURATIONS	0x09
 
-#ifdef CONFIG_PCIE_DPC
-void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
-void pci_acpi_remove_edr_notifier(struct pci_dev *pdev);
-#else
-static inline void pci_acpi_add_edr_notifier(struct pci_dev *pdev) { }
-static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
-#endif /* CONFIG_PCIE_DPC */
-
 int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_dev *));
 void pci_acpi_clear_companion_lookup_hook(void);
Signature · 2 lines
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
author
#13 lore raw

Re: [PATCH v2 3/3] PCI/DPC: Encapsulate pci_acpi_add_edr_notifier()

Quoted 15 lines · Kuppuswamy Sathyanarayanan
Quoted 13 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

pci_acpi_add_edr_notifier() and pci_acpi_remove_edr_notifier() are only referenced inside drivers/pci/. Move their declarations from include/linux/pci-acpi.h to drivers/pci/pci.h so they're not visible outside drivers/pci/.

Signed-off-by Bjorn Helgaas <[email protected]>
---
 drivers/pci/pci.h        | 4 ++++
 include/linux/pci-acpi.h | 8 --------
 2 files changed, 4 insertions(+), 8 deletions(-)
Quoted 12 lines

drivers/pci/pci.h void pci_dpc_init(struct pci_dev *pdev); · line 436

Quoted 10 lines
drivers/pci/pci.h +2 -0
index 2336a8d1edab..03bf2776d73b 100644
@@ -432,11 +432,15 @@ void pci_dpc_init(struct pci_dev *pdev);
 void dpc_process_error(struct pci_dev *pdev);
 pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
 bool pci_dpc_recovered(struct pci_dev *pdev);
+void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
+void pci_acpi_remove_edr_notifier(struct pci_dev *pdev);

Protect them with CONFIG_ACPI?

Good idea, thanks! They're called only from pci-acpi.c, so I moved them inside the #ifdef CONFIG_ACPI above:

  #ifdef CONFIG_ACPI
  int pci_acpi_program_hp_params(struct pci_dev *dev);
  ...
  #ifdef CONFIG_PCIE_DPC
  void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
  void pci_acpi_remove_edr_notifier(struct pci_dev *pdev);
  #endif
  #else
  static inline int pci_acpi_program_hp_params(struct pci_dev *dev)
  {
          return -ENODEV;
  }
  #ifdef CONFIG_PCIE_DPC
  static inline void pci_acpi_add_edr_notifier(struct pci_dev *pdev) { }
  static inline void pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
  #endif
  #endif

Bjorn

#14 lore raw
In reply to Bjorn Helgaas
Quoted 5 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

Previously we could request control of DPC without AER, which is illegal per spec. Also, we could enable CONFIG_PCIE_DPC without CONFIG_PCIE_EDR, which is also illegal. This series addresses both.

I have a question here, how to understand the relationship EDR & AER ? somewhere EDR touches AER status without checking _OSC granted bits, such as
pci_aer_raw_clear_status(edev);

sometimes EDR calling AER with host->native_aer checked, like

pcie_do_recovery()
{
...

 if (host->native_aer || pcie_ports_native) {
		pcie_clear_device_status(dev);
		pci_aer_clear_nonfatal_status(dev);
	}

...
}

That is really confusing. could we do some cleanup to eliminate it ? such as seperate AER code into common code and runtime part.

Thanks,
Ethan

Quoted 14 lines

Bjorn Helgaas (3):
PCI/DPC: Request DPC only if also requesting AER PCI/DPC: Remove CONFIG_PCIE_EDR
PCI/DPC: Encapsulate pci_acpi_add_edr_notifier()

  drivers/acpi/pci_root.c   | 22 ++++++++++++----------
  drivers/pci/pci.h         |  4 ++++
  drivers/pci/pcie/Kconfig  | 14 ++++----------
  drivers/pci/pcie/Makefile |  5 ++++-
  drivers/pci/pcie/dpc.c    | 10 ----------
  include/linux/pci-acpi.h  |  8 --------
  6 files changed, 24 insertions(+), 39 deletions(-)
#15 lore raw

Hi,

Quoted 11 lines · Ethan Zhao
Quoted 5 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

Previously we could request control of DPC without AER, which is illegal per spec.  Also, we could enable CONFIG_PCIE_DPC without CONFIG_PCIE_EDR, which is also illegal.  This series addresses both.

I have a question here, how to understand the relationship EDR & AER ? somewhere EDR touches AER status without checking _OSC granted bits, such as
   pci_aer_raw_clear_status(edev);

Which_OSC bits?

EDR code will only get triggered if OS advertises the EDR support (which also means OS supports AER and DPC), and both AER and DPC is owned by the firmware. During the EDR notification, the OS is allowed to touch AER and DPC registers. So there is no problem with EDR code using AER routines.

Quoted 35 lines

sometimes EDR calling AER with host->native_aer checked, like

pcie_do_recovery()
{
 ...
 if (host->native_aer || pcie_ports_native) {
        pcie_clear_device_status(dev);
        pci_aer_clear_nonfatal_status(dev);
    }
 ...
}

That is really confusing. could we do some cleanup to eliminate it ? such as seperate AER code into common code and runtime part.

Thanks,
Ethan

Quoted 14 lines

Bjorn Helgaas (3):
   PCI/DPC: Request DPC only if also requesting AER PCI/DPC: Remove CONFIG_PCIE_EDR
   PCI/DPC: Encapsulate pci_acpi_add_edr_notifier()

  drivers/acpi/pci_root.c   | 22 ++++++++++++---------- drivers/pci/pci.h         |  4 ++++ drivers/pci/pcie/Kconfig  | 14 ++++---------- drivers/pci/pcie/Makefile |  5 ++++- drivers/pci/pcie/dpc.c    | 10 ---------- include/linux/pci-acpi.h  |  8 --------

  6 files changed, 24 insertions(+), 39 deletions(-)
Signature · 2 lines
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
#16 lore raw
Quoted 14 lines · Kuppuswamy Sathyanarayanan

Hi,

Quoted 10 lines · Ethan Zhao
Quoted 5 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

Previously we could request control of DPC without AER, which is illegal per spec.  Also, we could enable CONFIG_PCIE_DPC without CONFIG_PCIE_EDR, which is also illegal.  This series addresses both.

I have a question here, how to understand the relationship EDR & AER ? somewhere EDR touches AER status without checking _OSC granted bits, such as
   pci_aer_raw_clear_status(edev);

Quoted 5 lines

Which_OSC bits?

EDR code will only get triggered if OS advertises the EDR support (which also means OS supports AER and DPC), and both AER and DPC is owned by the firmware. During the EDR notification, the OS is allowed to touch AER

Means no need to check if host->native_aer ? why checked in pcie_do_recovery() ?

Thanks,
Ethan

Quoted 36 lines

and DPC registers. So there is no problem with EDR code using AER routines.

Quoted 33 lines

sometimes EDR calling AER with host->native_aer checked, like

pcie_do_recovery()
{
 ...
 if (host->native_aer || pcie_ports_native) {
        pcie_clear_device_status(dev);
        pci_aer_clear_nonfatal_status(dev);
    }
 ...
}

That is really confusing. could we do some cleanup to eliminate it ? such as seperate AER code into common code and runtime part.

Thanks,
Ethan

Quoted 13 lines

Bjorn Helgaas (3):
   PCI/DPC: Request DPC only if also requesting AER PCI/DPC: Remove CONFIG_PCIE_EDR
   PCI/DPC: Encapsulate pci_acpi_add_edr_notifier()

   drivers/acpi/pci_root.c   | 22 ++++++++++++----------
   drivers/pci/pci.h         |  4 ++++
   drivers/pci/pcie/Kconfig  | 14 ++++----------
   drivers/pci/pcie/Makefile |  5 ++++-
   drivers/pci/pcie/dpc.c    | 10 ----------
   include/linux/pci-acpi.h  |  8 --------
   6 files changed, 24 insertions(+), 39 deletions(-)
#17 lore raw
Quoted 15 lines · Ethan Zhao
Quoted 14 lines · Kuppuswamy Sathyanarayanan

Hi,

Quoted 10 lines · Ethan Zhao
Quoted 5 lines · Bjorn Helgaas

From: Bjorn Helgaas <[email protected]>

Previously we could request control of DPC without AER, which is illegal per spec.  Also, we could enable CONFIG_PCIE_DPC without CONFIG_PCIE_EDR, which is also illegal.  This series addresses both.

I have a question here, how to understand the relationship EDR & AER ? somewhere EDR touches AER status without checking _OSC granted bits, such as
    pci_aer_raw_clear_status(edev);

Quoted 8 lines
Quoted 5 lines

Which_OSC bits?

EDR code will only get triggered if OS advertises the EDR support (which also means OS supports AER and DPC), and both AER and DPC is owned by the firmware. During the EDR notification, the OS is allowed to touch AER

Means no need to check if host->native_aer ? why checked in pcie_do_recovery() ?

pcie_do_recovery() can be called form EDR (FF) or native path. That check is used to skip device status clearing in FF mode. You can find details about it in

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/pci/pcie/err.c?id=068c29a248b6ddbfdf7bb150b547569759620d36

Quoted 40 lines

Thanks,
Ethan

Quoted 35 lines

and DPC registers. So there is no problem with EDR code using AER routines.

Quoted 32 lines

sometimes EDR calling AER with host->native_aer checked, like

pcie_do_recovery()
{
  ...
  if (host->native_aer || pcie_ports_native) {
         pcie_clear_device_status(dev);
         pci_aer_clear_nonfatal_status(dev);
     }
  ...
}

That is really confusing. could we do some cleanup to eliminate it ? such as seperate AER code into common code and runtime part.

Thanks,
Ethan

Quoted 13 lines

Bjorn Helgaas (3):
    PCI/DPC: Request DPC only if also requesting AER PCI/DPC: Remove CONFIG_PCIE_EDR
    PCI/DPC: Encapsulate pci_acpi_add_edr_notifier()

   drivers/acpi/pci_root.c   | 22 ++++++++++++---------- drivers/pci/pci.h         |  4 ++++ drivers/pci/pcie/Kconfig  | 14 ++++---------- drivers/pci/pcie/Makefile |  5 ++++- drivers/pci/pcie/dpc.c    | 10 ---------- include/linux/pci-acpi.h  |  8 --------

   6 files changed, 24 insertions(+), 39 deletions(-)
Signature · 2 lines
Sathyanarayanan Kuppuswamy
Linux Kernel Developer