Re: [PATCH 3/4] PCI: Add pci_pcie_cap2() check for PCIe feature capabilities >= v2

[Don Dutile <ddutile@redhat.com>]

On 06/01/2012 05:16 PM, Myron Stowe wrote:
> This patch resolves potential issues when accessing PCI Express
> capability structures.  The makeup of Express' capability structure
> varies substantially between v1 and v2:
>
>      PCI Express 1.0 and 1.1 base neither requires the endpoint to
>      implement the entire PCIe capability structure nor specifies
>      default values of registers that are not implemented by the
>      device.  Its capability version is 1.
>
>      PCIe 1.1 Capability Structure Expansion ECN, PCIe 2.0, 2.1, and
>      3.0 added additional registers to the structure and require all
>      registers to be either implemented or hardwired to 0.  Their PCIe
>      capability version is 2.
>
> Due to the differences in the capability structures, code dealing with
> capability features must be careful not to access the additional
> registers introduced with v2 unless the device is specifically known to
> be a v2 capable device.  Otherwise, attempts to access non-existant
> registers will occur.  This is a subtle issue that is hard to track down
> when it occurs (and it has - see commit 864d296cf94).
>
> To try and help mitigate such occurrances, this patch introduces
> pci_pcie_cap2() which is similar to pci_pcie_cap() but also checks
> that the PCIe capability version is>= 2.  pci_pcie_cap2() should be
> used for qualifying PCIe capability features introduced after v1.
>
> Suggested by Don Dutile.
>
Acked by him too!
... except one nit...

I would change the comment "is a PCIe v2 feature" to "is a PCIe cap v2 list feature"
so no one confuses spec version with a specific cap-struct version.

but I wouldn't hold up this patch for this nit.

> Signed-off-by: Myron Stowe<myron.stowe@redhat.com>
> ---
>
>   drivers/pci/pci.c        |   65 +++++++++++++++++++++++++++++++++++-----------
>   include/linux/pci_regs.h |    7 +++++
>   2 files changed, 57 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index ff0beb0..26933ff 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -279,6 +279,38 @@ int pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap)
>   }
>
>   /**
> + * pci_pcie_cap2 - query for devices' PCI_CAP_ID_EXP v2 capability structure
> + * @dev: PCI device to check
> + *
> + * Like pci_pcie_cap() but also checks that the PCIe capability version is
> + *>= 2.  Note that v1 capability structures could be sparse in that not
> + * all register fields were required.  v2 requires the entire structure to
> + * be present size wise, while still allowing for non-implemented registers
> + * to exist but they must be hardwired to 0.
> + *
> + * Due to the differences in the versions of capability structures, one
> + * must be careful not to try and access non-existant registers that may
> + * exist in early versions - v1 - of Express devices.
> + *
> + * Returns the offset of the PCIe capability structure as long as the
> + * capability version is>= 2; otherwise 0 is returned.
> + */
> +static int pci_pcie_cap2(struct pci_dev *dev)
> +{
> +	u16 flags;
> +	int pos;
> +
> +	pos = pci_pcie_cap(dev);
> +	if (pos) {
> +		pci_read_config_word(dev, pos + PCI_EXP_FLAGS,&flags);
> +		if ((flags&  PCI_EXP_FLAGS_VERS)<  2)
> +			pos = 0;
> +	}
> +
> +	return pos;
> +}
> +
> +/**
>    * pci_find_ext_capability - Find an extended capability
>    * @dev: PCI device to query
>    * @cap: capability code
> @@ -1984,7 +2016,7 @@ void pci_enable_ari(struct pci_dev *dev)
>   {
>   	int pos;
>   	u32 cap;
> -	u16 flags, ctrl;
> +	u16 ctrl;
>   	struct pci_dev *bridge;
>
>   	if (pcie_ari_disabled || !pci_is_pcie(dev) || dev->devfn)
> @@ -1998,13 +2030,9 @@ void pci_enable_ari(struct pci_dev *dev)
>   	if (!bridge)
>   		return;
>
> -	pos = pci_pcie_cap(bridge);
> -	if (!pos)
> -		return;
> -
>   	/* ARI is a PCIe v2 feature */
> -	pci_read_config_word(bridge, pos + PCI_EXP_FLAGS,&flags);
> -	if ((flags&  PCI_EXP_FLAGS_VERS)<  2)
> +	pos = pci_pcie_cap2(bridge);
> +	if (!pos)
>   		return;
>
>   	pci_read_config_dword(bridge, pos + PCI_EXP_DEVCAP2,&cap);
> @@ -2019,7 +2047,7 @@ void pci_enable_ari(struct pci_dev *dev)
>   }
>
>   /**
> - * pci_enable_ido - enable ID-based ordering on a device
> + * pci_enable_ido - enable ID-based Ordering on a device
>    * @dev: the PCI device
>    * @type: which types of IDO to enable
>    *
> @@ -2032,7 +2060,8 @@ void pci_enable_ido(struct pci_dev *dev, unsigned long type)
>   	int pos;
>   	u16 ctrl;
>
> -	pos = pci_pcie_cap(dev);
> +	/* ID-based Ordering is a PCIe v2 feature */
> +	pos = pci_pcie_cap2(dev);
>   	if (!pos)
>   		return;
>
> @@ -2055,7 +2084,8 @@ void pci_disable_ido(struct pci_dev *dev, unsigned long type)
>   	int pos;
>   	u16 ctrl;
>
> -	pos = pci_pcie_cap(dev);
> +	/* ID-based Ordering is a PCIe v2 feature */
> +	pos = pci_pcie_cap2(dev);
>   	if (!pos)
>   		return;
>
> @@ -2094,7 +2124,8 @@ int pci_enable_obff(struct pci_dev *dev, enum pci_obff_signal_type type)
>   	u16 ctrl;
>   	int ret;
>
> -	pos = pci_pcie_cap(dev);
> +	/* OBFF is a PCIe v2 feature */
> +	pos = pci_pcie_cap2(dev);
>   	if (!pos)
>   		return -ENOTSUPP;
>
> @@ -2144,7 +2175,8 @@ void pci_disable_obff(struct pci_dev *dev)
>   	int pos;
>   	u16 ctrl;
>
> -	pos = pci_pcie_cap(dev);
> +	/* OBFF is a PCIe v2 feature */
> +	pos = pci_pcie_cap2(dev);
>   	if (!pos)
>   		return;
>
> @@ -2166,7 +2198,8 @@ static bool pci_ltr_supported(struct pci_dev *dev)
>   	int pos;
>   	u32 cap;
>
> -	pos = pci_pcie_cap(dev);
> +	/* LTR is a PCIe v2 feature */
> +	pos = pci_pcie_cap2(dev);
>   	if (!pos)
>   		return false;
>
> @@ -2194,7 +2227,8 @@ int pci_enable_ltr(struct pci_dev *dev)
>   	if (!pci_ltr_supported(dev))
>   		return -ENOTSUPP;
>
> -	pos = pci_pcie_cap(dev);
> +	/* LTR is a PCIe v2 feature */
> +	pos = pci_pcie_cap2(dev);
>   	if (!pos)
>   		return -ENOTSUPP;
>
> @@ -2229,7 +2263,8 @@ void pci_disable_ltr(struct pci_dev *dev)
>   	if (!pci_ltr_supported(dev))
>   		return;
>
> -	pos = pci_pcie_cap(dev);
> +	/* LTR is a PCIe v2 feature */
> +	pos = pci_pcie_cap2(dev);
>   	if (!pos)
>   		return;
>
> diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
> index 4b608f5..bc1b54d 100644
> --- a/include/linux/pci_regs.h
> +++ b/include/linux/pci_regs.h
> @@ -507,6 +507,13 @@
>   #define PCI_EXP_RTSTA		32	/* Root Status */
>   #define PCI_EXP_RTSTA_PME	0x10000 /* PME status */
>   #define PCI_EXP_RTSTA_PENDING	0x20000 /* PME pending */
> +/*
> + * Note that the following PCI Express 'Capability Structure' registers
> + * were introduced with 'Capability Version' 0x2 (v2).  These registers
> + * do not exist, and thus should not be attempted to be accesses, by
> + * PCI Express v1 devices (see pci_pcie_cap2() for use in qualifying
> + * PCI Express v2 capability related features).
> + */
>   #define PCI_EXP_DEVCAP2		36	/* Device Capabilities 2 */
>   #define  PCI_EXP_DEVCAP2_ARI	0x20	/* Alternative Routing-ID */
>   #define  PCI_EXP_DEVCAP2_LTR	0x800	/* Latency tolerance reporting */
>