[PATCH 0/2] PCI: Change to using pci_dev_is_inaccessible()

[PATCH 0/2] PCI: Change to using pci_dev_is_inaccessible()

[Kelsey Skunberg]

Patch 1: Change pci_device_is_present() name to pci_dev_is_inaccessible()
         to encourage only using to learn if we should not access a
         device that's inaccessible. Return value will need to be reversed
	 to reflect the name change.

Patch 2: Relies on patch 1. Unify pci_dev_is_disconnected() with
         pci_dev_is_inaccessible() so there is only one function
         needed to learn if a device is inaccessible due to surprise
         removal or an error condition.

Kelsey Skunberg (2):
  PCI: Change pci_device_is_present() to pci_dev_is_inaccessible()
  PCI: Unify pci_dev_is_disconnected() and pci_dev_is_inaccessible()

 drivers/net/ethernet/broadcom/tg3.c       |  4 ++--
 drivers/net/ethernet/intel/igb/igb_main.c |  2 +-
 drivers/nvme/host/pci.c                   |  2 +-
 drivers/pci/access.c                      | 12 ++++++------
 drivers/pci/hotplug/acpiphp_glue.c        |  2 +-
 drivers/pci/msi.c                         |  4 ++--
 drivers/pci/pci.c                         | 12 ++++++------
 drivers/pci/pci.h                         |  5 -----
 drivers/pci/pcie/portdrv_core.c           |  2 +-
 drivers/thunderbolt/nhi.c                 |  2 +-
 include/linux/pci.h                       |  2 +-
 11 files changed, 22 insertions(+), 27 deletions(-)

-- 
2.20.1

[PATCH 1/2] PCI: Change pci_device_is_present() to pci_dev_is_inaccessible()

[Kelsey Skunberg]

The pci_device_is_present() name may encourage poor practice of calling
pci_device_is_present() and if true, assuming the device is still present
after the call. This type of practice can be racy when assuming a device
is still connected after checking.

Change pci_device_is_present() to pci_dev_is_inaccessible() to promote
only using to learn whether we should avoid accessing a device that's
inaccessible.

Change pci_device_is_inaccessible() to now return true if PCI device is
inaccessible.

Change the boolean values returned from calling pci_dev_is_inaccessible()
to their opposite value to reflect the change of checking if the device is
present to checking if the device is inaccessible. Example:
	Before:
		if (!pci_device_is_present(tp->pdev))
			return -ENODEV;
	After:
		if (pci_dev_is_inaccessible(tp->pdev))
                	return -ENODEV;

Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com>
---
 drivers/net/ethernet/broadcom/tg3.c       |  4 ++--
 drivers/net/ethernet/intel/igb/igb_main.c |  2 +-
 drivers/nvme/host/pci.c                   |  2 +-
 drivers/pci/hotplug/acpiphp_glue.c        |  2 +-
 drivers/pci/pci.c                         | 10 +++++-----
 drivers/thunderbolt/nhi.c                 |  2 +-
 include/linux/pci.h                       |  2 +-
 7 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 4c404d2213f9..7646a8303d01 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -9067,7 +9067,7 @@ static int tg3_chip_reset(struct tg3 *tp)
 	void (*write_op)(struct tg3 *, u32, u32);
 	int i, err;
 
-	if (!pci_device_is_present(tp->pdev))
+	if (pci_dev_is_inaccessible(tp->pdev))
 		return -ENODEV;
 
 	tg3_nvram_lock(tp);
@@ -11782,7 +11782,7 @@ static int tg3_close(struct net_device *dev)
 
 	tg3_stop(tp);
 
-	if (pci_device_is_present(tp->pdev)) {
+	if (!pci_dev_is_inaccessible(tp->pdev)) {
 		tg3_power_down_prepare(tp);
 
 		tg3_carrier_off(tp);
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index b4df3e319467..87bc067c2abc 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -8835,7 +8835,7 @@ static int __maybe_unused igb_resume(struct device *dev)
 	pci_restore_state(pdev);
 	pci_save_state(pdev);
 
-	if (!pci_device_is_present(pdev))
+	if (pci_dev_is_inaccessible(pdev))
 		return -ENODEV;
 	err = pci_enable_device_mem(pdev);
 	if (err) {
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index bb970ca82517..2a4500325471 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2812,7 +2812,7 @@ static void nvme_remove(struct pci_dev *pdev)
 	nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING);
 	pci_set_drvdata(pdev, NULL);
 
-	if (!pci_device_is_present(pdev)) {
+	if (pci_dev_is_inaccessible(pdev)) {
 		nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DEAD);
 		nvme_dev_disable(dev, true);
 		nvme_dev_remove_admin(dev);
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index e4c46637f32f..9cc2d65877bd 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -647,7 +647,7 @@ static void trim_stale_devices(struct pci_dev *dev)
 		alive = alive || (ACPI_SUCCESS(status) && device_status_valid(sta));
 	}
 	if (!alive)
-		alive = pci_device_is_present(dev);
+		alive = !pci_dev_is_inaccessible(dev);
 
 	if (!alive) {
 		pci_dev_set_disconnected(dev, NULL);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 29ed5ec1ac27..7b4e248db5f9 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -931,7 +931,7 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
 void pci_update_current_state(struct pci_dev *dev, pci_power_t state)
 {
 	if (platform_pci_get_power_state(dev) == PCI_D3cold ||
-	    !pci_device_is_present(dev)) {
+	    pci_dev_is_inaccessible(dev)) {
 		dev->current_state = PCI_D3cold;
 	} else if (dev->pm_cap) {
 		u16 pmcsr;
@@ -5906,15 +5906,15 @@ bool pci_devs_are_dma_aliases(struct pci_dev *dev1, struct pci_dev *dev2)
 		test_bit(dev1->devfn, dev2->dma_alias_mask));
 }
 
-bool pci_device_is_present(struct pci_dev *pdev)
+bool pci_dev_is_inaccessible(struct pci_dev *pdev)
 {
 	u32 v;
 
 	if (pci_dev_is_disconnected(pdev))
-		return false;
-	return pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
+		return true;
+	return !pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
 }
-EXPORT_SYMBOL_GPL(pci_device_is_present);
+EXPORT_SYMBOL_GPL(pci_dev_is_inaccessible);
 
 void pci_ignore_hotplug(struct pci_dev *dev)
 {
diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index 27fbe62c7ddd..597c7579d882 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -879,7 +879,7 @@ static int nhi_resume_noirq(struct device *dev)
 	 * unplugged last device which causes the host controller to go
 	 * away on PCs.
 	 */
-	if (!pci_device_is_present(pdev))
+	if (pci_dev_is_inaccessible(pdev))
 		tb->nhi->going_away = true;
 	else
 		nhi_enable_int_throttling(tb->nhi);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 9e700d9f9f28..e599068537cf 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1202,7 +1202,7 @@ int __must_check pci_reassign_resource(struct pci_dev *dev, int i, resource_size
 void pci_release_resource(struct pci_dev *dev, int resno);
 int __must_check pci_resize_resource(struct pci_dev *dev, int i, int size);
 int pci_select_bars(struct pci_dev *dev, unsigned long flags);
-bool pci_device_is_present(struct pci_dev *pdev);
+bool pci_dev_is_inaccessible(struct pci_dev *pdev);
 void pci_ignore_hotplug(struct pci_dev *dev);
 
 int __printf(6, 7) pci_request_irq(struct pci_dev *dev, unsigned int nr,
-- 
2.20.1

[PATCH 2/2] PCI: Unify pci_dev_is_disconnected() and pci_dev_is_inaccessible()

[Kelsey Skunberg]

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(-)

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) ||
-- 
2.20.1

Re: [PATCH 2/2] PCI: Unify pci_dev_is_disconnected() and pci_dev_is_inaccessible()

[Lukas Wunner]

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

Re: [PATCH 2/2] PCI: Unify pci_dev_is_disconnected() and pci_dev_is_inaccessible()

[Bjorn Helgaas]

[+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

Re: [PATCH 2/2] PCI: Unify pci_dev_is_disconnected() and pci_dev_is_inaccessible()

[Kelsey Skunberg]

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