PCI access on removed devices

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 <[email protected]>

Cc Atsushi Nemoto <[email protected]>

Cc Ralf Baechle <[email protected]>

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

drivers/pci/hotplug/pciehp_pci.c int pciehp_unconfigure_device(struct slot *p_slot) · line 116

Quoted 11 lines · Keith Busch

drivers/pci/hotplug/pciehp_pci.c +2 -0

@@ -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:

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.

Quoted 4 lines · Lukas Wunner

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.

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.

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

include/linux/pci.h +1 -1

index 5c43012..cc8b234 100644
@@ -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?

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.

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:

(quoted diff) +2 -0

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

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 <[email protected]>

Cc Lukas Wunner <[email protected]>

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

drivers/pci/pcie/aer/aerdrv.c +5 -5

index 48d21e0..24c6722 100644
@@ -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)

drivers/pci/pcie/aer/aerdrv_core.c +12 -6

index 521e39c..d9175ef 100644
@@ -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)

drivers/pci/probe.c +2 -1

index 93f280d..1575724 100644
@@ -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);
 }
 
 /*

include/linux/pci.h +5 -0

index 7a320ca..306e5c6 100644
@@ -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

Signature · 6 lines

2.7.2

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html