| Message ID | 20190904043633.65026-3-skunberg.kelsey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Change to using pci_dev_is_inaccessible() | expand |
On Tue, Sep 03, 2019 at 10:36:35PM -0600, Kelsey Skunberg wrote: > Change pci_dev_is_disconnected() call inside pci_dev_is_inaccessible() to: > > pdev->error_state == pci_channel_io_perm_failure > > Change remaining pci_dev_is_disconnected() calls to > pci_dev_is_inaccessible() calls. I don't think that's a good idea because it introduces a config space read (for the vendor ID) in places where we don't want that. E.g., after the check of pdev->error_state, a regular config space read may take place and if that returns all ones, we may already be able to determine that the device is inaccessible, obviating the need for a vendor ID check. Config space reads aren't for free. Thanks, Lukas
[+cc Carolyn, author of 17a402a0075c] On Wed, Sep 04, 2019 at 07:35:23AM +0200, Lukas Wunner wrote: > On Tue, Sep 03, 2019 at 10:36:35PM -0600, Kelsey Skunberg wrote: > > Change pci_dev_is_disconnected() call inside pci_dev_is_inaccessible() to: > > > > pdev->error_state == pci_channel_io_perm_failure > > > > Change remaining pci_dev_is_disconnected() calls to > > pci_dev_is_inaccessible() calls. > > I don't think that's a good idea because it introduces a config space read > (for the vendor ID) in places where we don't want that. E.g., after the > check of pdev->error_state, a regular config space read may take place and > if that returns all ones, we may already be able to determine that the > device is inaccessible, obviating the need for a vendor ID check. Oh, I think I see what you mean: Previously pci_read_config_byte() et al called pci_dev_is_disconnected(), which only checked dev->error_state. If we applied this patch, those sites would call pci_dev_is_inaccessible(), which would check error_state and then (in the common case where we haven't set error_state) do a config read of the vendor ID. So we would basically double the config access overhead because we'd be doing an extra read of the vendor ID before every access. That indeed doesn't seem practical. I think what we need to figure out is whether we really need two interfaces (one that looks only at dev->error_state and a second that looks at dev->error_state and also reads the vendor ID). If we do need both, then I think we need a little guidance in the function comments about when to use one vs the other. There are only a few uses of pci_device_is_present() (which looks at dev->error_state and also reads the vendor ID) and they were added here: 8496e85c20e7 ("PCI / tg3: Give up chip reset and carrier loss handling if PCI device is not present") 17a402a0075c ("igb: Fixes needed for surprise removal support") 6db28eda2660 ("nvme/pci: Disable on removal when disconnected") b8a62d540240 ("ACPI / hotplug / PCI: Use pci_device_is_present()") 4ebe34503baa ("ACPI / hotplug / PCI: Check for new devices on enabled slots") a6a64026c0cd ("PCI: Recognize D3cold in pci_update_current_state()") The ACPI and PCI core uses are basically enumeration-type things so that mostly makes sense to me. I'm not so sure about the driver uses though. I wonder if those could be better handled by having the drivers check for ~0 error response data from MMIO and config reads. Bjorn
On Wed, Sep 04, 2019 at 07:35:23AM +0200, Lukas Wunner wrote: > On Tue, Sep 03, 2019 at 10:36:35PM -0600, Kelsey Skunberg wrote: > > Change pci_dev_is_disconnected() call inside pci_dev_is_inaccessible() to: > > > > pdev->error_state == pci_channel_io_perm_failure > > > > Change remaining pci_dev_is_disconnected() calls to > > pci_dev_is_inaccessible() calls. > > I don't think that's a good idea because it introduces a config space read > (for the vendor ID) in places where we don't want that. E.g., after the > check of pdev->error_state, a regular config space read may take place and > if that returns all ones, we may already be able to determine that the > device is inaccessible, obviating the need for a vendor ID check. > Config space reads aren't for free. > > Thanks, > > Lukas Good note. I definitely see why that would be undesirable. Thanks for taking the time to point this out, Lukas. I'll look this over again to see if a better solution can be done, or as Bjorn suggested, at least see if clarification on when to use one vs. the other can be included. Thanks again! -Kelsey
diff --git a/drivers/pci/access.c b/drivers/pci/access.c index 544922f097c0..c096340afb8c 100644 --- a/drivers/pci/access.c +++ b/drivers/pci/access.c @@ -535,7 +535,7 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword); int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val) { - if (pci_dev_is_disconnected(dev)) { + if (pci_dev_is_inaccessible(dev)) { *val = ~0; return PCIBIOS_DEVICE_NOT_FOUND; } @@ -545,7 +545,7 @@ EXPORT_SYMBOL(pci_read_config_byte); int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val) { - if (pci_dev_is_disconnected(dev)) { + if (pci_dev_is_inaccessible(dev)) { *val = ~0; return PCIBIOS_DEVICE_NOT_FOUND; } @@ -556,7 +556,7 @@ EXPORT_SYMBOL(pci_read_config_word); int pci_read_config_dword(const struct pci_dev *dev, int where, u32 *val) { - if (pci_dev_is_disconnected(dev)) { + if (pci_dev_is_inaccessible(dev)) { *val = ~0; return PCIBIOS_DEVICE_NOT_FOUND; } @@ -566,7 +566,7 @@ EXPORT_SYMBOL(pci_read_config_dword); int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val) { - if (pci_dev_is_disconnected(dev)) + if (pci_dev_is_inaccessible(dev)) return PCIBIOS_DEVICE_NOT_FOUND; return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val); } @@ -574,7 +574,7 @@ EXPORT_SYMBOL(pci_write_config_byte); int pci_write_config_word(const struct pci_dev *dev, int where, u16 val) { - if (pci_dev_is_disconnected(dev)) + if (pci_dev_is_inaccessible(dev)) return PCIBIOS_DEVICE_NOT_FOUND; return pci_bus_write_config_word(dev->bus, dev->devfn, where, val); } @@ -583,7 +583,7 @@ EXPORT_SYMBOL(pci_write_config_word); int pci_write_config_dword(const struct pci_dev *dev, int where, u32 val) { - if (pci_dev_is_disconnected(dev)) + if (pci_dev_is_inaccessible(dev)) return PCIBIOS_DEVICE_NOT_FOUND; return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val); } diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index 0884bedcfc7a..4680043aa315 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -311,7 +311,7 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg) { struct pci_dev *dev = msi_desc_to_pci_dev(entry); - if (dev->current_state != PCI_D0 || pci_dev_is_disconnected(dev)) { + if (dev->current_state != PCI_D0 || pci_dev_is_inaccessible(dev)) { /* Don't touch the hardware now */ } else if (entry->msi_attrib.is_msix) { void __iomem *base = pci_msix_desc_addr(entry); @@ -1008,7 +1008,7 @@ static void pci_msix_shutdown(struct pci_dev *dev) if (!pci_msi_enable || !dev || !dev->msix_enabled) return; - if (pci_dev_is_disconnected(dev)) { + if (pci_dev_is_inaccessible(dev)) { dev->msix_enabled = 0; return; } diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 7b4e248db5f9..e5f46d98dbe1 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -5910,7 +5910,7 @@ bool pci_dev_is_inaccessible(struct pci_dev *pdev) { u32 v; - if (pci_dev_is_disconnected(pdev)) + if (pdev->error_state == pci_channel_io_perm_failure) return true; return !pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0); } diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 1be03a97cb92..f0dc86dc8aab 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -363,11 +363,6 @@ static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused) return 0; } -static inline bool pci_dev_is_disconnected(const struct pci_dev *dev) -{ - return dev->error_state == pci_channel_io_perm_failure; -} - /* pci_dev priv_flags */ #define PCI_DEV_ADDED 0 diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index 308c3e0c4a34..8bf6b47dd2c6 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -416,7 +416,7 @@ static void wait_for_downstream_link(struct pci_dev *pdev) pci_pcie_type(pdev) != PCI_EXP_TYPE_DOWNSTREAM) return; - if (pci_dev_is_disconnected(pdev)) + if (pci_dev_is_inaccessible(pdev)) return; if (!pdev->subordinate || list_empty(&pdev->subordinate->devices) ||
Combine pci_dev_is_disconnected() with pci_dev_is_inaccessible() so only one function is used to learn if we should avoid accessing a device that's inaccessible due to surprise removal or an error condition. The use cases for pci_dev_is_disconnected() do not need to distinguish between a device being inaccessible due to a surprise removal or an error condition. This provides the opportunity to unify pci_dev_is_disconnected() and pci_dev_is_inaccessible() to reduce multiple functions used for the same task. Change pci_dev_is_disconnected() call inside pci_dev_is_inaccessible() to: pdev->error_state == pci_channel_io_perm_failure Change remaining pci_dev_is_disconnected() calls to pci_dev_is_inaccessible() calls. Remove pci_dev_is_disconnected() from /pci/pci.h which would now no longer be used. Demonstration of changes to pci_dev_is_disconnected() and pci_dev_is_inaccessible(): Before combining: static inline bool pci_dev_is_disconnected(const struct pci_dev *dev) { return dev->error_state == pci_channel_io_perm_failure; } bool pci_dev_is_inaccessible(struct pci_dev *pdev) { u32 v; if (pci_dev_is_disconnected(pdev)) return true; return !pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0); } After combining: bool pci_dev_is_inaccessible(struct pci_dev *pdev) { u32 v; if (pdev->error_state == pci_channel_io_perm_failure) return true; return !pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0); } Signed-off-by: Kelsey Skunberg <skunberg.kelsey@gmail.com> --- drivers/pci/access.c | 12 ++++++------ drivers/pci/msi.c | 4 ++-- drivers/pci/pci.c | 2 +- drivers/pci/pci.h | 5 ----- drivers/pci/pcie/portdrv_core.c | 2 +- 5 files changed, 10 insertions(+), 15 deletions(-)