Patchwork [RFC,05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences

login
register
mail settings
Submitter Jiang Liu
Date July 10, 2012, 3:54 p.m.
Message ID <1341935655-5381-6-git-send-email-jiang.liu@huawei.com>
Download mbox | patch
Permalink /patch/170233/
State Changes Requested
Headers show

Comments

Jiang Liu - July 10, 2012, 3:54 p.m.
From: Jiang Liu <jiang.liu@huawei.com>

Introduce four configuration access functions for PCIe capabilities to
hide difference among PCIe Base Spec versions. With these functions,
we can remove callers responsible for using pci_pcie_cap_has_*().

pci_pcie_cap_read_word/dword() functions will store the pcie cap register
value by passed parameter val,if related pcie cap register is not implemented
on the pcie device, the passed parameter val will be set 0 and return -EINVAL.

pci_pcie_capability_write_word/dowrd() functions will write the value to pcie
cap registers,if related pcie cap register is not implemented on the pcie
device, it will return -EINVAL.

Signed-off-by: Jiang Liu <liuj97@gmail.com>
Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/access.c     |   88 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h      |   10 ++++++
 include/linux/pci_regs.h |   19 ++++++++--
 3 files changed, 115 insertions(+), 2 deletions(-)
Bjorn Helgaas - July 10, 2012, 6:35 p.m.
On Tue, Jul 10, 2012 at 9:54 AM, Jiang Liu <liuj97@gmail.com> wrote:
> From: Jiang Liu <jiang.liu@huawei.com>
>
> Introduce four configuration access functions for PCIe capabilities to
> hide difference among PCIe Base Spec versions. With these functions,
> we can remove callers responsible for using pci_pcie_cap_has_*().
>
> pci_pcie_cap_read_word/dword() functions will store the pcie cap register
> value by passed parameter val,if related pcie cap register is not implemented
> on the pcie device, the passed parameter val will be set 0 and return -EINVAL.
>
> pci_pcie_capability_write_word/dowrd() functions will write the value to pcie
> cap registers,if related pcie cap register is not implemented on the pcie
> device, it will return -EINVAL.
>
> Signed-off-by: Jiang Liu <liuj97@gmail.com>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/pci/access.c     |   88 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h      |   10 ++++++
>  include/linux/pci_regs.h |   19 ++++++++--
>  3 files changed, 115 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index ba91a7e..80ae022 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -469,3 +469,91 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
>         raw_spin_unlock_irqrestore(&pci_lock, flags);
>  }
>  EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
> +
> +static int
> +pci_pcie_cap_get_offset(struct pci_dev *dev, int where, size_t sz)
> +{
> +       bool valid;
> +
> +       if (!pci_is_pcie(dev))
> +               return -EINVAL;
> +       if (where & (sz - 1))
> +               return -EINVAL;
> +
> +       if (where < 0)
> +               valid = false;
> +       else if (where < PCI_EXP_DEVCAP)
> +               valid = true;
> +       else if (where < PCI_EXP_LNKCAP)
> +               valid = pci_pcie_cap_has_devctl(dev);
> +       else if (where < PCI_EXP_SLTCAP)
> +               valid = pci_pcie_cap_has_lnkctl(dev);
> +       else if (where < PCI_EXP_RTCTL)
> +               valid = pci_pcie_cap_has_sltctl(dev);
> +       else if (where < PCI_EXP_DEVCAP2)
> +               valid = pci_pcie_cap_has_rtctl(dev);
> +       else if (where < PCI_EXP_CAP2_SIZE)
> +               valid = pci_pcie_cap_has_cap2(dev);
> +       else
> +               valid = false;
> +
> +       return valid ? where + pci_pcie_cap(dev) : -EINVAL;
> +}
> +
> +int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp)
> +{
> +       *valp = 0;
> +       where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));

This is a really slick factorization; I like it much better than my
proposal.  I would like it even *better* if it read something like
this:

    bool implemented;

    *valp = 0;
    if (!pci_is_pcie(dev) || where & 1)
        return -EINVAL;

    implemented = pci_pcie_cap_implemented(dev, where);
    if (implemented)
        return pci_read_config_word(dev, pci_pcie_cap(dev) + where, valp);

    if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA ...

because I think it's useful to have the "pos + where" visual pattern
in the pci_read_config_word() arguments.

> +       if (where >= 0)
> +               return pci_read_config_word(dev, where, valp);
> +
> +       if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA &&
> +           pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
> +               *valp = PCI_EXP_SLTSTA_PDS;

I think we should be returning success in this case (SLTSTA for
downstream port).  In fact, I think we should return success even when
we're emulating the read of an unimplemented register from a v1
capability.  The caller should not be aware at all that there is a
difference between v1 and v2 capabilities.

I'd put the spec reference here rather than in read_dword(), since
SLTSTA is a u16 and this is the natural way to read it.  Then maybe a
short comment in read_dword() below.

> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL(pci_pcie_cap_read_word);
> +
> +int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp)
> +{
> +       *valp = 0;
> +       where = pci_pcie_cap_get_offset(dev, where, sizeof(u32));
> +       if (where >= 0)
> +               return pci_read_config_dword(dev, where, valp);
> +
> +       /*
> +        * 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.
> +        */
> +       if (pci_is_pcie(dev) && where == PCI_EXP_SLTCTL &&
> +           pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
> +               *valp = PCI_EXP_SLTSTA_PDS << 16;

Return success here, too.

> +
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL(pci_pcie_cap_read_dword);
> +
> +int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val)
> +{
> +       where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
> +       if (where >= 0)
> +               return pci_write_config_word(dev, where, val);
> +
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL(pci_pcie_cap_write_word);
> +
> +int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32 val)
> +{
> +       where = pci_pcie_cap_get_offset(dev, where, sizeof(u32));
> +       if (where >= 0)
> +               return pci_write_config_dword(dev, where, val);
> +
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL(pci_pcie_cap_write_dword);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 346b2d9..78767b2 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1703,6 +1703,11 @@ static inline bool pci_pcie_cap_has_rtctl(const struct pci_dev *pdev)
>                type == PCI_EXP_TYPE_RC_EC;
>  }
>
> +extern int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp);
> +extern int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp);
> +extern int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val);
> +extern int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32 val);

You don't need the "extern" here (and I think you'll probably remove
these altogether, see below).

> +
>  void pci_request_acs(void);
>  bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
>  bool pci_acs_path_enabled(struct pci_dev *start,
> @@ -1843,5 +1848,10 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>   */
>  struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
>
> +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);

There's some confusion here: pci_pcie_cap_* versus
pci_pcie_capability_*.  I think you only need one set, and I prefer
pci_pcie_capability_* to follow the example of
pci_bus_find_capability().

> +
>  #endif /* __KERNEL__ */
>  #endif /* LINUX_PCI_H */
> diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
> index 53274bf..ac60e22 100644
> --- a/include/linux/pci_regs.h
> +++ b/include/linux/pci_regs.h
> @@ -542,9 +542,24 @@
>  #define  PCI_EXP_OBFF_MSGA_EN  0x2000  /* OBFF enable with Message type A */
>  #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_DEVSTA2                42      /* Device Status 2 */
> +#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_SLTCTL2                56      /* Slot Control 2 */
> +#define  PCI_EXP_LNKCTL2_TLS   0x0f    /* Target Link Speed */
> +#define  PCI_EXP_LNKCTL2_EC    0x10    /* Enter Compliance */
> +#define  PCI_EXP_LNKCTL2_HASD  0x20    /* Hardware Autonomous Speed Disable */
> +#define  PCI_EXP_LNKCTL2_SD    0x40    /* Selectable De-emphasis */
> +#define  PCI_EXP_LNKCTL2_TM    0x380   /* Transmit Margin */
> +#define  PCI_EXP_LNKCTL2_EMC   0x400   /* Enter Modified Compliance */
> +#define  PCI_EXP_LNKCTL2_CS    0x800   /* Compliance SOS */
> +#define  PCI_EXP_LNKCTL2_CD    0x1000  /* Compliance De-emphasis */
> +#define PCI_EXP_LNKSTA2                50      /* Link Status 2 */
> +#define  PCI_EXP_LNKSTA2_CDL   0x01    /* Current De-emphasis Level */
> +#define PCI_EXP_SLTCAP2                52      /* Slot Capabilities 2 */
> +#define PCI_EXP_SLTCTL2                56      /* Slot Control 2*/
> +#define PCI_EXP_SLTSTA2                58      /* Slot Status 2*/
> +#define        PCI_EXP_CAP2_SIZE       60

Most of these changes look unrelated to the current patch.  They
should be moved to a patch that uses the symbols you're adding.

>
>  /* Extended Capabilities (PCI-X 2.0 and Express) */
>  #define PCI_EXT_CAP_ID(header)         (header & 0x0000ffff)
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiang Liu - July 11, 2012, 3:07 a.m.
On 2012-7-11 2:35, Bjorn Helgaas wrote:
>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>> index ba91a7e..80ae022 100644
>> --- a/drivers/pci/access.c
>> +++ b/drivers/pci/access.c
>> @@ -469,3 +469,91 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
>>         raw_spin_unlock_irqrestore(&pci_lock, flags);
>>  }
>>  EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
>> +
>> +static int
>> +pci_pcie_cap_get_offset(struct pci_dev *dev, int where, size_t sz)
>> +{
>> +       bool valid;
>> +
>> +       if (!pci_is_pcie(dev))
>> +               return -EINVAL;
>> +       if (where & (sz - 1))
>> +               return -EINVAL;
>> +
>> +       if (where < 0)
>> +               valid = false;
>> +       else if (where < PCI_EXP_DEVCAP)
>> +               valid = true;
>> +       else if (where < PCI_EXP_LNKCAP)
>> +               valid = pci_pcie_cap_has_devctl(dev);
>> +       else if (where < PCI_EXP_SLTCAP)
>> +               valid = pci_pcie_cap_has_lnkctl(dev);
>> +       else if (where < PCI_EXP_RTCTL)
>> +               valid = pci_pcie_cap_has_sltctl(dev);
>> +       else if (where < PCI_EXP_DEVCAP2)
>> +               valid = pci_pcie_cap_has_rtctl(dev);
>> +       else if (where < PCI_EXP_CAP2_SIZE)
>> +               valid = pci_pcie_cap_has_cap2(dev);
>> +       else
>> +               valid = false;
>> +
>> +       return valid ? where + pci_pcie_cap(dev) : -EINVAL;
>> +}
>> +
>> +int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp)
>> +{
>> +       *valp = 0;
>> +       where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
> 
> This is a really slick factorization; I like it much better than my
> proposal.  I would like it even *better* if it read something like
> this:
> 
>     bool implemented;
> 
>     *valp = 0;
>     if (!pci_is_pcie(dev) || where & 1)
>         return -EINVAL;
> 
>     implemented = pci_pcie_cap_implemented(dev, where);
>     if (implemented)
>         return pci_read_config_word(dev, pci_pcie_cap(dev) + where, valp);
> 
>     if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA ...
> 
> because I think it's useful to have the "pos + where" visual pattern
> in the pci_read_config_word() arguments.
Sure, for better readability.

> 
>> +       if (where >= 0)
>> +               return pci_read_config_word(dev, where, valp);
>> +
>> +       if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA &&
>> +           pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
>> +               *valp = PCI_EXP_SLTSTA_PDS;
> 
> I think we should be returning success in this case (SLTSTA for
> downstream port).  In fact, I think we should return success even when
> we're emulating the read of an unimplemented register from a v1
> capability.  The caller should not be aware at all that there is a
> difference between v1 and v2 capabilities.
> 
> I'd put the spec reference here rather than in read_dword(), since
> SLTSTA is a u16 and this is the natural way to read it.  Then maybe a
> short comment in read_dword() below.
Good point. Return success when reading unimplemented registeres, that
may simplify code. For we still should return -EINVAL when writing
unimplemented registers, right?

>> +EXPORT_SYMBOL(pci_pcie_cap_write_dword);
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 346b2d9..78767b2 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1703,6 +1703,11 @@ static inline bool pci_pcie_cap_has_rtctl(const struct pci_dev *pdev)
>>                type == PCI_EXP_TYPE_RC_EC;
>>  }
>>
>> +extern int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp);
>> +extern int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp);
>> +extern int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val);
>> +extern int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32 val);
> 
> You don't need the "extern" here (and I think you'll probably remove
> these altogether, see below).
> 
>> +
>>  void pci_request_acs(void);
>>  bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
>>  bool pci_acs_path_enabled(struct pci_dev *start,
>> @@ -1843,5 +1848,10 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>>   */
>>  struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
>>
>> +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);
> 
> There's some confusion here: pci_pcie_cap_* versus
> pci_pcie_capability_*.  I think you only need one set, and I prefer
> pci_pcie_capability_* to follow the example of
> pci_bus_find_capability().
The above confusion was caused by a dirty merge.

> 
>> +
>>  #endif /* __KERNEL__ */
>>  #endif /* LINUX_PCI_H */
>> diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
>> index 53274bf..ac60e22 100644
>> --- a/include/linux/pci_regs.h
>> +++ b/include/linux/pci_regs.h
>> @@ -542,9 +542,24 @@
>>  #define  PCI_EXP_OBFF_MSGA_EN  0x2000  /* OBFF enable with Message type A */
>>  #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_DEVSTA2                42      /* Device Status 2 */
>> +#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_SLTCTL2                56      /* Slot Control 2 */
>> +#define  PCI_EXP_LNKCTL2_TLS   0x0f    /* Target Link Speed */
>> +#define  PCI_EXP_LNKCTL2_EC    0x10    /* Enter Compliance */
>> +#define  PCI_EXP_LNKCTL2_HASD  0x20    /* Hardware Autonomous Speed Disable */
>> +#define  PCI_EXP_LNKCTL2_SD    0x40    /* Selectable De-emphasis */
>> +#define  PCI_EXP_LNKCTL2_TM    0x380   /* Transmit Margin */
>> +#define  PCI_EXP_LNKCTL2_EMC   0x400   /* Enter Modified Compliance */
>> +#define  PCI_EXP_LNKCTL2_CS    0x800   /* Compliance SOS */
>> +#define  PCI_EXP_LNKCTL2_CD    0x1000  /* Compliance De-emphasis */
>> +#define PCI_EXP_LNKSTA2                50      /* Link Status 2 */
>> +#define  PCI_EXP_LNKSTA2_CDL   0x01    /* Current De-emphasis Level */
>> +#define PCI_EXP_SLTCAP2                52      /* Slot Capabilities 2 */
>> +#define PCI_EXP_SLTCTL2                56      /* Slot Control 2*/
>> +#define PCI_EXP_SLTSTA2                58      /* Slot Status 2*/
>> +#define        PCI_EXP_CAP2_SIZE       60
> 
> Most of these changes look unrelated to the current patch.  They
> should be moved to a patch that uses the symbols you're adding.
Good point, create on demand:)

Thanks!
Gerry

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - July 11, 2012, 3:40 a.m.
On Tue, Jul 10, 2012 at 9:07 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
> On 2012-7-11 2:35, Bjorn Helgaas wrote:
>>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>>> index ba91a7e..80ae022 100644
>>> --- a/drivers/pci/access.c
>>> +++ b/drivers/pci/access.c
>>> @@ -469,3 +469,91 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
>>>         raw_spin_unlock_irqrestore(&pci_lock, flags);
>>>  }
>>>  EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
>>> +
>>> +static int
>>> +pci_pcie_cap_get_offset(struct pci_dev *dev, int where, size_t sz)
>>> +{
>>> +       bool valid;
>>> +
>>> +       if (!pci_is_pcie(dev))
>>> +               return -EINVAL;
>>> +       if (where & (sz - 1))
>>> +               return -EINVAL;
>>> +
>>> +       if (where < 0)
>>> +               valid = false;
>>> +       else if (where < PCI_EXP_DEVCAP)
>>> +               valid = true;
>>> +       else if (where < PCI_EXP_LNKCAP)
>>> +               valid = pci_pcie_cap_has_devctl(dev);
>>> +       else if (where < PCI_EXP_SLTCAP)
>>> +               valid = pci_pcie_cap_has_lnkctl(dev);
>>> +       else if (where < PCI_EXP_RTCTL)
>>> +               valid = pci_pcie_cap_has_sltctl(dev);
>>> +       else if (where < PCI_EXP_DEVCAP2)
>>> +               valid = pci_pcie_cap_has_rtctl(dev);
>>> +       else if (where < PCI_EXP_CAP2_SIZE)
>>> +               valid = pci_pcie_cap_has_cap2(dev);
>>> +       else
>>> +               valid = false;
>>> +
>>> +       return valid ? where + pci_pcie_cap(dev) : -EINVAL;
>>> +}
>>> +
>>> +int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp)
>>> +{
>>> +       *valp = 0;
>>> +       where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
>>
>> This is a really slick factorization; I like it much better than my
>> proposal.  I would like it even *better* if it read something like
>> this:
>>
>>     bool implemented;
>>
>>     *valp = 0;
>>     if (!pci_is_pcie(dev) || where & 1)
>>         return -EINVAL;
>>
>>     implemented = pci_pcie_cap_implemented(dev, where);
>>     if (implemented)
>>         return pci_read_config_word(dev, pci_pcie_cap(dev) + where, valp);
>>
>>     if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA ...
>>
>> because I think it's useful to have the "pos + where" visual pattern
>> in the pci_read_config_word() arguments.
> Sure, for better readability.
>
>>
>>> +       if (where >= 0)
>>> +               return pci_read_config_word(dev, where, valp);
>>> +
>>> +       if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA &&
>>> +           pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
>>> +               *valp = PCI_EXP_SLTSTA_PDS;
>>
>> I think we should be returning success in this case (SLTSTA for
>> downstream port).  In fact, I think we should return success even when
>> we're emulating the read of an unimplemented register from a v1
>> capability.  The caller should not be aware at all that there is a
>> difference between v1 and v2 capabilities.
>>
>> I'd put the spec reference here rather than in read_dword(), since
>> SLTSTA is a u16 and this is the natural way to read it.  Then maybe a
>> short comment in read_dword() below.
> Good point. Return success when reading unimplemented registeres, that
> may simplify code. For we still should return -EINVAL when writing
> unimplemented registers, right?

Yeah, I guess it's OK to return -EINVAL when *writing* to an
unimplemented register.  Hopefully the caller is structured such that
we don't even try to write in that case.  It'd be interesting to audit
the callers and explore that, but I haven't done that.

>>> +EXPORT_SYMBOL(pci_pcie_cap_write_dword);
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index 346b2d9..78767b2 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -1703,6 +1703,11 @@ static inline bool pci_pcie_cap_has_rtctl(const struct pci_dev *pdev)
>>>                type == PCI_EXP_TYPE_RC_EC;
>>>  }
>>>
>>> +extern int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp);
>>> +extern int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp);
>>> +extern int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val);
>>> +extern int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32 val);
>>
>> You don't need the "extern" here (and I think you'll probably remove
>> these altogether, see below).
>>
>>> +
>>>  void pci_request_acs(void);
>>>  bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
>>>  bool pci_acs_path_enabled(struct pci_dev *start,
>>> @@ -1843,5 +1848,10 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>>>   */
>>>  struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
>>>
>>> +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);
>>
>> There's some confusion here: pci_pcie_cap_* versus
>> pci_pcie_capability_*.  I think you only need one set, and I prefer
>> pci_pcie_capability_* to follow the example of
>> pci_bus_find_capability().
> The above confusion was caused by a dirty merge.
>
>>
>>> +
>>>  #endif /* __KERNEL__ */
>>>  #endif /* LINUX_PCI_H */
>>> diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
>>> index 53274bf..ac60e22 100644
>>> --- a/include/linux/pci_regs.h
>>> +++ b/include/linux/pci_regs.h
>>> @@ -542,9 +542,24 @@
>>>  #define  PCI_EXP_OBFF_MSGA_EN  0x2000  /* OBFF enable with Message type A */
>>>  #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_DEVSTA2                42      /* Device Status 2 */
>>> +#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_SLTCTL2                56      /* Slot Control 2 */
>>> +#define  PCI_EXP_LNKCTL2_TLS   0x0f    /* Target Link Speed */
>>> +#define  PCI_EXP_LNKCTL2_EC    0x10    /* Enter Compliance */
>>> +#define  PCI_EXP_LNKCTL2_HASD  0x20    /* Hardware Autonomous Speed Disable */
>>> +#define  PCI_EXP_LNKCTL2_SD    0x40    /* Selectable De-emphasis */
>>> +#define  PCI_EXP_LNKCTL2_TM    0x380   /* Transmit Margin */
>>> +#define  PCI_EXP_LNKCTL2_EMC   0x400   /* Enter Modified Compliance */
>>> +#define  PCI_EXP_LNKCTL2_CS    0x800   /* Compliance SOS */
>>> +#define  PCI_EXP_LNKCTL2_CD    0x1000  /* Compliance De-emphasis */
>>> +#define PCI_EXP_LNKSTA2                50      /* Link Status 2 */
>>> +#define  PCI_EXP_LNKSTA2_CDL   0x01    /* Current De-emphasis Level */
>>> +#define PCI_EXP_SLTCAP2                52      /* Slot Capabilities 2 */
>>> +#define PCI_EXP_SLTCTL2                56      /* Slot Control 2*/
>>> +#define PCI_EXP_SLTSTA2                58      /* Slot Status 2*/
>>> +#define        PCI_EXP_CAP2_SIZE       60
>>
>> Most of these changes look unrelated to the current patch.  They
>> should be moved to a patch that uses the symbols you're adding.
> Good point, create on demand:)
>
> Thanks!
> Gerry
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiang Liu - July 11, 2012, 6:40 a.m.
On 2012-7-11 11:40, Bjorn Helgaas wrote:

>> Good point. Return success when reading unimplemented registeres, that
>> may simplify code. For we still should return -EINVAL when writing
>> unimplemented registers, right?
> 
> Yeah, I guess it's OK to return -EINVAL when *writing* to an
> unimplemented register.  Hopefully the caller is structured such that
> we don't even try to write in that case.  It'd be interesting to audit
> the callers and explore that, but I haven't done that.
Hi Bjorn,
	Seems it would be better to return error code for unimplemented
registers, otherwise following code will becomes more complex. A special
error code for unimplemented registers, such as -EIO?

static void rtl_disable_clock_request(struct pci_dev *pdev)
{
        u16 ctl;

        if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl)) {
                ctl &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
                pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl);
        }
}

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - July 11, 2012, 5:52 p.m.
On Wed, Jul 11, 2012 at 12:40 AM, Jiang Liu <jiang.liu@huawei.com> wrote:
> On 2012-7-11 11:40, Bjorn Helgaas wrote:
>
>>> Good point. Return success when reading unimplemented registeres, that
>>> may simplify code. For we still should return -EINVAL when writing
>>> unimplemented registers, right?
>>
>> Yeah, I guess it's OK to return -EINVAL when *writing* to an
>> unimplemented register.  Hopefully the caller is structured such that
>> we don't even try to write in that case.  It'd be interesting to audit
>> the callers and explore that, but I haven't done that.
> Hi Bjorn,
>         Seems it would be better to return error code for unimplemented
> registers, otherwise following code will becomes more complex. A special
> error code for unimplemented registers, such as -EIO?

I think you're asking about returning error for *reads* of
unimplemented registers?  I guess I still think it's OK to completely
hide the v1 nastiness inside these accessors, and return success with
a zero value when reading.  Having several different error returns
seems like overkill for this case.  Nobody wants to distinguish
between different reasons for failure.

I'm actually not sure that it's worth returning an error even when
*writing* an unimplemented register.  What if we return success and
just drop the write?

Maybe these should even be void functions.  It feels like the only
real use of the return value is to detect programmer error, and I
don't think that's very effective.  If we remove the return values,
people will have to focus on the *data*, which seems more important
anyway.

> static void rtl_disable_clock_request(struct pci_dev *pdev)
> {
>         u16 ctl;
>
>         if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl)) {
>                 ctl &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
>                 pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl);
>         }
> }

I would write that as:

    if (!pci_is_pcie(pdev)
        return;

    pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl);
    if (ctl & PCI_EXP_LNKCTL_CLKREQ_EN)
        pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl &
~PCI_EXP_LNKCTL_CLKREQ_EN);

which does the right thing regardless of what we do for return values,
and saves a config write in the case where LNKCTL is implemented and
CLKREQ_EN is already cleared.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiang Liu - July 12, 2012, 2:56 a.m.
On 2012-7-12 1:52, Bjorn Helgaas wrote:
>> Hi Bjorn,
>>         Seems it would be better to return error code for unimplemented
>> registers, otherwise following code will becomes more complex. A special
>> error code for unimplemented registers, such as -EIO?
> 
> I think you're asking about returning error for *reads* of
> unimplemented registers?  I guess I still think it's OK to completely
> hide the v1 nastiness inside these accessors, and return success with
> a zero value when reading.  Having several different error returns
> seems like overkill for this case.  Nobody wants to distinguish
> between different reasons for failure.
> 
> I'm actually not sure that it's worth returning an error even when
> *writing* an unimplemented register.  What if we return success and
> just drop the write?
> 
> Maybe these should even be void functions.  It feels like the only
> real use of the return value is to detect programmer error, and I
> don't think that's very effective.  If we remove the return values,
> people will have to focus on the *data*, which seems more important
> anyway.
Hi Bjorn,
	It's a little risk to change these PCIe capabilities access
functions as void. On some platform with hardware error detecting/correcting
capabilities, such as EEH on Power, it would be better to return
error code if hardware error happens during accessing configuration registers.
	As I know, coming Intel Xeon processor may provide PCIe hardware
error detecting capability similar to EEH on power.

>> static void rtl_disable_clock_request(struct pci_dev *pdev)
>> {
>>         u16 ctl;
>>
>>         if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl)) {
>>                 ctl &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
>>                 pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl);
>>         }
>> }
> 
> I would write that as:
> 
>     if (!pci_is_pcie(pdev)
>         return;
> 
>     pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl);
>     if (ctl & PCI_EXP_LNKCTL_CLKREQ_EN)
>         pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl &
> ~PCI_EXP_LNKCTL_CLKREQ_EN);
> 
> which does the right thing regardless of what we do for return values,
> and saves a config write in the case where LNKCTL is implemented and
> CLKREQ_EN is already cleared.
When clearing a flag, we could do that. But if we are trying to set a
flag, it would be better to make sure the target register does exist.

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - July 12, 2012, 8:49 p.m.
On Wed, Jul 11, 2012 at 8:56 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
> On 2012-7-12 1:52, Bjorn Helgaas wrote:
>>> Hi Bjorn,
>>>         Seems it would be better to return error code for unimplemented
>>> registers, otherwise following code will becomes more complex. A special
>>> error code for unimplemented registers, such as -EIO?
>>
>> I think you're asking about returning error for *reads* of
>> unimplemented registers?  I guess I still think it's OK to completely
>> hide the v1 nastiness inside these accessors, and return success with
>> a zero value when reading.  Having several different error returns
>> seems like overkill for this case.  Nobody wants to distinguish
>> between different reasons for failure.
>>
>> I'm actually not sure that it's worth returning an error even when
>> *writing* an unimplemented register.  What if we return success and
>> just drop the write?
>>
>> Maybe these should even be void functions.  It feels like the only
>> real use of the return value is to detect programmer error, and I
>> don't think that's very effective.  If we remove the return values,
>> people will have to focus on the *data*, which seems more important
>> anyway.
> Hi Bjorn,
>         It's a little risk to change these PCIe capabilities access
> functions as void. On some platform with hardware error detecting/correcting
> capabilities, such as EEH on Power, it would be better to return
> error code if hardware error happens during accessing configuration registers.
>         As I know, coming Intel Xeon processor may provide PCIe hardware
> error detecting capability similar to EEH on power.

I guess I'm playing devil's advocate here.  As a general rule, people
don't check the return value of pci_read_config_*() or
pci_write_config_*().  Unless you change them all, most callers of
pci_pcie_capability_read_*() and _write_*() won't check the returns
either.  So I'm not sure return values are an effective way to detect
those hardware errors.

How do these EEH errors get detected or reported today?  Do the
drivers check every config access for success?  Adding those checks
and figuring out how to handle errors at every possible point doesn't
seem like a recipe for success.

>>> static void rtl_disable_clock_request(struct pci_dev *pdev)
>>> {
>>>         u16 ctl;
>>>
>>>         if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl)) {
>>>                 ctl &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
>>>                 pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl);
>>>         }
>>> }
>>
>> I would write that as:
>>
>>     if (!pci_is_pcie(pdev)
>>         return;
>>
>>     pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl);
>>     if (ctl & PCI_EXP_LNKCTL_CLKREQ_EN)
>>         pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl &
>> ~PCI_EXP_LNKCTL_CLKREQ_EN);
>>
>> which does the right thing regardless of what we do for return values,
>> and saves a config write in the case where LNKCTL is implemented and
>> CLKREQ_EN is already cleared.
> When clearing a flag, we could do that. But if we are trying to set a
> flag, it would be better to make sure the target register does exist.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiang Liu - July 15, 2012, 4:47 p.m.
On 07/13/2012 04:49 AM, Bjorn Helgaas wrote:
>> Hi Bjorn,
>>         It's a little risk to change these PCIe capabilities access
>> functions as void. On some platform with hardware error detecting/correcting
>> capabilities, such as EEH on Power, it would be better to return
>> error code if hardware error happens during accessing configuration registers.
>>         As I know, coming Intel Xeon processor may provide PCIe hardware
>> error detecting capability similar to EEH on power.
> 
> I guess I'm playing devil's advocate here.  As a general rule, people
> don't check the return value of pci_read_config_*() or
> pci_write_config_*().  Unless you change them all, most callers of
> pci_pcie_capability_read_*() and _write_*() won't check the returns
> either.  So I'm not sure return values are an effective way to detect
> those hardware errors.
> 
> How do these EEH errors get detected or reported today?  Do the
> drivers check every config access for success?  Adding those checks
> and figuring out how to handle errors at every possible point doesn't
> seem like a recipe for success.

Hi Bjorn,
	Sorry for later reply, on travel these days.
	Yeah, it's true that most driver doesn't check return values of configuration
access functions, but there are still some drivers which do check return value of
pci_read_config_xxx(). For example, pciehp driver checks return value of CFG access
functions.

	It's not realistic to enhance all drivers, but we may focus on a small set of
drivers for hardwares on specific high-end servers. For RAS features, we can never provide
perfect solutions, so we prefer some improvements. After all a small improvement is still
an improvement:)

	I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know that the OS
may query firmware whether there's some hardware faults if pci_cfg_read_xxx() returns
all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error code to
pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report hardware faults
like SAL on IA64.

	So how about keeping consistence with pci_cfg_read_xxx() and pci_user_cfg_read_xxx()?
	Thanks!
	Gerry

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - July 16, 2012, 5:29 p.m.
On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu <liuj97@gmail.com> wrote:
> On 07/13/2012 04:49 AM, Bjorn Helgaas wrote:
>>> Hi Bjorn,
>>>         It's a little risk to change these PCIe capabilities access
>>> functions as void. On some platform with hardware error detecting/correcting
>>> capabilities, such as EEH on Power, it would be better to return
>>> error code if hardware error happens during accessing configuration registers.
>>>         As I know, coming Intel Xeon processor may provide PCIe hardware
>>> error detecting capability similar to EEH on power.
>>
>> I guess I'm playing devil's advocate here.  As a general rule, people
>> don't check the return value of pci_read_config_*() or
>> pci_write_config_*().  Unless you change them all, most callers of
>> pci_pcie_capability_read_*() and _write_*() won't check the returns
>> either.  So I'm not sure return values are an effective way to detect
>> those hardware errors.
>>
>> How do these EEH errors get detected or reported today?  Do the
>> drivers check every config access for success?  Adding those checks
>> and figuring out how to handle errors at every possible point doesn't
>> seem like a recipe for success.
>
> Hi Bjorn,
>         Sorry for later reply, on travel these days.
>         Yeah, it's true that most driver doesn't check return values of configuration
> access functions, but there are still some drivers which do check return value of
> pci_read_config_xxx(). For example, pciehp driver checks return value of CFG access
> functions.
>
>         It's not realistic to enhance all drivers, but we may focus on a small set of
> drivers for hardwares on specific high-end servers. For RAS features, we can never provide
> perfect solutions, so we prefer some improvements. After all a small improvement is still
> an improvement:)
>
>         I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know that the OS
> may query firmware whether there's some hardware faults if pci_cfg_read_xxx() returns
> all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error code to
> pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report hardware faults
> like SAL on IA64.
>
>         So how about keeping consistence with pci_cfg_read_xxx() and pci_user_cfg_read_xxx()?

My goal is "the caller should never have to know whether this is a v1
or v2 capability."  Returning any error other than one passed along
from pci_read/write_config_xxx() means we miss that goal.  Perhaps the
goal is unattainable, but I haven't been convinced yet.

I think hardware error detection is irrelevant to this discussion.
After reading Documentation/PCI/pci-error-recovery.txt, I'm even less
convinced that checking return values from pci_read/write_config_xxx()
or pci_pcie_capability_read/write_xxx() is a useful way to detect
hardware errors.

Having drivers detect hardware failures by checking for config access
errors is neither necessary nor sufficient.  It's not necessary
because a platform can implement a config accessor that checks *every*
access and reports failures to the driver via the pci_error_handler
framework.  It's not sufficient because config accesses are rare
(usually only at init-time), and hardware failures may happen at
arbitrary other times.

In my opinion, the only relevant question is whether a caller of
pci_pcie_capability_read/write_xxx() needs to know whether a register
is implemented (i.e., we have a v2 capability) or not.  For reads, I
don't think there's a case where fabricating a value of zero when
reading an unimplemented register is a problem.

Writes are obviously more interesting, but I'm still not sure there's
a case where silently dropping a write to an unimplemented register is
a problem.  The "capability" registers are read-only, so there's no
problem if we drop writes to them.  The "status" registers are
generally RO or RW1C, where it's only meaningful to write a non-zero
value if you're previously *read* a non-zero value.  The "control"
registers are often RW, of course, but generally it's only meaningful
to write a non-zero value when a non-zero bit in the "capability"
register has previously told you that something is supported.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Don Dutile - July 16, 2012, 6:57 p.m.
On 07/16/2012 01:29 PM, Bjorn Helgaas wrote:
> On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu<liuj97@gmail.com>  wrote:
>> On 07/13/2012 04:49 AM, Bjorn Helgaas wrote:
>>>> Hi Bjorn,
>>>>          It's a little risk to change these PCIe capabilities access
>>>> functions as void. On some platform with hardware error detecting/correcting
>>>> capabilities, such as EEH on Power, it would be better to return
>>>> error code if hardware error happens during accessing configuration registers.
>>>>          As I know, coming Intel Xeon processor may provide PCIe hardware
>>>> error detecting capability similar to EEH on power.
>>>
>>> I guess I'm playing devil's advocate here.  As a general rule, people
>>> don't check the return value of pci_read_config_*() or
>>> pci_write_config_*().  Unless you change them all, most callers of
>>> pci_pcie_capability_read_*() and _write_*() won't check the returns
>>> either.  So I'm not sure return values are an effective way to detect
>>> those hardware errors.
>>>
>>> How do these EEH errors get detected or reported today?  Do the
>>> drivers check every config access for success?  Adding those checks
>>> and figuring out how to handle errors at every possible point doesn't
>>> seem like a recipe for success.
>>
>> Hi Bjorn,
>>          Sorry for later reply, on travel these days.
>>          Yeah, it's true that most driver doesn't check return values of configuration
>> access functions, but there are still some drivers which do check return value of
>> pci_read_config_xxx(). For example, pciehp driver checks return value of CFG access
>> functions.
>>
>>          It's not realistic to enhance all drivers, but we may focus on a small set of
>> drivers for hardwares on specific high-end servers. For RAS features, we can never provide
>> perfect solutions, so we prefer some improvements. After all a small improvement is still
>> an improvement:)
>>
>>          I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know that the OS
>> may query firmware whether there's some hardware faults if pci_cfg_read_xxx() returns
>> all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error code to
>> pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report hardware faults
>> like SAL on IA64.
>>
>>          So how about keeping consistence with pci_cfg_read_xxx() and pci_user_cfg_read_xxx()?
>
> My goal is "the caller should never have to know whether this is a v1
> or v2 capability."  Returning any error other than one passed along
> from pci_read/write_config_xxx() means we miss that goal.  Perhaps the
> goal is unattainable, but I haven't been convinced yet.
>
> I think hardware error detection is irrelevant to this discussion.
> After reading Documentation/PCI/pci-error-recovery.txt, I'm even less
> convinced that checking return values from pci_read/write_config_xxx()
> or pci_pcie_capability_read/write_xxx() is a useful way to detect
> hardware errors.
>
> Having drivers detect hardware failures by checking for config access
> errors is neither necessary nor sufficient.  It's not necessary
> because a platform can implement a config accessor that checks *every*
> access and reports failures to the driver via the pci_error_handler
> framework.  It's not sufficient because config accesses are rare
> (usually only at init-time), and hardware failures may happen at
> arbitrary other times.
>
> In my opinion, the only relevant question is whether a caller of
> pci_pcie_capability_read/write_xxx() needs to know whether a register
> is implemented (i.e., we have a v2 capability) or not.  For reads, I
> don't think there's a case where fabricating a value of zero when
> reading an unimplemented register is a problem.
>
> Writes are obviously more interesting, but I'm still not sure there's
> a case where silently dropping a write to an unimplemented register is
> a problem.  The "capability" registers are read-only, so there's no
> problem if we drop writes to them.  The "status" registers are
> generally RO or RW1C, where it's only meaningful to write a non-zero
> value if you're previously *read* a non-zero value.  The "control"
> registers are often RW, of course, but generally it's only meaningful
> to write a non-zero value when a non-zero bit in the "capability"
> register has previously told you that something is supported.
>
> Bjorn
+1
Returning 0 on capability reads -- due to unimplemented
features/register or due to failures,
should translate into the (core) code doing no writes.
Thus, the reason I suggested returning 0 on failure in original posting.


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiang Liu - July 17, 2012, 12:09 a.m.
On 07/17/2012 01:29 AM, Bjorn Helgaas wrote:
> On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu <liuj97@gmail.com> wrote:
>> On 07/13/2012 04:49 AM, Bjorn Helgaas wrote:
>>>> Hi Bjorn,
>>>>         It's a little risk to change these PCIe capabilities access
>>>> functions as void. On some platform with hardware error detecting/correcting
>>>> capabilities, such as EEH on Power, it would be better to return
>>>> error code if hardware error happens during accessing configuration registers.
>>>>         As I know, coming Intel Xeon processor may provide PCIe hardware
>>>> error detecting capability similar to EEH on power.
>>>
>>> I guess I'm playing devil's advocate here.  As a general rule, people
>>> don't check the return value of pci_read_config_*() or
>>> pci_write_config_*().  Unless you change them all, most callers of
>>> pci_pcie_capability_read_*() and _write_*() won't check the returns
>>> either.  So I'm not sure return values are an effective way to detect
>>> those hardware errors.
>>>
>>> How do these EEH errors get detected or reported today?  Do the
>>> drivers check every config access for success?  Adding those checks
>>> and figuring out how to handle errors at every possible point doesn't
>>> seem like a recipe for success.
>>
>> Hi Bjorn,
>>         Sorry for later reply, on travel these days.
>>         Yeah, it's true that most driver doesn't check return values of configuration
>> access functions, but there are still some drivers which do check return value of
>> pci_read_config_xxx(). For example, pciehp driver checks return value of CFG access
>> functions.
>>
>>         It's not realistic to enhance all drivers, but we may focus on a small set of
>> drivers for hardwares on specific high-end servers. For RAS features, we can never provide
>> perfect solutions, so we prefer some improvements. After all a small improvement is still
>> an improvement:)
>>
>>         I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know that the OS
>> may query firmware whether there's some hardware faults if pci_cfg_read_xxx() returns
>> all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error code to
>> pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report hardware faults
>> like SAL on IA64.
>>
>>         So how about keeping consistence with pci_cfg_read_xxx() and pci_user_cfg_read_xxx()?
> 
> My goal is "the caller should never have to know whether this is a v1
> or v2 capability."  Returning any error other than one passed along
> from pci_read/write_config_xxx() means we miss that goal.  Perhaps the
> goal is unattainable, but I haven't been convinced yet.
> 
> I think hardware error detection is irrelevant to this discussion.
> After reading Documentation/PCI/pci-error-recovery.txt, I'm even less
> convinced that checking return values from pci_read/write_config_xxx()
> or pci_pcie_capability_read/write_xxx() is a useful way to detect
> hardware errors.
> 
> Having drivers detect hardware failures by checking for config access
> errors is neither necessary nor sufficient.  It's not necessary
> because a platform can implement a config accessor that checks *every*
> access and reports failures to the driver via the pci_error_handler
> framework.  It's not sufficient because config accesses are rare
> (usually only at init-time), and hardware failures may happen at
> arbitrary other times.
> 
> In my opinion, the only relevant question is whether a caller of
> pci_pcie_capability_read/write_xxx() needs to know whether a register
> is implemented (i.e., we have a v2 capability) or not.  For reads, I
> don't think there's a case where fabricating a value of zero when
> reading an unimplemented register is a problem.
> 
> Writes are obviously more interesting, but I'm still not sure there's
> a case where silently dropping a write to an unimplemented register is
> a problem.  The "capability" registers are read-only, so there's no
> problem if we drop writes to them.  The "status" registers are
> generally RO or RW1C, where it's only meaningful to write a non-zero
> value if you're previously *read* a non-zero value.  The "control"
> registers are often RW, of course, but generally it's only meaningful
> to write a non-zero value when a non-zero bit in the "capability"
> register has previously told you that something is supported.
Hi Bjorn,
	I'm convinced by you that we shouldn't return error when accessing
an unimplemented PCIe capabilities register and just hide the differences 
among V1/V2 specification. Then how about returning error from
"pci_read/write_config_xxx()" to callers of pci_pcie_capabilitiy_read/write_xxx()?
I still prefer to return error code to keep consistence with other configuration
space access interfaces:)
	Thanks!
	Gerry

> 
> Bjorn
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - July 17, 2012, 12:14 a.m.
On Mon, Jul 16, 2012 at 6:09 PM, Jiang Liu <liuj97@gmail.com> wrote:
> On 07/17/2012 01:29 AM, Bjorn Helgaas wrote:
>> On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu <liuj97@gmail.com> wrote:
>>> On 07/13/2012 04:49 AM, Bjorn Helgaas wrote:
>>>>> Hi Bjorn,
>>>>>         It's a little risk to change these PCIe capabilities access
>>>>> functions as void. On some platform with hardware error detecting/correcting
>>>>> capabilities, such as EEH on Power, it would be better to return
>>>>> error code if hardware error happens during accessing configuration registers.
>>>>>         As I know, coming Intel Xeon processor may provide PCIe hardware
>>>>> error detecting capability similar to EEH on power.
>>>>
>>>> I guess I'm playing devil's advocate here.  As a general rule, people
>>>> don't check the return value of pci_read_config_*() or
>>>> pci_write_config_*().  Unless you change them all, most callers of
>>>> pci_pcie_capability_read_*() and _write_*() won't check the returns
>>>> either.  So I'm not sure return values are an effective way to detect
>>>> those hardware errors.
>>>>
>>>> How do these EEH errors get detected or reported today?  Do the
>>>> drivers check every config access for success?  Adding those checks
>>>> and figuring out how to handle errors at every possible point doesn't
>>>> seem like a recipe for success.
>>>
>>> Hi Bjorn,
>>>         Sorry for later reply, on travel these days.
>>>         Yeah, it's true that most driver doesn't check return values of configuration
>>> access functions, but there are still some drivers which do check return value of
>>> pci_read_config_xxx(). For example, pciehp driver checks return value of CFG access
>>> functions.
>>>
>>>         It's not realistic to enhance all drivers, but we may focus on a small set of
>>> drivers for hardwares on specific high-end servers. For RAS features, we can never provide
>>> perfect solutions, so we prefer some improvements. After all a small improvement is still
>>> an improvement:)
>>>
>>>         I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know that the OS
>>> may query firmware whether there's some hardware faults if pci_cfg_read_xxx() returns
>>> all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error code to
>>> pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report hardware faults
>>> like SAL on IA64.
>>>
>>>         So how about keeping consistence with pci_cfg_read_xxx() and pci_user_cfg_read_xxx()?
>>
>> My goal is "the caller should never have to know whether this is a v1
>> or v2 capability."  Returning any error other than one passed along
>> from pci_read/write_config_xxx() means we miss that goal.  Perhaps the
>> goal is unattainable, but I haven't been convinced yet.
>>
>> I think hardware error detection is irrelevant to this discussion.
>> After reading Documentation/PCI/pci-error-recovery.txt, I'm even less
>> convinced that checking return values from pci_read/write_config_xxx()
>> or pci_pcie_capability_read/write_xxx() is a useful way to detect
>> hardware errors.
>>
>> Having drivers detect hardware failures by checking for config access
>> errors is neither necessary nor sufficient.  It's not necessary
>> because a platform can implement a config accessor that checks *every*
>> access and reports failures to the driver via the pci_error_handler
>> framework.  It's not sufficient because config accesses are rare
>> (usually only at init-time), and hardware failures may happen at
>> arbitrary other times.
>>
>> In my opinion, the only relevant question is whether a caller of
>> pci_pcie_capability_read/write_xxx() needs to know whether a register
>> is implemented (i.e., we have a v2 capability) or not.  For reads, I
>> don't think there's a case where fabricating a value of zero when
>> reading an unimplemented register is a problem.
>>
>> Writes are obviously more interesting, but I'm still not sure there's
>> a case where silently dropping a write to an unimplemented register is
>> a problem.  The "capability" registers are read-only, so there's no
>> problem if we drop writes to them.  The "status" registers are
>> generally RO or RW1C, where it's only meaningful to write a non-zero
>> value if you're previously *read* a non-zero value.  The "control"
>> registers are often RW, of course, but generally it's only meaningful
>> to write a non-zero value when a non-zero bit in the "capability"
>> register has previously told you that something is supported.
> Hi Bjorn,
>         I'm convinced by you that we shouldn't return error when accessing
> an unimplemented PCIe capabilities register and just hide the differences
> among V1/V2 specification. Then how about returning error from
> "pci_read/write_config_xxx()" to callers of pci_pcie_capabilitiy_read/write_xxx()?
> I still prefer to return error code to keep consistence with other configuration
> space access interfaces:)

I think it's fine to return the status of pci_read/write_config_xxx(), e.g.,

    int pci_pcie_cap_read_word(...)
    {
        ...
        if (<implemented>)
            return pci_read_config_word(...);

        ...
        return 0;
    }

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ba91a7e..80ae022 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -469,3 +469,91 @@  void pci_cfg_access_unlock(struct pci_dev *dev)
 	raw_spin_unlock_irqrestore(&pci_lock, flags);
 }
 EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
+
+static int
+pci_pcie_cap_get_offset(struct pci_dev *dev, int where, size_t sz)
+{
+	bool valid;
+
+	if (!pci_is_pcie(dev))
+		return -EINVAL;
+	if (where & (sz - 1))
+		return -EINVAL;
+
+	if (where < 0)
+		valid = false;
+	else if (where < PCI_EXP_DEVCAP)
+		valid = true;
+	else if (where < PCI_EXP_LNKCAP)
+		valid = pci_pcie_cap_has_devctl(dev);
+	else if (where < PCI_EXP_SLTCAP)
+		valid = pci_pcie_cap_has_lnkctl(dev);
+	else if (where < PCI_EXP_RTCTL)
+		valid = pci_pcie_cap_has_sltctl(dev);
+	else if (where < PCI_EXP_DEVCAP2)
+		valid = pci_pcie_cap_has_rtctl(dev);
+	else if (where < PCI_EXP_CAP2_SIZE)
+		valid = pci_pcie_cap_has_cap2(dev);
+	else
+		valid = false;
+
+	return valid ? where + pci_pcie_cap(dev) : -EINVAL;
+}
+
+int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp)
+{
+	*valp = 0;
+	where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
+	if (where >= 0)
+		return pci_read_config_word(dev, where, valp);
+
+	if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA &&
+	    pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
+		*valp = PCI_EXP_SLTSTA_PDS;
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_read_word);
+
+int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp)
+{
+	*valp = 0;
+	where = pci_pcie_cap_get_offset(dev, where, sizeof(u32));
+	if (where >= 0)
+		return pci_read_config_dword(dev, where, valp);
+
+	/*
+	 * 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.
+	 */
+	if (pci_is_pcie(dev) && where == PCI_EXP_SLTCTL &&
+	    pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
+		*valp = PCI_EXP_SLTSTA_PDS << 16;
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_read_dword);
+
+int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val)
+{
+	where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
+	if (where >= 0)
+		return pci_write_config_word(dev, where, val);
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_write_word);
+
+int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32 val)
+{
+	where = pci_pcie_cap_get_offset(dev, where, sizeof(u32));
+	if (where >= 0)
+		return pci_write_config_dword(dev, where, val);
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_write_dword);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 346b2d9..78767b2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1703,6 +1703,11 @@  static inline bool pci_pcie_cap_has_rtctl(const struct pci_dev *pdev)
 	       type == PCI_EXP_TYPE_RC_EC;
 }
 
+extern int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp);
+extern int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp);
+extern int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val);
+extern int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32 val);
+
 void pci_request_acs(void);
 bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
 bool pci_acs_path_enabled(struct pci_dev *start,
@@ -1843,5 +1848,10 @@  static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
  */
 struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
 
+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);
+
 #endif /* __KERNEL__ */
 #endif /* LINUX_PCI_H */
diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
index 53274bf..ac60e22 100644
--- a/include/linux/pci_regs.h
+++ b/include/linux/pci_regs.h
@@ -542,9 +542,24 @@ 
 #define  PCI_EXP_OBFF_MSGA_EN	0x2000	/* OBFF enable with Message type A */
 #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_DEVSTA2		42	/* Device Status 2 */
+#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_SLTCTL2		56	/* Slot Control 2 */
+#define  PCI_EXP_LNKCTL2_TLS	0x0f	/* Target Link Speed */
+#define  PCI_EXP_LNKCTL2_EC	0x10	/* Enter Compliance */
+#define  PCI_EXP_LNKCTL2_HASD	0x20	/* Hardware Autonomous Speed Disable */
+#define  PCI_EXP_LNKCTL2_SD	0x40	/* Selectable De-emphasis */
+#define  PCI_EXP_LNKCTL2_TM	0x380	/* Transmit Margin */
+#define  PCI_EXP_LNKCTL2_EMC	0x400	/* Enter Modified Compliance */
+#define  PCI_EXP_LNKCTL2_CS	0x800	/* Compliance SOS */
+#define  PCI_EXP_LNKCTL2_CD	0x1000	/* Compliance De-emphasis */
+#define PCI_EXP_LNKSTA2		50	/* Link Status 2 */
+#define  PCI_EXP_LNKSTA2_CDL	0x01	/* Current De-emphasis Level */
+#define PCI_EXP_SLTCAP2		52	/* Slot Capabilities 2 */
+#define PCI_EXP_SLTCTL2		56	/* Slot Control 2*/
+#define PCI_EXP_SLTSTA2		58	/* Slot Status 2*/
+#define	PCI_EXP_CAP2_SIZE	60
 
 /* Extended Capabilities (PCI-X 2.0 and Express) */
 #define PCI_EXT_CAP_ID(header)		(header & 0x0000ffff)