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

[Gavin Shan]

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 at 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