[PATCH v4 0/3] PCI: pciehp: Add support for native AER and DPC handling on async remove

[PATCH v4 0/3] PCI: pciehp: Add support for native AER and DPC handling on async remove

[Smita Koralahalli]

This series of patches adds native support to handle AER and DPC events
occurred as a side-effect on an async remove.

Link to v3:
https://lore.kernel.org/all/20230621185152.105320-1-Smita.KoralahalliChannabasappa@amd.com

Smita Koralahalli (3):
  PCI: pciehp: Add support for async hotplug with native AER and DPC/EDR
  PCI: Enable support for 10-bit Tag during device enumeration
  PCI: pciehp: Clear AtomicOps unconditionally on hot remove.

 drivers/pci/hotplug/pciehp_pci.c |  3 ++
 drivers/pci/pci.c                | 59 +++++++++++++++++++++++++++
 drivers/pci/pci.h                |  1 +
 drivers/pci/pcie/dpc.c           | 69 ++++++++++++++++++++++++++++++++
 drivers/pci/probe.c              |  1 +
 include/uapi/linux/pci_regs.h    |  3 ++
 6 files changed, 136 insertions(+)

-- 
2.17.1

[PATCH v4 1/3] PCI: pciehp: Add support for async hotplug with native AER and DPC/EDR

[Smita Koralahalli]

According to PCIe r6.0 sec 6.7.6 [1], async removal with DPC may result in
surprise down error. This error is expected and is just a side-effect of
async remove.

Add support to handle the surprise down error generated as a side-effect
of async remove. Typically, this error is benign as the pciehp handler
invoked by PDC or/and DLLSC alongside DPC, de-enumerates and brings down
the device appropriately. But the error messages might confuse users. Get
rid of these irritating log messages with a 1s delay while pciehp waits
for dpc recovery.

The implementation is as follows: On an async remove a DPC is triggered
along with a Presence Detect State change and/or DLL State Change.
Determine it's an async remove by checking for DPC Trigger Status in DPC
Status Register and Surprise Down Error Status in AER Uncorrected Error
Status to be non-zero. If true, treat the DPC event as a side-effect of
async remove, clear the error status registers and continue with hot-plug
tear down routines. If not, follow the existing routine to handle AER and
DPC errors.

Please note that, masking Surprise Down Errors was explored as an
alternative approach, but left due to the odd behavior that masking only
avoids the interrupt, but still records an error per PCIe r6.0.1 Section
6.2.3.2.2. That stale error is going to be reported the next time some
error other than Surprise Down is handled.

Dmesg before:

  pcieport 0000:00:01.4: DPC: containment event, status:0x1f01 source:0x0000
  pcieport 0000:00:01.4: DPC: unmasked uncorrectable error detected
  pcieport 0000:00:01.4: PCIe Bus Error: severity=Uncorrected (Fatal), type=Transaction Layer, (Receiver ID)
  pcieport 0000:00:01.4:   device [1022:14ab] error status/mask=00000020/04004000
  pcieport 0000:00:01.4:    [ 5] SDES (First)
  nvme nvme2: frozen state error detected, reset controller
  pcieport 0000:00:01.4: DPC: Data Link Layer Link Active not set in 1000 msec
  pcieport 0000:00:01.4: AER: subordinate device reset failed
  pcieport 0000:00:01.4: AER: device recovery failed
  pcieport 0000:00:01.4: pciehp: Slot(16): Link Down
  nvme2n1: detected capacity change from 1953525168 to 0
  pci 0000:04:00.0: Removing from iommu group 49

Dmesg after:

 pcieport 0000:00:01.4: pciehp: Slot(16): Link Down
 nvme1n1: detected capacity change from 1953525168 to 0
 pci 0000:04:00.0: Removing from iommu group 37

[1] PCI Express Base Specification Revision 6.0, Dec 16 2021.
    https://members.pcisig.com/wg/PCI-SIG/document/16609

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
---
v2:
	Indentation is taken care. (Bjorn)
	Unrelevant dmesg logs are removed. (Bjorn)
	Rephrased commit message, to be clear on native vs FW-First
	handling. (Bjorn and Sathyanarayanan)
	Prefix changed from pciehp_ to dpc_. (Lukas)
	Clearing ARI and AtomicOp Requester are performed as a part of
	(de-)enumeration in pciehp_unconfigure_device(). (Lukas)
	Changed to clearing all optional capabilities in DEVCTL2.
	OS-First -> native. (Sathyanarayanan)

v3:
	Added error message when root port become inactive.
	Modified commit description to add more details.
	Rearranged code comments and function calls with no functional
	change.
	Additional check for is_hotplug_bridge.
	dpc_completed_waitqueue to wakeup pciehp handler.
	Cleared only Fatal error detected in DEVSTA.

v4:
	Made read+write conditional on "if (pdev->dpc_rp_extensions)"
	for DPC_RP_PIO_STATUS.
	Wrapped to 80 chars.
	Code comment for clearing PCI_STATUS and PCI_EXP_DEVSTA.
	Added pcie_wait_for_link() check.
	Removed error message for root port inactive as the message
	already existed.
	Check for is_hotplug_bridge before registers read.
	Section 6.7.6 of the PCIe Base Spec 6.0 -> PCIe r6.0 sec 6.7.6.
	Made code comment more meaningful.
---
 drivers/pci/pcie/dpc.c | 69 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 3ceed8e3de41..25e9ddeeb271 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -292,10 +292,79 @@ void dpc_process_error(struct pci_dev *pdev)
 	}
 }
 
+static void pci_clear_surpdn_errors(struct pci_dev *pdev)
+{
+	u16 reg16;
+	u32 reg32;
+
+	if (pdev->dpc_rp_extensions) {
+		pci_read_config_dword(pdev, pdev->dpc_cap + PCI_EXP_DPC_RP_PIO_STATUS,
+				      &reg32);
+		pci_write_config_dword(pdev, pdev->dpc_cap + PCI_EXP_DPC_RP_PIO_STATUS,
+				       reg32);
+	}
+
+	/*
+	 * In practice, Surprise Down errors have been observed to also set
+	 * error bits in the Status Register as well as the Fatal Error
+	 * Detected bit in the Device Status Register.
+	 */
+	pci_read_config_word(pdev, PCI_STATUS, &reg16);
+	pci_write_config_word(pdev, PCI_STATUS, reg16);
+
+	pcie_capability_write_word(pdev, PCI_EXP_DEVSTA, PCI_EXP_DEVSTA_FED);
+}
+
+static void dpc_handle_surprise_removal(struct pci_dev *pdev)
+{
+	if (!pcie_wait_for_link(pdev, false)) {
+		pci_info(pdev, "Data Link Layer Link Active not cleared in 1000 msec\n");
+		goto out;
+	}
+
+	if (pdev->dpc_rp_extensions && dpc_wait_rp_inactive(pdev))
+		goto out;
+
+	pci_aer_raw_clear_status(pdev);
+	pci_clear_surpdn_errors(pdev);
+
+	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_STATUS,
+			      PCI_EXP_DPC_STATUS_TRIGGER);
+
+out:
+	clear_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
+	wake_up_all(&dpc_completed_waitqueue);
+}
+
+static bool dpc_is_surprise_removal(struct pci_dev *pdev)
+{
+	u16 status;
+
+	if (!pdev->is_hotplug_bridge)
+		return false;
+
+	pci_read_config_word(pdev, pdev->aer_cap + PCI_ERR_UNCOR_STATUS,
+			     &status);
+
+	if (!(status & PCI_ERR_UNC_SURPDN))
+		return false;
+
+	return true;
+}
+
 static irqreturn_t dpc_handler(int irq, void *context)
 {
 	struct pci_dev *pdev = context;
 
+	/*
+	 * According to PCIe r6.0 sec 6.7.6, errors are an expected side effect
+	 * of async removal and should be ignored by software.
+	 */
+	if (dpc_is_surprise_removal(pdev)) {
+		dpc_handle_surprise_removal(pdev);
+		return IRQ_HANDLED;
+	}
+
 	dpc_process_error(pdev);
 
 	/* We configure DPC so it only triggers on ERR_FATAL */
-- 
2.17.1

Re: [PATCH v4 1/3] PCI: pciehp: Add support for async hotplug with native AER and DPC/EDR

[Lukas Wunner]

On Tue, Aug 15, 2023 at 09:20:41PM +0000, Smita Koralahalli wrote:
> According to PCIe r6.0 sec 6.7.6 [1], async removal with DPC may result in
> surprise down error. This error is expected and is just a side-effect of
> async remove.
> 
> Add support to handle the surprise down error generated as a side-effect
> of async remove. Typically, this error is benign as the pciehp handler
> invoked by PDC or/and DLLSC alongside DPC, de-enumerates and brings down
> the device appropriately. But the error messages might confuse users. Get
> rid of these irritating log messages with a 1s delay while pciehp waits
> for dpc recovery.
[...]
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

The subject is slightly inaccurate as this doesn't touch pciehp source
files, although it is *related* to pciehp.

As an example, a perhaps more accurate subject might be something like...

  PCI/DPC: Ignore Surprise Down errors on hot removal

...but I don't think it's necessary to respin just for that as Bjorn is
probably able to adjust the subject to his liking when applying the patch.

Thanks a lot for patiently pursuing this issue, good to see it fixed.

Five years ago there was an attempt to solve it through masking Surprise
Down errors, which you've verified to not be a viable approach:

https://patchwork.kernel.org/project/linux-pci/patch/20180818065126.77912-2-okaya@kernel.org/

Lukas

Re: [PATCH v4 1/3] PCI: pciehp: Add support for async hotplug with native AER and DPC/EDR

[Smita Koralahalli]

On 8/28/2023 12:35 AM, Lukas Wunner wrote:
> On Tue, Aug 15, 2023 at 09:20:41PM +0000, Smita Koralahalli wrote:
>> According to PCIe r6.0 sec 6.7.6 [1], async removal with DPC may result in
>> surprise down error. This error is expected and is just a side-effect of
>> async remove.
>>
>> Add support to handle the surprise down error generated as a side-effect
>> of async remove. Typically, this error is benign as the pciehp handler
>> invoked by PDC or/and DLLSC alongside DPC, de-enumerates and brings down
>> the device appropriately. But the error messages might confuse users. Get
>> rid of these irritating log messages with a 1s delay while pciehp waits
>> for dpc recovery.
> [...]
>> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> 
> Reviewed-by: Lukas Wunner <lukas@wunner.de>
> 
> The subject is slightly inaccurate as this doesn't touch pciehp source
> files, although it is *related* to pciehp.
> 
> As an example, a perhaps more accurate subject might be something like...
> 
>    PCI/DPC: Ignore Surprise Down errors on hot removal
> 
> ...but I don't think it's necessary to respin just for that as Bjorn is
> probably able to adjust the subject to his liking when applying the patch.
> 
> Thanks a lot for patiently pursuing this issue, good to see it fixed.
> 
> Five years ago there was an attempt to solve it through masking Surprise
> Down errors, which you've verified to not be a viable approach:
> 
> https://patchwork.kernel.org/project/linux-pci/patch/20180818065126.77912-2-okaya@kernel.org/
> 
> Lukas

Thanks for the review. Would it be possible to consider this patch as a 
standalone while I work on 10-bit tags enumeration? I can do v5 for this 
patch with $SUBJECT changes and also include clearing Atomic Ops and 
10-bit tags unconditionally on hot-remove if that works..

Thanks,
Smita

Re: [PATCH v4 1/3] PCI: pciehp: Add support for async hotplug with native AER and DPC/EDR

[Lukas Wunner]

On Tue, Sep 12, 2023 at 03:45:34PM -0700, Smita Koralahalli wrote:
> Thanks for the review. Would it be possible to consider this patch as a
> standalone while I work on 10-bit tags enumeration? I can do v5 for this
> patch with $SUBJECT changes and also include clearing Atomic Ops and 10-bit
> tags unconditionally on hot-remove if that works..

Right, this patch is still in state "New" in patchwork:

https://patchwork.kernel.org/project/linux-pci/patch/20230815212043.114913-2-Smita.KoralahalliChannabasappa@amd.com/

Which means Bjorn probably hasn't gotten a chance to look at it.
Indeed it can be applied standalone if Bjorn gets to it this cycle
and doesn't have any objections.

Feel free to include my Reviewed-by tag if/when resending.

Thanks,

Lukas

[PATCH v4 2/3] PCI: Enable support for 10-bit Tag during device enumeration

[Smita Koralahalli]

Enable support for PCI Express 10-bit Tag.

A requester may use 10-bit Tag only if its "10-bit Tag Requester Enable"
control bit (PCI_EXP_DEVCTL2_10BIT_TAG_REQ) is set. Enable 10-bit Tag
Requester Enable if the requester supports 10-bit Tag Requester capability
and its completer supports 10-bit Tag Completions.

Platform FW may enable 10-bit Tag Requester during boot for performance
reasons as per PCIe r6.0 sec 2.2.6.2 [1]. It states that "For platforms
where the RC supports 10-Bit Tag Completer capability, it is highly
recommended for platform firmware or operating software that configures
PCIe hierarchies to Set the 10-Bit Tag Requester Enable bit automatically
in Endpoints with 10-Bit Tag Requester capability".

And, failure to enable 10-bit Tag appropriately has led to issues reaching
to the device. The device became inaccessible and the port was not able to
be recovered without a system reset when a device with 10-bit Tag was
removed and replaced with a device that didn't support 10-bit Tag.

PCIe r6.0 sec 2.2.6.2 [1], also implies that:

* If a Requester sends a 10-Bit Tag Request to a Completer that lacks
10-Bit Completer capability, the returned Completion(s) will have Tags with
Tag[9:8] equal to 00b. Since the Requester is forbidden to generate these
Tag values for 10-Bit Tags, such Completions will be handled as Unexpected
Completions, which by default are Advisory Non-Fatal Errors. The Requester
must follow standard PCI Express error handling requirements.

* In configurations where a Requester with 10-Bit Tag Requester capability
needs to target multiple Completers, one needs to ensure that the Requester
sends 10-Bit Tag Requests only to Completers that have 10-Bit Tag Completer
capability.

Hence, ensure whether these capabilities are re-negotiated and enable them
appropriately, especially when a device is surprise removed and replaced
with a new one.

[1] PCI Express Base Specification Revision 6.0, Dec 16 2021.
    https://members.pcisig.com/wg/PCI-SIG/document/16609

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
---
 drivers/pci/pci.c             | 59 +++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h             |  1 +
 drivers/pci/probe.c           |  1 +
 include/uapi/linux/pci_regs.h |  3 ++
 4 files changed, 64 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 60230da957e0..7e640694fa03 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3795,6 +3795,65 @@ int pci_rebar_set_size(struct pci_dev *pdev, int bar, int size)
 	return 0;
 }
 
+/*
+ * pci_configure_ten_bit_tag - enable or disable 10-bit Tag Requester
+ * @dev: the PCI device
+ */
+void pci_configure_ten_bit_tag(struct pci_dev *dev)
+{
+	struct pci_dev *bridge;
+	u32 cap;
+
+	if (!pci_is_pcie(dev))
+		return;
+
+	bridge = dev->bus->self;
+	if (!bridge)
+		return;
+
+	/*
+	 * According to PCIe r6.0 sec 7.5.3.16, the result is undefined if
+	 * the value of this bit is changed while the Function has outstanding
+	 * Non-Posted Requests.
+	 */
+	if (!pci_wait_for_pending_transaction(dev)) {
+		pci_info(dev, "Transaction in progress, 10-bit Tag not configured properly\n");
+		return;
+	}
+
+	/*
+	 * According to PCIe r6.0 sec 7.5.3.15, Requester Supported can only be
+	 * set if 10-Bit Tag Completer Supported bit is set.
+	 */
+	pcie_capability_read_dword(bridge, PCI_EXP_DEVCAP2, &cap);
+	if (!(cap & PCI_EXP_DEVCAP2_10BIT_TAG_COMP))
+		goto out;
+
+	if (cap & PCI_EXP_DEVCAP2_10BIT_TAG_REQ) {
+		pcie_capability_read_dword(dev, PCI_EXP_DEVCAP2, &cap);
+
+		if (!(cap & PCI_EXP_DEVCAP2_10BIT_TAG_COMP))
+			goto out;
+
+		pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
+					 PCI_EXP_DEVCTL2_10BIT_TAG_REQ);
+
+		if (cap & PCI_EXP_DEVCAP2_10BIT_TAG_REQ)
+			pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
+						 PCI_EXP_DEVCTL2_10BIT_TAG_REQ);
+		else
+			pcie_capability_clear_word(dev, PCI_EXP_DEVCTL2,
+						   PCI_EXP_DEVCTL2_10BIT_TAG_REQ);
+		return;
+	}
+
+out:
+	pcie_capability_clear_word(bridge, PCI_EXP_DEVCTL2,
+				   PCI_EXP_DEVCTL2_10BIT_TAG_REQ);
+	pcie_capability_clear_word(dev, PCI_EXP_DEVCTL2,
+				   PCI_EXP_DEVCTL2_10BIT_TAG_REQ);
+}
+
 /**
  * pci_enable_atomic_ops_to_root - enable AtomicOp requests to root port
  * @dev: the PCI device
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index a4c397434057..dee6241878fc 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -239,6 +239,7 @@ int pci_setup_device(struct pci_dev *dev);
 int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		    struct resource *res, unsigned int reg);
 void pci_configure_ari(struct pci_dev *dev);
+void pci_configure_ten_bit_tag(struct pci_dev *dev);
 void __pci_bus_size_bridges(struct pci_bus *bus,
 			struct list_head *realloc_head);
 void __pci_bus_assign_resources(const struct pci_bus *bus,
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8bac3ce02609..5a3c1ec6fad6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2476,6 +2476,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	pci_pm_init(dev);		/* Power Management */
 	pci_vpd_init(dev);		/* Vital Product Data */
 	pci_configure_ari(dev);		/* Alternative Routing-ID Forwarding */
+	pci_configure_ten_bit_tag(dev); /* 10-bit Tag Requester */
 	pci_iov_init(dev);		/* Single Root I/O Virtualization */
 	pci_ats_init(dev);		/* Address Translation Services */
 	pci_pri_init(dev);		/* Page Request Interface */
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index e5f558d96493..b0a41c987ac5 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -656,6 +656,8 @@
 #define  PCI_EXP_DEVCAP2_ATOMIC_COMP128	0x00000200 /* 128b AtomicOp completion */
 #define  PCI_EXP_DEVCAP2_LTR		0x00000800 /* Latency tolerance reporting */
 #define  PCI_EXP_DEVCAP2_OBFF_MASK	0x000c0000 /* OBFF support mechanism */
+#define  PCI_EXP_DEVCAP2_10BIT_TAG_COMP 0x00010000 /* 10-bit Tag Completer */
+#define  PCI_EXP_DEVCAP2_10BIT_TAG_REQ  0x00020000 /* 10-bit Tag Requester */
 #define  PCI_EXP_DEVCAP2_OBFF_MSG	0x00040000 /* New message signaling */
 #define  PCI_EXP_DEVCAP2_OBFF_WAKE	0x00080000 /* Re-use WAKE# for OBFF */
 #define  PCI_EXP_DEVCAP2_EE_PREFIX	0x00200000 /* End-End TLP Prefix */
@@ -668,6 +670,7 @@
 #define  PCI_EXP_DEVCTL2_IDO_REQ_EN	0x0100	/* Allow IDO for requests */
 #define  PCI_EXP_DEVCTL2_IDO_CMP_EN	0x0200	/* Allow IDO for completions */
 #define  PCI_EXP_DEVCTL2_LTR_EN		0x0400	/* Enable LTR mechanism */
+#define  PCI_EXP_DEVCTL2_10BIT_TAG_REQ  0x1000  /* Enable 10-bit Tag Requester */
 #define  PCI_EXP_DEVCTL2_OBFF_MSGA_EN	0x2000	/* Enable OBFF Message type A */
 #define  PCI_EXP_DEVCTL2_OBFF_MSGB_EN	0x4000	/* Enable OBFF Message type B */
 #define  PCI_EXP_DEVCTL2_OBFF_WAKE_EN	0x6000	/* OBFF using WAKE# signaling */
-- 
2.17.1

Re: [PATCH v4 2/3] PCI: Enable support for 10-bit Tag during device enumeration

[Lukas Wunner]

On Tue, Aug 15, 2023 at 09:20:42PM +0000, Smita Koralahalli wrote:
> +void pci_configure_ten_bit_tag(struct pci_dev *dev)
> +{
> +	struct pci_dev *bridge;
> +	u32 cap;
> +
> +	if (!pci_is_pcie(dev))
> +		return;
> +
> +	bridge = dev->bus->self;
> +	if (!bridge)
> +		return;

I think you need to use bridge = pcie_find_root_port(dev) because
"dev" may be further down in the hierarchy with several switches
in-between it and the Root Port.

Note that pcie_find_root_port(dev) returns NULL if !pci_is_pcie(dev),
so the check above may become unnecessary.

If pcie_find_root_port(dev) == dev, then dev itself is a Root Port,
in which case you need to bail out.


> +	/*
> +	 * According to PCIe r6.0 sec 7.5.3.15, Requester Supported can only be
> +	 * set if 10-Bit Tag Completer Supported bit is set.
> +	 */
> +	pcie_capability_read_dword(bridge, PCI_EXP_DEVCAP2, &cap);
> +	if (!(cap & PCI_EXP_DEVCAP2_10BIT_TAG_COMP))
> +		goto out;
> +
> +	if (cap & PCI_EXP_DEVCAP2_10BIT_TAG_REQ) {

Hm, if Requester Supported cannot be set unless Completer Supported is
also set, why check for Completer Supported at all?


> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2476,6 +2476,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
>  	pci_pm_init(dev);		/* Power Management */
>  	pci_vpd_init(dev);		/* Vital Product Data */
>  	pci_configure_ari(dev);		/* Alternative Routing-ID Forwarding */
> +	pci_configure_ten_bit_tag(dev); /* 10-bit Tag Requester */
>  	pci_iov_init(dev);		/* Single Root I/O Virtualization */
>  	pci_ats_init(dev);		/* Address Translation Services */
>  	pci_pri_init(dev);		/* Page Request Interface */

Hm, isn't this too late to disable 10-bit tags if a hot-plugged device
doesn't support it?  There are plenty of config space reads/writes
happening before pci_configure_ten_bit_tag() and if the Root Port
has 10-bit tags enabled by BIOS because a previously unplugged
device supported it, I assume the Root Port may use 10-bit tags for
those config space accesses, despite the newly hotplugged device not
supporting them?

If so, you may indeed have to unconditionally disable 10-bit tags
upon device removal and re-enable them once a 10-bit capable device
is hotplugged.

I'm wondering what happens if there are switches between the hotplugged
device and the Root Port.  In that case, there may be further devices
in the hierarchy below the Root Port.  I assume 10-bit tags can only be
enabled if *all* devices below the Root Port support them, is that correct?

The corollary would be that if there's an unoccupied hotplug port somewhere
in the hierarchy below a Root Port, 10-bit tags cannot be enabled at all
on the Root Port.  Maybe we can leave 10-bit tags enabled on hot-removal
and only disable them on hot-add?  That wouldn't work however if TLPs
are sent to the hot-added device without operating system involvement
prior to enumeration by the operating system.  Don't CXL devices
autonomously send PM messages upstream on hot-add?

There's another quagmire:  Endpoint devices may talk to each other via
p2pdma (see drivers/pci/p2pdma.c) and if either of them doesn't support
10-bit tags, we need to disable 10-bit tags on them upon commencing
p2pdma.  We may re-enable 10-bit tags once either of the devices is
hot-removed or p2pdma between them is stopped.

Finally, PCIe r6.0 added 14-bit tag support.  It may be worth adding
10-bit tag support in a way that 14-bit tag support can easily be added
later on (or is added together with 10-bit tag support).

Thanks,

Lukas

Re: [PATCH v4 2/3] PCI: Enable support for 10-bit Tag during device enumeration

[Smita Koralahalli]

On 8/28/2023 2:54 AM, Lukas Wunner wrote:
> On Tue, Aug 15, 2023 at 09:20:42PM +0000, Smita Koralahalli wrote:
>> +void pci_configure_ten_bit_tag(struct pci_dev *dev)
>> +{
>> +	struct pci_dev *bridge;
>> +	u32 cap;
>> +
>> +	if (!pci_is_pcie(dev))
>> +		return;
>> +
>> +	bridge = dev->bus->self;
>> +	if (!bridge)
>> +		return;
> 
> I think you need to use bridge = pcie_find_root_port(dev) because
> "dev" may be further down in the hierarchy with several switches
> in-between it and the Root Port.
> 
> Note that pcie_find_root_port(dev) returns NULL if !pci_is_pcie(dev),
> so the check above may become unnecessary.
> 
> If pcie_find_root_port(dev) == dev, then dev itself is a Root Port,
> in which case you need to bail out.

Will fix thanks!

> 
> 
>> +	/*
>> +	 * According to PCIe r6.0 sec 7.5.3.15, Requester Supported can only be
>> +	 * set if 10-Bit Tag Completer Supported bit is set.
>> +	 */
>> +	pcie_capability_read_dword(bridge, PCI_EXP_DEVCAP2, &cap);
>> +	if (!(cap & PCI_EXP_DEVCAP2_10BIT_TAG_COMP))
>> +		goto out;
>> +
>> +	if (cap & PCI_EXP_DEVCAP2_10BIT_TAG_REQ) {
> 
> Hm, if Requester Supported cannot be set unless Completer Supported is
> also set, why check for Completer Supported at all?

Makes sense to me. Will change.
> 
> 
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -2476,6 +2476,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
>>   	pci_pm_init(dev);		/* Power Management */
>>   	pci_vpd_init(dev);		/* Vital Product Data */
>>   	pci_configure_ari(dev);		/* Alternative Routing-ID Forwarding */
>> +	pci_configure_ten_bit_tag(dev); /* 10-bit Tag Requester */
>>   	pci_iov_init(dev);		/* Single Root I/O Virtualization */
>>   	pci_ats_init(dev);		/* Address Translation Services */
>>   	pci_pri_init(dev);		/* Page Request Interface */
> 
> Hm, isn't this too late to disable 10-bit tags if a hot-plugged device
> doesn't support it?  There are plenty of config space reads/writes
> happening before pci_configure_ten_bit_tag() and if the Root Port
> has 10-bit tags enabled by BIOS because a previously unplugged
> device supported it, I assume the Root Port may use 10-bit tags for
> those config space accesses, despite the newly hotplugged device not
> supporting them?
> 
> If so, you may indeed have to unconditionally disable 10-bit tags
> upon device removal and re-enable them once a 10-bit capable device
> is hotplugged.
> 
> I'm wondering what happens if there are switches between the hotplugged
> device and the Root Port.  In that case, there may be further devices
> in the hierarchy below the Root Port.  I assume 10-bit tags can only be
> enabled if *all* devices below the Root Port support them, is that correct?

You are right! I understand I missed the consideration of involving 
switches and hierarchical PCIe structures.

> 
> The corollary would be that if there's an unoccupied hotplug port somewhere
> in the hierarchy below a Root Port, 10-bit tags cannot be enabled at all
> on the Root Port.

Yes, but the BIOS might have already enabled 10-bit tags on root port 
before this hotplug port becomes unoccupied on hot-remove.

   Maybe we can leave 10-bit tags enabled on hot-removal
> and only disable them on hot-add?

Considering all the challenges, would you think we should 
unconditionally clear 10-bit tags on remove and enable them on add? 
Because the TLPs issue will exist even if we leave the tags enabled on 
removal. Disabling unconditionally would atleast resolve config space 
read/writes and p2pdma issues. What do you think?

  That wouldn't work however if TLPs
> are sent to the hot-added device without operating system involvement
> prior to enumeration by the operating system.  Don't CXL devices
> autonomously send PM messages upstream on hot-add?
> 
> There's another quagmire:  Endpoint devices may talk to each other via
> p2pdma (see drivers/pci/p2pdma.c) and if either of them doesn't support
> 10-bit tags, we need to disable 10-bit tags on them upon commencing
> p2pdma.  We may re-enable 10-bit tags once either of the devices is
> hot-removed or p2pdma between them is stopped.
> 
> Finally, PCIe r6.0 added 14-bit tag support.  It may be worth adding
> 10-bit tag support in a way that 14-bit tag support can easily be added
> later on (or is added together with 10-bit tag support).

I agree.

Thanks,
Smita

> 
> Thanks,
> 
> Lukas

[PATCH v4 3/3] PCI: pciehp: Clear AtomicOps unconditionally on hot remove.

[Smita Koralahalli]

On a hot-plug, the optional capabilities ARI Forwarding Enable, AtomicOp
Requester Enable and 10-Bit Tag Requester Enable in DEVCTL2 must be
re-negotiated between endpoint and root port for optimal operation.

PCIe r6.0 sec 6.13 and 6.15 [1], points out that following a hot-plug
event, clear the ARI Forwarding Enable bit and AtomicOp Requester Enable
as its not determined whether the next device inserted will support these
capabilities. AtomicOp capabilities are not supported on PCI Express to
PCI/PCI-X Bridges and any newly added component may not be an ARI device.

The enablement and disablement of ARI Forwarding Enable and 10-bit Tag
Requester Enable is already been taken care in pci_configure_ari() and
pci_configure_ten_bit_tag() respectively.

AtomicOp requests are not enabled indiscriminately by PCI core as there
could be devices where AtomicOps are nominally supported but untested or
broken. Additionally, there is no explicit capability bit to determine
the support for AtomicOps Requester.

Moreover, it is difficult to determine if the AtomicOps are enabled by
reading the "AtomicOp Requester Enable" Device Control 2 register as
the PCIe r6.0 sec 7.5.3.16 [1], states "AtomicOps Requester Enable is
permitted to be RW even if no AtomicOp Requester capabilities are
supported by the Endpoint or Root Port", thereby substantiating devices
that hardwires this bit to '1' is also valid. Hence, clear AtomicOp
Requester Enable unconditionally on hot remove.

[1] PCI Express Base Specification Revision 6.0, Dec 16 2021.
    https://members.pcisig.com/wg/PCI-SIG/document/16609

Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
---
v2:
	Clear all optional capabilities in Device Control 2 register
	instead of individually clearing ARI Forwarding Enable,
	AtomicOp Requestor Enable and 10-bit Tag Requestor Enable.
v3:
	Restore clearing only ARI, Atomic Op and 10 bit tags as these are
	the optional capabilities.
	Provide all necessary information in commit description.
	Clear register bits of the hotplug port.
v4:
	Cleared only AtomicOps instead of all three bits.
	Removed brackets.
	Moved clearing at the end after pci_unlock_rescan_remove().
---
 drivers/pci/hotplug/pciehp_pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index ad12515a4a12..a3adbe89239c 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -134,4 +134,7 @@ void pciehp_unconfigure_device(struct controller *ctrl, bool presence)
 	}
 
 	pci_unlock_rescan_remove();
+
+	pcie_capability_clear_word(ctrl->pcie->port, PCI_EXP_DEVCTL2,
+				   PCI_EXP_DEVCTL2_ATOMIC_REQ);
 }
-- 
2.17.1