| Message ID | 1341935655-5381-6-git-send-email-jiang.liu@huawei.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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
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
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
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
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
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
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
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
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
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
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
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)