Add Error Disconnect Recover (EDR) support

PATCH Superseded [email protected] 6 participants 21 messages Jul 26, 2019 – Aug 16, 2019

Subsystems (3)

ACPI PCI PCI enhanced error handling (eeh) for PowerPC

lore

Share & tools

lore

[email protected]

author

2019-07-26 21:45 UTC #0 lore raw

v6 cover Add Error Disconnect Recover (EDR) support

From: Kuppuswamy Sathyanarayanan <[email protected]>

This patchset adds support for following features:

1. Error Disconnect Recover (EDR) support.
2. _OSC based negotiation support for DPC.

You can find EDR spec in the following link.

https://members.pcisig.com/wg/PCI-SIG/document/12614

Changes since v5:
* Addressed Keith's comments.
* Added additional check for FF mode in pci_aer_init().
* Updated commit history of "PCI/DPC: Add support for DPC recovery on NON_FATAL errors" patch.

Changes since v4:
* Rebased on top of v5.3-rc1
* Fixed lock/unlock issue in edr_handle_event().
* Merged "Update error status after reset_link()" patch into this patchset.

Changes since v3:
* Moved EDR related ACPI functions/definitions to pci-acpi.c
* Modified commit history in few patches to include spec reference.
* Added support to handle DPC triggered by NON_FATAL errors.
* Added edr_lock to protect PCI device receiving duplicate EDR notifications.
* Addressed Bjorn comments.

Changes since v2:
* Split EDR support patch into multiple patches.
* Addressed Bjorn comments.

Changes since v1:
* Rebased on top of v5.1-rc1

Kuppuswamy Sathyanarayanan (9):
PCI/ERR: Update error status after reset_link()
PCI/ACPI: Add _OSC based negotiation support for DPC PCI/ACPI: Expose EDR support via _OSC to BIOS
PCI/DPC: Allow dpc_probe() even if firmware first mode is enabled PCI/DPC: Add dpc_process_error() wrapper function PCI/DPC: Add Error Disconnect Recover (EDR) support PCI/AER: Allow clearing Error Status Register in FF mode PCI/DPC: Update comments related to DPC recovery on NON_FATAL errors PCI/DPC: Clear AER registers in EDR mode

 drivers/acpi/pci_root.c         |   9 ++
 drivers/pci/pci-acpi.c          |  91 +++++++++++++++
 drivers/pci/pcie/Kconfig        |  10 ++
 drivers/pci/pcie/aer.c          |  12 +-
 drivers/pci/pcie/dpc.c          | 196 +++++++++++++++++++++++++++++---
 drivers/pci/pcie/err.c          |  11 +-
 drivers/pci/pcie/portdrv_core.c |   8 +-
 drivers/pci/probe.c             |   1 +
 include/linux/acpi.h            |   4 +-
 include/linux/pci-acpi.h        |  11 ++
 include/linux/pci.h             |   2 +-
 11 files changed, 322 insertions(+), 33 deletions(-)

Signature · 1 line

2.21.0

[email protected]

author

2019-07-26 21:46 UTC #1 lore raw

PATCH v6 1/9 PCI/ERR: Update error status after reset_link()

From: Kuppuswamy Sathyanarayanan <[email protected]>

Commit bdb5ac85777d ("PCI/ERR: Handle fatal error recovery") uses reset_link() to recover from fatal errors. But, if the reset is successful there is no need to continue the rest of the error recovery checks. Also, during fatal error recovery, if the initial value of error status is PCI_ERS_RESULT_DISCONNECT or PCI_ERS_RESULT_NO_AER_DRIVER then even after successful recovery (using reset_link()) pcie_do_recovery() will report the recovery result as failure. So update the status of error after reset_link().

Fixes bdb5ac85777d ("PCI/ERR: Handle fatal error recovery")

Cc Ashok Raj <[email protected]>

Cc Keith Busch <[email protected]>

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

---
 drivers/pci/pcie/err.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

drivers/pci/pcie/err.c +8 -3

index 773197a12568..aecec124a829 100644
@@ -204,9 +204,13 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
 	else
 		pci_walk_bus(bus, report_normal_detected, &status);
 
-	if (state == pci_channel_io_frozen &&
-	    reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
-		goto failed;
+	if (state == pci_channel_io_frozen) {
+		status = reset_link(dev, service);
+		if (status != PCI_ERS_RESULT_RECOVERED)
+			goto failed;
+		else
+			goto done;
+	}
 
 	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
 		status = PCI_ERS_RESULT_RECOVERED;

@@ -228,6 +232,7 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
 	if (status != PCI_ERS_RESULT_RECOVERED)
 		goto failed;
 
+done:
 	pci_dbg(dev, "broadcast resume message\n");
 	pci_walk_bus(bus, report_resume, &status);

Signature · 1 line

2.21.0

Bjorn Helgaas

2019-08-15 22:17 UTC #2 lore raw

Re: [PATCH v6 1/9] PCI/ERR: Update error status after reset_link()

Quoted 26 lines · On Fri, Jul 26, 2019 at 02:43:11PM -0700…

From: Kuppuswamy Sathyanarayanan <[email protected]>
Commit bdb5ac85777d ("PCI/ERR: Handle fatal error recovery") uses reset_link() to recover from fatal errors. But, if the reset is successful there is no need to continue the rest of the error recovery checks. Also, during fatal error recovery, if the initial value of error status is PCI_ERS_RESULT_DISCONNECT or PCI_ERS_RESULT_NO_AER_DRIVER then even after successful recovery (using reset_link()) pcie_do_recovery() will report the recovery result as failure. So update the status of error after reset_link().
Fixes bdb5ac85777d ("PCI/ERR: Handle fatal error recovery")
Cc Ashok Raj <[email protected]>
Cc Keith Busch <[email protected]>
Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>
---
 drivers/pci/pcie/err.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/pci/pcie/err.c +0 -0
index 773197a12568..aecec124a829 100644
@@ -204,9 +204,13 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
 	else
 		pci_walk_bus(bus, report_normal_detected, &status);

drivers/pci/pcie/err.c void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state, · line 212

Quoted 10 lines

drivers/pci/pcie/err.c +6 -3

 
-	if (state == pci_channel_io_frozen &&
-	    reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
-		goto failed;
+	if (state == pci_channel_io_frozen) {
+		status = reset_link(dev, service);
+		if (status != PCI_ERS_RESULT_RECOVERED)
+			goto failed;
+		else
+			goto done;

This will be easier to read without the negation, i.e.,

                if (status == PCI_ERS_RESULT_RECOVERED)
                        goto done;
                else
                        goto failed;

Quoted 15 lines

drivers/pci/pcie/err.c +2 -0

+	}
 
 	if (status == PCI_ERS_RESULT_CAN_RECOVER) {
 		status = PCI_ERS_RESULT_RECOVERED;

@@ -228,6 +232,7 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
 	if (status != PCI_ERS_RESULT_RECOVERED)
 		goto failed;
 
+done:
 	pci_dbg(dev, "broadcast resume message\n");
 	pci_walk_bus(bus, report_resume, &status);

Signature · 1 line

2.21.0

Kuppuswamy Sathyanarayanan

author

2019-08-15 22:55 UTC #3 lore raw

Re: [PATCH v6 1/9] PCI/ERR: Update error status after reset_link()

Hi,

Quoted 39 lines · Bjorn Helgaas

Quoted 26 lines · On Fri, Jul 26, 2019 at 02:43:11PM -0700…
From: Kuppuswamy Sathyanarayanan <[email protected]>
Commit bdb5ac85777d ("PCI/ERR: Handle fatal error recovery") uses reset_link() to recover from fatal errors. But, if the reset is successful there is no need to continue the rest of the error recovery checks. Also, during fatal error recovery, if the initial value of error status is PCI_ERS_RESULT_DISCONNECT or PCI_ERS_RESULT_NO_AER_DRIVER then even after successful recovery (using reset_link()) pcie_do_recovery() will report the recovery result as failure. So update the status of error after reset_link().
Fixes bdb5ac85777d ("PCI/ERR: Handle fatal error recovery")
Cc Ashok Raj <[email protected]>
Cc Keith Busch <[email protected]>
Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>
---
  drivers/pci/pcie/err.c | 11 ++++++++---
  1 file changed, 8 insertions(+), 3 deletions(-)
drivers/pci/pcie/err.c +0 -0
index 773197a12568..aecec124a829 100644
@@ -204,9 +204,13 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
  	else
  		pci_walk_bus(bus, report_normal_detected, &status);
drivers/pci/pcie/err.c void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state, · line 212
Quoted 10 lines
drivers/pci/pcie/err.c +6 -3
 
-	if (state == pci_channel_io_frozen &&
-	    reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
-		goto failed;
+	if (state == pci_channel_io_frozen) {
+		status = reset_link(dev, service);
+		if (status != PCI_ERS_RESULT_RECOVERED)
+			goto failed;
+		else
+			goto done;
This will be easier to read without the negation, i.e.,

Quoted 4 lines

                 if (status == PCI_ERS_RESULT_RECOVERED)
                         goto done;
                 else
                         goto failed;

will change it in next version.

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Linux kernel developer

Joe Perches

2019-08-16 01:54 UTC #4 lore raw

Re: [PATCH v6 1/9] PCI/ERR: Update error status after reset_link()

In reply to Bjorn Helgaas

Quoted 11 lines · Bjorn Helgaas

Quoted 10 lines · On Fri, Jul 26, 2019 at 02:43:11PM -0700…
From: Kuppuswamy Sathyanarayanan <[email protected]>
Commit bdb5ac85777d ("PCI/ERR: Handle fatal error recovery") uses reset_link() to recover from fatal errors. But, if the reset is successful there is no need to continue the rest of the error recovery checks. Also, during fatal error recovery, if the initial value of error status is PCI_ERS_RESULT_DISCONNECT or PCI_ERS_RESULT_NO_AER_DRIVER then even after successful recovery (using reset_link()) pcie_do_recovery() will report the recovery result as failure. So update the status of error after reset_link().

[]

Quoted 1 line

drivers/pci/pcie/err.c
Quoted 1 line
drivers/pci/pcie/err.c +0 -0

[]

Quoted 16 lines

drivers/pci/pcie/err.c void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state, · line 212

Quoted 13 lines

drivers/pci/pcie/err.c +6 -3

@@ -204,9 +204,13 @@ void pcie_do_recovery(struct pci_dev *dev, enum pci_channel_state state,
 	else
 		pci_walk_bus(bus, report_normal_detected, &status);
 
-	if (state == pci_channel_io_frozen &&
-	    reset_link(dev, service) != PCI_ERS_RESULT_RECOVERED)
-		goto failed;
+	if (state == pci_channel_io_frozen) {
+		status = reset_link(dev, service);
+		if (status != PCI_ERS_RESULT_RECOVERED)
+			goto failed;
+		else
+			goto done;

This will be easier to read without the negation, i.e.,

Quoted 4 lines

                if (status == PCI_ERS_RESULT_RECOVERED)
                        goto done;
                else
                        goto failed;

bikeshed: I think it'd be easier to read without the else

		status = reset_link(dev, service);
		if (status != PCI_ERS_RESULT_RECOVERED)
			goto failed;

		goto done;

[email protected]

author

2019-07-26 21:46 UTC #5 lore raw

PATCH v6 2/9 PCI/ACPI: Add _OSC based negotiation support for DPC

In reply to [email protected]

From: Kuppuswamy Sathyanarayanan <[email protected]>

As per PCI firmware specification r3.2 Downstream Port Containment Related Enhancements ECN, sec 4.5.1, table 4-6, OS can use bit 7 of _OSC Control Field to negotiate control over Downstream Port Containment (DPC) configuration of PCIe port.

After _OSC negotiation, firmware will Set this bit to grant OS control over PCIe DPC configuration and Clear it if this feature was requested and denied, or was not requested.

Cc Bjorn Helgaas <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Len Brown <[email protected]>

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

---
 drivers/acpi/pci_root.c         | 6 ++++++
 drivers/pci/pcie/portdrv_core.c | 3 ++-
 drivers/pci/probe.c             | 1 +
 include/linux/acpi.h            | 3 ++-
 include/linux/pci.h             | 2 +-
 5 files changed, 12 insertions(+), 3 deletions(-)

drivers/acpi/pci_root.c +6 -0

index 314a187ed572..73b08f40b0da 100644
@@ -142,6 +142,7 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = {
 	{ OSC_PCI_EXPRESS_AER_CONTROL, "AER" },
 	{ OSC_PCI_EXPRESS_CAPABILITY_CONTROL, "PCIeCapability" },
 	{ OSC_PCI_EXPRESS_LTR_CONTROL, "LTR" },
+	{ OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" },
 };
 
 static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word,

@@ -488,6 +489,9 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 			control |= OSC_PCI_EXPRESS_AER_CONTROL;
 	}
 
+	if (IS_ENABLED(CONFIG_PCIE_DPC))
+		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
+
 	requested = control;
 	status = acpi_pci_osc_control_set(handle, &control,
 					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);

@@ -917,6 +921,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 		host_bridge->native_pme = 0;
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
 		host_bridge->native_ltr = 0;
+	if (!(root->osc_control_set & OSC_PCI_EXPRESS_DPC_CONTROL))
+		host_bridge->native_dpc = 0;
 
 	/*
 	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it

drivers/pci/pcie/portdrv_core.c +2 -1

index 308c3e0c4a34..58c40fe7856f 100644
@@ -252,7 +252,8 @@ static int get_port_device_capability(struct pci_dev *dev)
 	}
 
 	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
-	    pci_aer_available() && services & PCIE_PORT_SERVICE_AER)
+	    pci_aer_available() && services & PCIE_PORT_SERVICE_AER &&
+	    (pcie_ports_native || host->native_dpc))
 		services |= PCIE_PORT_SERVICE_DPC;
 
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||

drivers/pci/probe.c +1 -0

index a3c7338fad86..cf8acdd62089 100644
@@ -601,6 +601,7 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge)
 	bridge->native_shpc_hotplug = 1;
 	bridge->native_pme = 1;
 	bridge->native_ltr = 1;
+	bridge->native_dpc = 1;
 }
 
 struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)

include/linux/acpi.h +2 -1

index 9426b9aaed86..8959ed322e15 100644
@@ -525,7 +525,8 @@ extern bool osc_pc_lpi_support_confirmed;
 #define OSC_PCI_EXPRESS_AER_CONTROL		0x00000008
 #define OSC_PCI_EXPRESS_CAPABILITY_CONTROL	0x00000010
 #define OSC_PCI_EXPRESS_LTR_CONTROL		0x00000020
-#define OSC_PCI_CONTROL_MASKS			0x0000003f
+#define OSC_PCI_EXPRESS_DPC_CONTROL		0x00000080
+#define OSC_PCI_CONTROL_MASKS			0x000000ff
 
 #define ACPI_GSB_ACCESS_ATTRIB_QUICK		0x00000002
 #define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV         0x00000004

include/linux/pci.h +1 -1

index 9e700d9f9f28..9145136ca728 100644
@@ -510,7 +510,7 @@ struct pci_host_bridge {
 	unsigned int	native_pme:1;		/* OS may use PCIe PME */
 	unsigned int	native_ltr:1;		/* OS may use PCIe LTR */
 	unsigned int	preserve_config:1;	/* Preserve FW resource setup */
-
+	unsigned int	native_dpc:1;		/* OS may use PCIe DPC */
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
 			const struct resource *res,

Signature · 1 line

2.21.0

Bjorn Helgaas

2019-08-15 22:19 UTC #6 lore raw

Re: [PATCH v6 2/9] PCI/ACPI: Add _OSC based negotiation support for DPC

Quoted 36 lines · On Fri, Jul 26, 2019 at 02:43:12PM -0700…

From: Kuppuswamy Sathyanarayanan <[email protected]>
As per PCI firmware specification r3.2 Downstream Port Containment Related Enhancements ECN, sec 4.5.1, table 4-6, OS can use bit 7 of _OSC Control Field to negotiate control over Downstream Port Containment (DPC) configuration of PCIe port.
After _OSC negotiation, firmware will Set this bit to grant OS control over PCIe DPC configuration and Clear it if this feature was requested and denied, or was not requested.
Cc Bjorn Helgaas <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Len Brown <[email protected]>
Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>
---
 drivers/acpi/pci_root.c         | 6 ++++++
 drivers/pci/pcie/portdrv_core.c | 3 ++-
 drivers/pci/probe.c             | 1 +
 include/linux/acpi.h            | 3 ++-
 include/linux/pci.h             | 2 +-
 5 files changed, 12 insertions(+), 3 deletions(-)
drivers/acpi/pci_root.c +1 -0
index 314a187ed572..73b08f40b0da 100644
@@ -142,6 +142,7 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = {
 	{ OSC_PCI_EXPRESS_AER_CONTROL, "AER" },
 	{ OSC_PCI_EXPRESS_CAPABILITY_CONTROL, "PCIeCapability" },
 	{ OSC_PCI_EXPRESS_LTR_CONTROL, "LTR" },
+	{ OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" },
 };
 
 static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word,

drivers/acpi/pci_root.c static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, · line 493

Quoted 5 lines

drivers/acpi/pci_root.c +2 -0

@@ -488,6 +489,9 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 			control |= OSC_PCI_EXPRESS_AER_CONTROL;
 	}
 
+	if (IS_ENABLED(CONFIG_PCIE_DPC))
+		control |= OSC_PCI_EXPRESS_DPC_CONTROL;

Sec 4.5.2.4 says:

If the OS sets bit 7 of the Control field, it must set bit 7 of the Support field, indicating support for the Error Disconnect Recover event.

I see that you do set bit 7 (OSC_PCI_EDR_SUPPORT) in the Support field in a later patch, but I don't think we should have this intermediate state where we set OSC_PCI_EXPRESS_DPC_CONTROL in Control but not OSC_PCI_EDR_SUPPORT in Support.

Quoted 43 lines

drivers/acpi/pci_root.c +2 -0

 	requested = control;
 	status = acpi_pci_osc_control_set(handle, &control,
 					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);

@@ -917,6 +921,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 		host_bridge->native_pme = 0;
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
 		host_bridge->native_ltr = 0;
+	if (!(root->osc_control_set & OSC_PCI_EXPRESS_DPC_CONTROL))
+		host_bridge->native_dpc = 0;
 
 	/*
 	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it

drivers/pci/pcie/portdrv_core.c +2 -1

index 308c3e0c4a34..58c40fe7856f 100644
@@ -252,7 +252,8 @@ static int get_port_device_capability(struct pci_dev *dev)
 	}
 
 	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
-	    pci_aer_available() && services & PCIE_PORT_SERVICE_AER)
+	    pci_aer_available() && services & PCIE_PORT_SERVICE_AER &&
+	    (pcie_ports_native || host->native_dpc))
 		services |= PCIE_PORT_SERVICE_DPC;
 
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||

drivers/pci/probe.c +1 -0

index a3c7338fad86..cf8acdd62089 100644
@@ -601,6 +601,7 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge)
 	bridge->native_shpc_hotplug = 1;
 	bridge->native_pme = 1;
 	bridge->native_ltr = 1;
+	bridge->native_dpc = 1;
 }
 
 struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)

include/linux/acpi.h +0 -0

index 9426b9aaed86..8959ed322e15 100644

include/linux/acpi.h extern bool osc_pc_lpi_support_confirmed; · line 529

Quoted 6 lines

include/linux/acpi.h +2 -1

@@ -525,7 +525,8 @@ extern bool osc_pc_lpi_support_confirmed;
 #define OSC_PCI_EXPRESS_AER_CONTROL		0x00000008
 #define OSC_PCI_EXPRESS_CAPABILITY_CONTROL	0x00000010
 #define OSC_PCI_EXPRESS_LTR_CONTROL		0x00000020
-#define OSC_PCI_CONTROL_MASKS			0x0000003f
+#define OSC_PCI_EXPRESS_DPC_CONTROL		0x00000080
+#define OSC_PCI_CONTROL_MASKS			0x000000ff

You added 0x80, but 0x3f | 0x80 == 0xbf, not 0xff, so I expected OSC_PCI_CONTROL_MASKS would change to 0xbf. Why the difference?

include/linux/pci.h struct pci_host_bridge { · line 513

Quoted 12 lines

include/linux/acpi.h +0 -0

 #define ACPI_GSB_ACCESS_ATTRIB_QUICK		0x00000002
 #define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV         0x00000004

include/linux/pci.h +1 -1

index 9e700d9f9f28..9145136ca728 100644
@@ -510,7 +510,7 @@ struct pci_host_bridge {
 	unsigned int	native_pme:1;		/* OS may use PCIe PME */
 	unsigned int	native_ltr:1;		/* OS may use PCIe LTR */
 	unsigned int	preserve_config:1;	/* Preserve FW resource setup */
-
+	unsigned int	native_dpc:1;		/* OS may use PCIe DPC */

Please put this next to the other "native_*" bits and preserve the blank line.

Quoted 6 lines

include/linux/pci.h +0 -0

 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
 			const struct resource *res,

Signature · 1 line

2.21.0

Kuppuswamy Sathyanarayanan

author

2019-08-15 23:47 UTC #7 lore raw

Re: [PATCH v6 2/9] PCI/ACPI: Add _OSC based negotiation support for DPC

Quoted 40 lines · Bjorn Helgaas

Quoted 39 lines · On Fri, Jul 26, 2019 at 02:43:12PM -0700…
From: Kuppuswamy Sathyanarayanan <[email protected]>
As per PCI firmware specification r3.2 Downstream Port Containment Related Enhancements ECN, sec 4.5.1, table 4-6, OS can use bit 7 of _OSC Control Field to negotiate control over Downstream Port Containment (DPC) configuration of PCIe port.
After _OSC negotiation, firmware will Set this bit to grant OS control over PCIe DPC configuration and Clear it if this feature was requested and denied, or was not requested.
Cc Bjorn Helgaas <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Len Brown <[email protected]>
Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>
---
  drivers/acpi/pci_root.c         | 6 ++++++
  drivers/pci/pcie/portdrv_core.c | 3 ++-
  drivers/pci/probe.c             | 1 +
  include/linux/acpi.h            | 3 ++-
  include/linux/pci.h             | 2 +-
  5 files changed, 12 insertions(+), 3 deletions(-)
drivers/acpi/pci_root.c +1 -0
index 314a187ed572..73b08f40b0da 100644
@@ -142,6 +142,7 @@ static struct pci_osc_bit_struct pci_osc_control_bit[] = {
  	{ OSC_PCI_EXPRESS_AER_CONTROL, "AER" },
  	{ OSC_PCI_EXPRESS_CAPABILITY_CONTROL, "PCIeCapability" },
  	{ OSC_PCI_EXPRESS_LTR_CONTROL, "LTR" },
+	{ OSC_PCI_EXPRESS_DPC_CONTROL, "DPC" },
  };
 
  static void decode_osc_bits(struct acpi_pci_root *root, char *msg, u32 word,
@@ -488,6 +489,9 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
  			control |= OSC_PCI_EXPRESS_AER_CONTROL;
  	}
 

Quoted 7 lines

drivers/acpi/pci_root.c static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, · line 493
Quoted 2 lines
drivers/acpi/pci_root.c +2 -0
+	if (IS_ENABLED(CONFIG_PCIE_DPC))
+		control |= OSC_PCI_EXPRESS_DPC_CONTROL;
Sec 4.5.2.4 says:
If the OS sets bit 7 of the Control field, it must set bit 7 of the Support field, indicating support for the Error Disconnect Recover event.

I think the correct dependency should be , if OS indicates support for EDR then it must set bit 7 of the Control field to indicate support for DPC. As per DPC control bit definition, setting this bit indicates requesting control of DPC for OS (not EDR).

I will ask the spec author for clarification. But for now I will go with spec requirement, I will merge this patch with "Expose EDR support via _OSC to BIOS" patch and push it to the end of the patch set.

Quoted 43 lines

Quoted 37 lines

drivers/acpi/pci_root.c +2 -0

  	requested = control;
  	status = acpi_pci_osc_control_set(handle, &control,
  					  OSC_PCI_EXPRESS_CAPABILITY_CONTROL);

@@ -917,6 +921,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
  		host_bridge->native_pme = 0;
  	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
  		host_bridge->native_ltr = 0;
+	if (!(root->osc_control_set & OSC_PCI_EXPRESS_DPC_CONTROL))
+		host_bridge->native_dpc = 0;
 
  	/*
  	 * Evaluate the "PCI Boot Configuration" _DSM Function.  If it

drivers/pci/pcie/portdrv_core.c +2 -1

index 308c3e0c4a34..58c40fe7856f 100644
@@ -252,7 +252,8 @@ static int get_port_device_capability(struct pci_dev *dev)
  	}
 
  	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
-	    pci_aer_available() && services & PCIE_PORT_SERVICE_AER)
+	    pci_aer_available() && services & PCIE_PORT_SERVICE_AER &&
+	    (pcie_ports_native || host->native_dpc))
  		services |= PCIE_PORT_SERVICE_DPC;
 
  	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||

drivers/pci/probe.c +1 -0

index a3c7338fad86..cf8acdd62089 100644
@@ -601,6 +601,7 @@ static void pci_init_host_bridge(struct pci_host_bridge *bridge)
  	bridge->native_shpc_hotplug = 1;
  	bridge->native_pme = 1;
  	bridge->native_ltr = 1;
+	bridge->native_dpc = 1;
  }

Quoted 14 lines

include/linux/acpi.h extern bool osc_pc_lpi_support_confirmed; · line 529
Quoted 12 lines
drivers/pci/probe.c +0 -0
  struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
include/linux/acpi.h +2 -1
index 9426b9aaed86..8959ed322e15 100644
@@ -525,7 +525,8 @@ extern bool osc_pc_lpi_support_confirmed;
  #define OSC_PCI_EXPRESS_AER_CONTROL		0x00000008
  #define OSC_PCI_EXPRESS_CAPABILITY_CONTROL	0x00000010
  #define OSC_PCI_EXPRESS_LTR_CONTROL		0x00000020
-#define OSC_PCI_CONTROL_MASKS			0x0000003f
+#define OSC_PCI_EXPRESS_DPC_CONTROL		0x00000080
+#define OSC_PCI_CONTROL_MASKS			0x000000ff
You added 0x80, but 0x3f | 0x80 == 0xbf, not 0xff, so I expected OSC_PCI_CONTROL_MASKS would change to 0xbf. Why the difference?

Good catch. Even though spec has support for bit[6], it should be masked till OS supports it. I will fix it in next version.

Quoted 15 lines

include/linux/pci.h struct pci_host_bridge { · line 513

Quoted 13 lines

include/linux/acpi.h +0 -0

  #define ACPI_GSB_ACCESS_ATTRIB_QUICK		0x00000002
  #define ACPI_GSB_ACCESS_ATTRIB_SEND_RCV         0x00000004

include/linux/pci.h +1 -1

index 9e700d9f9f28..9145136ca728 100644
@@ -510,7 +510,7 @@ struct pci_host_bridge {
  	unsigned int	native_pme:1;		/* OS may use PCIe PME */
  	unsigned int	native_ltr:1;		/* OS may use PCIe LTR */
  	unsigned int	preserve_config:1;	/* Preserve FW resource setup */
-
+	unsigned int	native_dpc:1;		/* OS may use PCIe DPC */

Please put this next to the other "native_*" bits and preserve the blank line.

ok.

Quoted 7 lines

include/linux/pci.h +0 -0

  	/* Resource alignment requirements */
  	resource_size_t (*align_resource)(struct pci_dev *dev,
  			const struct resource *res,

Signature · 1 line

2.21.0

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Linux kernel developer

[email protected]

author

2019-07-26 21:45 UTC #8 lore raw

PATCH v6 3/9 PCI/ACPI: Expose EDR support via _OSC to BIOS

In reply to [email protected]

From: Kuppuswamy Sathyanarayanan <[email protected]>

As per PCI firmware specification r3.2 Downstream Port Containment Related Enhancements ECN, sec 4.5.1, table 4-4, if OS supports EDR, it should expose its support to BIOS by setting bit 7 of _OSC Support Field.

Cc Bjorn Helgaas <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Len Brown <[email protected]>

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

---
 drivers/acpi/pci_root.c | 3 +++
 include/linux/acpi.h    | 1 +
 2 files changed, 4 insertions(+)

drivers/acpi/pci_root.c +3 -0

index 73b08f40b0da..988d09d788b6 100644
@@ -132,6 +132,7 @@ static struct pci_osc_bit_struct pci_osc_support_bit[] = {
 	{ OSC_PCI_CLOCK_PM_SUPPORT, "ClockPM" },
 	{ OSC_PCI_SEGMENT_GROUPS_SUPPORT, "Segments" },
 	{ OSC_PCI_MSI_SUPPORT, "MSI" },
+	{ OSC_PCI_EDR_SUPPORT, "EDR" },
 	{ OSC_PCI_HPX_TYPE_3_SUPPORT, "HPX-Type3" },
 };

@@ -442,6 +443,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 		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))
+		support |= OSC_PCI_EDR_SUPPORT;
 
 	decode_osc_support(root, "OS supports", support);
 	status = acpi_pci_osc_support(root, support);

include/linux/acpi.h +1 -0

index 8959ed322e15..b6b43da85d26 100644
@@ -515,6 +515,7 @@ extern bool osc_pc_lpi_support_confirmed;
 #define OSC_PCI_CLOCK_PM_SUPPORT		0x00000004
 #define OSC_PCI_SEGMENT_GROUPS_SUPPORT		0x00000008
 #define OSC_PCI_MSI_SUPPORT			0x00000010
+#define OSC_PCI_EDR_SUPPORT			0x00000080
 #define OSC_PCI_HPX_TYPE_3_SUPPORT		0x00000100
 #define OSC_PCI_SUPPORT_MASKS			0x0000011f

Signature · 1 line

2.21.0

Bjorn Helgaas

2019-08-15 22:19 UTC #9 lore raw

Re: [PATCH v6 3/9] PCI/ACPI: Expose EDR support via _OSC to BIOS

Quoted 29 lines · On Fri, Jul 26, 2019 at 02:43:13PM -0700…

From: Kuppuswamy Sathyanarayanan <[email protected]>
As per PCI firmware specification r3.2 Downstream Port Containment Related Enhancements ECN, sec 4.5.1, table 4-4, if OS supports EDR, it should expose its support to BIOS by setting bit 7 of _OSC Support Field.
Cc Bjorn Helgaas <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Len Brown <[email protected]>
Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>
---
 drivers/acpi/pci_root.c | 3 +++
 include/linux/acpi.h    | 1 +
 2 files changed, 4 insertions(+)
drivers/acpi/pci_root.c +1 -0
index 73b08f40b0da..988d09d788b6 100644
@@ -132,6 +132,7 @@ static struct pci_osc_bit_struct pci_osc_support_bit[] = {
 	{ OSC_PCI_CLOCK_PM_SUPPORT, "ClockPM" },
 	{ OSC_PCI_SEGMENT_GROUPS_SUPPORT, "Segments" },
 	{ OSC_PCI_MSI_SUPPORT, "MSI" },
+	{ OSC_PCI_EDR_SUPPORT, "EDR" },
 	{ OSC_PCI_HPX_TYPE_3_SUPPORT, "HPX-Type3" },
 };
 

drivers/acpi/pci_root.c static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, · line 447

Quoted 5 lines

drivers/acpi/pci_root.c +2 -0

@@ -442,6 +443,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
 		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))
+		support |= OSC_PCI_EDR_SUPPORT;

Do we really support it here? This is patch [3/9] and it looks like patch [6/9] might be where EDR support really gets added. It's good to split changes into small pieces, but only if each piece is logically self-contained.

include/linux/acpi.h extern bool osc_pc_lpi_support_confirmed; · line 520

Quoted 13 lines

drivers/acpi/pci_root.c +0 -0

 	decode_osc_support(root, "OS supports", support);
 	status = acpi_pci_osc_support(root, support);

include/linux/acpi.h +1 -0

index 8959ed322e15..b6b43da85d26 100644
@@ -515,6 +515,7 @@ extern bool osc_pc_lpi_support_confirmed;
 #define OSC_PCI_CLOCK_PM_SUPPORT		0x00000004
 #define OSC_PCI_SEGMENT_GROUPS_SUPPORT		0x00000008
 #define OSC_PCI_MSI_SUPPORT			0x00000010
+#define OSC_PCI_EDR_SUPPORT			0x00000080
 #define OSC_PCI_HPX_TYPE_3_SUPPORT		0x00000100
 #define OSC_PCI_SUPPORT_MASKS			0x0000011f

You defined a new bit above but didn't update OSC_PCI_SUPPORT_MASKS to include that bit. This looks broken.

Kuppuswamy Sathyanarayanan

author

2019-08-15 23:48 UTC #10 lore raw

Re: [PATCH v6 3/9] PCI/ACPI: Expose EDR support via _OSC to BIOS

Quoted 29 lines · Bjorn Helgaas

Quoted 28 lines · On Fri, Jul 26, 2019 at 02:43:13PM -0700…
From: Kuppuswamy Sathyanarayanan <[email protected]>
As per PCI firmware specification r3.2 Downstream Port Containment Related Enhancements ECN, sec 4.5.1, table 4-4, if OS supports EDR, it should expose its support to BIOS by setting bit 7 of _OSC Support Field.
Cc Bjorn Helgaas <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Len Brown <[email protected]>
Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>
---
  drivers/acpi/pci_root.c | 3 +++
  include/linux/acpi.h    | 1 +
  2 files changed, 4 insertions(+)
drivers/acpi/pci_root.c +1 -0
index 73b08f40b0da..988d09d788b6 100644
@@ -132,6 +132,7 @@ static struct pci_osc_bit_struct pci_osc_support_bit[] = {
  	{ OSC_PCI_CLOCK_PM_SUPPORT, "ClockPM" },
  	{ OSC_PCI_SEGMENT_GROUPS_SUPPORT, "Segments" },
  	{ OSC_PCI_MSI_SUPPORT, "MSI" },
+	{ OSC_PCI_EDR_SUPPORT, "EDR" },
  	{ OSC_PCI_HPX_TYPE_3_SUPPORT, "HPX-Type3" },
  };
 

Quoted 10 lines

drivers/acpi/pci_root.c static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm, · line 447
Quoted 6 lines
drivers/acpi/pci_root.c +2 -0
@@ -442,6 +443,8 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm,
  		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))
+		support |= OSC_PCI_EDR_SUPPORT;
Do we really support it here? This is patch [3/9] and it looks like patch [6/9] might be where EDR support really gets added. It's good to split changes into small pieces, but only if each piece is logically self-contained.

I will move this patch to the end of this series.

Quoted 17 lines

Quoted 8 lines
drivers/acpi/pci_root.c +0 -0
  	decode_osc_support(root, "OS supports", support);
  	status = acpi_pci_osc_support(root, support);
include/linux/acpi.h +0 -0
index 8959ed322e15..b6b43da85d26 100644
include/linux/acpi.h extern bool osc_pc_lpi_support_confirmed; · line 520
Quoted 6 lines
include/linux/acpi.h +1 -0
@@ -515,6 +515,7 @@ extern bool osc_pc_lpi_support_confirmed;
  #define OSC_PCI_CLOCK_PM_SUPPORT		0x00000004
  #define OSC_PCI_SEGMENT_GROUPS_SUPPORT		0x00000008
  #define OSC_PCI_MSI_SUPPORT			0x00000010
+#define OSC_PCI_EDR_SUPPORT			0x00000080
  #define OSC_PCI_HPX_TYPE_3_SUPPORT		0x00000100
  #define OSC_PCI_SUPPORT_MASKS			0x0000011f
You defined a new bit above but didn't update OSC_PCI_SUPPORT_MASKS to include that bit. This looks broken.

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Linux kernel developer

[email protected]

author

2019-07-26 21:46 UTC #11 lore raw

PATCH v6 4/9 PCI/DPC: Allow dpc_probe() even if firmware first mode is enabled

In reply to [email protected]

From: Kuppuswamy Sathyanarayanan <[email protected]>

As per ACPI specification v6.3, sec 5.6.6, Error Disconnect Recover (EDR) notification used by firmware to let OS know about the DPC event and permit OS to perform error recovery when processing the EDR notification. Also, as per PCI firmware specification r3.2 Downstream Port Containment Related Enhancements ECN, sec 4.5.1, table 4-6, if DPC is controlled by firmware (firmware first mode), it's responsible for initializing Downstream Port Containment Extended Capability Structures per firmware policy. And, OS is permitted to read or write DPC Control and Status registers of a port while processing an Error Disconnect Recover (EDR) notification from firmware on that port.

Currently, if firmware controls DPC (firmware first mode), OS will not create/enumerate DPC PCIe port services. But, if OS supports EDR feature, then as mentioned in above spec references, it should permit enumeration of DPC driver and also support handling ACPI EDR notification. So as first step, allow dpc_probe() to continue even if firmware first mode is enabled. Also add appropriate checks to ensure device registers are not modified outside EDR notification window in firmware first mode. This is a preparatory patch for adding EDR support.

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

---
 drivers/pci/pcie/dpc.c | 49 +++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 13 deletions(-)

drivers/pci/pcie/dpc.c +36 ~~-13~~

index a32ec3487a8d..9717fda012f8 100644
@@ -22,6 +22,8 @@ struct dpc_dev {
 	u16			cap_pos;
 	bool			rp_extensions;
 	u8			rp_log_size;
+	/* Set True if DPC is controlled by firmware */
+	bool			firmware_dpc;
 };
 
 static const char * const rp_pio_error_string[] = {

@@ -69,6 +71,9 @@ void pci_save_dpc_state(struct pci_dev *dev)
 	if (!dpc)
 		return;
 
+	if (dpc->firmware_dpc)
+		return;
+
 	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
 	if (!save_state)
 		return;

@@ -90,6 +95,9 @@ void pci_restore_dpc_state(struct pci_dev *dev)
 	if (!dpc)
 		return;
 
+	if (dpc->firmware_dpc)
+		return;
+
 	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DPC);
 	if (!save_state)
 		return;

@@ -291,9 +299,6 @@ static int dpc_probe(struct pcie_device *dev)
 	int status;
 	u16 ctl, cap;
 
-	if (pcie_aer_get_firmware_first(pdev))
-		return -ENOTSUPP;
-
 	dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
 	if (!dpc)
 		return -ENOMEM;

@@ -302,13 +307,25 @@ static int dpc_probe(struct pcie_device *dev)
 	dpc->dev = dev;
 	set_service_data(dev, dpc);
 
-	status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
-					   dpc_handler, IRQF_SHARED,
-					   "pcie-dpc", dpc);
-	if (status) {
-		pci_warn(pdev, "request IRQ%d failed: %d\n", dev->irq,
-			 status);
-		return status;
+	if (pcie_aer_get_firmware_first(pdev))
+		dpc->firmware_dpc = 1;
+
+	/*
+	 * If DPC is handled in firmware and ACPI support is not enabled
+	 * in OS, skip probe and return error.
+	 */
+	if (dpc->firmware_dpc && !IS_ENABLED(CONFIG_ACPI))
+		return -ENODEV;
+
+	if (!dpc->firmware_dpc) {
+		status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
+						   dpc_handler, IRQF_SHARED,
+						   "pcie-dpc", dpc);
+		if (status) {
+			pci_warn(pdev, "request IRQ%d failed: %d\n", dev->irq,
+				 status);
+			return status;
+		}
 	}
 
 	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);

@@ -323,9 +340,12 @@ static int dpc_probe(struct pcie_device *dev)
 			dpc->rp_log_size = 0;
 		}
 	}
-
-	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
-	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);
+	if (!dpc->firmware_dpc) {
+		ctl = (ctl & 0xfff4) |
+			(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN);
+		pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL,
+				      ctl);
+	}
 
 	pci_info(pdev, "error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n",
 		 cap & PCI_EXP_DPC_IRQ, FLAG(cap, PCI_EXP_DPC_CAP_RP_EXT),

@@ -343,6 +363,9 @@ static void dpc_remove(struct pcie_device *dev)
 	struct pci_dev *pdev = dev->port;
 	u16 ctl;
 
+	if (dpc->firmware_dpc)
+		return;
+
 	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
 	ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN);
 	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);

Signature · 1 line

2.21.0

[email protected]

author

2019-07-26 21:45 UTC #12 lore raw

PATCH v6 5/9 PCI/DPC: Add dpc_process_error() wrapper function

In reply to [email protected]

From: Kuppuswamy Sathyanarayanan <[email protected]>

With Error Disconnect Recover (EDR) support, we need to support processing DPC event either from DPC IRQ or ACPI EDR event. So create a wrapper function dpc_process_error() and move common error handling code in to it. It will be used to process the DPC event in both DPC IRQ and EDR ACPI event contexts.

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

---
 drivers/pci/pcie/dpc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/pci/pcie/dpc.c +8 -2

index 9717fda012f8..4137ec7b48cc 100644
@@ -232,10 +232,9 @@ static int dpc_get_aer_uncorrect_severity(struct pci_dev *dev,
 	return 1;
 }
 
-static irqreturn_t dpc_handler(int irq, void *context)
+static void dpc_process_error(struct dpc_dev *dpc)
 {
 	struct aer_err_info info;
-	struct dpc_dev *dpc = context;
 	struct pci_dev *pdev = dpc->dev->port;
 	u16 cap = dpc->cap_pos, status, source, reason, ext_reason;

@@ -268,6 +267,13 @@ static irqreturn_t dpc_handler(int irq, void *context)
 
 	/* We configure DPC so it only triggers on ERR_FATAL */
 	pcie_do_recovery(pdev, pci_channel_io_frozen, PCIE_PORT_SERVICE_DPC);
+}
+
+static irqreturn_t dpc_handler(int irq, void *context)
+{
+	struct dpc_dev *dpc = context;
+
+	dpc_process_error(dpc);
 
 	return IRQ_HANDLED;
 }

Signature · 1 line

2.21.0

[email protected]

author

2019-07-26 21:46 UTC #13 lore raw

PATCH v6 6/9 PCI/DPC: Add Error Disconnect Recover (EDR) support

In reply to [email protected]

From: Kuppuswamy Sathyanarayanan <[email protected]>

As per ACPI specification r6.3, sec 5.6.6, when firmware owns Downstream Port Containment (DPC), its expected to use the "Error Disconnect Recover" (EDR) notification to alert OSPM of a DPC event and if OS supports EDR, its expected to handle the software state invalidation and port recovery in OS, and also let firmware know the recovery status via _OST ACPI call. Related _OST status codes can be found in ACPI specification r6.3, sec 6.3.5.2.

Also, as per PCI firmware specification r3.2 Downstream Port Containment Related Enhancements ECN, sec 4.5.1, table 4-6, If DPC is controlled by firmware (firmware first mode), firmware is responsible for configuring the DPC and OS is responsible for error recovery. Also, OS is allowed to modify DPC registers only during the EDR notification window. So with EDR support, OS should provide DPC port services even in firmware first mode.

Cc Keith Busch <[email protected]>

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

---
 drivers/pci/pci-acpi.c          |  91 +++++++++++++++++++++++
 drivers/pci/pcie/Kconfig        |  10 +++
 drivers/pci/pcie/dpc.c          | 127 +++++++++++++++++++++++++++++++-
 drivers/pci/pcie/portdrv_core.c |   9 ++-
 include/linux/pci-acpi.h        |  11 +++
 5 files changed, 245 insertions(+), 3 deletions(-)

drivers/pci/pci-acpi.c +91 -0

index 45049f558860..82abab25daf2 100644
@@ -104,6 +104,97 @@ int acpi_get_rc_resources(struct device *dev, const char *hid, u16 segment,
 }
 #endif
 
+#if defined(CONFIG_PCIE_DPC) && defined(CONFIG_ACPI)
+
+/*
+ * _DSM wrapper function to enable/disable DPC port.
+ * @dpc   : DPC device structure
+ * @enable: status of DPC port (0 or 1).
+ *
+ * returns 0 on success or errno on failure.
+ */
+int acpi_enable_dpc_port(struct pci_dev *pdev, acpi_handle handle, bool enable)
+{
+	union acpi_object *obj;
+	int status = 0;
+	union acpi_object argv4;
+
+	argv4.type = ACPI_TYPE_INTEGER;
+	argv4.integer.value = enable;
+
+	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 1,
+				EDR_PORT_ENABLE_DSM, &argv4);
+	if (!obj)
+		return -EIO;
+
+	if (obj->type != ACPI_TYPE_INTEGER || obj->integer.value != enable)
+		status = -EIO;
+
+	ACPI_FREE(obj);
+
+	return status;
+}
+
+/*
+ * _DSM wrapper function to locate DPC port.
+ * @dpc   : DPC device structure
+ *
+ * returns pci_dev or NULL.
+ */
+struct pci_dev *acpi_locate_dpc_port(struct pci_dev *pdev, acpi_handle handle)
+{
+	union acpi_object *obj;
+	u16 port;
+
+	obj = acpi_evaluate_dsm(handle, &pci_acpi_dsm_guid, 1,
+				EDR_PORT_LOCATE_DSM, NULL);
+	if (!obj)
+		return pci_dev_get(pdev);
+
+	if (obj->type != ACPI_TYPE_INTEGER) {
+		ACPI_FREE(obj);
+		return NULL;
+	}
+
+	/*
+	 * Firmware returns DPC port BDF details in following format:
+	 *	15:8 = bus
+	 *	7:3 = device
+	 *	2:0 = function
+	 */
+	port = obj->integer.value;
+
+	ACPI_FREE(obj);
+
+	return pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
+					   PCI_BUS_NUM(port), port & 0xff);
+}
+
+/*
+ * _OST wrapper function to let firmware know the status of EDR event.
+ * @dpc   : DPC device structure.
+ * @status: Status of EDR event.
+ */
+int acpi_send_edr_status(struct pci_dev *pdev,  acpi_handle handle, u16 status)
+{
+	u32 ost_status;
+
+	pci_dbg(pdev, "Sending EDR status :%x\n", status);
+
+	ost_status =  PCI_DEVID(pdev->bus->number, pdev->devfn);
+	ost_status = (ost_status << 16) | status;
+
+	status = acpi_evaluate_ost(handle,
+				   ACPI_NOTIFY_DISCONNECT_RECOVER,
+				   ost_status, NULL);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+
+	return 0;
+}
+
+#endif /* CONFIG_PCIE_DPC && CONFIG_ACPI */
+
 phys_addr_t acpi_pci_root_get_mcfg_addr(acpi_handle handle)
 {
 	acpi_status status = AE_NOT_EXIST;

drivers/pci/pcie/Kconfig +10 -0

index 362eb8cfa53b..9a05015af7cd 100644
@@ -150,3 +150,13 @@ config PCIE_BW
 	  This enables PCI Express Bandwidth Change Notification.  If
 	  you know link width or rate changes occur only to correct
 	  unreliable links, you may answer Y.
+
+config PCIE_EDR
+	bool "PCI Express Error Disconnect Recover support"
+	default n
+	depends on PCIE_DPC && ACPI
+	help
+	  This options adds Error Disconnect Recover support as specified
+	  in PCI firmware specification v3.2 Downstream Port Containment
+	  Related Enhancements ECN. Enable this if you want to support hybrid
+	  DPC model which uses both firmware and OS to implement DPC.

drivers/pci/pcie/dpc.c +126 -1

175 lines not on this page · raw message

drivers/pci/pcie/portdrv_core.c +7 -2

index 58c40fe7856f..acf3a9ec2227 100644
@@ -251,9 +251,14 @@ static int get_port_device_capability(struct pci_dev *dev)
 		pcie_pme_interrupt_enable(dev, false);
 	}
 
+	/*
+	 * If EDR support is enabled in OS, then even if AER is not handled in
+	 * OS, DPC service can be enabled.
+	 */
 	if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
-	    pci_aer_available() && services & PCIE_PORT_SERVICE_AER &&
-	    (pcie_ports_native || host->native_dpc))
+	    ((IS_ENABLED(CONFIG_PCIE_EDR) && !host->native_dpc) ||
+	    (pci_aer_available() && services & PCIE_PORT_SERVICE_AER &&
+	    (pcie_ports_native || host->native_dpc))))
 		services |= PCIE_PORT_SERVICE_DPC;
 
 	if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||

include/linux/pci-acpi.h +11 -0

index 62b7fdcc661c..36ffe1e16e69 100644
@@ -117,6 +117,17 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
 static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
 #endif	/* CONFIG_ACPI */
 
+#if defined(CONFIG_PCIE_DPC) && defined(CONFIG_ACPI)
+#define EDR_PORT_ENABLE_DSM     0x0C
+#define EDR_PORT_LOCATE_DSM     0x0D
+int acpi_enable_dpc_port(struct pci_dev *pdev, acpi_handle handle,
+			 bool enable);
+struct pci_dev *acpi_locate_dpc_port(struct pci_dev *pdev,
+				     acpi_handle handle);
+int acpi_send_edr_status(struct pci_dev *pdev,
+			 acpi_handle handle, u16 status);
+#endif
+
 #ifdef CONFIG_ACPI_APEI
 extern bool aer_acpi_firmware_first(void);
 #else

Signature · 1 line

2.21.0

[email protected]

author

2019-07-26 21:46 UTC #14 lore raw

PATCH v6 7/9 PCI/AER: Allow clearing Error Status Register in FF mode

In reply to [email protected]

From: Kuppuswamy Sathyanarayanan <[email protected]>

As per PCI firmware specification r3.2 Downstream Port Containment Related Enhancements ECN, sec 4.5.1, table 4-6, Error Disconnect Recover (EDR) support allows OS to handle error recovery and clearing Error Registers even in FF mode. So remove FF mode checks in pci_cleanup_aer_uncorrect_error_status(), pci_aer_clear_fatal_status() and pci_cleanup_aer_error_status_regs() functions.

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

---
 drivers/pci/pcie/aer.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

drivers/pci/pcie/aer.c +2 ~~-10~~

index b45bc47d04fe..e1509bb900c5 100644
@@ -383,9 +383,6 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
 	if (!pos)
 		return -EIO;
 
-	if (pcie_aer_get_firmware_first(dev))
-		return -EIO;
-
 	/* Clear status bits for ERR_NONFATAL errors only */
 	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
 	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);

@@ -406,9 +403,6 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
 	if (!pos)
 		return;
 
-	if (pcie_aer_get_firmware_first(dev))
-		return;
-
 	/* Clear status bits for ERR_FATAL errors only */
 	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
 	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);

@@ -430,9 +424,6 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
 	if (!pos)
 		return -EIO;
 
-	if (pcie_aer_get_firmware_first(dev))
-		return -EIO;
-
 	port_type = pci_pcie_type(dev);
 	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
 		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);

@@ -455,7 +446,8 @@ void pci_aer_init(struct pci_dev *dev)
 	if (dev->aer_cap)
 		dev->aer_stats = kzalloc(sizeof(struct aer_stats), GFP_KERNEL);
 
-	pci_cleanup_aer_error_status_regs(dev);
+	if (!pcie_aer_get_firmware_first(dev))
+		pci_cleanup_aer_error_status_regs(dev);
 }
 
 void pci_aer_exit(struct pci_dev *dev)

Signature · 1 line

2.21.0

[email protected]

author

2019-07-26 21:45 UTC #15 lore raw

PATCH v6 8/9 PCI/DPC: Update comments related to DPC recovery on NON_FATAL errors

In reply to [email protected]

From: Kuppuswamy Sathyanarayanan <[email protected]>

Currently, in native mode, DPC driver is configured to trigger DPC only for FATAL errors and hence it only supports port recovery for FATAL errors. But with Error Disconnect Recover (EDR) support, DPC configuration is done by firmware, and hence we should expect DPC triggered for both FATAL/NON_FATAL errors. So update comments and add details about how NON_FATAL dpc recovery is handled.

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

---
 drivers/pci/pcie/dpc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/pci/pcie/dpc.c +5 -1

index 6e350149d793..369df422692f 100644
@@ -275,7 +275,11 @@ static void dpc_process_error(struct dpc_dev *dpc)
 		pci_aer_clear_fatal_status(pdev);
 	}
 
-	/* We configure DPC so it only triggers on ERR_FATAL */
+	/*
+	 * Irrespective of whether the DPC event is triggered by
+	 * ERR_FATAL or ERR_NONFATAL, since the link is already down,
+	 * use the FATAL error recovery path for both cases.
+	 */
 	pcie_do_recovery(pdev, pci_channel_io_frozen, PCIE_PORT_SERVICE_DPC);
 }

Signature · 1 line

2.21.0

[email protected]

author

2019-07-26 21:46 UTC #16 lore raw

PATCH v6 9/9 PCI/DPC: Clear AER registers in EDR mode

In reply to [email protected]

From: Kuppuswamy Sathyanarayanan <[email protected]>

As per PCI firmware specification r3.2 Downstream Port Containment Related Enhancements ECN, OS is responsible for clearing the AER registers in EDR mode. So clear AER registers in dpc_process_error() function.

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

---
 drivers/pci/pcie/dpc.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/pci/pcie/dpc.c +4 -0

index 369df422692f..115590ad10ed 100644
@@ -275,6 +275,10 @@ static void dpc_process_error(struct dpc_dev *dpc)
 		pci_aer_clear_fatal_status(pdev);
 	}
 
+	/* In EDR mode, OS is responsible for clearing AER registers */
+	if (dpc->firmware_dpc)
+		pci_cleanup_aer_error_status_regs(pdev);
+
 	/*
 	 * Irrespective of whether the DPC event is triggered by
 	 * ERR_FATAL or ERR_NONFATAL, since the link is already down,

Signature · 1 line

2.21.0

Keith Busch

Acked-by on the v6 series

2019-07-26 21:56 UTC #17 lore raw

In reply to [email protected]

Quoted 10 lines · On Fri, Jul 26, 2019 at 02:43:10PM -0700…

From: Kuppuswamy Sathyanarayanan <[email protected]>
This patchset adds support for following features:
1. Error Disconnect Recover (EDR) support.
2. _OSC based negotiation support for DPC.
You can find EDR spec in the following link.
https://members.pcisig.com/wg/PCI-SIG/document/12614

Thank you for sticking with this. I've reviewed the series and I think this looks good for the next merge window.

Acked-by Keith Busch <[email protected]>

sathyanarayanan kuppuswamy

author

2019-07-26 23:33 UTC #18 lore raw

+Austin , Huong

Quoted 16 lines · Keith Busch

Quoted 10 lines · On Fri, Jul 26, 2019 at 02:43:10PM -0700…
From: Kuppuswamy Sathyanarayanan <[email protected]>
This patchset adds support for following features:
1. Error Disconnect Recover (EDR) support.
2. _OSC based negotiation support for DPC.
You can find EDR spec in the following link.
https://members.pcisig.com/wg/PCI-SIG/document/12614
Thank you for sticking with this. I've reviewed the series and I think this looks good for the next merge window.
Acked-by Keith Busch <[email protected]>

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Linux kernel developer

[email protected]

Tested-by Austin Bolen

2019-07-27 00:40 UTC #19 lore raw

Quoted 18 lines · sathyanarayanan kuppuswamy

+Austin , Huong
Quoted 15 lines · Keith Busch
Quoted 10 lines · On Fri, Jul 26, 2019 at 02:43:10PM -0700…
From: Kuppuswamy Sathyanarayanan <[email protected]>
This patchset adds support for following features:
1. Error Disconnect Recover (EDR) support.
2. _OSC based negotiation support for DPC.
You can find EDR spec in the following link.
https://members.pcisig.com/wg/PCI-SIG/document/12614
Thank you for sticking with this. I've reviewed the series and I think this looks good for the next merge window.
Acked-by Keith Busch <[email protected]>

Tested on a DPC-enabled PCIe switch (Broadcom PEX9733) in a Dell PowerEdge R740xd. Injected fatal and non-fatal errors on an NVMe endpoint below the switch and on the switch downstream port itself and verified errors were contained and then recovered at the PCIe level.

Tested-by Austin Bolen <[email protected]‎>

Quoted 1 line

Quoted 1 line

[email protected]

Tested-by Huong Nguyen

2019-07-28 16:10 UTC #20 lore raw

In reply to sathyanarayanan kuppuswamy

-----Original Message-----
From: sathyanarayanan kuppuswamy <[email protected]>
Sent: Friday, July 26, 2019 6:31 PM
To: Keith Busch
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Bolen, Austin; Nguyen, Huong
Subject: Re: [PATCH v6 0/9] Add Error Disconnect Recover (EDR) support

[EXTERNAL EMAIL]

+Austin , Huong

Quoted 9 lines · Keith Busch

Quoted 8 lines · On Fri, Jul 26, 2019 at 02:43:10PM -0700…
From: Kuppuswamy Sathyanarayanan
<[email protected]>
This patchset adds support for following features:
1. Error Disconnect Recover (EDR) support.
2. _OSC based negotiation support for DPC.

Quoted 7 lines

Quoted 3 lines
You can find EDR spec in the following link.
https://members.pcisig.com/wg/PCI-SIG/document/12614
Thank you for sticking with this. I've reviewed the series and I think this looks good for the next merge window.
Acked-by Keith Busch <[email protected]>

Tested-by Huong Nguyen <[email protected]‎>

Quoted 1 line

--
Sathyanarayanan Kuppuswamy
Linux kernel developer