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

[Jiang Liu <liuj97@gmail.com>]

On 07/25/2012 05:12 AM, Don Dutile wrote:
> On 07/24/2012 12:31 PM, Jiang Liu wrote:
>> +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 ?
Hi Don,
	Yes, that's the purpose. PCIe spec v2/v3 defines that hardware should return
a value with bit PCI_EXP_SLTSTA_PDS set if PCI_EXP_SLTSTA register is not implemented.
So for PCIe v1 hardwares, we try to behave in the same way as v2/v3.
	Regards!
	Gerry