[PATCHv2 0/4] Limiting pci access requsets

[PATCHv2 0/4] Limiting pci access requsets

[Keith Busch]

Here's version two of this, and is even more aggressive than before
about reducing access that we know will end in failure.

The first two patches are new. It attempts to completely remove config
space access on removed devices.

Patch 1/4 adds a new state to pci_dev called "is_removed" that's set
when the device is detected to be inaccessible.

Patch 2/4 returns immediately from any config space access request on
devices in such a state so we don't rely on hardware to handle this.

I dropped the patch to abort searching pcie extended capabilities. Caching
the AER capability obviates any immediate need for that.

Patch 3/4 moves the cached aer_cap from the aer driver's private structure
to the pci_dev structure so all devices in the sub-tree know about this
capability postition.

Patch 4/4 is still skipping disabling MSI-x when removed, and I added a
new check prior to writing any MSI messages as well. The criteria being
checked is the new "is_removed" state rather than verifying the VID/DID is
valid, which potentially just introduces additional config space access.

This series reduces the number of non-functional MMIO and config space
accesses on a hot removed device from ~1000 commands to ~1, improving
software's time to teardown surprise removed devices.

Keith Busch (4):
  pci: Add is_removed state
  pci: No config access for removed devices
  pcie/aer: Cache capability position
  pci/msix: Skip disabling removed devices

 drivers/pci/hotplug/pciehp_pci.c   |  2 ++
 drivers/pci/msi.c                  |  7 ++++++-
 drivers/pci/pci.c                  |  9 ++++++++-
 drivers/pci/pcie/aer/aerdrv.c      | 10 +++++-----
 drivers/pci/pcie/aer/aerdrv_core.c | 12 ++++++------
 drivers/pci/pcie/pcie-dpc.c        |  1 +
 drivers/pci/probe.c                |  2 ++
 include/linux/pci.h                | 20 ++++++++++++++++++++
 8 files changed, 50 insertions(+), 13 deletions(-)

-- 
2.7.2

[PATCHv2 1/4] pci: Add is_removed state

[Keith Busch]

This adds a new state for devices that were once in the system, but
unexpectedly removed. This is so device teardown functions can observe
the device is not accessible so it may skip attempting to initialize
the hardware.

The pciehp and pcie-dpc drivers are aware of when the link is down, so
they explicitly set this flag when its handlers detect the device is gone.

The flag is also cached any time pci_device_is_present returns false. The
function checks the flag first to avoid additional config accesses to
removed devices.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/hotplug/pciehp_pci.c | 2 ++
 drivers/pci/pci.c                | 9 ++++++++-
 drivers/pci/pcie/pcie-dpc.c      | 1 +
 include/linux/pci.h              | 1 +
 4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index 9e69403..299ea5e 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -109,6 +109,8 @@ int pciehp_unconfigure_device(struct slot *p_slot)
 				break;
 			}
 		}
+		if (!presence)
+			dev->is_removed = 1;
 		pci_stop_and_remove_bus_device(dev);
 		/*
 		 * Ensure that no new Requests will be generated from
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index aab9d51..010e5f6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4924,7 +4924,14 @@ bool pci_device_is_present(struct pci_dev *pdev)
 {
 	u32 v;
 
-	return pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
+	if (pdev->is_removed)
+		return false;
+
+	if (!pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0)) {
+		pdev->is_removed = 1;
+		return false;
+	}
+	return true;
 }
 EXPORT_SYMBOL_GPL(pci_device_is_present);
 
diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 9811b14..2e5876c5 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -46,6 +46,7 @@ static void interrupt_event_handler(struct work_struct *work)
 	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
 					 bus_list) {
 		pci_dev_get(dev);
+		dev->is_removed = 1;
 		pci_stop_and_remove_bus_device(dev);
 		pci_dev_put(dev);
 	}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 7256f33..865d3ec 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -334,6 +334,7 @@ struct pci_dev {
 	unsigned int	multifunction:1;/* Part of multi-function device */
 	/* keep track of device state */
 	unsigned int	is_added:1;
+	unsigned int	is_removed:1;	/* device was surprise removed */
 	unsigned int	is_busmaster:1; /* device is busmaster */
 	unsigned int	no_msi:1;	/* device may not use msi */
 	unsigned int	no_64bit_msi:1; /* device may only use 32-bit MSIs */
-- 
2.7.2

[PATCHv2 2/4] pci: No config access for removed devices

[Keith Busch]

If we've  detected that the PCI device is removed, do not attempt to
access the device's config space.

On a config read, if the device is not present, the value returned will be
set to all 1's. This is the same as what hardware would normally return
when accessing a removed device, but we do not need to repeatedly test
hardware's completion capabilities if software can take a short-cut.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 include/linux/pci.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 865d3ec..1b62f7a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -932,28 +932,46 @@ struct pci_ops *pci_bus_set_ops(struct pci_bus *bus, struct pci_ops *ops);
 
 static inline int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
 {
+	if (unlikely(dev->is_removed)) {
+		*val = ~0;
+		return -ENODEV;
+	}
 	return pci_bus_read_config_byte(dev->bus, dev->devfn, where, val);
 }
 static inline int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
 {
+	if (unlikely(dev->is_removed)) {
+		*val = ~0;
+		return -ENODEV;
+	}
 	return pci_bus_read_config_word(dev->bus, dev->devfn, where, val);
 }
 static inline int pci_read_config_dword(const struct pci_dev *dev, int where,
 					u32 *val)
 {
+	if (unlikely(dev->is_removed)) {
+		*val = ~0;
+		return -ENODEV;
+	}
 	return pci_bus_read_config_dword(dev->bus, dev->devfn, where, val);
 }
 static inline int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val)
 {
+	if (unlikely(dev->is_removed))
+		return -ENODEV;
 	return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val);
 }
 static inline int pci_write_config_word(const struct pci_dev *dev, int where, u16 val)
 {
+	if (unlikely(dev->is_removed))
+		return -ENODEV;
 	return pci_bus_write_config_word(dev->bus, dev->devfn, where, val);
 }
 static inline int pci_write_config_dword(const struct pci_dev *dev, int where,
 					 u32 val)
 {
+	if (unlikely(dev->is_removed))
+		return -ENODEV;
 	return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
 }
 
-- 
2.7.2

[PATCHv2 3/4] pcie/aer: Cache capability position

[Keith Busch]

This saves the postition of the error reporting capability so that it
doesn't need to be rediscovered during error handling.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pcie/aer/aerdrv.c      | 10 +++++-----
 drivers/pci/pcie/aer/aerdrv_core.c | 12 ++++++------
 drivers/pci/probe.c                |  2 ++
 include/linux/pci.h                |  1 +
 4 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
index 49805a4..df64adb 100644
--- a/drivers/pci/pcie/aer/aerdrv.c
+++ b/drivers/pci/pcie/aer/aerdrv.c
@@ -130,7 +130,7 @@ static void aer_enable_rootport(struct aer_rpc *rpc)
 	pcie_capability_clear_word(pdev, PCI_EXP_RTCTL,
 				   SYSTEM_ERROR_INTR_ON_MESG_MASK);
 
-	aer_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR);
+	aer_pos = pdev->aer_cap;
 	/* Clear error status */
 	pci_read_config_dword(pdev, aer_pos + PCI_ERR_ROOT_STATUS, &reg32);
 	pci_write_config_dword(pdev, aer_pos + PCI_ERR_ROOT_STATUS, reg32);
@@ -169,7 +169,7 @@ static void aer_disable_rootport(struct aer_rpc *rpc)
 	 */
 	set_downstream_devices_error_reporting(pdev, false);
 
-	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR);
+	pos = pdev->aer_cap;
 	/* Disable Root's interrupt in response to error messages */
 	pci_read_config_dword(pdev, pos + PCI_ERR_ROOT_COMMAND, &reg32);
 	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
@@ -196,7 +196,7 @@ irqreturn_t aer_irq(int irq, void *context)
 	unsigned long flags;
 	int pos;
 
-	pos = pci_find_ext_capability(pdev->port, PCI_EXT_CAP_ID_ERR);
+	pos = pdev->port->aer_cap;
 	/*
 	 * Must lock access to Root Error Status Reg, Root Error ID Reg,
 	 * and Root error producer/consumer index
@@ -339,7 +339,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
 	u32 reg32;
 	int pos;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
+	pos = dev->aer_cap;
 
 	/* Disable Root's interrupt in response to error messages */
 	pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, &reg32);
@@ -392,7 +392,7 @@ static void aer_error_resume(struct pci_dev *dev)
 	pcie_capability_write_word(dev, PCI_EXP_DEVSTA, reg16);
 
 	/* Clean AER Root Error Status */
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
+	pos = dev->aer_cap;
 	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
 	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &mask);
 	if (dev->error_state == pci_channel_io_normal)
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 521e39c..47ac510 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -40,7 +40,7 @@ int pci_enable_pcie_error_reporting(struct pci_dev *dev)
 	if (pcie_aer_get_firmware_first(dev))
 		return -EIO;
 
-	if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR))
+	if (!dev->aer_cap)
 		return -EIO;
 
 	return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS);
@@ -62,7 +62,7 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
 	int pos;
 	u32 status;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
+	pos = dev->aer_cap;
 	if (!pos)
 		return -EIO;
 
@@ -83,7 +83,7 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
 	if (!pci_is_pcie(dev))
 		return -ENODEV;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
+	pos = dev->aer_cap;
 	if (!pos)
 		return -EIO;
 
@@ -158,7 +158,7 @@ static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info)
 	if (!(reg16 & PCI_EXP_AER_FLAGS))
 		return false;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
+	pos = dev->aer_cap;
 	if (!pos)
 		return false;
 
@@ -555,7 +555,7 @@ static void handle_error_source(struct pcie_device *aerdev,
 		 * Correctable error does not need software intervention.
 		 * No need to go through error recovery process.
 		 */
-		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
+		pos = dev->aer_cap;
 		if (pos)
 			pci_write_config_dword(dev, pos + PCI_ERR_COR_STATUS,
 					info->status);
@@ -647,7 +647,7 @@ static int get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
 	info->status = 0;
 	info->tlp_header_valid = 0;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
+	pos = dev->aer_cap;
 
 	/* The device might not support AER */
 	if (!pos)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index e2e4244..7c3fcba 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1666,6 +1666,8 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	/* Enable ACS P2P upstream forwarding */
 	pci_enable_acs(dev);
 
+	/* Advanced Error Reporting */
+	dev->aer_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
 	pci_cleanup_aer_error_status_regs(dev);
 
 	/* Precision Time Measurement */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1b62f7a..ee289da 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -268,6 +268,7 @@ struct pci_dev {
 	unsigned int	class;		/* 3 bytes: (base,sub,prog-if) */
 	u8		revision;	/* PCI revision, low byte of class word */
 	u8		hdr_type;	/* PCI header type (`multi' flag masked out) */
+	u16		aer_cap;	/* AER capability offset */
 	u8		pcie_cap;	/* PCIe capability offset */
 	u8		msi_cap;	/* MSI capability offset */
 	u8		msix_cap;	/* MSI-X capability offset */
-- 
2.7.2

[PATCHv2 4/4] pci/msix: Skip disabling removed devices

[Keith Busch]

There is no need to disable MSIx interrupts or write message data on a
device that isn't present. This patch checks the device removed state
prior to executing device shutdown operations so that teardown on removed
devices is quicker.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/msi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index a02981e..474d808 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -316,7 +316,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) {
+	if (dev->current_state != PCI_D0 || dev->is_removed) {
 		/* Don't touch the hardware now */
 	} else if (entry->msi_attrib.is_msix) {
 		void __iomem *base = pci_msix_desc_addr(entry);
@@ -999,6 +999,11 @@ void pci_msix_shutdown(struct pci_dev *dev)
 	if (!pci_msi_enable || !dev || !dev->msix_enabled)
 		return;
 
+	if (dev->is_removed) {
+		dev->msix_enabled = 0;
+		return;
+	}
+
 	/* Return the device with MSI-X masked as initial states */
 	for_each_pci_msi_entry(entry, dev) {
 		/* Keep cached states to be restored */
-- 
2.7.2

Re: [PATCHv2 1/4] pci: Add is_removed state

[Lukas Wunner]

On Tue, Sep 06, 2016 at 04:00:16PM -0600, Keith Busch wrote:
> This adds a new state for devices that were once in the system, but
> unexpectedly removed. This is so device teardown functions can observe
> the device is not accessible so it may skip attempting to initialize
                                  ^ they                    ^ access (?)

> the hardware.
> 
> The pciehp and pcie-dpc drivers are aware of when the link is down, so
> they explicitly set this flag when its handlers detect the device is gone.
                                     ^ their

> 
> The flag is also cached any time pci_device_is_present returns false. The
> function checks the flag first to avoid additional config accesses to
> removed devices.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

I think this is a significant improvement because with Thunderbolt,
devices can disappear at any time and many drivers currently access
the device in their ->remove hook, which is fine if the driver
detachment is caused by "modprobe -r" but entirely wrong in the case
of a surprise removal. This patch adds the necessary infrastructure
for drivers to adjust their behaviour during ->remove if the device
is gone.

Thanks,

Lukas

> ---
>  drivers/pci/hotplug/pciehp_pci.c | 2 ++
>  drivers/pci/pci.c                | 9 ++++++++-
>  drivers/pci/pcie/pcie-dpc.c      | 1 +
>  include/linux/pci.h              | 1 +
>  4 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
> index 9e69403..299ea5e 100644
> --- a/drivers/pci/hotplug/pciehp_pci.c
> +++ b/drivers/pci/hotplug/pciehp_pci.c
> @@ -109,6 +109,8 @@ int pciehp_unconfigure_device(struct slot *p_slot)
>  				break;
>  			}
>  		}
> +		if (!presence)
> +			dev->is_removed = 1;
>  		pci_stop_and_remove_bus_device(dev);
>  		/*
>  		 * Ensure that no new Requests will be generated from
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aab9d51..010e5f6 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4924,7 +4924,14 @@ bool pci_device_is_present(struct pci_dev *pdev)
>  {
>  	u32 v;
>  
> -	return pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
> +	if (pdev->is_removed)
> +		return false;
> +
> +	if (!pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0)) {
> +		pdev->is_removed = 1;
> +		return false;
> +	}
> +	return true;
>  }
>  EXPORT_SYMBOL_GPL(pci_device_is_present);
>  
> diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
> index 9811b14..2e5876c5 100644
> --- a/drivers/pci/pcie/pcie-dpc.c
> +++ b/drivers/pci/pcie/pcie-dpc.c
> @@ -46,6 +46,7 @@ static void interrupt_event_handler(struct work_struct *work)
>  	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
>  					 bus_list) {
>  		pci_dev_get(dev);
> +		dev->is_removed = 1;
>  		pci_stop_and_remove_bus_device(dev);
>  		pci_dev_put(dev);
>  	}
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 7256f33..865d3ec 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -334,6 +334,7 @@ struct pci_dev {
>  	unsigned int	multifunction:1;/* Part of multi-function device */
>  	/* keep track of device state */
>  	unsigned int	is_added:1;
> +	unsigned int	is_removed:1;	/* device was surprise removed */
>  	unsigned int	is_busmaster:1; /* device is busmaster */
>  	unsigned int	no_msi:1;	/* device may not use msi */
>  	unsigned int	no_64bit_msi:1; /* device may only use 32-bit MSIs */
> -- 
> 2.7.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 3/4] pcie/aer: Cache capability position

[Lukas Wunner]

On Tue, Sep 06, 2016 at 04:00:18PM -0600, Keith Busch wrote:
> This saves the postition of the error reporting capability so that it
                 ^ position

I'm wondering if the flag in struct pci_dev as well as the additional
code in probe.c should be enclosed in #ifdef CONFIG_PCIEAER, as we're
doing for ats_cap.

Otherwise LGTM.

Lukas

> doesn't need to be rediscovered during error handling.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/pci/pcie/aer/aerdrv.c      | 10 +++++-----
>  drivers/pci/pcie/aer/aerdrv_core.c | 12 ++++++------
>  drivers/pci/probe.c                |  2 ++
>  include/linux/pci.h                |  1 +
>  4 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
> index 49805a4..df64adb 100644
> --- a/drivers/pci/pcie/aer/aerdrv.c
> +++ b/drivers/pci/pcie/aer/aerdrv.c
> @@ -130,7 +130,7 @@ static void aer_enable_rootport(struct aer_rpc *rpc)
>  	pcie_capability_clear_word(pdev, PCI_EXP_RTCTL,
>  				   SYSTEM_ERROR_INTR_ON_MESG_MASK);
>  
> -	aer_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR);
> +	aer_pos = pdev->aer_cap;
>  	/* Clear error status */
>  	pci_read_config_dword(pdev, aer_pos + PCI_ERR_ROOT_STATUS, &reg32);
>  	pci_write_config_dword(pdev, aer_pos + PCI_ERR_ROOT_STATUS, reg32);
> @@ -169,7 +169,7 @@ static void aer_disable_rootport(struct aer_rpc *rpc)
>  	 */
>  	set_downstream_devices_error_reporting(pdev, false);
>  
> -	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR);
> +	pos = pdev->aer_cap;
>  	/* Disable Root's interrupt in response to error messages */
>  	pci_read_config_dword(pdev, pos + PCI_ERR_ROOT_COMMAND, &reg32);
>  	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> @@ -196,7 +196,7 @@ irqreturn_t aer_irq(int irq, void *context)
>  	unsigned long flags;
>  	int pos;
>  
> -	pos = pci_find_ext_capability(pdev->port, PCI_EXT_CAP_ID_ERR);
> +	pos = pdev->port->aer_cap;
>  	/*
>  	 * Must lock access to Root Error Status Reg, Root Error ID Reg,
>  	 * and Root error producer/consumer index
> @@ -339,7 +339,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
>  	u32 reg32;
>  	int pos;
>  
> -	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> +	pos = dev->aer_cap;
>  
>  	/* Disable Root's interrupt in response to error messages */
>  	pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, &reg32);
> @@ -392,7 +392,7 @@ static void aer_error_resume(struct pci_dev *dev)
>  	pcie_capability_write_word(dev, PCI_EXP_DEVSTA, reg16);
>  
>  	/* Clean AER Root Error Status */
> -	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> +	pos = dev->aer_cap;
>  	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
>  	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &mask);
>  	if (dev->error_state == pci_channel_io_normal)
> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
> index 521e39c..47ac510 100644
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -40,7 +40,7 @@ int pci_enable_pcie_error_reporting(struct pci_dev *dev)
>  	if (pcie_aer_get_firmware_first(dev))
>  		return -EIO;
>  
> -	if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR))
> +	if (!dev->aer_cap)
>  		return -EIO;
>  
>  	return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS);
> @@ -62,7 +62,7 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
>  	int pos;
>  	u32 status;
>  
> -	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> +	pos = dev->aer_cap;
>  	if (!pos)
>  		return -EIO;
>  
> @@ -83,7 +83,7 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>  	if (!pci_is_pcie(dev))
>  		return -ENODEV;
>  
> -	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> +	pos = dev->aer_cap;
>  	if (!pos)
>  		return -EIO;
>  
> @@ -158,7 +158,7 @@ static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info)
>  	if (!(reg16 & PCI_EXP_AER_FLAGS))
>  		return false;
>  
> -	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> +	pos = dev->aer_cap;
>  	if (!pos)
>  		return false;
>  
> @@ -555,7 +555,7 @@ static void handle_error_source(struct pcie_device *aerdev,
>  		 * Correctable error does not need software intervention.
>  		 * No need to go through error recovery process.
>  		 */
> -		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> +		pos = dev->aer_cap;
>  		if (pos)
>  			pci_write_config_dword(dev, pos + PCI_ERR_COR_STATUS,
>  					info->status);
> @@ -647,7 +647,7 @@ static int get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
>  	info->status = 0;
>  	info->tlp_header_valid = 0;
>  
> -	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> +	pos = dev->aer_cap;
>  
>  	/* The device might not support AER */
>  	if (!pos)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index e2e4244..7c3fcba 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1666,6 +1666,8 @@ static void pci_init_capabilities(struct pci_dev *dev)
>  	/* Enable ACS P2P upstream forwarding */
>  	pci_enable_acs(dev);
>  
> +	/* Advanced Error Reporting */
> +	dev->aer_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
>  	pci_cleanup_aer_error_status_regs(dev);
>  
>  	/* Precision Time Measurement */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 1b62f7a..ee289da 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -268,6 +268,7 @@ struct pci_dev {
>  	unsigned int	class;		/* 3 bytes: (base,sub,prog-if) */
>  	u8		revision;	/* PCI revision, low byte of class word */
>  	u8		hdr_type;	/* PCI header type (`multi' flag masked out) */
> +	u16		aer_cap;	/* AER capability offset */
>  	u8		pcie_cap;	/* PCIe capability offset */
>  	u8		msi_cap;	/* MSI capability offset */
>  	u8		msix_cap;	/* MSI-X capability offset */
> -- 
> 2.7.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 2/4] pci: No config access for removed devices

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3241 bytes --]

Hi Keith,

[auto build test ERROR on pci/next]
[also build test ERROR on v4.8-rc5 next-20160907]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Keith-Busch/Limiting-pci-access-requsets/20160907-170240
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: mips-jmr3927_defconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mips 

All errors (new ones prefixed by >>):

   arch/mips/txx9/generic/pci.c: In function 'early_read_config_word':
>> arch/mips/txx9/generic/pci.c:49:1: error: the frame size of 1528 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
    }
    ^
   cc1: all warnings being treated as errors

vim +49 arch/mips/txx9/generic/pci.c

89d63fe1 Atsushi Nemoto 2008-07-11  33  	struct pci_bus fake_bus;
89d63fe1 Atsushi Nemoto 2008-07-11  34  
89d63fe1 Atsushi Nemoto 2008-07-11  35  	fake_dev.bus = &fake_bus;
89d63fe1 Atsushi Nemoto 2008-07-11  36  	fake_dev.sysdata = hose;
89d63fe1 Atsushi Nemoto 2008-07-11  37  	fake_dev.devfn = devfn;
89d63fe1 Atsushi Nemoto 2008-07-11  38  	fake_bus.number = bus;
89d63fe1 Atsushi Nemoto 2008-07-11  39  	fake_bus.sysdata = hose;
89d63fe1 Atsushi Nemoto 2008-07-11  40  	fake_bus.ops = hose->pci_ops;
89d63fe1 Atsushi Nemoto 2008-07-11  41  
89d63fe1 Atsushi Nemoto 2008-07-11  42  	if (bus != top_bus)
89d63fe1 Atsushi Nemoto 2008-07-11  43  		/* Fake a parent bus structure. */
89d63fe1 Atsushi Nemoto 2008-07-11  44  		fake_bus.parent = &fake_bus;
89d63fe1 Atsushi Nemoto 2008-07-11  45  	else
89d63fe1 Atsushi Nemoto 2008-07-11  46  		fake_bus.parent = NULL;
89d63fe1 Atsushi Nemoto 2008-07-11  47  
89d63fe1 Atsushi Nemoto 2008-07-11  48  	return pci_read_config_word(&fake_dev, offset, value);
89d63fe1 Atsushi Nemoto 2008-07-11 @49  }
89d63fe1 Atsushi Nemoto 2008-07-11  50  
89d63fe1 Atsushi Nemoto 2008-07-11  51  int __init txx9_pci66_check(struct pci_controller *hose, int top_bus,
89d63fe1 Atsushi Nemoto 2008-07-11  52  			    int current_bus)
89d63fe1 Atsushi Nemoto 2008-07-11  53  {
89d63fe1 Atsushi Nemoto 2008-07-11  54  	u32 pci_devfn;
89d63fe1 Atsushi Nemoto 2008-07-11  55  	unsigned short vid;
89d63fe1 Atsushi Nemoto 2008-07-11  56  	int cap66 = -1;
89d63fe1 Atsushi Nemoto 2008-07-11  57  	u16 stat;

:::::: The code at line 49 was first introduced by commit
:::::: 89d63fe179520b11f54de1f26755b7444c79e73a [MIPS] TXx9: Reorganize PCI code

:::::: TO: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
:::::: CC: Ralf Baechle <ralf@linux-mips.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 10651 bytes --]

Re: [PATCHv2 0/4] Limiting pci access requsets

[Lukas Wunner]

On Tue, Sep 06, 2016 at 04:00:15PM -0600, Keith Busch wrote:
> Here's version two of this, and is even more aggressive than before
> about reducing access that we know will end in failure.

I've briefly tested the whole series with Thunderbolt on the Mac
(which uses native pciehp, unlike the firmware-based Thunderbolt
on non-Macs, which somehow interfaces with the OS with acpiphp),
i.e. plug/unplug, suspend/resume. Everything still seems to work
as before, found no regressions, so this is

Tested-by: Lukas Wunner <lukas@wunner.de>

Thanks,

Lukas

Re: [PATCHv2 2/4] pci: No config access for removed devices

[Keith Busch]

On Wed, Sep 07, 2016 at 08:03:32PM +0800, kbuild test robot wrote:
> All errors (new ones prefixed by >>):
> 
>    arch/mips/txx9/generic/pci.c: In function 'early_read_config_word':
> >> arch/mips/txx9/generic/pci.c:49:1: error: the frame size of 1528 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>     }
>     ^
>
>    cc1: all warnings being treated as errors
> 
> vim +49 arch/mips/txx9/generic/pci.c
> 
> 89d63fe1 Atsushi Nemoto 2008-07-11  33  	struct pci_bus fake_bus;
> 89d63fe1 Atsushi Nemoto 2008-07-11  34  
> 89d63fe1 Atsushi Nemoto 2008-07-11  35  	fake_dev.bus = &fake_bus;
> 89d63fe1 Atsushi Nemoto 2008-07-11  36  	fake_dev.sysdata = hose;
> 89d63fe1 Atsushi Nemoto 2008-07-11  37  	fake_dev.devfn = devfn;
> 89d63fe1 Atsushi Nemoto 2008-07-11  38  	fake_bus.number = bus;
> 89d63fe1 Atsushi Nemoto 2008-07-11  39  	fake_bus.sysdata = hose;
> 89d63fe1 Atsushi Nemoto 2008-07-11  40  	fake_bus.ops = hose->pci_ops;
> 89d63fe1 Atsushi Nemoto 2008-07-11  41  
> 89d63fe1 Atsushi Nemoto 2008-07-11  42  	if (bus != top_bus)
> 89d63fe1 Atsushi Nemoto 2008-07-11  43  		/* Fake a parent bus structure. */
> 89d63fe1 Atsushi Nemoto 2008-07-11  44  		fake_bus.parent = &fake_bus;
> 89d63fe1 Atsushi Nemoto 2008-07-11  45  	else
> 89d63fe1 Atsushi Nemoto 2008-07-11  46  		fake_bus.parent = NULL;
> 89d63fe1 Atsushi Nemoto 2008-07-11  47  
> 89d63fe1 Atsushi Nemoto 2008-07-11  48  	return pci_read_config_word(&fake_dev, offset, value);
> 89d63fe1 Atsushi Nemoto 2008-07-11 @49  }


Hmm, why is this function allocating such large structures on the
stack? It looks like the size of struct pci_dev and struct pci_bus
already exceeded 1024 bytes.

I have no hardware to test this, but it looks to me there is no reason to
have struct pci_dev and the following should save quite a bite of space:

---
diff --git a/arch/mips/txx9/generic/pci.c b/arch/mips/txx9/generic/pci.c
index 1f6bc9a..285d84e 100644
--- a/arch/mips/txx9/generic/pci.c
+++ b/arch/mips/txx9/generic/pci.c
@@ -29,12 +29,8 @@ static int __init
 early_read_config_word(struct pci_controller *hose,
 		       int top_bus, int bus, int devfn, int offset, u16 *value)
 {
-	struct pci_dev fake_dev;
 	struct pci_bus fake_bus;
 
-	fake_dev.bus = &fake_bus;
-	fake_dev.sysdata = hose;
-	fake_dev.devfn = devfn;
 	fake_bus.number = bus;
 	fake_bus.sysdata = hose;
 	fake_bus.ops = hose->pci_ops;
@@ -45,7 +41,7 @@ early_read_config_word(struct pci_controller *hose,
 	else
 		fake_bus.parent = NULL;
 
-	return pci_read_config_word(&fake_dev, offset, value);
+	return pci_bus_read_config_word(&fake_bus, devfn, offset, value);
 }
 
 int __init txx9_pci66_check(struct pci_controller *hose, int top_bus,
--

Re: [PATCHv2 3/4] pcie/aer: Cache capability position

[Bjorn Helgaas]

On Wed, Sep 07, 2016 at 01:30:01PM +0200, Lukas Wunner wrote:
> On Tue, Sep 06, 2016 at 04:00:18PM -0600, Keith Busch wrote:
> > This saves the postition of the error reporting capability so that it
>                  ^ position
> 
> I'm wondering if the flag in struct pci_dev as well as the additional
> code in probe.c should be enclosed in #ifdef CONFIG_PCIEAER, as we're
> doing for ats_cap.

I agree about the flag in struct pci_dev, but instead of an #ifdef on
probe.c, can you make a pci_aer_init() that looks like pci_ats_init(), and
also includes the pci_cleanup_aer_error_status_regs() call?  I was hoping
we could get rid of the stub pci_cleanup_aer_error_status_regs() at the
same time, but I guess we can't because that's still called from
pci_restore_state().

> > doesn't need to be rediscovered during error handling.
> > 
> > Signed-off-by: Keith Busch <keith.busch@intel.com>
> > ---
> >  drivers/pci/pcie/aer/aerdrv.c      | 10 +++++-----
> >  drivers/pci/pcie/aer/aerdrv_core.c | 12 ++++++------
> >  drivers/pci/probe.c                |  2 ++
> >  include/linux/pci.h                |  1 +
> >  4 files changed, 14 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
> > index 49805a4..df64adb 100644
> > --- a/drivers/pci/pcie/aer/aerdrv.c
> > +++ b/drivers/pci/pcie/aer/aerdrv.c
> > @@ -130,7 +130,7 @@ static void aer_enable_rootport(struct aer_rpc *rpc)
> >  	pcie_capability_clear_word(pdev, PCI_EXP_RTCTL,
> >  				   SYSTEM_ERROR_INTR_ON_MESG_MASK);
> >  
> > -	aer_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR);
> > +	aer_pos = pdev->aer_cap;
> >  	/* Clear error status */
> >  	pci_read_config_dword(pdev, aer_pos + PCI_ERR_ROOT_STATUS, &reg32);
> >  	pci_write_config_dword(pdev, aer_pos + PCI_ERR_ROOT_STATUS, reg32);
> > @@ -169,7 +169,7 @@ static void aer_disable_rootport(struct aer_rpc *rpc)
> >  	 */
> >  	set_downstream_devices_error_reporting(pdev, false);
> >  
> > -	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR);
> > +	pos = pdev->aer_cap;
> >  	/* Disable Root's interrupt in response to error messages */
> >  	pci_read_config_dword(pdev, pos + PCI_ERR_ROOT_COMMAND, &reg32);
> >  	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> > @@ -196,7 +196,7 @@ irqreturn_t aer_irq(int irq, void *context)
> >  	unsigned long flags;
> >  	int pos;
> >  
> > -	pos = pci_find_ext_capability(pdev->port, PCI_EXT_CAP_ID_ERR);
> > +	pos = pdev->port->aer_cap;
> >  	/*
> >  	 * Must lock access to Root Error Status Reg, Root Error ID Reg,
> >  	 * and Root error producer/consumer index
> > @@ -339,7 +339,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
> >  	u32 reg32;
> >  	int pos;
> >  
> > -	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> > +	pos = dev->aer_cap;
> >  
> >  	/* Disable Root's interrupt in response to error messages */
> >  	pci_read_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, &reg32);
> > @@ -392,7 +392,7 @@ static void aer_error_resume(struct pci_dev *dev)
> >  	pcie_capability_write_word(dev, PCI_EXP_DEVSTA, reg16);
> >  
> >  	/* Clean AER Root Error Status */
> > -	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> > +	pos = dev->aer_cap;
> >  	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
> >  	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &mask);
> >  	if (dev->error_state == pci_channel_io_normal)
> > diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
> > index 521e39c..47ac510 100644
> > --- a/drivers/pci/pcie/aer/aerdrv_core.c
> > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> > @@ -40,7 +40,7 @@ int pci_enable_pcie_error_reporting(struct pci_dev *dev)
> >  	if (pcie_aer_get_firmware_first(dev))
> >  		return -EIO;
> >  
> > -	if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR))
> > +	if (!dev->aer_cap)
> >  		return -EIO;
> >  
> >  	return pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_AER_FLAGS);
> > @@ -62,7 +62,7 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
> >  	int pos;
> >  	u32 status;
> >  
> > -	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> > +	pos = dev->aer_cap;
> >  	if (!pos)
> >  		return -EIO;
> >  
> > @@ -83,7 +83,7 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
> >  	if (!pci_is_pcie(dev))
> >  		return -ENODEV;
> >  
> > -	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> > +	pos = dev->aer_cap;
> >  	if (!pos)
> >  		return -EIO;
> >  
> > @@ -158,7 +158,7 @@ static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info)
> >  	if (!(reg16 & PCI_EXP_AER_FLAGS))
> >  		return false;
> >  
> > -	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> > +	pos = dev->aer_cap;
> >  	if (!pos)
> >  		return false;
> >  
> > @@ -555,7 +555,7 @@ static void handle_error_source(struct pcie_device *aerdev,
> >  		 * Correctable error does not need software intervention.
> >  		 * No need to go through error recovery process.
> >  		 */
> > -		pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> > +		pos = dev->aer_cap;
> >  		if (pos)
> >  			pci_write_config_dword(dev, pos + PCI_ERR_COR_STATUS,
> >  					info->status);
> > @@ -647,7 +647,7 @@ static int get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
> >  	info->status = 0;
> >  	info->tlp_header_valid = 0;
> >  
> > -	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> > +	pos = dev->aer_cap;
> >  
> >  	/* The device might not support AER */
> >  	if (!pos)
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index e2e4244..7c3fcba 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -1666,6 +1666,8 @@ static void pci_init_capabilities(struct pci_dev *dev)
> >  	/* Enable ACS P2P upstream forwarding */
> >  	pci_enable_acs(dev);
> >  
> > +	/* Advanced Error Reporting */
> > +	dev->aer_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> >  	pci_cleanup_aer_error_status_regs(dev);
> >  
> >  	/* Precision Time Measurement */
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 1b62f7a..ee289da 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -268,6 +268,7 @@ struct pci_dev {
> >  	unsigned int	class;		/* 3 bytes: (base,sub,prog-if) */
> >  	u8		revision;	/* PCI revision, low byte of class word */
> >  	u8		hdr_type;	/* PCI header type (`multi' flag masked out) */
> > +	u16		aer_cap;	/* AER capability offset */
> >  	u8		pcie_cap;	/* PCIe capability offset */
> >  	u8		msi_cap;	/* MSI capability offset */
> >  	u8		msix_cap;	/* MSI-X capability offset */
> > -- 
> > 2.7.2
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 1/4] pci: Add is_removed state

[Lukas Wunner]

On Tue, Sep 06, 2016 at 04:00:16PM -0600, Keith Busch wrote:
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4924,7 +4924,14 @@ bool pci_device_is_present(struct pci_dev *pdev)
>  {
>  	u32 v;
>  
> -	return pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
> +	if (pdev->is_removed)
> +		return false;
> +
> +	if (!pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0)) {
> +		pdev->is_removed = 1;
> +		return false;
> +	}
> +	return true;
>  }
>  EXPORT_SYMBOL_GPL(pci_device_is_present);

I've kept this series on my development branch and found a bug now:

In the above hunk, it's okay to return false if pdev->is_removed is set,
but it's not okay to set pdev->is_removed if pci_bus_read_dev_vendor_id()
returns false.  That's because pci_bus_read_dev_vendor_id() can fail for
other reasons, such as the device being powered down to D3cold, or
currently unreachable because a PCIe port above it was suspended to D3hot
so that the link is down.  Those are transient issues, the device isn't
removed in those cases.

IOW the hunk should look like this:

 {
 	u32 v;
 
+	if (pdev->is_removed)
+		return false;
+
 	return pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
 }

And this should then probably be moved to patch [2/4].

Best regards,

Lukas

Re: [PATCHv2 1/4] pci: Add is_removed state

[Keith Busch]

On Sat, Sep 17, 2016 at 10:35:06AM +0200, Lukas Wunner wrote:
> On Tue, Sep 06, 2016 at 04:00:16PM -0600, Keith Busch wrote:
> > -	return pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
> > +	if (pdev->is_removed)
> > +		return false;
> > +
> > +	if (!pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0)) {
> > +		pdev->is_removed = 1;
> > +		return false;
> > +	}
> > +	return true;
> >  }
> >  EXPORT_SYMBOL_GPL(pci_device_is_present);
> 
> I've kept this series on my development branch and found a bug now:
> 
> In the above hunk, it's okay to return false if pdev->is_removed is set,
> but it's not okay to set pdev->is_removed if pci_bus_read_dev_vendor_id()
> returns false.  That's because pci_bus_read_dev_vendor_id() can fail for
> other reasons, such as the device being powered down to D3cold, or
> currently unreachable because a PCIe port above it was suspended to D3hot
> so that the link is down.  Those are transient issues, the device isn't
> removed in those cases.

Thanks for the info. I wasn't sure about making the removed return sticky
on a failed vendor id check, so I will remove that from this path and
resend the entire series.