[3/3] core/pci: Fix access non-existing registers on disabling cmpl timeout

Message ID 1495763586-27238-3-git-send-email-gwshan@linux.vnet.ibm.com
State New
Headers show

Commit Message

Gavin Shan May 26, 2017, 1:53 a.m.
The PCIe transaction timeout is disabled if the capability is claimed
in register PCIECAP_EXP_DCAP2, meaning we read it unconditionally.
However, the reigster can be invalid if PCIe capability version isn't
bigger than 1. It potentially causes pending (hanged) PCIe transaction.

This fixes the issue by check the cached PCIe capability version. We
won't touch register PCIECAP_EXP_DCAP2 and PCICAP_EXP_DCTL2 if PCIe
capability version isn't bigger than 1. The limitation isn't applied
to P7/P8/P9's root port as we clearly know those registers are valid
there.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 core/pci.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Benjamin Herrenschmidt June 15, 2017, 5:46 a.m. | #1
On Fri, 2017-05-26 at 11:53 +1000, Gavin Shan wrote:
> The PCIe transaction timeout is disabled if the capability is claimed
> in register PCIECAP_EXP_DCAP2, meaning we read it unconditionally.
> However, the reigster can be invalid if PCIe capability version isn't
> bigger than 1. It potentially causes pending (hanged) PCIe transaction.
> 
> This fixes the issue by check the cached PCIe capability version. We
> won't touch register PCIECAP_EXP_DCAP2 and PCICAP_EXP_DCTL2 if PCIe
> capability version isn't bigger than 1. The limitation isn't applied
> to P7/P8/P9's root port as we clearly know those registers are valid
> there.

> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
>  core/pci.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/core/pci.c b/core/pci.c
> index 29c3df6..f173cc2 100644
> --- a/core/pci.c
> +++ b/core/pci.c
> @@ -162,8 +162,6 @@ static void pci_init_pcie_cap(struct phb *phb, struct pci_device *pd)
>  		return;
>  	}
>  
> -	pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, 0ul, false);
> -
>  	/*
>  	 * XXX We observe a problem on some PLX switches where one
>  	 * of the downstream ports appears as an upstream port, we
> @@ -174,9 +172,15 @@ static void pci_init_pcie_cap(struct phb *phb, struct pci_device *pd)
>  	if (pd->parent && pd->parent->dev_type == PCIE_TYPE_SWITCH_UPPORT &&
>  	    pd->vdid == 0x874810b5 && pd->dev_type == PCIE_TYPE_SWITCH_UPPORT) {
>  		PCIDBG(phb, pd->bdfn, "Fixing up bad PLX downstream port !\n");
> +
> +		reg &= ~PCICAP_EXP_CAP_TYPE;
> +		reg |= PCIE_TYPE_SWITCH_DNPORT;
>  		pd->dev_type = PCIE_TYPE_SWITCH_DNPORT;
>  	}
>  
> +	/* Set PCIe capability */
> +	pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, reg, false);

Why would you or "PCIE_TYPE_SWITCH_DNPORT" ?

The cap cache should just contain the offsets. The versions can be
read explicitely. If you want to cache the versions do a separate
array.

The above is very confusing. That cap "Data" need to die. A generic
untyped place holder is a bad thing.

>  	/* XXX Handle ARI */
>  	if (pd->dev_type == PCIE_TYPE_SWITCH_DNPORT ||
>  	    pd->dev_type == PCIE_TYPE_ROOT_PORT)
> @@ -833,11 +837,13 @@ static int pci_configure_mps(struct phb *phb,
>  
>  static void pci_disable_completion_timeout(struct phb *phb, struct pci_device *pd)
>  {
> -	uint32_t ecap;
> -	uint32_t val;
> +	uint32_t ecap, val;
> +	uint16_t ecap_data;
>  
>  	/* PCIE capability required */
> -	if (!pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false))
> +	ecap_data = (uint16_t)pci_cap_data(pd, PCI_CFG_CAP_ID_EXP, false);
> +	if (!pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false) ||
> +	    (ecap_data & PCICAP_EXP_CAP_VERSION) <= 1)
>  		return;

Just read the capability version off the HW here.
>  
>  	/* Check if it has capability to disable completion timeout */
Gavin Shan June 19, 2017, 3:39 a.m. | #2
On Thu, Jun 15, 2017 at 03:46:41PM +1000, Benjamin Herrenschmidt wrote:
>On Fri, 2017-05-26 at 11:53 +1000, Gavin Shan wrote:
>> The PCIe transaction timeout is disabled if the capability is claimed
>> in register PCIECAP_EXP_DCAP2, meaning we read it unconditionally.
>> However, the reigster can be invalid if PCIe capability version isn't
>> bigger than 1. It potentially causes pending (hanged) PCIe transaction.
>> 
>> This fixes the issue by check the cached PCIe capability version. We
>> won't touch register PCIECAP_EXP_DCAP2 and PCICAP_EXP_DCTL2 if PCIe
>> capability version isn't bigger than 1. The limitation isn't applied
>> to P7/P8/P9's root port as we clearly know those registers are valid
>> there.
>
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>>  core/pci.c | 16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>> 
>> diff --git a/core/pci.c b/core/pci.c
>> index 29c3df6..f173cc2 100644
>> --- a/core/pci.c
>> +++ b/core/pci.c
>> @@ -162,8 +162,6 @@ static void pci_init_pcie_cap(struct phb *phb, struct pci_device *pd)
>>  		return;
>>  	}
>>  
>> -	pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, 0ul, false);
>> -
>>  	/*
>>  	 * XXX We observe a problem on some PLX switches where one
>>  	 * of the downstream ports appears as an upstream port, we
>> @@ -174,9 +172,15 @@ static void pci_init_pcie_cap(struct phb *phb, struct pci_device *pd)
>>  	if (pd->parent && pd->parent->dev_type == PCIE_TYPE_SWITCH_UPPORT &&
>>  	    pd->vdid == 0x874810b5 && pd->dev_type == PCIE_TYPE_SWITCH_UPPORT) {
>>  		PCIDBG(phb, pd->bdfn, "Fixing up bad PLX downstream port !\n");
>> +
>> +		reg &= ~PCICAP_EXP_CAP_TYPE;
>> +		reg |= PCIE_TYPE_SWITCH_DNPORT;
>>  		pd->dev_type = PCIE_TYPE_SWITCH_DNPORT;
>>  	}
>>  
>> +	/* Set PCIe capability */
>> +	pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, reg, false);
>
>Why would you or "PCIE_TYPE_SWITCH_DNPORT" ?
>
>The cap cache should just contain the offsets. The versions can be
>read explicitely. If you want to cache the versions do a separate
>array.
>
>The above is very confusing. That cap "Data" need to die. A generic
>untyped place holder is a bad thing.
>

Ok, It's going to die in v2.

>>  	/* XXX Handle ARI */
>>  	if (pd->dev_type == PCIE_TYPE_SWITCH_DNPORT ||
>>  	    pd->dev_type == PCIE_TYPE_ROOT_PORT)
>> @@ -833,11 +837,13 @@ static int pci_configure_mps(struct phb *phb,
>>  
>>  static void pci_disable_completion_timeout(struct phb *phb, struct pci_device *pd)
>>  {
>> -	uint32_t ecap;
>> -	uint32_t val;
>> +	uint32_t ecap, val;
>> +	uint16_t ecap_data;
>>  
>>  	/* PCIE capability required */
>> -	if (!pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false))
>> +	ecap_data = (uint16_t)pci_cap_data(pd, PCI_CFG_CAP_ID_EXP, false);
>> +	if (!pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false) ||
>> +	    (ecap_data & PCICAP_EXP_CAP_VERSION) <= 1)
>>  		return;
>
>Just read the capability version off the HW here.

Yes, Will do in v2.

>>  
>>  	/* Check if it has capability to disable completion timeout */

Cheers,
Gavin

Patch

diff --git a/core/pci.c b/core/pci.c
index 29c3df6..f173cc2 100644
--- a/core/pci.c
+++ b/core/pci.c
@@ -162,8 +162,6 @@  static void pci_init_pcie_cap(struct phb *phb, struct pci_device *pd)
 		return;
 	}
 
-	pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, 0ul, false);
-
 	/*
 	 * XXX We observe a problem on some PLX switches where one
 	 * of the downstream ports appears as an upstream port, we
@@ -174,9 +172,15 @@  static void pci_init_pcie_cap(struct phb *phb, struct pci_device *pd)
 	if (pd->parent && pd->parent->dev_type == PCIE_TYPE_SWITCH_UPPORT &&
 	    pd->vdid == 0x874810b5 && pd->dev_type == PCIE_TYPE_SWITCH_UPPORT) {
 		PCIDBG(phb, pd->bdfn, "Fixing up bad PLX downstream port !\n");
+
+		reg &= ~PCICAP_EXP_CAP_TYPE;
+		reg |= PCIE_TYPE_SWITCH_DNPORT;
 		pd->dev_type = PCIE_TYPE_SWITCH_DNPORT;
 	}
 
+	/* Set PCIe capability */
+	pci_set_cap(pd, PCI_CFG_CAP_ID_EXP, ecap, reg, false);
+
 	/* XXX Handle ARI */
 	if (pd->dev_type == PCIE_TYPE_SWITCH_DNPORT ||
 	    pd->dev_type == PCIE_TYPE_ROOT_PORT)
@@ -833,11 +837,13 @@  static int pci_configure_mps(struct phb *phb,
 
 static void pci_disable_completion_timeout(struct phb *phb, struct pci_device *pd)
 {
-	uint32_t ecap;
-	uint32_t val;
+	uint32_t ecap, val;
+	uint16_t ecap_data;
 
 	/* PCIE capability required */
-	if (!pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false))
+	ecap_data = (uint16_t)pci_cap_data(pd, PCI_CFG_CAP_ID_EXP, false);
+	if (!pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false) ||
+	    (ecap_data & PCICAP_EXP_CAP_VERSION) <= 1)
 		return;
 
 	/* Check if it has capability to disable completion timeout */