Re: [PATCH v4] pci: Make return value of pcie_capability_read*() consistent
From: Yicong Yang <yangyicong@hisilicon.com>
Date: 2020-04-28 02:19:06
Also in:
linux-mips, linux-pci, lkml
Subsystem:
pci subsystem, the rest · Maintainers:
Bjorn Helgaas, Linus Torvalds
On 2020/4/28 2:13, Bjorn Helgaas wrote:
I'm starting to think we're approaching this backwards. I searched for PCIBIOS_FUNC_NOT_SUPPORTED, PCIBIOS_BAD_VENDOR_ID, and the other error values. Almost every use is a *return* in a config accessor. There are very, very few *tests* for these values.
If we have certain reasons to reserve PCI_BIOS* error to identify PCI errors in PCI drivers, maybe redefine the PCI_BIOS* to generic error codes can solve the issues, and no need to call pcibios_err_to_errno() to do the conversion. Few changes may be made to current codes. One possible patch may look like below. Otherwise, maybe convert all PCI_BIOS* errors to generic error codes is a better idea. Not sure it's the best way or not. Just FYI.
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 83ce1cd..843987c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h@@ -675,14 +675,18 @@ 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)
@@ -690,17 +694,12 @@ static inline int pcibios_err_to_errno(int err) 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;
quoted hunk ↗ jump to hunk
For example, the only tests for PCIBIOS_FUNC_NOT_SUPPORTED are in xen_pcibios_err_to_errno() and pcibios_err_to_errno(), i.e., we're just converting that value to -ENOENT or the Xen-specific thing. So I think the best approach might be to remove the PCIBIOS_* error values completely and replace them with the corresponding values from pcibios_err_to_errno(). For example, a part of the patch would look like this:diff --git a/arch/mips/pci/ops-emma2rh.c b/arch/mips/pci/ops-emma2rh.c index 65f47344536c..d4d9c902c147 100644 --- a/arch/mips/pci/ops-emma2rh.c +++ b/arch/mips/pci/ops-emma2rh.c@@ -100,7 +100,7 @@ static int pci_config_read(struct pci_bus *bus, unsigned int devfn, int where, break; default: emma2rh_out32(EMMA2RH_PCI_IWIN0_CTR, backup_win0); - return PCIBIOS_FUNC_NOT_SUPPORTED; + return -ENOENT; } emma2rh_out32(EMMA2RH_PCI_IWIN0_CTR, backup_win0);@@ -149,7 +149,7 @@ static int pci_config_write(struct pci_bus *bus, unsigned int devfn, int where, break; default: emma2rh_out32(EMMA2RH_PCI_IWIN0_CTR, backup_win0); - return PCIBIOS_FUNC_NOT_SUPPORTED; + return -ENOENT; } *(volatile u32 *)(base + (PCI_FUNC(devfn) << 8) + (where & 0xfffffffc)) = data;diff --git a/include/linux/pci.h b/include/linux/pci.h index 83ce1cdf5676..f95637a8d391 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h@@ -675,7 +675,6 @@ 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@@ -689,8 +688,6 @@ static inline int pcibios_err_to_errno(int err) return err; /* Assume already errno */ switch (err) { - case PCIBIOS_FUNC_NOT_SUPPORTED: - return -ENOENT; case PCIBIOS_BAD_VENDOR_ID: return -ENOTTY; case PCIBIOS_DEVICE_NOT_FOUND:.