[2/2] PCI: Unify pci_dev_is_disconnected() and pci_dev_is_inaccessible()
diff mbox series

Message ID 20190904043633.65026-3-skunberg.kelsey@gmail.com
State New
Headers show
Series
  • PCI: Change to using pci_dev_is_inaccessible()
Related show

Commit Message

Kelsey Skunberg Sept. 4, 2019, 4:36 a.m. UTC
Combine pci_dev_is_disconnected() with pci_dev_is_inaccessible() so only
one function is used to learn if we should avoid accessing a device that's
inaccessible due to surprise removal or an error condition.

The use cases for pci_dev_is_disconnected() do not need to distinguish
between a device being inaccessible due to a surprise removal or an error
condition. This provides the opportunity to unify
pci_dev_is_disconnected() and pci_dev_is_inaccessible() to reduce multiple
functions used for the same task.

Change pci_dev_is_disconnected() call inside pci_dev_is_inaccessible() to:

	pdev->error_state == pci_channel_io_perm_failure

Change remaining pci_dev_is_disconnected() calls to
pci_dev_is_inaccessible() calls.

Remove pci_dev_is_disconnected() from /pci/pci.h which would now no longer
be used.

Demonstration of changes to pci_dev_is_disconnected() and
pci_dev_is_inaccessible():

Before combining:

	static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
	{
		return dev->error_state == pci_channel_io_perm_failure;
	}

	bool pci_dev_is_inaccessible(struct pci_dev *pdev)
	{
		u32 v;

		if (pci_dev_is_disconnected(pdev))
			return true;
		return !pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
	}

After combining:

	bool pci_dev_is_inaccessible(struct pci_dev *pdev)
	{
		u32 v;

		if (pdev->error_state == pci_channel_io_perm_failure)
			return true;
		return !pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
	}

Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
---
 drivers/pci/access.c            | 12 ++++++------
 drivers/pci/msi.c               |  4 ++--
 drivers/pci/pci.c               |  2 +-
 drivers/pci/pci.h               |  5 -----
 drivers/pci/pcie/portdrv_core.c |  2 +-
 5 files changed, 10 insertions(+), 15 deletions(-)

Comments

Lukas Wunner Sept. 4, 2019, 5:35 a.m. UTC | #1
On Tue, Sep 03, 2019 at 10:36:35PM -0600, Kelsey Skunberg wrote:
> Change pci_dev_is_disconnected() call inside pci_dev_is_inaccessible() to:
> 
> 	pdev->error_state == pci_channel_io_perm_failure
> 
> Change remaining pci_dev_is_disconnected() calls to
> pci_dev_is_inaccessible() calls.

I don't think that's a good idea because it introduces a config space read
(for the vendor ID) in places where we don't want that.  E.g., after the
check of pdev->error_state, a regular config space read may take place and
if that returns all ones, we may already be able to determine that the
device is inaccessible, obviating the need for a vendor ID check.
Config space reads aren't for free.

Thanks,

Lukas
Bjorn Helgaas Sept. 4, 2019, 6:45 p.m. UTC | #2
[+cc Carolyn, author of 17a402a0075c]

On Wed, Sep 04, 2019 at 07:35:23AM +0200, Lukas Wunner wrote:
> On Tue, Sep 03, 2019 at 10:36:35PM -0600, Kelsey Skunberg wrote:
> > Change pci_dev_is_disconnected() call inside pci_dev_is_inaccessible() to:
> > 
> > 	pdev->error_state == pci_channel_io_perm_failure
> > 
> > Change remaining pci_dev_is_disconnected() calls to
> > pci_dev_is_inaccessible() calls.
> 
> I don't think that's a good idea because it introduces a config space read
> (for the vendor ID) in places where we don't want that.  E.g., after the
> check of pdev->error_state, a regular config space read may take place and
> if that returns all ones, we may already be able to determine that the
> device is inaccessible, obviating the need for a vendor ID check.

Oh, I think I see what you mean: Previously pci_read_config_byte() et
al called pci_dev_is_disconnected(), which only checked
dev->error_state.

If we applied this patch, those sites would call
pci_dev_is_inaccessible(), which would check error_state and then (in
the common case where we haven't set error_state) do a config read of
the vendor ID.

So we would basically double the config access overhead because we'd
be doing an extra read of the vendor ID before every access.  That
indeed doesn't seem practical.

I think what we need to figure out is whether we really need two
interfaces (one that looks only at dev->error_state and a second that
looks at dev->error_state and also reads the vendor ID).  If we do
need both, then I think we need a little guidance in the function
comments about when to use one vs the other.

There are only a few uses of pci_device_is_present() (which looks at
dev->error_state and also reads the vendor ID) and they were added
here:

  8496e85c20e7 ("PCI / tg3: Give up chip reset and carrier loss handling if PCI device is not present")
  17a402a0075c ("igb: Fixes needed for surprise removal support")
  6db28eda2660 ("nvme/pci: Disable on removal when disconnected")
  b8a62d540240 ("ACPI / hotplug / PCI: Use pci_device_is_present()")
  4ebe34503baa ("ACPI / hotplug / PCI: Check for new devices on enabled slots")
  a6a64026c0cd ("PCI: Recognize D3cold in pci_update_current_state()")

The ACPI and PCI core uses are basically enumeration-type things so
that mostly makes sense to me.

I'm not so sure about the driver uses though.  I wonder if those could
be better handled by having the drivers check for ~0 error response
data from MMIO and config reads.

Bjorn
Kelsey Skunberg Sept. 5, 2019, 6:43 a.m. UTC | #3
On Wed, Sep 04, 2019 at 07:35:23AM +0200, Lukas Wunner wrote:
> On Tue, Sep 03, 2019 at 10:36:35PM -0600, Kelsey Skunberg wrote:
> > Change pci_dev_is_disconnected() call inside pci_dev_is_inaccessible() to:
> > 
> > 	pdev->error_state == pci_channel_io_perm_failure
> > 
> > Change remaining pci_dev_is_disconnected() calls to
> > pci_dev_is_inaccessible() calls.
> 
> I don't think that's a good idea because it introduces a config space read
> (for the vendor ID) in places where we don't want that.  E.g., after the
> check of pdev->error_state, a regular config space read may take place and
> if that returns all ones, we may already be able to determine that the
> device is inaccessible, obviating the need for a vendor ID check.
> Config space reads aren't for free.
> 
> Thanks,
> 
> Lukas

Good note. I definitely see why that would be undesirable. Thanks for
taking the time to point this out, Lukas. I'll look this over again to see
if a better solution can be done, or as Bjorn suggested, at least see if
clarification on when to use one vs. the other can be included.

Thanks again!

-Kelsey

Patch
diff mbox series

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 544922f097c0..c096340afb8c 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -535,7 +535,7 @@  EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
 
 int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
 {
-	if (pci_dev_is_disconnected(dev)) {
+	if (pci_dev_is_inaccessible(dev)) {
 		*val = ~0;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
@@ -545,7 +545,7 @@  EXPORT_SYMBOL(pci_read_config_byte);
 
 int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
 {
-	if (pci_dev_is_disconnected(dev)) {
+	if (pci_dev_is_inaccessible(dev)) {
 		*val = ~0;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
@@ -556,7 +556,7 @@  EXPORT_SYMBOL(pci_read_config_word);
 int pci_read_config_dword(const struct pci_dev *dev, int where,
 					u32 *val)
 {
-	if (pci_dev_is_disconnected(dev)) {
+	if (pci_dev_is_inaccessible(dev)) {
 		*val = ~0;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
@@ -566,7 +566,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))
+	if (pci_dev_is_inaccessible(dev))
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val);
 }
@@ -574,7 +574,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))
+	if (pci_dev_is_inaccessible(dev))
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	return pci_bus_write_config_word(dev->bus, dev->devfn, where, val);
 }
@@ -583,7 +583,7 @@  EXPORT_SYMBOL(pci_write_config_word);
 int pci_write_config_dword(const struct pci_dev *dev, int where,
 					 u32 val)
 {
-	if (pci_dev_is_disconnected(dev))
+	if (pci_dev_is_inaccessible(dev))
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
 }
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 0884bedcfc7a..4680043aa315 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -311,7 +311,7 @@  void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
 	struct pci_dev *dev = msi_desc_to_pci_dev(entry);
 
-	if (dev->current_state != PCI_D0 || pci_dev_is_disconnected(dev)) {
+	if (dev->current_state != PCI_D0 || pci_dev_is_inaccessible(dev)) {
 		/* Don't touch the hardware now */
 	} else if (entry->msi_attrib.is_msix) {
 		void __iomem *base = pci_msix_desc_addr(entry);
@@ -1008,7 +1008,7 @@  static void pci_msix_shutdown(struct pci_dev *dev)
 	if (!pci_msi_enable || !dev || !dev->msix_enabled)
 		return;
 
-	if (pci_dev_is_disconnected(dev)) {
+	if (pci_dev_is_inaccessible(dev)) {
 		dev->msix_enabled = 0;
 		return;
 	}
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7b4e248db5f9..e5f46d98dbe1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5910,7 +5910,7 @@  bool pci_dev_is_inaccessible(struct pci_dev *pdev)
 {
 	u32 v;
 
-	if (pci_dev_is_disconnected(pdev))
+	if (pdev->error_state == pci_channel_io_perm_failure)
 		return true;
 	return !pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
 }
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 1be03a97cb92..f0dc86dc8aab 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -363,11 +363,6 @@  static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
 	return 0;
 }
 
-static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
-{
-	return dev->error_state == pci_channel_io_perm_failure;
-}
-
 /* pci_dev priv_flags */
 #define PCI_DEV_ADDED 0
 
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 308c3e0c4a34..8bf6b47dd2c6 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -416,7 +416,7 @@  static void wait_for_downstream_link(struct pci_dev *pdev)
 	    pci_pcie_type(pdev) != PCI_EXP_TYPE_DOWNSTREAM)
 		return;
 
-	if (pci_dev_is_disconnected(pdev))
+	if (pci_dev_is_inaccessible(pdev))
 		return;
 
 	if (!pdev->subordinate || list_empty(&pdev->subordinate->devices) ||