Limiting pci access requsets
v2 cover Limiting pci access requsets
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(-)
Signature · 1 line
2.7.2
PATCH v2 1/4 pci: Add is_removed state
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.
--- 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(-)
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
drivers/pci/pci.c +8 -1
@@ -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);
drivers/pci/pcie/pcie-dpc.c +1 -0
@@ -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); }
include/linux/pci.h +1 -0
@@ -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 */
Signature · 1 line
2.7.2
Re: [PATCHv2 1/4] pci: Add is_removed state
Quoted 3 lines · 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
^ they ^ access (?)
Quoted 4 lines
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
Quoted 6 lines
The flag is also cached any time
pci_device_is_presentreturns false. The function checks the flag first to avoid additional config accesses to removed devices.Signed-off-by Keith Busch <[email protected]>
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
Quoted 71 lines
--- 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(-)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 drivers/pci/pci.c +8
-1@@ -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);drivers/pci/pcie/pcie-dpc.c +1
-0@@ -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); }include/linux/pci.h +1
-0@@ -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 */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
Re: [PATCHv2 1/4] pci: Add is_removed state
In reply to Keith BuschQuoted 7 lines · Keith Busch
drivers/pci/pci.c +0
-1@@ -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 );
drivers/pci/pci.c bool pci_device_is_present(struct pci_dev *pdev) · line 4936
Quoted 10 lines
drivers/pci/pci.c +8
-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
Quoted 15 lines · Lukas Wunner
drivers/pci/pci.c bool pci_device_is_present(struct pci_dev *pdev) · line 4936
Quoted 11 lines · Keith Busch
drivers/pci/pci.c +8
-1-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:
Quoted 7 lines
In the above hunk, it's okay to return false if
pdev->is_removedis set, but it's not okay to setpdev->is_removedifpci_bus_read_dev_vendor_id()returns false. That's becausepci_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.
PATCH v2 2/4 pci: No config access for removed devices
In reply to Keith BuschIf 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.
--- include/linux/pci.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
include/linux/pci.h +18 -0
@@ -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); }
Signature · 1 line
2.7.2
kbuild test robot
build botRe: [PATCHv2 2/4] pci: No config access for removed devices
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…
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 <[email protected]>
:::::: CC: Ralf Baechle <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCHv2 2/4] pci: No config access for removed devices
Quoted 27 lines · kbuild test robot
All errors (new ones prefixed by >>):
arch/mips/txx9/generic/pci.c: In function 'early_read_config_word':Quoted 1 line
Quoted 1 line
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.c89d63fe1 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);
Quoted 1 line
89d63fe1Atsushi 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:
---
arch/mips/txx9/generic/pci.c +1 -5
@@ -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,
--
PATCH v2 3/4 pcie/aer: Cache capability position
In reply to Keith BuschThis saves the postition of the error reporting capability so that it doesn't need to be rediscovered during error handling.
--- 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(-)
drivers/pci/pcie/aer/aerdrv.c +5 -5
@@ -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 , ®32);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 , ®32); 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 , ®32);
@@ -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)
drivers/pci/pcie/aer/aerdrv_core.c +6 -6
@@ -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)
drivers/pci/probe.c +2 -0
@@ -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 */
include/linux/pci.h +1 -0
@@ -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 */
Signature · 1 line
2.7.2
Re: [PATCHv2 3/4] pcie/aer: Cache capability position
Quoted 1 line · Keith Busch
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
Quoted 149 lines
doesn't need to be rediscovered during error handling.
Signed-off-by Keith Busch <[email protected]>--- 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(-)drivers/pci/pcie/aer/aerdrv.c +5
-5@@ -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 , ®32);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 , ®32); 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 , ®32);@@ -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)drivers/pci/pcie/aer/aerdrv_core.c +6
-6@@ -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)drivers/pci/probe.c +2
-0@@ -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 */ include/linux/pci.h +1
-0@@ -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 */ 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
Re: [PATCHv2 3/4] pcie/aer: Cache capability position
Quoted 7 lines · Lukas Wunner
Quoted 1 line · Keith Busch
This saves the postition of the error reporting capability so that it
^ position
I'm wondering if the flag in struct
pci_devas well as the additional code inprobe.cshould be enclosed in #ifdefCONFIG_PCIEAER, as we're doing forats_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().
Quoted 153 lines
Quoted 149 lines
doesn't need to be rediscovered during error handling.
Signed-off-by Keith Busch <[email protected]>--- 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(-)drivers/pci/pcie/aer/aerdrv.c +5
-5@@ -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 , ®32);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 , ®32); 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 , ®32);@@ -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)drivers/pci/pcie/aer/aerdrv_core.c +6
-6@@ -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)drivers/pci/probe.c +2
-0@@ -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 */ include/linux/pci.h +1
-0@@ -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 */ 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--
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
PATCH v2 4/4 pci/msix: Skip disabling removed devices
In reply to Keith BuschThere 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.
--- drivers/pci/msi.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/pci/msi.c +6 -1
@@ -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 */
Signature · 1 line
2.7.2
Quoted 2 lines · 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.
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
Thanks,
Lukas