From: Yicong Yang <yangyicong@hisilicon.com>
To: Bjorn Helgaas <helgaas@kernel.org>,
Saheed Bolarinwa <refactormyself@gmail.com>
Cc: <bjorn@helgaas.com>, <skhan@linuxfoundation.org>,
<linux-pci@vger.kernel.org>,
Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
<linux-mips@vger.kernel.org>,
Michael Ellerman <mpe@ellerman.id.au>,
<linuxppc-dev@lists.ozlabs.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4] pci: Make return value of pcie_capability_read*() consistent
Date: Tue, 28 Apr 2020 10:19:08 +0800 [thread overview]
Message-ID: <4cc16e59-d346-5523-5072-eebe77d06a08@hisilicon.com> (raw)
In-Reply-To: <20200427181304.GA214573@google.com>
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;
>
> 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:
> .
>
next prev parent reply other threads:[~2020-04-28 2:19 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <c9811866-8fea-9398-9337-45818136fe84@gmail.com>
2020-04-27 18:13 ` [PATCH v4] pci: Make return value of pcie_capability_read*() consistent Bjorn Helgaas
2020-04-28 2:19 ` Yicong Yang [this message]
2020-04-28 11:43 ` Bjorn Helgaas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4cc16e59-d346-5523-5072-eebe77d06a08@hisilicon.com \
--to=yangyicong@hisilicon.com \
--cc=bjorn@helgaas.com \
--cc=helgaas@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
--cc=refactormyself@gmail.com \
--cc=skhan@linuxfoundation.org \
--cc=tsbogend@alpha.franken.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).