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

[Benjamin Herrenschmidt]

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.

>  	/* 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 */