[PATCHv3 0/5] PCI access on removed devices

[PATCHv3 0/5] PCI access on removed devices

[Keith Busch]

We observe that software's removing of a device generates potentially
1000's of MMIO and config space access when unbinding from the removed
hardware, and this reliance on hardware to handle this slows down
software's total tear down time for handling a removed device.

This series reduces the number of non-functional MMIO and config space
accesses on a hot removed device. In some cases, we measure device
unbinding that used to take 100's of milliseconds to complete in < 1ms.

v3 -> v4:

  Merged up to current pci/hotplug.

  The aer initialization is done only when CONFIG_PCIAER is enabled,
  similar to how ATS support is managed.

  Removed making "is_removed" sticky on the checking
  pci_device_is_present. Only pciehp and pcie-dpc set this state in
  this series.

  Added the MIPs dependency. This was okay'ed, but I don't see it in
  the linux-mips tree.

Keith Busch (5):
  mips/pci: Reduce stack frame usage
  pci: Add is_removed state
  pci: No config access for removed devices
  pcie/aer: Cache capability position
  pci/msix: Skip disabling removed devices

 arch/mips/txx9/generic/pci.c       |  6 +-----
 drivers/pci/hotplug/pciehp_pci.c   |  2 ++
 drivers/pci/msi.c                  |  7 ++++++-
 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, 43 insertions(+), 17 deletions(-)

-- 
2.7.2

[PATCHv3 1/5] mips/pci: Reduce stack frame usage

[Keith Busch]

The struct pci_dev is too large to allocate on the stack. This patch
removes the fake pci device in early config access and instead uses the
pci bus to get the same functionality.

Signed-off-by: Keith Busch <keith.busch@intel.com>
Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
Got the 'ok' (link below), but I don't see this is merged yet. That's
okay, since I need to include it in this series to avoid 0'day build
failure. If this is ok with everyone, perhaps this can go through 'pci'.

  https://www.linux-mips.org/archives/linux-mips/2016-09/msg00255.html

 arch/mips/txx9/generic/pci.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

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,
-- 
2.7.2

[PATCHv3 2/5] 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 tear down 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 these explicitly set this flag when its handlers detect the device
is gone.

Signed-off-by: Keith Busch <keith.busch@intel.com>
Cc: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/hotplug/pciehp_pci.c | 2 ++
 drivers/pci/pcie/pcie-dpc.c      | 1 +
 include/linux/pci.h              | 1 +
 3 files changed, 4 insertions(+)

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/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 250f878..756eae4 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -44,6 +44,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 c81fbf7..36b759c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -337,6 +337,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

[PATCHv3 3/5] pci: No config access for removed devices

[Keith Busch]

If we've  detected that the PCI device is removed, there is no need to
access the device's config space. This patch has all the device config
reads and writes return error when in such a state.

If the device is not present on a config read, the value returned will be
set to all 1's. This is the same as what hardware is expected to return
when accessing a removed device, but software can do this faster without
relying on hardware.

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

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 36b759c..7a320ca 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -929,28 +929,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

[PATCHv3 4/5] pcie/aer: Cache capability position

[Keith Busch]

This saves the position 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>
Cc: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pcie/aer/aerdrv.c      | 10 +++++-----
 drivers/pci/pcie/aer/aerdrv_core.c | 18 ++++++++++++------
 drivers/pci/probe.c                |  3 ++-
 include/linux/pci.h                |  5 +++++
 4 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
index 48d21e0..24c6722 100644
--- a/drivers/pci/pcie/aer/aerdrv.c
+++ b/drivers/pci/pcie/aer/aerdrv.c
@@ -134,7 +134,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);
@@ -173,7 +173,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;
@@ -200,7 +200,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
@@ -343,7 +343,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);
@@ -396,7 +396,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..d9175ef 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;
 
@@ -102,6 +102,12 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
 	return 0;
 }
 
+int pci_aer_init(struct pci_dev *dev)
+{
+	dev->aer_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
+	return pci_cleanup_aer_error_status_regs(dev);
+}
+
 /**
  * add_error_device - list device to be handled
  * @e_info: pointer to error info
@@ -158,7 +164,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 +561,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 +653,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 93f280d..1575724 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1666,7 +1666,8 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	/* Enable ACS P2P upstream forwarding */
 	pci_enable_acs(dev);
 
-	pci_cleanup_aer_error_status_regs(dev);
+	/* Advanced Error Reporting */
+	pci_aer_init(dev);
 }
 
 /*
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 7a320ca..306e5c6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -268,6 +268,9 @@ 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) */
+#ifdef CONFIG_PCIEAER
+	u16		aer_cap;	/* AER capability offset */
+#endif
 	u8		pcie_cap;	/* PCIe capability offset */
 	u8		msi_cap;	/* MSI capability offset */
 	u8		msix_cap;	/* MSI-X capability offset */
@@ -1390,9 +1393,11 @@ static inline bool pcie_aspm_support_enabled(void) { return false; }
 #ifdef CONFIG_PCIEAER
 void pci_no_aer(void);
 bool pci_aer_available(void);
+int pci_aer_init(struct pci_dev *dev);
 #else
 static inline void pci_no_aer(void) { }
 static inline bool pci_aer_available(void) { return false; }
+static inline int pci_aer_init(struct pci_dev *d) { return -ENODEV; }
 #endif
 
 #ifdef CONFIG_PCIE_ECRC
-- 
2.7.2

[PATCHv3 5/5] 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 tear down on
removed devices completes quicker.

Signed-off-by: Keith Busch <keith.busch@intel.com>
Cc: Lukas Wunner <lukas@wunner.de>
---
 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..2b5e67d 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: [PATCHv3 4/5] pcie/aer: Cache capability position

[Bjorn Helgaas]

On Tue, Sep 27, 2016 at 04:23:34PM -0400, Keith Busch wrote:
> This saves the position 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>
> Cc: Lukas Wunner <lukas@wunner.de>

Applied to pci/aer for v4.9, thanks, Keith!

> ---
>  drivers/pci/pcie/aer/aerdrv.c      | 10 +++++-----
>  drivers/pci/pcie/aer/aerdrv_core.c | 18 ++++++++++++------
>  drivers/pci/probe.c                |  3 ++-
>  include/linux/pci.h                |  5 +++++
>  4 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
> index 48d21e0..24c6722 100644
> --- a/drivers/pci/pcie/aer/aerdrv.c
> +++ b/drivers/pci/pcie/aer/aerdrv.c
> @@ -134,7 +134,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);
> @@ -173,7 +173,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;
> @@ -200,7 +200,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
> @@ -343,7 +343,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);
> @@ -396,7 +396,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..d9175ef 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;
>  
> @@ -102,6 +102,12 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
>  	return 0;
>  }
>  
> +int pci_aer_init(struct pci_dev *dev)
> +{
> +	dev->aer_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
> +	return pci_cleanup_aer_error_status_regs(dev);
> +}
> +
>  /**
>   * add_error_device - list device to be handled
>   * @e_info: pointer to error info
> @@ -158,7 +164,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 +561,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 +653,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 93f280d..1575724 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1666,7 +1666,8 @@ static void pci_init_capabilities(struct pci_dev *dev)
>  	/* Enable ACS P2P upstream forwarding */
>  	pci_enable_acs(dev);
>  
> -	pci_cleanup_aer_error_status_regs(dev);
> +	/* Advanced Error Reporting */
> +	pci_aer_init(dev);
>  }
>  
>  /*
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 7a320ca..306e5c6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -268,6 +268,9 @@ 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) */
> +#ifdef CONFIG_PCIEAER
> +	u16		aer_cap;	/* AER capability offset */
> +#endif
>  	u8		pcie_cap;	/* PCIe capability offset */
>  	u8		msi_cap;	/* MSI capability offset */
>  	u8		msix_cap;	/* MSI-X capability offset */
> @@ -1390,9 +1393,11 @@ static inline bool pcie_aspm_support_enabled(void) { return false; }
>  #ifdef CONFIG_PCIEAER
>  void pci_no_aer(void);
>  bool pci_aer_available(void);
> +int pci_aer_init(struct pci_dev *dev);
>  #else
>  static inline void pci_no_aer(void) { }
>  static inline bool pci_aer_available(void) { return false; }
> +static inline int pci_aer_init(struct pci_dev *d) { return -ENODEV; }
>  #endif
>  
>  #ifdef CONFIG_PCIE_ECRC
> -- 
> 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: [PATCHv3 1/5] mips/pci: Reduce stack frame usage

[Atsushi Nemoto]

On Tue, 27 Sep 2016 16:23:31 -0400, Keith Busch <keith.busch@intel.com> wrote:
> The struct pci_dev is too large to allocate on the stack. This patch
> removes the fake pci device in early config access and instead uses the
> pci bus to get the same functionality.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---
> Got the 'ok' (link below), but I don't see this is merged yet. That's
> okay, since I need to include it in this series to avoid 0'day build
> failure. If this is ok with everyone, perhaps this can go through 'pci'.
> 
>   https://www.linux-mips.org/archives/linux-mips/2016-09/msg00255.html

Sorry, I had missed the patch.  Looks good.  Thank you.

Reviewed-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

---
Atsushi Nemoto

Re: [PATCHv3 2/5] pci: Add is_removed state

[Lukas Wunner]

On Tue, Sep 27, 2016 at 04:23:32PM -0400, Keith Busch wrote:
> --- 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

Sorry for the delay Keith, I finally got around to test v3 of your
series with hot-removed Thunderbolt devices on Apple Macs.

I've found that the above isn't sufficient, it's necessary to also
set the is_removed bit on any child devices.  E.g. on my system
when an Apple Gigabit Ethernet adapter is plugged in, the topology
looks like this:

0000:06:04.0 --- 0000:09:00.0 --- 0000:0a:00.0 --- 0000:0b:00.0

Hotplug port     Upstream bridge  Downstream br.   Broadcom 57762
of TB host       of TB switch in
                 Ethernet adapter

With your patch above, the is_removed bit is only set on 0000:09:00.0
but not on its children.  Consequently the "tg3" driver tries to
access the hot-removed Broadcom 57762 Ethernet chip as before,
causing a soft lockup.

The diff below fixes this for me, could you fold that into your patch?
The same change might also be necessary in pcie-dpc.c. Feel free to
rename "set_is_removed_cb()" if you don't like it.

Thanks,

Lukas

-- >8 --
diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index 299ea5e..ec26eb7 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -73,6 +73,12 @@ int pciehp_configure_device(struct slot *p_slot)
 	return ret;
 }
 
+static int set_is_removed_cb(struct pci_dev *pdev, void *unused)
+{
+	pdev->is_removed = 1;
+	return 0;
+}
+
 int pciehp_unconfigure_device(struct slot *p_slot)
 {
 	int rc = 0;
@@ -109,8 +115,11 @@ int pciehp_unconfigure_device(struct slot *p_slot)
 				break;
 			}
 		}
-		if (!presence)
+		if (!presence) {
 			dev->is_removed = 1;
+			if (pci_has_subordinate(dev))
+				pci_walk_bus(dev->subordinate, set_is_removed_cb, NULL);
+		}
 		pci_stop_and_remove_bus_device(dev);
 		/*
 		 * Ensure that no new Requests will be generated from

Re: [PATCHv3 2/5] pci: Add is_removed state

[Keith Busch]

On Fri, Oct 21, 2016 at 05:37:14PM +0200, Lukas Wunner wrote:
> On Tue, Sep 27, 2016 at 04:23:32PM -0400, Keith Busch wrote:
> > --- 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
> 
> Sorry for the delay Keith, I finally got around to test v3 of your
> series with hot-removed Thunderbolt devices on Apple Macs.
> 
> I've found that the above isn't sufficient, it's necessary to also
> set the is_removed bit on any child devices.  E.g. on my system
> when an Apple Gigabit Ethernet adapter is plugged in, the topology
> looks like this:

Thanks for the catch. Your proposal looks good to me. I'll send a new
revision incorporating something like this that the dpc driver can
also use.
 
> With your patch above, the is_removed bit is only set on 0000:09:00.0
> but not on its children.  Consequently the "tg3" driver tries to
> access the hot-removed Broadcom 57762 Ethernet chip as before,
> causing a soft lockup.

Is that something that can be fixed in the tg3 driver? I don't think
drivers can rely on this patch to fense off their unintended access since
we can't stop tg3 from accesses a removed device before 'is_removed'
is set.

Re: [PATCHv3 2/5] pci: Add is_removed state

[Lukas Wunner]

On Tue, Sep 27, 2016 at 04:23:32PM -0400, Keith Busch wrote:
> This adds a new state for devices that were once in the system, but
> unexpectedly removed. This is so device tear down functions can observe
> the device is not accessible so it may skip attempting to initialize
> the hardware.
[...]
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -337,6 +337,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 */

The tg3 driver (as well as many other drivers) already check reachability
of a device before accessing its mmio space by calling pci_channel_offline().

I've found that the following simple change on top of your series is
already sufficient to make hot-removal of the Apple Gigabit Ethernet
adapter "just work" (no more soft lockups, which is a giant improvement):


diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5c43012..cc8b234 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -406,7 +406,7 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus);
 
 static inline int pci_channel_offline(struct pci_dev *pdev)
 {
-	return (pdev->error_state != pci_channel_io_normal);
+	return pdev->error_state != pci_channel_io_normal || pdev->is_removed;
 }
 
 struct pci_host_bridge {


This got me thinking:  We've got three pci_channel_state values defined
in include/linux/pci.h, "normal", "frozen" and "perm_failure".  Instead
of adding a new "is_removed" bit to struct pci_dev, would it perhaps
make more sense to just add a new type of pci_channel_state for removed
devices?  Then the above change to pci_channel_offline() wouldn't even
be necessary.  The pciehp and dpc drivers would just change the channel
status to "removed" and all the drivers already querying it with
pci_channel_offline() would pick up the change automatically.

Thoughts?

Thanks,

Lukas

Re: [PATCHv3 2/5] pci: Add is_removed state

[Lukas Wunner]

On Fri, Oct 21, 2016 at 12:15:16PM -0400, Keith Busch wrote:
> On Fri, Oct 21, 2016 at 05:37:14PM +0200, Lukas Wunner wrote:
> > With your patch above, the is_removed bit is only set on 0000:09:00.0
> > but not on its children.  Consequently the "tg3" driver tries to
> > access the hot-removed Broadcom 57762 Ethernet chip as before,
> > causing a soft lockup.
> 
> Is that something that can be fixed in the tg3 driver? I don't think
> drivers can rely on this patch to fense off their unintended access since
> we can't stop tg3 from accesses a removed device before 'is_removed'
> is set.

I haven't tested yet what happens when the adapter is unplugged while
packets are in-flight, but at least unplugging works fine when the
adapter is idle (with your series plus the small changes I outlined).

*Without* your series, I have to set the interface to down with ifconfig
before unplugging.  If I ever forget that, the machine locks up:


NMI watchdog: BUG: soft lockup - CPU#2 stuck for 22s! [kworker/2:2:299]
...
Workqueue: pciehp-4 pciehp_power_thread
RIP: 0010:[<ffffffffa105b01d>]  [<ffffffffa105b01d>] tg3_read32+0xd/0x10 [tg3]
...
Call Trace:
 [<ffffffffa1063610>] ? tg3_stop_block.constprop.126+0x80/0x110 [tg3]
 [<ffffffffa1066298>] ? tg3_abort_hw+0x68/0x2f0 [tg3]
 [<ffffffffa106654d>] ? tg3_halt+0x2d/0x180 [tg3]
 [<ffffffffa1072a07>] ? tg3_stop+0x157/0x210 [tg3]
 [<ffffffffa1072aeb>] ? tg3_close+0x2b/0xe0 [tg3]
 [<ffffffff81465ff4>] ? __dev_close_many+0x84/0xd0
 [<ffffffff814660b4>] ? dev_close_many+0x74/0x100
 [<ffffffff8146790b>] ? rollback_registered_many+0xfb/0x2e0
 [<ffffffff81467b19>] ? rollback_registered+0x29/0x40
 [<ffffffff81468950>] ? unregister_netdevice_queue+0x40/0x90
 [<ffffffff814689b8>] ? unregister_netdev+0x18/0x20
 [<ffffffffa106604b>] ? tg3_remove_one+0x8b/0x130 [tg3]
 [<ffffffff8130b556>] ? pci_device_remove+0x36/0xb0
 [<ffffffff813df92a>] ? __device_release_driver+0x9a/0x140
 [<ffffffff813df9ee>] ? device_release_driver+0x1e/0x30
 [<ffffffff81304bf4>] ? pci_stop_bus_device+0x84/0xa0
 [<ffffffff81304b9b>] ? pci_stop_bus_device+0x2b/0xa0
 [<ffffffff81304b9b>] ? pci_stop_bus_device+0x2b/0xa0
 [<ffffffff81304cee>] ? pci_stop_and_remove_bus_device+0xe/0x20
 [<ffffffff8131ecea>] ? pciehp_unconfigure_device+0x9a/0x180
 [<ffffffff8131e7ef>] ? pciehp_disable_slot+0x3f/0xb0
 [<ffffffff8131e8e5>] ? pciehp_power_thread+0x85/0xa0
 [<ffffffff810855df>] ? process_one_work+0x19f/0x3d0
 [<ffffffff8108585d>] ? worker_thread+0x4d/0x450
 [<ffffffff81085810>] ? process_one_work+0x3d0/0x3d0
 [<ffffffff8108b32d>] ? kthread+0xbd/0xe0
 [<ffffffff8108b270>] ? kthread_create_on_node+0x170/0x170
 [<ffffffff8155ee1f>] ? ret_from_fork+0x3f/0x70
 [<ffffffff8108b270>] ? kthread_create_on_node+0x170/0x170


Being able to just unplug without having to think of ifconfig is already
a massive improvement.

Thanks,

Lukas

Re: [PATCHv3 2/5] pci: Add is_removed state

[Lukas Wunner]

On Tue, Sep 27, 2016 at 04:23:32PM -0400, Keith Busch wrote:
> This adds a new state for devices that were once in the system, but
> unexpectedly removed. This is so device tear down 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 these explicitly set this flag when its handlers detect the device
> is gone.

I note that you've removed the change to pci_device_is_present()
completely from v3 of this patch, however only a portion of the
change was problematic.  This portion would have been okay:


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


Up to you if you want to include this in the next iteration or not.

Thanks,

Lukas

Re: [PATCHv3 2/5] pci: Add is_removed state

[Keith Busch]

On Fri, Oct 21, 2016 at 06:20:10PM +0200, Lukas Wunner wrote:
> I've found that the following simple change on top of your series is
> already sufficient to make hot-removal of the Apple Gigabit Ethernet
> adapter "just work" (no more soft lockups, which is a giant improvement):
> 
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 5c43012..cc8b234 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -406,7 +406,7 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus);
>  
>  static inline int pci_channel_offline(struct pci_dev *pdev)
>  {
> -	return (pdev->error_state != pci_channel_io_normal);
> +	return pdev->error_state != pci_channel_io_normal || pdev->is_removed;
>  }
>  
>  struct pci_host_bridge {
> 
> 
> This got me thinking:  We've got three pci_channel_state values defined
> in include/linux/pci.h, "normal", "frozen" and "perm_failure".  Instead
> of adding a new "is_removed" bit to struct pci_dev, would it perhaps
> make more sense to just add a new type of pci_channel_state for removed
> devices?  Then the above change to pci_channel_offline() wouldn't even
> be necessary.  The pciehp and dpc drivers would just change the channel
> status to "removed" and all the drivers already querying it with
> pci_channel_offline() would pick up the change automatically.
> 
> Thoughts?

I'd be happy if we can reuse that, but concerned about overloading
error_state's intended purpose for AER. The conditions under which an
'is_removed' may be set can also create AER events, and the aer driver
overrides the error_state.

Re: [PATCHv3 2/5] pci: Add is_removed state

[Keith Busch]

On Fri, Oct 21, 2016 at 06:58:10PM +0200, Lukas Wunner wrote:
> On Tue, Sep 27, 2016 at 04:23:32PM -0400, Keith Busch wrote:
> I note that you've removed the change to pci_device_is_present()
> completely from v3 of this patch, however only a portion of the
> change was problematic.  This portion would have been okay:
> 
> 
> +	if (pdev->is_removed)
> +		return false;
>  
> 	return pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
> 
> 
> Up to you if you want to include this in the next iteration or not.

I was thinking it didn't matter since patch 3/5 has config access return
-ENODEV on a removed pci_dev, but pci_bus_read_dev_vendor_id doesn't use
the pci_dev. I'll add it back in.