[PATCH RFC 0/2] pci: Make return value of pcie_capability_*() consistent

[PATCH RFC 0/2] pci: Make return value of pcie_capability_*() consistent

[refactormyself]

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

[PATCH RFC 1/2] pci: Make return value of pcie_capability_*() consistent

[refactormyself]

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

Re: [PATCH RFC 1/2] pci: Make return value of pcie_capability_*() consistent

[Bjorn Helgaas]

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;
>  }

[PATCH RFC 2/2] pci: Set all PCIBIOS_* error codes to generic error codes

[refactormyself]

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