All of lore.kernel.org
 help / color / mirror / Atom feed
From: Brian Norris <briannorris@chromium.org>
To: Shawn Lin <shawn.lin@rock-chips.com>,
	Bjorn Helgaas <bhelgaas@google.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci@vger.kernel.org, Jeffy Chen <jeffy.chen@rock-chips.com>,
	linux-rockchip@lists.infradead.org,
	Krishna Dhulipala <krishnad@fb.com>,
	Keith Busch <keith.busch@intel.com>,
	Christoph Hellwig <hch@lst.de>, Wei Zhang <wzhang@fb.com>
Subject: [PATCH] PCI: Make error code types consistent in pci_{read,write}_config_*
Date: Tue, 23 May 2017 12:36:58 -0700	[thread overview]
Message-ID: <20170523193655.GA144183@google.com> (raw)
In-Reply-To: <20170523184359.GB115572@google.com>

Callers normally treat the config space accessors as returning PCBIOS_*
error codes, not Linux error codes (or they don't look at them at all).
We have pcibios_err_to_errno(), in case the error code needs translated.

Fixes: 4b1038834739 ("PCI: Don't attempt config access to disconnected devices")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
+ others, change subject

On Tue, May 23, 2017 at 11:44:01AM -0700, Brian Norris wrote:
> But the high level code doesn't handle this
> consistently. See, e.g., pci_read_config_byte() which can return regular
> Linux error codes (like -ENODEV), except it also passes up the return
> code of pci_read_config_byte() (a PCIBIOS_* code) directly.

Apparently this is new (inconsistent) behavior in 4.12-rc1. Seems like
an oversight to me.

> So callers don't really know whether to treat the value from
> pci_read_config_<foo>() as a PCIBIOS_* code (which should be translated
> with pcibios_err_to_errno()) or as a standard Linux errno.
> 
> But then, there are relatively few callers (less than 10% of
> pci_read_config_<foo>(); even fewer for writes) that actually check the
> error codes...
> 
> Maybe the "fix" is to replace -ENODEV with PCIBIOS_DEVICE_NOT_FOUND for
> the inconsistent cases (pci_{read,write}_config_{byte,word,dword}()).

Fix implemented in the surrounding patch.

 drivers/pci/access.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 74cf5fffb1e1..c80e37a69305 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -896,7 +896,7 @@ int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
 {
 	if (pci_dev_is_disconnected(dev)) {
 		*val = ~0;
-		return -ENODEV;
+		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 	return pci_bus_read_config_byte(dev->bus, dev->devfn, where, val);
 }
@@ -906,7 +906,7 @@ int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
 {
 	if (pci_dev_is_disconnected(dev)) {
 		*val = ~0;
-		return -ENODEV;
+		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 	return pci_bus_read_config_word(dev->bus, dev->devfn, where, val);
 }
@@ -917,7 +917,7 @@ int pci_read_config_dword(const struct pci_dev *dev, int where,
 {
 	if (pci_dev_is_disconnected(dev)) {
 		*val = ~0;
-		return -ENODEV;
+		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 	return pci_bus_read_config_dword(dev->bus, dev->devfn, where, val);
 }
@@ -926,7 +926,7 @@ EXPORT_SYMBOL(pci_read_config_dword);
 int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val)
 {
 	if (pci_dev_is_disconnected(dev))
-		return -ENODEV;
+		return PCIBIOS_DEVICE_NOT_FOUND;
 	return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val);
 }
 EXPORT_SYMBOL(pci_write_config_byte);
@@ -934,7 +934,7 @@ EXPORT_SYMBOL(pci_write_config_byte);
 int pci_write_config_word(const struct pci_dev *dev, int where, u16 val)
 {
 	if (pci_dev_is_disconnected(dev))
-		return -ENODEV;
+		return PCIBIOS_DEVICE_NOT_FOUND;
 	return pci_bus_write_config_word(dev->bus, dev->devfn, where, val);
 }
 EXPORT_SYMBOL(pci_write_config_word);
@@ -943,7 +943,7 @@ int pci_write_config_dword(const struct pci_dev *dev, int where,
 					 u32 val)
 {
 	if (pci_dev_is_disconnected(dev))
-		return -ENODEV;
+		return PCIBIOS_DEVICE_NOT_FOUND;
 	return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
 }
 EXPORT_SYMBOL(pci_write_config_dword);
-- 
2.13.0.219.gdb65acc882-goog

WARNING: multiple messages have this Message-ID (diff)
From: Brian Norris <briannorris@chromium.org>
To: Shawn Lin <shawn.lin@rock-chips.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci@vger.kernel.org, Jeffy Chen <jeffy.chen@rock-chips.com>,
	linux-rockchip@lists.infradead.org,
	Krishna Dhulipala <krishnad@fb.com>,
	Keith Busch <keith.busch@intel.com>,
	Christoph Hellwig <hch@lst.de>, Wei Zhang <wzhang@fb.com>
Subject: [PATCH] PCI: Make error code types consistent in pci_{read,write}_config_*
Date: Tue, 23 May 2017 12:36:58 -0700	[thread overview]
Message-ID: <20170523193655.GA144183@google.com> (raw)
In-Reply-To: <20170523184359.GB115572@google.com>

Callers normally treat the config space accessors as returning PCBIOS_*
error codes, not Linux error codes (or they don't look at them at all).
We have pcibios_err_to_errno(), in case the error code needs translated.

Fixes: 4b1038834739 ("PCI: Don't attempt config access to disconnected devices")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
+ others, change subject

On Tue, May 23, 2017 at 11:44:01AM -0700, Brian Norris wrote:
> But the high level code doesn't handle this
> consistently. See, e.g., pci_read_config_byte() which can return regular
> Linux error codes (like -ENODEV), except it also passes up the return
> code of pci_read_config_byte() (a PCIBIOS_* code) directly.

Apparently this is new (inconsistent) behavior in 4.12-rc1. Seems like
an oversight to me.

> So callers don't really know whether to treat the value from
> pci_read_config_<foo>() as a PCIBIOS_* code (which should be translated
> with pcibios_err_to_errno()) or as a standard Linux errno.
> 
> But then, there are relatively few callers (less than 10% of
> pci_read_config_<foo>(); even fewer for writes) that actually check the
> error codes...
> 
> Maybe the "fix" is to replace -ENODEV with PCIBIOS_DEVICE_NOT_FOUND for
> the inconsistent cases (pci_{read,write}_config_{byte,word,dword}()).

Fix implemented in the surrounding patch.

 drivers/pci/access.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 74cf5fffb1e1..c80e37a69305 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -896,7 +896,7 @@ int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
 {
 	if (pci_dev_is_disconnected(dev)) {
 		*val = ~0;
-		return -ENODEV;
+		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 	return pci_bus_read_config_byte(dev->bus, dev->devfn, where, val);
 }
@@ -906,7 +906,7 @@ int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
 {
 	if (pci_dev_is_disconnected(dev)) {
 		*val = ~0;
-		return -ENODEV;
+		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 	return pci_bus_read_config_word(dev->bus, dev->devfn, where, val);
 }
@@ -917,7 +917,7 @@ int pci_read_config_dword(const struct pci_dev *dev, int where,
 {
 	if (pci_dev_is_disconnected(dev)) {
 		*val = ~0;
-		return -ENODEV;
+		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 	return pci_bus_read_config_dword(dev->bus, dev->devfn, where, val);
 }
@@ -926,7 +926,7 @@ EXPORT_SYMBOL(pci_read_config_dword);
 int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val)
 {
 	if (pci_dev_is_disconnected(dev))
-		return -ENODEV;
+		return PCIBIOS_DEVICE_NOT_FOUND;
 	return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val);
 }
 EXPORT_SYMBOL(pci_write_config_byte);
@@ -934,7 +934,7 @@ EXPORT_SYMBOL(pci_write_config_byte);
 int pci_write_config_word(const struct pci_dev *dev, int where, u16 val)
 {
 	if (pci_dev_is_disconnected(dev))
-		return -ENODEV;
+		return PCIBIOS_DEVICE_NOT_FOUND;
 	return pci_bus_write_config_word(dev->bus, dev->devfn, where, val);
 }
 EXPORT_SYMBOL(pci_write_config_word);
@@ -943,7 +943,7 @@ int pci_write_config_dword(const struct pci_dev *dev, int where,
 					 u32 val)
 {
 	if (pci_dev_is_disconnected(dev))
-		return -ENODEV;
+		return PCIBIOS_DEVICE_NOT_FOUND;
 	return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
 }
 EXPORT_SYMBOL(pci_write_config_dword);
-- 
2.13.0.219.gdb65acc882-goog

  reply	other threads:[~2017-05-23 19:37 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-19  6:58 [PATCH] PCI: rockchip: check link status when validating device Shawn Lin
2017-05-23 18:00 ` Brian Norris
2017-05-24  0:54   ` Shawn Lin
2017-05-24  1:00     ` Brian Norris
2017-05-24  1:14       ` Shawn Lin
2017-05-24  1:25         ` Brian Norris
2017-05-23 18:44 ` Brian Norris
2017-05-23 19:36   ` Brian Norris [this message]
2017-05-23 19:36     ` [PATCH] PCI: Make error code types consistent in pci_{read,write}_config_* Brian Norris
2017-05-25  6:50     ` Keith Busch
2017-05-26 21:40     ` Bjorn Helgaas
2017-05-26 21:40       ` Bjorn Helgaas
2017-05-23 19:44 ` [PATCH] PCI: rockchip: check link status when validating device Bjorn Helgaas
2017-05-24  1:04   ` Shawn Lin
2017-05-24  1:04     ` Shawn Lin
2017-05-24  1:15     ` Brian Norris
2017-05-24 21:33       ` Bjorn Helgaas
2017-05-24 21:43         ` Brian Norris

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=20170523193655.GA144183@google.com \
    --to=briannorris@chromium.org \
    --cc=bhelgaas@google.com \
    --cc=hch@lst.de \
    --cc=jeffy.chen@rock-chips.com \
    --cc=keith.busch@intel.com \
    --cc=krishnad@fb.com \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=shawn.lin@rock-chips.com \
    --cc=wzhang@fb.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.