Re: [RFC PATCH v2 04/32] PCI: add PCIe capabilities access functions to hide differences among PCIe specs

[Don Dutile <ddutile@redhat.com>]

On 07/24/2012 12:31 PM, Jiang Liu wrote:
> From: Jiang Liu<jiang.liu@huawei.com>
>
> Introduce five configuration access functions for PCIe capabilities registers
> to hide differences among PCIe Base Spec versions.
>
> Function pci_pcie_capability_read_word/dword() stores the PCIe Capabilities
> register value by the passed parameter val. If related PCIe Capabilities
> register is not implemented on the PCIe device, the passed parameter val
> will be set 0.
>
> Function pci_pcie_capability_write_word/dowrd() writes the value to PCIe
> Capability register.
>
> Function pci_pcie_capability_reg_implemeneted() checks whether a Capabilities
> register is implemented by the PCIe device.
>
> Signed-off-by: Jiang Liu<liuj97@gmail.com>
> Signed-off-by: Yijing Wang<wangyijing@huawei.com>
> ---
>   drivers/pci/access.c     |  157 ++++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/pci.h      |    6 ++
>   include/linux/pci_regs.h |    2 +
>   3 files changed, 165 insertions(+)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index ba91a7e..59409e8 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -469,3 +469,160 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
>   	raw_spin_unlock_irqrestore(&pci_lock, flags);
>   }
>   EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
> +
> +static inline int pci_pcie_cap_version(const struct pci_dev *dev)
> +{
> +	return dev->pcie_flags_reg&  PCI_EXP_FLAGS_VERS;
> +}
> +
> +static inline bool pci_pcie_cap_has_devctl(const struct pci_dev *dev)
> +{
> +	return true;
> +}
> +
> +static inline bool pci_pcie_cap_has_lnkctl(const struct pci_dev *dev)
> +{
> +	int type = pci_pcie_type(dev);
> +
> +	return pci_pcie_cap_version(dev)>  1 ||
> +	       type == PCI_EXP_TYPE_ROOT_PORT ||
> +	       type == PCI_EXP_TYPE_ENDPOINT ||
> +	       type == PCI_EXP_TYPE_LEG_END;
> +}
> +
> +static inline bool pci_pcie_cap_has_sltctl(const struct pci_dev *dev)
> +{
> +	int type = pci_pcie_type(dev);
> +
> +	return pci_pcie_cap_version(dev)>  1 ||
> +	       type == PCI_EXP_TYPE_ROOT_PORT ||
> +	       (type == PCI_EXP_TYPE_DOWNSTREAM&&
> +		dev->pcie_flags_reg&  PCI_EXP_FLAGS_SLOT);
> +}
> +
> +static inline bool pci_pcie_cap_has_rtctl(const struct pci_dev *dev)
> +{
> +	int type = pci_pcie_type(dev);
> +
> +	return pci_pcie_cap_version(dev)>  1 ||
> +	       type == PCI_EXP_TYPE_ROOT_PORT ||
> +	       type == PCI_EXP_TYPE_RC_EC;
> +}
> +
> +bool pci_pcie_capability_reg_implemented(struct pci_dev *dev, int pos)
> +{
> +	if (!pci_is_pcie(dev))
> +		return false;
> +
> +	switch (pos) {
> +	case PCI_EXP_FLAGS_TYPE:
> +		return true;
> +	case PCI_EXP_DEVCAP:
> +	case PCI_EXP_DEVCTL:
> +	case PCI_EXP_DEVSTA:
> +		return pci_pcie_cap_has_devctl(dev);
> +	case PCI_EXP_LNKCAP:
> +	case PCI_EXP_LNKCTL:
> +	case PCI_EXP_LNKSTA:
> +		return pci_pcie_cap_has_lnkctl(dev);
> +	case PCI_EXP_SLTCAP:
> +	case PCI_EXP_SLTCTL:
> +	case PCI_EXP_SLTSTA:
> +		return pci_pcie_cap_has_sltctl(dev);
> +	case PCI_EXP_RTCTL:
> +	case PCI_EXP_RTCAP:
> +	case PCI_EXP_RTSTA:
> +		return pci_pcie_cap_has_rtctl(dev);
> +	case PCI_EXP_DEVCAP2:
> +	case PCI_EXP_DEVCTL2:
> +	case PCI_EXP_LNKCAP2:
> +	case PCI_EXP_LNKCTL2:
> +	case PCI_EXP_LNKSTA2:
> +		return pci_pcie_cap_version(dev)>  1;
> +	default:
> +		return false;
> +	}
> +}
> +EXPORT_SYMBOL(pci_pcie_capability_reg_implemented);
> +
> +/*
> + * Quotation from PCIe Base Spec 3.0:
> + * For Functions that do not implement the Slot Capabilities,
> + * Slot Status, and Slot Control registers, these spaces must
> + * be hardwired to 0b, with the exception of the Presence Detect
> + * State bit in the Slot Status register of Downstream Ports,
> + * which must be hardwired to 1b.
> + */
> +int pci_pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val)
> +{
> +	int ret = 0;
> +
> +	*val = 0;
> +	if (pos&  1)
> +		return -EINVAL;
> +
> +	if (pci_pcie_capability_reg_implemented(dev, pos)) {
> +		ret = pci_read_config_word(dev, pci_pcie_cap(dev) + pos, val);
> +		/*
> +		 * Reset *val to 0 if pci_read_config_word() fails, it may
> +		 * have been written as 0xFFFF if hardware error happens
> +		 * during pci_read_config_word().
> +		 */
> +		if (ret)
> +			*val = 0;
> +	} else if (pos == PCI_EXP_SLTSTA&&
> +		 pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) {
> +		*val = PCI_EXP_SLTSTA_PDS;
> +	}
Don't you want the above if check done 1st, and not the
pci_pcie_capability_reg_implemented(dev, pos) check ?
Isn't PCI_EXP_SLTCTL an implemented register, looking at this snippet?:
> +	switch (pos) {
         <snip>
> +	case PCI_EXP_SLTCAP:
> +	case PCI_EXP_SLTCTL:
> +	case PCI_EXP_SLTSTA:
> +		return pci_pcie_cap_has_sltctl(dev);
   and the above function is:

> +static inline bool pci_pcie_cap_has_sltctl(const struct pci_dev *dev)
> +{
> +	int type = pci_pcie_type(dev);
> +
> +	return pci_pcie_cap_version(dev)>  1 ||
> +	       type == PCI_EXP_TYPE_ROOT_PORT ||
> +	       (type == PCI_EXP_TYPE_DOWNSTREAM&&
> +		dev->pcie_flags_reg&  PCI_EXP_FLAGS_SLOT);
> +}

or is PCI_EXP_FLAGS_SLOT not set when it should be, then the first condition
fails, and this function forces the val to 1b ?
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(pci_pcie_capability_read_word);
> +
> +int pci_pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val)
> +{
> +	int ret = 0;
> +
> +	*val = 0;
> +	if (pos&  3)
> +		return -EINVAL;
> +
> +	if (pci_pcie_capability_reg_implemented(dev, pos)) {
> +		ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val);
> +		/*
> +		 * Reset *val to 0 if pci_read_config_dword() fails, it may
> +		 * have been written as 0xFFFFFFFF if hardware error happens
> +		 * during pci_read_config_dword().
> +		 */
> +		if (ret)
> +			*val = 0;
> +	} else if (pos == PCI_EXP_SLTCTL&&
> +		 pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) {
> +		*val = PCI_EXP_SLTSTA_PDS;
> +	}
ditto above...

> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(pci_pcie_capability_read_dword);
> +
> +int pci_pcie_capability_write_word(struct pci_dev *dev, int pos, u16 val)
> +{
> +	if (pos&  1)
> +		return -EINVAL;
> +	else if (!pci_pcie_capability_reg_implemented(dev, pos))
> +		return 0;
> +
> +	return pci_write_config_word(dev, pci_pcie_cap(dev) + pos, val);
> +}
> +EXPORT_SYMBOL(pci_pcie_capability_write_word);
> +
> +int pci_pcie_capability_write_dword(struct pci_dev *dev, int pos, u32 val)
> +{
> +	if (pos&  3)
> +		return -EINVAL;
> +	else if (!pci_pcie_capability_reg_implemented(dev, pos))
> +		return 0;
> +
> +	return pci_write_config_dword(dev, pci_pcie_cap(dev) + pos, val);
> +}
> +EXPORT_SYMBOL(pci_pcie_capability_write_dword);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 9807da5..a9b7605 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -816,6 +816,12 @@ static inline int pci_write_config_dword(const struct pci_dev *dev, int where,
>   	return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
>   }
>
> +int pci_pcie_capability_read_word(struct pci_dev *dev, int where, u16 *val);
> +int pci_pcie_capability_read_dword(struct pci_dev *dev, int where, u32 *val);
> +int pci_pcie_capability_write_word(struct pci_dev *dev, int where, u16 val);
> +int pci_pcie_capability_write_dword(struct pci_dev *dev, int where, u32 val);
> +bool pci_pcie_capability_reg_implemented(struct pci_dev *dev, int where);
> +
>   /* user-space driven config access */
>   int pci_user_read_config_byte(struct pci_dev *dev, int where, u8 *val);
>   int pci_user_read_config_word(struct pci_dev *dev, int where, u16 *val);
> diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
> index 53274bf..5300fdf 100644
> --- a/include/linux/pci_regs.h
> +++ b/include/linux/pci_regs.h
> @@ -543,7 +543,9 @@
>   #define  PCI_EXP_OBFF_MSGB_EN	0x4000	/* OBFF enable with Message type B */
>   #define  PCI_EXP_OBFF_WAKE_EN	0x6000	/* OBFF using WAKE# signaling */
>   #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2	44	/* v2 endpoints end here */
> +#define PCI_EXP_LNKCAP2		44	/* Link Capabilities 2 */
>   #define PCI_EXP_LNKCTL2		48	/* Link Control 2 */
> +#define PCI_EXP_LNKSTA2		50	/* Link Status 2 */
>   #define PCI_EXP_SLTCTL2		56	/* Slot Control 2 */
>
>   /* Extended Capabilities (PCI-X 2.0 and Express) */