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

[PATCH v3 0/2] 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 v2:
https://lore.kernel.org/all/20230418210526.36514-1-Smita.KoralahalliChannabasappa@amd.com/

Smita Koralahalli (2):
  PCI: pciehp: Add support for async hotplug with native AER and DPC/EDR
  PCI: pciehp: Clear the optional capabilities in DEVCTL2 on a hot-plug

 drivers/pci/hotplug/pciehp_pci.c |  4 +++
 drivers/pci/pcie/dpc.c           | 58 ++++++++++++++++++++++++++++++++
 include/uapi/linux/pci_regs.h    |  1 +
 3 files changed, 63 insertions(+)

-- 
2.17.1

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

[Smita Koralahalli]

According to Section 6.7.6 of PCIe Base Specification [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.
---
 drivers/pci/pcie/dpc.c | 58 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 3ceed8e3de41..5153ac8ea91c 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -292,10 +292,68 @@ void dpc_process_error(struct pci_dev *pdev)
 	}
 }
 
+static void pci_clear_surpdn_errors(struct pci_dev *pdev)
+{
+	u16 reg16;
+	u32 reg32;
+
+	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);
+
+	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 (pdev->dpc_rp_extensions && dpc_wait_rp_inactive(pdev)) {
+		pci_err(pdev, "failed to retrieve DPC root port on async remove\n");
+		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;
+
+	pci_read_config_word(pdev, pdev->aer_cap + PCI_ERR_UNCOR_STATUS, &status);
+
+	if (!pdev->is_hotplug_bridge)
+		return false;
+
+	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 Section 6.7.6 of the PCIe Base Spec 6.0, since async
+	 * removal might be unexpected, errors might be reported as a side
+	 * effect of the event and software should handle them as an expected
+	 * part of this event.
+	 */
+	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 v3 1/2] PCI: pciehp: Add support for async hotplug with native AER and DPC/EDR

[Lukas Wunner]

On Wed, Jun 21, 2023 at 06:51:51PM +0000, Smita Koralahalli wrote:
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -292,10 +292,68 @@ void dpc_process_error(struct pci_dev *pdev)
>  	}
>  }
>  
> +static void pci_clear_surpdn_errors(struct pci_dev *pdev)
> +{
> +	u16 reg16;
> +	u32 reg32;
> +
> +	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);

Make this read+write conditional on "if (pdev->dpc_rp_extensions)"
as the register otherwise doesn't exist.

Wrap to 80 chars per line.


> +	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);
> +}

A code comment might be useful here saying that 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.


> +static void dpc_handle_surprise_removal(struct pci_dev *pdev)
> +{

I'm wondering if we also need

	if (!pcie_wait_for_link(pdev, false)) {
		pci_info(pdev, "Data Link Layer Link Active not cleared in 1000 msec\n");
		goto out;
	}

here, similar to dpc_reset_link() and in accordance with PCIe r6.1
sec 6.2.11:

	"To ensure that the LTSSM has time to reach the Disabled state
	or at least to bring the Link down under a variety of error
	conditions, software must leave the Downstream Port in DPC
	until the Data Link Layer Link Active bit in the Link Status
	Register reads 0b; otherwise, the result is undefined."


> +	if (pdev->dpc_rp_extensions && dpc_wait_rp_inactive(pdev)) {
> +		pci_err(pdev, "failed to retrieve DPC root port on async remove\n");
> +		goto out;
> +	}

I don't think pci_err() is needed here as dpc_wait_rp_inactive()
already emits a message.  (I think I mistakenly gave the advice
to emit an error here in an earlier review comment -- sorry!)


> +
> +	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;
> +
> +	pci_read_config_word(pdev, pdev->aer_cap + PCI_ERR_UNCOR_STATUS, &status);

Wrap to 80 chars.


> +
> +	if (!pdev->is_hotplug_bridge)
> +		return false;

Move this if-clause to the beginning if the function so that
you omit the unnecessary register read if it's not a hotplug
bridge.


> +
> +	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 Section 6.7.6 of the PCIe Base Spec 6.0, since async
> +	 * removal might be unexpected, errors might be reported as a side
> +	 * effect of the event and software should handle them as an expected
> +	 * part of this event.
> +	 */

I think the usual way to reference the spec is "PCIe r6.0 sec 6.7.6".

Maybe that's just me but I find the code comment a little difficult
to parse.  Maybe something like the following?

	/*
	 * According to PCIe r6.0 sec 6.7.6, errors are an expected side effect
	 * of async removal and should be ignored by software.
	 */

Thanks,

Lukas

> +	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 v3 1/2] PCI: pciehp: Add support for async hotplug with native AER and DPC/EDR

[Smita Koralahalli]

On 6/22/2023 2:04 AM, Lukas Wunner wrote:
> On Wed, Jun 21, 2023 at 06:51:51PM +0000, Smita Koralahalli wrote:
>> --- a/drivers/pci/pcie/dpc.c
>> +++ b/drivers/pci/pcie/dpc.c
>> @@ -292,10 +292,68 @@ void dpc_process_error(struct pci_dev *pdev)
>>   	}
>>   }
>>   
>> +static void pci_clear_surpdn_errors(struct pci_dev *pdev)
>> +{
>> +	u16 reg16;
>> +	u32 reg32;
>> +
>> +	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);
> 
> Make this read+write conditional on "if (pdev->dpc_rp_extensions)"
> as the register otherwise doesn't exist.

I'm checking for pdev->dpc_rpc_extensions inside 
dpc_handle_surprise_removal() before calling pci_clear_surpdn_errors(). 
Should I recheck it once again here?

> 
> Wrap to 80 chars per line.

Okay.

> 
> 
>> +	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);
>> +}
> 
> A code comment might be useful here saying that 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.

And probably move this code comment below to where this function is 
called inside dpc_handle_surprise_removal()..?

> 
> 
>> +static void dpc_handle_surprise_removal(struct pci_dev *pdev)
>> +{
> 
> I'm wondering if we also need
> 
> 	if (!pcie_wait_for_link(pdev, false)) {
> 		pci_info(pdev, "Data Link Layer Link Active not cleared in 1000 msec\n");
> 		goto out;
> 	}
> 
> here, similar to dpc_reset_link() and in accordance with PCIe r6.1
> sec 6.2.11:
> 
> 	"To ensure that the LTSSM has time to reach the Disabled state
> 	or at least to bring the Link down under a variety of error
> 	conditions, software must leave the Downstream Port in DPC
> 	until the Data Link Layer Link Active bit in the Link Status
> 	Register reads 0b; otherwise, the result is undefined."

And include the above comment in code..
> 
> 
>> +	if (pdev->dpc_rp_extensions && dpc_wait_rp_inactive(pdev)) {
>> +		pci_err(pdev, "failed to retrieve DPC root port on async remove\n");
>> +		goto out;
>> +	}
> 
> I don't think pci_err() is needed here as dpc_wait_rp_inactive()
> already emits a message.  (I think I mistakenly gave the advice
> to emit an error here in an earlier review comment -- sorry!)

:)

Will take care of other comments below as well.

Thanks,
Smita

>>   
>>   	/* We configure DPC so it only triggers on ERR_FATAL */
>> -- 
>> 2.17.1
>>

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

[Lukas Wunner]

On Thu, Jun 22, 2023 at 02:02:03PM -0700, Smita Koralahalli wrote:
> On 6/22/2023 2:04 AM, Lukas Wunner wrote:
> > On Wed, Jun 21, 2023 at 06:51:51PM +0000, Smita Koralahalli wrote:
> > > --- a/drivers/pci/pcie/dpc.c
> > > +++ b/drivers/pci/pcie/dpc.c
> > > @@ -292,10 +292,68 @@ void dpc_process_error(struct pci_dev *pdev)
> > >   	}
> > >   }
> > > +static void pci_clear_surpdn_errors(struct pci_dev *pdev)
> > > +{
> > > +	u16 reg16;
> > > +	u32 reg32;
> > > +
> > > +	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);
> > 
> > Make this read+write conditional on "if (pdev->dpc_rp_extensions)"
> > as the register otherwise doesn't exist.
> 
> I'm checking for pdev->dpc_rpc_extensions inside
> dpc_handle_surprise_removal() before calling pci_clear_surpdn_errors().
> Should I recheck it once again here?

Yes.


> > > +	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);
> > > +}
> > 
> > A code comment might be useful here saying that 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.
> 
> And probably move this code comment below to where this function is called
> inside dpc_handle_surprise_removal()..?

No right here would be good because that's the piece of code to which
the code comment would pertain.


> > 	if (!pcie_wait_for_link(pdev, false)) {
> > 		pci_info(pdev, "Data Link Layer Link Active not cleared in 1000 msec\n");
> > 		goto out;
> > 	}
> > 
> > here, similar to dpc_reset_link() and in accordance with PCIe r6.1
> > sec 6.2.11:
> > 
> > 	"To ensure that the LTSSM has time to reach the Disabled state
> > 	or at least to bring the Link down under a variety of error
> > 	conditions, software must leave the Downstream Port in DPC
> > 	until the Data Link Layer Link Active bit in the Link Status
> > 	Register reads 0b; otherwise, the result is undefined."
> 
> And include the above comment in code..

I'd say that's optional.  dpc_reset_link() doesn't have a code comment
for that either, so...

Thanks,

Lukas

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

[Sathyanarayanan Kuppuswamy]

On 6/21/23 11:51 AM, Smita Koralahalli wrote:
> According to Section 6.7.6 of PCIe Base Specification [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.

I think this fix is applicable to the EDR code path as well.

> 
> 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.
> ---
>  drivers/pci/pcie/dpc.c | 58 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 3ceed8e3de41..5153ac8ea91c 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -292,10 +292,68 @@ void dpc_process_error(struct pci_dev *pdev)
>  	}
>  }
>  
> +static void pci_clear_surpdn_errors(struct pci_dev *pdev)
> +{
> +	u16 reg16;
> +	u32 reg32;
> +
> +	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);

It is not clear why you want to clear it.

> +
> +	pci_read_config_word(pdev, PCI_STATUS, &reg16);
> +	pci_write_config_word(pdev, PCI_STATUS, reg16);

Same as above. Can you add some comment about why you are clearing it?

> +
> +	pcie_capability_write_word(pdev, PCI_EXP_DEVSTA, PCI_EXP_DEVSTA_FED);
> +}
> +
> +static void dpc_handle_surprise_removal(struct pci_dev *pdev)
> +{
> +	if (pdev->dpc_rp_extensions && dpc_wait_rp_inactive(pdev)) {
> +		pci_err(pdev, "failed to retrieve DPC root port on async remove\n");
> +		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);

Don't you need to wait for the link to go down?

> +
> +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;
> +
> +	pci_read_config_word(pdev, pdev->aer_cap + PCI_ERR_UNCOR_STATUS, &status);
> +
> +	if (!pdev->is_hotplug_bridge)
> +		return false;
> +
> +	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 Section 6.7.6 of the PCIe Base Spec 6.0, since async
> +	 * removal might be unexpected, errors might be reported as a side
> +	 * effect of the event and software should handle them as an expected
> +	 * part of this event.
> +	 */
> +	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 */

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

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

[Smita Koralahalli]

On 6/22/2023 4:22 PM, Sathyanarayanan Kuppuswamy wrote:
> 
> 
> On 6/21/23 11:51 AM, Smita Koralahalli wrote:
>> According to Section 6.7.6 of PCIe Base Specification [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.
> 
> I think this fix is applicable to the EDR code path as well.
> 
>>
>> 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.
>> ---
>>   drivers/pci/pcie/dpc.c | 58 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 58 insertions(+)
>>
>> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>> index 3ceed8e3de41..5153ac8ea91c 100644
>> --- a/drivers/pci/pcie/dpc.c
>> +++ b/drivers/pci/pcie/dpc.c
>> @@ -292,10 +292,68 @@ void dpc_process_error(struct pci_dev *pdev)
>>   	}
>>   }
>>   
>> +static void pci_clear_surpdn_errors(struct pci_dev *pdev)
>> +{
>> +	u16 reg16;
>> +	u32 reg32;
>> +
>> +	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);
> 
> It is not clear why you want to clear it.

We are observing, Surprise Down Errors setting error bits in these 
status registers and also Fatal Error Detected bit in DEVSTA. Hence, we 
are clearing them to mask any kind of appearance that there was an 
error. Will add appropriate code comments in v4..
> 
>> +
>> +	pci_read_config_word(pdev, PCI_STATUS, &reg16);
>> +	pci_write_config_word(pdev, PCI_STATUS, reg16);
> 
> Same as above. Can you add some comment about why you are clearing it?

Will add.
> 
>> +
>> +	pcie_capability_write_word(pdev, PCI_EXP_DEVSTA, PCI_EXP_DEVSTA_FED);
>> +}
>> +
>> +static void dpc_handle_surprise_removal(struct pci_dev *pdev)
>> +{
>> +	if (pdev->dpc_rp_extensions && dpc_wait_rp_inactive(pdev)) {
>> +		pci_err(pdev, "failed to retrieve DPC root port on async remove\n");
>> +		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);
> 
> Don't you need to wait for the link to go down?

Yes will include, pcie_wait_for_link()..

Should this check be here or at the beginning of the function before we
check pdev->dpc_rp_extensions?

Thanks,
Smita

> 
>> +
>> +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;
>> +
>> +	pci_read_config_word(pdev, pdev->aer_cap + PCI_ERR_UNCOR_STATUS, &status);
>> +
>> +	if (!pdev->is_hotplug_bridge)
>> +		return false;
>> +
>> +	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 Section 6.7.6 of the PCIe Base Spec 6.0, since async
>> +	 * removal might be unexpected, errors might be reported as a side
>> +	 * effect of the event and software should handle them as an expected
>> +	 * part of this event.
>> +	 */
>> +	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 */
> 

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

[Lukas Wunner]

On Tue, Jun 27, 2023 at 10:48:37AM -0700, Smita Koralahalli wrote:
> On 6/22/2023 4:22 PM, Sathyanarayanan Kuppuswamy wrote:
> > On 6/21/23 11:51 AM, Smita Koralahalli wrote:
> > > +static void dpc_handle_surprise_removal(struct pci_dev *pdev)
> > > +{
> > > +	if (pdev->dpc_rp_extensions && dpc_wait_rp_inactive(pdev)) {
> > > +		pci_err(pdev, "failed to retrieve DPC root port on async remove\n");
> > > +		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);
> > 
> > Don't you need to wait for the link to go down?
> 
> Yes will include, pcie_wait_for_link()..
> 
> Should this check be here or at the beginning of the function before we
> check pdev->dpc_rp_extensions?

I'd just use the same order as dpc_reset_link(), i.e. checking for
!pcie_wait_for_link() happens before the check for pdev->dpc_rp_extensions.

Thanks,

Lukas

[PATCH v3 2/2] PCI: pciehp: Clear the optional capabilities in DEVCTL2 on a hot-plug

[Smita Koralahalli]

Clear the optional capabilities ARI Forwarding Enable, AtomicOp Requester
Enable and 10-Bit Tag Requester Enable in DEVCTL2 unconditionally on a
hot-plug event. These are the bits which are negotiated between endpoint
and root port and should be re-negotiated and set up for optimal operation
on a hot add.

According to implementation notes in 2.2.6.2, of PCIe Base Specification
[1], "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. This
enables the important class of 10-Bit Tag capable adapters that send
Memory Read Requests only to host memory". Hence, it must be noted that
Platform FW may enable these bits if endpoint supports them at boot time
for performance reasons even if Linux hasn't defined them.

Issues are being observed where a device became inaccessible and the port
was not able to be recovered without a system reset when a device with
10-bit tags was removed and replaced with a device that didn't support
10-bit tags.

Section 2.2.6.2, in PCIe Base Specification 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.

Additionally, Section 6.13 and 6.15 of the PCIe Base Spec 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.

[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.
---
 drivers/pci/hotplug/pciehp_pci.c | 4 ++++
 include/uapi/linux/pci_regs.h    | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index ad12515a4a12..e27fd2bc4ceb 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -102,6 +102,10 @@ void pciehp_unconfigure_device(struct controller *ctrl, bool presence)
 
 	pci_lock_rescan_remove();
 
+	pcie_capability_clear_word(ctrl->pcie->port, PCI_EXP_DEVCTL2,
+				   (PCI_EXP_DEVCTL2_ARI | PCI_EXP_DEVCTL2_ATOMIC_REQ |
+				    PCI_EXP_DEVCTL2_TAG_REQ_EN));
+
 	/*
 	 * Stopping an SR-IOV PF device removes all the associated VFs,
 	 * which will update the bus->devices list and confuse the
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index dc2000e0fe3a..6fbc47f23d52 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -668,6 +668,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_TAG_REQ_EN	0x1000  /* Allow 10 Tags for 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 v3 2/2] PCI: pciehp: Clear the optional capabilities in DEVCTL2 on a hot-plug

[Lukas Wunner]

On Wed, Jun 21, 2023 at 06:51:52PM +0000, Smita Koralahalli wrote:
> --- a/drivers/pci/hotplug/pciehp_pci.c
> +++ b/drivers/pci/hotplug/pciehp_pci.c
> @@ -102,6 +102,10 @@ void pciehp_unconfigure_device(struct controller *ctrl, bool presence)
>  
>  	pci_lock_rescan_remove();
>  
> +	pcie_capability_clear_word(ctrl->pcie->port, PCI_EXP_DEVCTL2,
> +				   (PCI_EXP_DEVCTL2_ARI | PCI_EXP_DEVCTL2_ATOMIC_REQ |
> +				    PCI_EXP_DEVCTL2_TAG_REQ_EN));
> +

Hm, this will clear the bits while the device may still be present.
Note that the subsequent pci_stop_and_remove_bus_device() will unbind
the driver and may thus cause communication with the device.
Can clearing those bits in the hotplug port hamper communication with
the device?

I'd recommend avoiding that issue altogether by clearing the bits at
the end of the function after the call to pci_unlock_rescan_remove(),
so that negotiated state of the hotplug port gets cleared after all
subordinate devices are de-enumerated.

The commit message doesn't point out that PCI_EXP_DEVCTL2_ARI is
already being taken care of on enumeration of future subordinate
devices in pci_configure_ari() and is only cleared here for good
measure.  If you intend to configure 10 bit tags and atomic ops
on enumeration in future patches, I'd recommend omitting
PCI_EXP_DEVCTL2_ARI here and clearing each of the other two bits
in the future patches which configure them on enumeration.

You don't need braces around the "or" operation for the bits.

Thanks,

Lukas

>  	/*
>  	 * Stopping an SR-IOV PF device removes all the associated VFs,
>  	 * which will update the bus->devices list and confuse the
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index dc2000e0fe3a..6fbc47f23d52 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -668,6 +668,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_TAG_REQ_EN	0x1000  /* Allow 10 Tags for 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 v3 2/2] PCI: pciehp: Clear the optional capabilities in DEVCTL2 on a hot-plug

[Lukas Wunner]

On Thu, Jun 22, 2023 at 08:31:05AM +0200, Lukas Wunner wrote:
> On Wed, Jun 21, 2023 at 06:51:52PM +0000, Smita Koralahalli wrote:
> The commit message doesn't point out that PCI_EXP_DEVCTL2_ARI is
> already being taken care of on enumeration of future subordinate
> devices in pci_configure_ari() and is only cleared here for good
> measure.  If you intend to configure 10 bit tags and atomic ops
> on enumeration in future patches, I'd recommend omitting
> PCI_EXP_DEVCTL2_ARI here and clearing each of the other two bits
                               ^^^^^^^^

Sorry, I meant to say "dropping".  I.e. once you enable or disable
the feature on enumeration and there's no longer a need to
unconditionally disable it on de-enumeration, drop clearing the bit
here.

Thanks,

Lukas

Re: [PATCH v3 2/2] PCI: pciehp: Clear the optional capabilities in DEVCTL2 on a hot-plug

[Smita Koralahalli]

On 6/21/2023 11:31 PM, Lukas Wunner wrote:
> On Wed, Jun 21, 2023 at 06:51:52PM +0000, Smita Koralahalli wrote:
>> --- a/drivers/pci/hotplug/pciehp_pci.c
>> +++ b/drivers/pci/hotplug/pciehp_pci.c
>> @@ -102,6 +102,10 @@ void pciehp_unconfigure_device(struct controller *ctrl, bool presence)
>>   
>>   	pci_lock_rescan_remove();
>>   
>> +	pcie_capability_clear_word(ctrl->pcie->port, PCI_EXP_DEVCTL2,
>> +				   (PCI_EXP_DEVCTL2_ARI | PCI_EXP_DEVCTL2_ATOMIC_REQ |
>> +				    PCI_EXP_DEVCTL2_TAG_REQ_EN));
>> +
> 
> Hm, this will clear the bits while the device may still be present.
> Note that the subsequent pci_stop_and_remove_bus_device() will unbind
> the driver and may thus cause communication with the device.
> Can clearing those bits in the hotplug port hamper communication with
> the device?
> 
> I'd recommend avoiding that issue altogether by clearing the bits at
> the end of the function after the call to pci_unlock_rescan_remove(),
> so that negotiated state of the hotplug port gets cleared after all
> subordinate devices are de-enumerated.

This is a good point. Thanks!

> 
> The commit message doesn't point out that PCI_EXP_DEVCTL2_ARI is
> already being taken care of on enumeration of future subordinate
> devices in pci_configure_ari() and is only cleared here for good
> measure.  If you intend to configure 10 bit tags and atomic ops
> on enumeration in future patches, I'd recommend omitting
> PCI_EXP_DEVCTL2_ARI here and clearing each of the other two bits
> in the future patches which configure them on enumeration.

Would it be fair to just reuse pci_enable_atomic_ops_to_root() for
Atomic_Ops configuration?

> 
> You don't need braces around the "or" operation for the bits.

Sure!

Thanks,
Smita

> 
> Thanks,
> 
> Lukas
> 
>>   	/*
>>   	 * Stopping an SR-IOV PF device removes all the associated VFs,
>>   	 * which will update the bus->devices list and confuse the
>> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
>> index dc2000e0fe3a..6fbc47f23d52 100644
>> --- a/include/uapi/linux/pci_regs.h
>> +++ b/include/uapi/linux/pci_regs.h
>> @@ -668,6 +668,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_TAG_REQ_EN	0x1000  /* Allow 10 Tags for 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 v3 2/2] PCI: pciehp: Clear the optional capabilities in DEVCTL2 on a hot-plug

[Lukas Wunner]

[cc += Jay, Felix]

On Thu, Jun 22, 2023 at 02:02:12PM -0700, Smita Koralahalli wrote:
> Would it be fair to just reuse pci_enable_atomic_ops_to_root() for
> Atomic_Ops configuration?

Hm, that's a good question.  I'm not an expert on that corner of
the PCI core.

But indeed what you could try is amend that function to not only
*set* PCI_EXP_DEVCTL2_ATOMIC_REQ if it's supported, but to also
*clear* it if it's not supported.

And you'd have to call pci_enable_atomic_ops_to_root() on enumeration,
e.g. from pci_init_capabilities().

That should obviate the need to call pci_enable_atomic_ops_to_root()
from drivers, so you could probably remove the call from all the
drivers which currently call it (amdgpu, infiniband, mellanox),
in one separate patch per driver.

An then you could drop the EXPORT clause for pci_enable_atomic_ops_to_root()
and make it private to the PCI core.

So that would be 5 patches (enablement/disablement on enumeration,
amendmend of the 3 drivers, making the call private).

I'm not sure if anyone will cry foul if you do that but if you want
to give it a try, go for it. :)

I don't now why commit 430a23689dea, which introduced
pci_enable_atomic_ops_to_root(), chose to add it as a library function
which is only called from specific drivers, instead of universally
enabling the feature for all devices.  Adding the commit authors to cc
so they can chime in.

Thanks,

Lukas

Re: [PATCH v3 2/2] PCI: pciehp: Clear the optional capabilities in DEVCTL2 on a hot-plug

[Felix Kuehling]

On 2023-06-22 23:42, Lukas Wunner wrote:
> [cc += Jay, Felix]
>
> On Thu, Jun 22, 2023 at 02:02:12PM -0700, Smita Koralahalli wrote:
>> Would it be fair to just reuse pci_enable_atomic_ops_to_root() for
>> Atomic_Ops configuration?
> Hm, that's a good question.  I'm not an expert on that corner of
> the PCI core.
>
> But indeed what you could try is amend that function to not only
> *set* PCI_EXP_DEVCTL2_ATOMIC_REQ if it's supported, but to also
> *clear* it if it's not supported.
>
> And you'd have to call pci_enable_atomic_ops_to_root() on enumeration,
> e.g. from pci_init_capabilities().
>
> That should obviate the need to call pci_enable_atomic_ops_to_root()
> from drivers, so you could probably remove the call from all the
> drivers which currently call it (amdgpu, infiniband, mellanox),
> in one separate patch per driver.
>
> An then you could drop the EXPORT clause for pci_enable_atomic_ops_to_root()
> and make it private to the PCI core.
Then our driver would need an alternative way to determine whether 
atomic capabilities are enabled for a device. We currently use the 
return value from pci_enable_atomic_ops_to_root to determine this.

Regards,
   Felix


>
> So that would be 5 patches (enablement/disablement on enumeration,
> amendmend of the 3 drivers, making the call private).
>
> I'm not sure if anyone will cry foul if you do that but if you want
> to give it a try, go for it. :)
>
> I don't now why commit 430a23689dea, which introduced
> pci_enable_atomic_ops_to_root(), chose to add it as a library function
> which is only called from specific drivers, instead of universally
> enabling the feature for all devices.  Adding the commit authors to cc
> so they can chime in.
>
> Thanks,
>
> Lukas

Re: [PATCH v3 2/2] PCI: pciehp: Clear the optional capabilities in DEVCTL2 on a hot-plug

[Lukas Wunner]

On Fri, Jun 23, 2023 at 05:59:55AM +0200, Felix Kuehling wrote:
> On 2023-06-22 23:42, Lukas Wunner wrote:
> > On Thu, Jun 22, 2023 at 02:02:12PM -0700, Smita Koralahalli wrote:
> > > Would it be fair to just reuse pci_enable_atomic_ops_to_root() for
> > > Atomic_Ops configuration?
> > 
> > Hm, that's a good question.  I'm not an expert on that corner of
> > the PCI core.
> > 
> > But indeed what you could try is amend that function to not only
> > *set* PCI_EXP_DEVCTL2_ATOMIC_REQ if it's supported, but to also
> > *clear* it if it's not supported.
> > 
> > And you'd have to call pci_enable_atomic_ops_to_root() on enumeration,
> > e.g. from pci_init_capabilities().
> > 
> > That should obviate the need to call pci_enable_atomic_ops_to_root()
> > from drivers, so you could probably remove the call from all the
> > drivers which currently call it (amdgpu, infiniband, mellanox),
> > in one separate patch per driver.
> > 
> > An then you could drop the EXPORT clause for pci_enable_atomic_ops_to_root()
> > and make it private to the PCI core.
> 
> Then our driver would need an alternative way to determine whether atomic
> capabilities are enabled for a device. We currently use the return value
> from pci_enable_atomic_ops_to_root to determine this.

Just read PCI_EXP_DEVCTL2 and check whether PCI_EXP_DEVCTL2_ATOMIC_REQ
is set.  (I.e. has been set by the PCI core on device enumeration.)

Problem solved, I guess?

Thanks,

Lukas

Re: [PATCH v3 2/2] PCI: pciehp: Clear the optional capabilities in DEVCTL2 on a hot-plug

[Jay Cornwall]

On 6/22/2023 16:42, Lukas Wunner wrote:

> I don't now why commit 430a23689dea, which introduced
> pci_enable_atomic_ops_to_root(), chose to add it as a library function
> which is only called from specific drivers, instead of universally
> enabling the feature for all devices.  Adding the commit authors to cc
> so they can chime in.

IIRC during the initial design discussion on linux-pci this approach was suggested to avoid triggering potential bugs in devices without AtomicOps support. See quote below.

I've no objections to changing it.

On 2016-05-06 10:48, Bjorn Helgaas wrote:

> Once enabled in Device Control 2, a device's use of AtomicOps is
> competely device-specific.  In many cases, the device probably doesn't
> support AtomicOps, so enabling them would be a no-op.  But there could
> be devices where AtomicOps are nominally supported but untested or
> broken.  Even if we didn't change their drivers, those devices could
> start using AtomicOps, so I'm not comfortable with the PCI core
> enabling AtomicOp requests indiscriminately.

Re: [PATCH v3 2/2] PCI: pciehp: Clear the optional capabilities in DEVCTL2 on a hot-plug

[Smita Koralahalli]

On 6/22/2023 2:42 PM, Lukas Wunner wrote:
> [cc += Jay, Felix]
> 
> On Thu, Jun 22, 2023 at 02:02:12PM -0700, Smita Koralahalli wrote:
>> Would it be fair to just reuse pci_enable_atomic_ops_to_root() for
>> Atomic_Ops configuration?
> 
> Hm, that's a good question.  I'm not an expert on that corner of
> the PCI core.
> 
> But indeed what you could try is amend that function to not only
> *set* PCI_EXP_DEVCTL2_ATOMIC_REQ if it's supported, but to also
> *clear* it if it's not supported.
> 
> And you'd have to call pci_enable_atomic_ops_to_root() on enumeration,
> e.g. from pci_init_capabilities().
> 
> That should obviate the need to call pci_enable_atomic_ops_to_root()
> from drivers, so you could probably remove the call from all the
> drivers which currently call it (amdgpu, infiniband, mellanox),
> in one separate patch per driver.
> 
> An then you could drop the EXPORT clause for pci_enable_atomic_ops_to_root()
> and make it private to the PCI core.
> 
> So that would be 5 patches (enablement/disablement on enumeration,
> amendmend of the 3 drivers, making the call private).
> 
> I'm not sure if anyone will cry foul if you do that but if you want
> to give it a try, go for it. :)

Okay, I see there are no objections except for Bjorn/Jay's comments on

"But there could be devices where AtomicOps are nominally supported but 
untested or broken.."

Would this be an issue?

If not, I will start working on those 5 patches.

Thanks,
Smita
> 
> I don't now why commit 430a23689dea, which introduced
> pci_enable_atomic_ops_to_root(), chose to add it as a library function
> which is only called from specific drivers, instead of universally
> enabling the feature for all devices.  Adding the commit authors to cc
> so they can chime in.
> 
> Thanks,
> 
> Lukas

Re: [PATCH v3 2/2] PCI: pciehp: Clear the optional capabilities in DEVCTL2 on a hot-plug

[Lukas Wunner]

On Tue, Jun 27, 2023 at 10:38:54AM -0700, Smita Koralahalli wrote:
> Okay, I see there are no objections except for Bjorn/Jay's comments on
> 
> "But there could be devices where AtomicOps are nominally supported but
> untested or broken.."
> 
> Would this be an issue?

I think you said that BIOS enables AtomicOps on certain AMD machines?
Or did that observation only apply to 10 Bit tags?

If BIOS does enable AtomicOps, it would be interesting to know which
logic BIOS follows, i.e. how does it determine whether to set
AtomicOp Requester Enable on Endpoints?

It would also be interesting to know how far that BIOS has proliferated,
i.e. how much experience with various Endpoint devices exists in the
real world.  If it turns out that BIOS has enabled the feature for
years on a wide range of Endpoints without any issues, I think
that would render concerns mute that enabling it in the kernel
might cause regressions.

I don't know why the spec says that "discovery of AtomicOp Requester
capabilities is outside the scope of this specification".  I imagine
it would be possible to set AtomicOp Requester Enable, then read it
to determine whether the bit is now indeed 1 or hard-wired to 0.
In the latter case, AtomicOp Requester capabilities can be assumed
to be absent.  So that would be a way to make do without any other
specific discovery of AtomicOp Requester capabilities.

Thanks,

Lukas

Re: [PATCH v3 2/2] PCI: pciehp: Clear the optional capabilities in DEVCTL2 on a hot-plug

[Smita Koralahalli]

On 6/28/2023 6:25 AM, Lukas Wunner wrote:
> On Tue, Jun 27, 2023 at 10:38:54AM -0700, Smita Koralahalli wrote:
>> Okay, I see there are no objections except for Bjorn/Jay's comments on
>>
>> "But there could be devices where AtomicOps are nominally supported but
>> untested or broken.."
>>
>> Would this be an issue?
> 
> I think you said that BIOS enables AtomicOps on certain AMD machines?
> Or did that observation only apply to 10 Bit tags?

Yes, that observation right now applies only to 10 bit tags.

> 
> If BIOS does enable AtomicOps, it would be interesting to know which
> logic BIOS follows, i.e. how does it determine whether to set
> AtomicOp Requester Enable on Endpoints?

I agree this is a very good thing to know. I have followed up with the 
BIOS team to get some pointers on this. I will get back to you soon.

> 
> It would also be interesting to know how far that BIOS has proliferated,
> i.e. how much experience with various Endpoint devices exists in the
> real world.  If it turns out that BIOS has enabled the feature for
> years on a wide range of Endpoints without any issues, I think
> that would render concerns mute that enabling it in the kernel
> might cause regressions.
> 
> I don't know why the spec says that "discovery of AtomicOp Requester
> capabilities is outside the scope of this specification".  I imagine
> it would be possible to set AtomicOp Requester Enable, then read it
> to determine whether the bit is now indeed 1 or hard-wired to 0.
> In the latter case, AtomicOp Requester capabilities can be assumed
> to be absent.  So that would be a way to make do without any other
> specific discovery of AtomicOp Requester capabilities.
> 
> Thanks,
> 
> Lukas

Re: [PATCH v3 2/2] PCI: pciehp: Clear the optional capabilities in DEVCTL2 on a hot-plug

[Smita Koralahalli]

On 6/28/2023 6:25 AM, Lukas Wunner wrote:
> On Tue, Jun 27, 2023 at 10:38:54AM -0700, Smita Koralahalli wrote:
>> Okay, I see there are no objections except for Bjorn/Jay's comments on
>>
>> "But there could be devices where AtomicOps are nominally supported but
>> untested or broken.."
>>
>> Would this be an issue?
> 
> I think you said that BIOS enables AtomicOps on certain AMD machines?
> Or did that observation only apply to 10 Bit tags?
> 
> If BIOS does enable AtomicOps, it would be interesting to know which
> logic BIOS follows, i.e. how does it determine whether to set
> AtomicOp Requester Enable on Endpoints?
> 
> It would also be interesting to know how far that BIOS has proliferated,
> i.e. how much experience with various Endpoint devices exists in the
> real world.  If it turns out that BIOS has enabled the feature for
> years on a wide range of Endpoints without any issues, I think
> that would render concerns mute that enabling it in the kernel
> might cause regressions.
> 
> I don't know why the spec says that "discovery of AtomicOp Requester
> capabilities is outside the scope of this specification".  I imagine
> it would be possible to set AtomicOp Requester Enable, then read it
> to determine whether the bit is now indeed 1 or hard-wired to 0.
> In the latter case, AtomicOp Requester capabilities can be assumed
> to be absent.  So that would be a way to make do without any other
> specific discovery of AtomicOp Requester capabilities.

Sorry for getting back to this very late.

The approach taken by our Platform FW is not as robust as how it is
handled by SW. And also they haven't come across issues for AtomicOps 
similar to 10-bit tags.

Hence, I worked on the approach of reading back the "AtomicOp Requester 
Enable" to determine whether it is indeed 1 or hard-wired to 0.

I came across two issues.

This approach did not eliminate calling pci_enable_atomic_ops_to_root() 
on all drivers as some of them enabled AtomicOp by only checking for 
32-bit and 64-bit Completion capabilities.

And, on internal review I received comments that this approach wouldn't 
help in determining the presence/absence of AtomicOps. 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..

I have sent v4 considering all these constraints. Let me know what you 
think.

Thanks,
Smita

> 
> Thanks,
> 
> Lukas