* [PATCH RFC 0/2] pci: Make return value of pcie_capability_*() consistent
@ 2020-05-04 5:18 refactormyself
2020-05-04 5:18 ` [PATCH RFC 1/2] " refactormyself
2020-05-04 5:18 ` [PATCH RFC 2/2] pci: Set all PCIBIOS_* error codes to generic error codes refactormyself
0 siblings, 2 replies; 4+ messages in thread
From: refactormyself @ 2020-05-04 5:18 UTC (permalink / raw)
To: helgaas; +Cc: Bolarinwa Olayemi Saheed, bjorn, yangyicong, skhan, linux-pci
From: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
Changes in version 5:
- Extend consitency to all accesors, they all now return PCIBIOS_* error
codes
- Enforce -ve error code, any case of > 0 error code is return as -ERANGE
this is done in line with the implementation of pcibios_err_to_errno()
which is now deprecated.
- Set all PCIBIOS_* error codes to generic error codes.
- pcibios_err_to_errno() is now dormant and deprecated. It will return
-ERANGE if a any positive error code is passed into it.
- Remove verbose dependency audit report from commit log. No conflict was
discovered so far.
Changes in version 4:
- make patch independent of earlier versions
- add commit log
- add justificaation and report on audit of affected functions
Bolarinwa Olayemi Saheed (2):
pci: Make return value of pcie_capability_{read|write}_*() consistent
Set all PCIBIOS_* error codes to generic error codes
drivers/pci/access.c | 64 +++++++++++++++++++++++++++++++++++---------
include/linux/pci.h | 31 +++++++++++----------
2 files changed, 66 insertions(+), 29 deletions(-)
--
2.18.2
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH RFC 1/2] pci: Make return value of pcie_capability_*() consistent 2020-05-04 5:18 [PATCH RFC 0/2] pci: Make return value of pcie_capability_*() consistent refactormyself @ 2020-05-04 5:18 ` refactormyself 2020-05-05 0:06 ` Bjorn Helgaas 2020-05-04 5:18 ` [PATCH RFC 2/2] pci: Set all PCIBIOS_* error codes to generic error codes refactormyself 1 sibling, 1 reply; 4+ messages in thread From: refactormyself @ 2020-05-04 5:18 UTC (permalink / raw) To: helgaas; +Cc: Bolarinwa Olayemi Saheed, bjorn, yangyicong, skhan, linux-pci From: Bolarinwa Olayemi Saheed <refactormyself@gmail.com> pcie_capability_{read|write}_*() could return 0, -EINVAL, or any of the PCIBIOS_* error codes. This is behaviour is now changed to ONLY return a PCIBIOS_* error code or -ERANGE on error. This has now been made consistent across all accessors. Callers now have a consistent way for checking which error has occurred. An audit of the callers of these functions was made and no contradicting case was found in the examined call chains. Signed-off-by: Bolarinwa Olayemi Saheed <refactormyself@gmail.com> Suggested-by: Bjorn Helgaas <bjorn@helgaas.com> --- drivers/pci/access.c | 64 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 51 insertions(+), 13 deletions(-) diff --git a/drivers/pci/access.c b/drivers/pci/access.c index 79c4a2ef269a..10c771079e35 100644 --- a/drivers/pci/access.c +++ b/drivers/pci/access.c @@ -402,6 +402,8 @@ static bool pcie_capability_reg_implemented(struct pci_dev *dev, int pos) * Note that these accessor functions are only for the "PCI Express * Capability" (see PCIe spec r3.0, sec 7.8). They do not apply to the * other "PCI Express Extended Capabilities" (AER, VC, ACS, MFVC, etc.) + * + * Return 0 on success, otherwise a negative value */ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val) { @@ -409,7 +411,7 @@ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val) *val = 0; if (pos & 1) - return -EINVAL; + return PCIBIOS_BAD_REGISTER_NUMBER; if (pcie_capability_reg_implemented(dev, pos)) { ret = pci_read_config_word(dev, pci_pcie_cap(dev) + pos, val); @@ -444,7 +446,7 @@ int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val) *val = 0; if (pos & 3) - return -EINVAL; + return PCIBIOS_BAD_REGISTER_NUMBER; if (pcie_capability_reg_implemented(dev, pos)) { ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val); @@ -453,9 +455,9 @@ int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val) * have been written as 0xFFFFFFFF if hardware error happens * during pci_read_config_dword(). */ - if (ret) - *val = 0; - return ret; + if (ret) + *val = 0; + return ret; } if (pci_is_pcie(dev) && pcie_downstream_port(dev) && @@ -469,7 +471,7 @@ EXPORT_SYMBOL(pcie_capability_read_dword); int pcie_capability_write_word(struct pci_dev *dev, int pos, u16 val) { if (pos & 1) - return -EINVAL; + return PCIBIOS_BAD_REGISTER_NUMBER; if (!pcie_capability_reg_implemented(dev, pos)) return 0; @@ -481,7 +483,7 @@ EXPORT_SYMBOL(pcie_capability_write_word); int pcie_capability_write_dword(struct pci_dev *dev, int pos, u32 val) { if (pos & 3) - return -EINVAL; + return PCIBIOS_BAD_REGISTER_NUMBER; if (!pcie_capability_reg_implemented(dev, pos)) return 0; @@ -526,56 +528,92 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword); int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val) { + int ret; if (pci_dev_is_disconnected(dev)) { *val = ~0; return PCIBIOS_DEVICE_NOT_FOUND; } - return pci_bus_read_config_byte(dev->bus, dev->devfn, where, val); + + ret = pci_bus_read_config_byte(dev->bus, dev->devfn, where, val); + + if (ret > 0) + ret = -ERANGE; + return ret; } EXPORT_SYMBOL(pci_read_config_byte); int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val) { + int ret; if (pci_dev_is_disconnected(dev)) { *val = ~0; return PCIBIOS_DEVICE_NOT_FOUND; } - return pci_bus_read_config_word(dev->bus, dev->devfn, where, val); + + ret = pci_bus_read_config_word(dev->bus, dev->devfn, where, val); + + if (ret > 0) + ret = -ERANGE; + return ret; } EXPORT_SYMBOL(pci_read_config_word); int pci_read_config_dword(const struct pci_dev *dev, int where, u32 *val) { + int ret; if (pci_dev_is_disconnected(dev)) { *val = ~0; return PCIBIOS_DEVICE_NOT_FOUND; } - return pci_bus_read_config_dword(dev->bus, dev->devfn, where, val); + + ret = pci_bus_read_config_dword(dev->bus, dev->devfn, where, val); + + if (ret > 0) + ret = -ERANGE; + return ret; } EXPORT_SYMBOL(pci_read_config_dword); int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val) { + int ret; if (pci_dev_is_disconnected(dev)) return PCIBIOS_DEVICE_NOT_FOUND; - return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val); + + ret = pci_bus_write_config_byte(dev->bus, dev->devfn, where, val); + + if (ret > 0) + ret = -ERANGE; + return ret; } EXPORT_SYMBOL(pci_write_config_byte); int pci_write_config_word(const struct pci_dev *dev, int where, u16 val) { + int ret; if (pci_dev_is_disconnected(dev)) return PCIBIOS_DEVICE_NOT_FOUND; - return pci_bus_write_config_word(dev->bus, dev->devfn, where, val); + + ret = pci_bus_write_config_word(dev->bus, dev->devfn, where, val); + + if (ret > 0) + ret = -ERANGE; + return ret; } EXPORT_SYMBOL(pci_write_config_word); int pci_write_config_dword(const struct pci_dev *dev, int where, u32 val) { + int ret; if (pci_dev_is_disconnected(dev)) return PCIBIOS_DEVICE_NOT_FOUND; - return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val); + + ret = pci_bus_write_config_dword(dev->bus, dev->devfn, where, val); + + if (ret > 0) + ret = -ERANGE; + return ret; } EXPORT_SYMBOL(pci_write_config_dword); -- 2.18.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH RFC 1/2] pci: Make return value of pcie_capability_*() consistent 2020-05-04 5:18 ` [PATCH RFC 1/2] " refactormyself @ 2020-05-05 0:06 ` Bjorn Helgaas 0 siblings, 0 replies; 4+ messages in thread From: Bjorn Helgaas @ 2020-05-05 0:06 UTC (permalink / raw) To: refactormyself; +Cc: bjorn, yangyicong, skhan, linux-pci Wherever you're working in the tree, pay attention to the existing style of code, comments, and commit logs and make yours match. For example, $ git log --oneline drivers/pci/access.c af65d1ad416b PCI/AER: Save AER Capability for suspend/resume 984998e3404e PCI: Make pcie_downstream_port() available outside of access.c 5180fd913558 PCI: Uninline PCI bus accessors for better ftracing df62ab5e0f75 PCI: Tidy comments f0eb77ae6b85 PCI/VPD: Move VPD access code to vpd.c ab8c609356fb Merge branch 'pci/spdx' into next 7328c8f48d18 PCI: Add SPDX GPL-2.0 when no license was specified 7506dc798993 PCI: Add wrappers for dev_printk() So your subject needs to be something like: PCI: Make return value of pcie_capability_*() consistent On Mon, May 04, 2020 at 07:18:11AM +0200, refactormyself@gmail.com wrote: > From: Bolarinwa Olayemi Saheed <refactormyself@gmail.com> > > pcie_capability_{read|write}_*() could return 0, -EINVAL, or any of the > PCIBIOS_* error codes. This is behaviour is now changed to ONLY return a > PCIBIOS_* error code or -ERANGE on error. > This has now been made consistent across all accessors. Callers now have > a consistent way for checking which error has occurred. > > An audit of the callers of these functions was made and no contradicting > case was found in the examined call chains. > > Signed-off-by: Bolarinwa Olayemi Saheed <refactormyself@gmail.com> > Suggested-by: Bjorn Helgaas <bjorn@helgaas.com> > --- > drivers/pci/access.c | 64 +++++++++++++++++++++++++++++++++++--------- > 1 file changed, 51 insertions(+), 13 deletions(-) > > diff --git a/drivers/pci/access.c b/drivers/pci/access.c > index 79c4a2ef269a..10c771079e35 100644 > --- a/drivers/pci/access.c > +++ b/drivers/pci/access.c > @@ -402,6 +402,8 @@ static bool pcie_capability_reg_implemented(struct pci_dev *dev, int pos) > * Note that these accessor functions are only for the "PCI Express > * Capability" (see PCIe spec r3.0, sec 7.8). They do not apply to the > * other "PCI Express Extended Capabilities" (AER, VC, ACS, MFVC, etc.) > + * > + * Return 0 on success, otherwise a negative value Actually, a negative *error value*. > int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val) > { > @@ -409,7 +411,7 @@ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val) > > *val = 0; > if (pos & 1) > - return -EINVAL; > + return PCIBIOS_BAD_REGISTER_NUMBER; This does not match the commit log or the function comment. I know the *next* patch will make it true, but the commit log and function comment must describe the code at this point. Everything must be consistent and buildable after every commit because future patches may not ever be applied. I think there's no reason to change these pcie_capability_*() return values. I think the end state we want to get to would be to deprecate the PCIBIOS_* #defines and use the -E* definitions instead. > int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val) > { > + int ret; > if (pci_dev_is_disconnected(dev)) { Nit: style is to leave a blank line between local variables and the first line of executable code. > *val = ~0; > return PCIBIOS_DEVICE_NOT_FOUND; > } > - return pci_bus_read_config_byte(dev->bus, dev->devfn, where, val); > + > + ret = pci_bus_read_config_byte(dev->bus, dev->devfn, where, val); > + > + if (ret > 0) > + ret = -ERANGE; I don't understand what you're doing here. pci_bus_read_config_byte() returns things like PCIBIOS_BAD_REGISTER_NUMBER, PCIBIOS_FUNC_NOT_SUPPORTED, PCIBIOS_BAD_VENDOR_ID, etc. These are all positive at this point, so this collapses all of them to -ERANGE, which throws away the information about the different types of errors, which is not what we want. Another coding style nit: normally a check for error would be immediately after the function call, so omit the blank line in this case. Generally you can learn things like this by looking at the existing code and following the same style. > + return ret; > } ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH RFC 2/2] pci: Set all PCIBIOS_* error codes to generic error codes 2020-05-04 5:18 [PATCH RFC 0/2] pci: Make return value of pcie_capability_*() consistent refactormyself 2020-05-04 5:18 ` [PATCH RFC 1/2] " refactormyself @ 2020-05-04 5:18 ` refactormyself 1 sibling, 0 replies; 4+ messages in thread From: refactormyself @ 2020-05-04 5:18 UTC (permalink / raw) To: helgaas; +Cc: Bolarinwa Olayemi Saheed, bjorn, yangyicong, skhan, linux-pci From: Bolarinwa Olayemi Saheed <refactormyself@gmail.com> PCIBIOS_* error codes now set to negative value generic codes. pcibios_err_to_errno() is modified to reflect this changes and the function is now marked deprecated. The function now only returns negative error codes. These changes effect the consistency across pci and pcie accessors. Now they all return 0 on success and < 0, otherwise. Signed-off-by: Bolarinwa Olayemi Saheed <refactormyself@gmail.com> Suggested-by: Yicong Yang <yangyicong@hisilicon.com> --- include/linux/pci.h | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/include/linux/pci.h b/include/linux/pci.h index 3840a541a9de..ee87def7242a 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -661,32 +661,31 @@ static inline bool pci_dev_msi_enabled(struct pci_dev *pci_dev) { return false; /* Error values that may be returned by PCI functions */ #define PCIBIOS_SUCCESSFUL 0x00 -#define PCIBIOS_FUNC_NOT_SUPPORTED 0x81 -#define PCIBIOS_BAD_VENDOR_ID 0x83 -#define PCIBIOS_DEVICE_NOT_FOUND 0x86 -#define PCIBIOS_BAD_REGISTER_NUMBER 0x87 -#define PCIBIOS_SET_FAILED 0x88 -#define PCIBIOS_BUFFER_TOO_SMALL 0x89 - -/* Translate above to generic errno for passing back through non-PCI code */ +#define PCIBIOS_FUNC_NOT_SUPPORTED -ENOENT +#define PCIBIOS_BAD_VENDOR_ID -ENOTTY +#define PCIBIOS_DEVICE_NOT_FOUND -ENODEV +#define PCIBIOS_BAD_REGISTER_NUMBER -EFAULT +#define PCIBIOS_SET_FAILED -EIO +#define PCIBIOS_BUFFER_TOO_SMALL -ENOSPC + +/* * + * Translate above to generic errno for passing back through non-PCI code + * + * Deprecated. Use the PCIBIOS_* directly without a translation. + */ static inline int pcibios_err_to_errno(int err) { - if (err <= PCIBIOS_SUCCESSFUL) - return err; /* Assume already errno */ + if (err == PCIBIOS_SUCCESSFUL) + return err; /* preserve success */ switch (err) { case PCIBIOS_FUNC_NOT_SUPPORTED: - return -ENOENT; case PCIBIOS_BAD_VENDOR_ID: - return -ENOTTY; case PCIBIOS_DEVICE_NOT_FOUND: - return -ENODEV; case PCIBIOS_BAD_REGISTER_NUMBER: - return -EFAULT; case PCIBIOS_SET_FAILED: - return -EIO; case PCIBIOS_BUFFER_TOO_SMALL: - return -ENOSPC; + return err; } return -ERANGE; -- 2.18.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-05-05 0:06 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-04 5:18 [PATCH RFC 0/2] pci: Make return value of pcie_capability_*() consistent refactormyself 2020-05-04 5:18 ` [PATCH RFC 1/2] " refactormyself 2020-05-05 0:06 ` Bjorn Helgaas 2020-05-04 5:18 ` [PATCH RFC 2/2] pci: Set all PCIBIOS_* error codes to generic error codes refactormyself
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).