linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv6 0/5] Limiting PCI access
@ 2017-02-07 19:32 Keith Busch
  2017-02-07 19:32 ` [PATCHv6 1/5] pci: Export pci device config accessors Keith Busch
                   ` (6 more replies)
  0 siblings, 7 replies; 13+ messages in thread
From: Keith Busch @ 2017-02-07 19:32 UTC (permalink / raw)
  To: linux-pci, Bjorn Helgaas
  Cc: Greg Kroah-Hartman, Lukas Wunner, Wei Zhang, Austin Bolen,
	Christoph Hellwig, Keith Busch

The code is functionally equivalent to v5. I've fixed the style
points mentioned in review on line warpping, adjusted the change log
on the msi/msix skippage to not exclude mentioning msi, and added the
accumulated Reviewed-by's.

Keith Busch (5):
  pci: Export pci device config accessors
  pci: Add device disconnected state
  pci: No config access for disconnected devices
  pci/msix: Skip disabling disconnected devices
  pci: Quick return for pci_device_is_present

 drivers/pci/access.c             | 56 ++++++++++++++++++++++++++++++++++++++++
 drivers/pci/hotplug/pciehp_pci.c |  6 +++++
 drivers/pci/msi.c                |  7 ++++-
 drivers/pci/pci.c                |  2 ++
 drivers/pci/pci.h                | 14 ++++++++++
 drivers/pci/pcie/pcie-dpc.c      |  5 ++++
 include/linux/pci.h              | 34 ++++++------------------
 7 files changed, 97 insertions(+), 27 deletions(-)

-- 
2.7.2

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCHv6 1/5] pci: Export pci device config accessors
  2017-02-07 19:32 [PATCHv6 0/5] Limiting PCI access Keith Busch
@ 2017-02-07 19:32 ` Keith Busch
  2017-02-07 19:32 ` [PATCHv6 2/5] pci: Add device disconnected state Keith Busch
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Keith Busch @ 2017-02-07 19:32 UTC (permalink / raw)
  To: linux-pci, Bjorn Helgaas
  Cc: Greg Kroah-Hartman, Lukas Wunner, Wei Zhang, Austin Bolen,
	Christoph Hellwig, Keith Busch

This patch replaces the inline pci device config read and write accessors
with exported functions. This is preparing for these functions to make
use of private data.

Signed-off-by: Keith Busch <keith.busch@intel.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/access.c | 38 ++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h  | 32 ++++++--------------------------
 2 files changed, 44 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index db23954..d37b2ed 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -889,3 +889,41 @@ int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
 	return ret;
 }
 EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
+
+int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
+{
+	return pci_bus_read_config_byte(dev->bus, dev->devfn, where, val);
+}
+EXPORT_SYMBOL(pci_read_config_byte);
+
+int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
+{
+	return pci_bus_read_config_word(dev->bus, dev->devfn, where, val);
+}
+EXPORT_SYMBOL(pci_read_config_word);
+
+int pci_read_config_dword(const struct pci_dev *dev, int where,
+					u32 *val)
+{
+	return pci_bus_read_config_dword(dev->bus, dev->devfn, where, val);
+}
+EXPORT_SYMBOL(pci_read_config_dword);
+
+int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val)
+{
+	return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val);
+}
+EXPORT_SYMBOL(pci_write_config_byte);
+
+int pci_write_config_word(const struct pci_dev *dev, int where, u16 val)
+{
+	return pci_bus_write_config_word(dev->bus, dev->devfn, where, val);
+}
+EXPORT_SYMBOL(pci_write_config_word);
+
+int pci_write_config_dword(const struct pci_dev *dev, int where,
+					 u32 val)
+{
+	return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
+}
+EXPORT_SYMBOL(pci_write_config_dword);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e2d1a12..b00a2d3 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -944,32 +944,12 @@ int pci_generic_config_write32(struct pci_bus *bus, unsigned int devfn,
 
 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)
-{
-	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)
-{
-	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)
-{
-	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)
-{
-	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)
-{
-	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)
-{
-	return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
-}
+int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val);
+int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val);
+int pci_read_config_dword(const struct pci_dev *dev, int where, u32 *val);
+int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val);
+int pci_write_config_word(const struct pci_dev *dev, int where, u16 val);
+int pci_write_config_dword(const struct pci_dev *dev, int where, u32 val);
 
 int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val);
 int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val);
-- 
2.7.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCHv6 2/5] pci: Add device disconnected state
  2017-02-07 19:32 [PATCHv6 0/5] Limiting PCI access Keith Busch
  2017-02-07 19:32 ` [PATCHv6 1/5] pci: Export pci device config accessors Keith Busch
@ 2017-02-07 19:32 ` Keith Busch
  2017-02-07 19:32 ` [PATCHv6 3/5] pci: No config access for disconnected devices Keith Busch
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Keith Busch @ 2017-02-07 19:32 UTC (permalink / raw)
  To: linux-pci, Bjorn Helgaas
  Cc: Greg Kroah-Hartman, Lukas Wunner, Wei Zhang, Austin Bolen,
	Christoph Hellwig, Keith Busch

This patch adds a new state to pci_dev to be set when it is unexpectedly
disconnected. The pci driver tear down functions can observe this new
device state so they may skip operations that will fail.

The pciehp and pcie-dpc drivers are aware when the link is down, so
these set the flag when their handlers detect the device is disconnected.

Signed-off-by: Keith Busch <keith.busch@intel.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/hotplug/pciehp_pci.c |  6 ++++++
 drivers/pci/pci.h                | 14 ++++++++++++++
 drivers/pci/pcie/pcie-dpc.c      |  5 +++++
 include/linux/pci.h              |  2 ++
 4 files changed, 27 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index 9e69403..19f30a9 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -109,6 +109,12 @@ int pciehp_unconfigure_device(struct slot *p_slot)
 				break;
 			}
 		}
+		if (!presence) {
+			pci_dev_set_disconnected(dev, NULL);
+			if (pci_has_subordinate(dev))
+				pci_walk_bus(dev->subordinate,
+					     pci_dev_set_disconnected, NULL);
+		}
 		pci_stop_and_remove_bus_device(dev);
 		/*
 		 * Ensure that no new Requests will be generated from
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index cb17db2..81512bc 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -274,6 +274,20 @@ struct pci_sriov {
 	resource_size_t barsz[PCI_SRIOV_NUM_BARS];	/* VF BAR size */
 };
 
+/* pci_dev priv_flags */
+#define PCI_DEV_DISCONNECTED 0
+
+static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
+{
+	set_bit(PCI_DEV_DISCONNECTED, &dev->priv_flags);
+	return 0;
+}
+
+static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
+{
+	return test_bit(PCI_DEV_DISCONNECTED, &dev->priv_flags);
+}
+
 #ifdef CONFIG_PCI_ATS
 void pci_restore_ats_state(struct pci_dev *dev);
 #else
diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 9811b14..1480458 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -14,6 +14,7 @@
 #include <linux/init.h>
 #include <linux/pci.h>
 #include <linux/pcieport_if.h>
+#include "../pci.h"
 
 struct dpc_dev {
 	struct pcie_device	*dev;
@@ -46,6 +47,10 @@ 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);
+		pci_dev_set_disconnected(dev, NULL);
+		if (pci_has_subordinate(dev))
+			pci_walk_bus(dev->subordinate,
+				     pci_dev_set_disconnected, NULL);
 		pci_stop_and_remove_bus_device(dev);
 		pci_dev_put(dev);
 	}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index b00a2d3..5d17f1b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -396,6 +396,8 @@ struct pci_dev {
 	phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */
 	size_t romlen; /* Length of ROM if it's not from the BAR */
 	char *driver_override; /* Driver name to force a match */
+
+	unsigned long priv_flags; /* Private flags for the pci driver */
 };
 
 static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
-- 
2.7.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCHv6 3/5] pci: No config access for disconnected devices
  2017-02-07 19:32 [PATCHv6 0/5] Limiting PCI access Keith Busch
  2017-02-07 19:32 ` [PATCHv6 1/5] pci: Export pci device config accessors Keith Busch
  2017-02-07 19:32 ` [PATCHv6 2/5] pci: Add device disconnected state Keith Busch
@ 2017-02-07 19:32 ` Keith Busch
  2017-02-08  6:04   ` Lukas Wunner
  2017-02-07 19:32 ` [PATCHv6 4/5] pci/msix: Skip disabling " Keith Busch
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 13+ messages in thread
From: Keith Busch @ 2017-02-07 19:32 UTC (permalink / raw)
  To: linux-pci, Bjorn Helgaas
  Cc: Greg Kroah-Hartman, Lukas Wunner, Wei Zhang, Austin Bolen,
	Christoph Hellwig, Keith Busch

If we've  detected the PCI device is disconnected, there is no need to
attempt to access its config space since we know the operation will
fail. This patch has all the config reads and writes return -ENODEV
error immediately when in such a state.

If a config read is sent to a disconnected device, the value 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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/access.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index d37b2ed..d63e9dd 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -892,12 +892,20 @@ 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)) {
+		*val = ~0;
+		return -ENODEV;
+	}
 	return pci_bus_read_config_byte(dev->bus, dev->devfn, where, val);
 }
 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)) {
+		*val = ~0;
+		return -ENODEV;
+	}
 	return pci_bus_read_config_word(dev->bus, dev->devfn, where, val);
 }
 EXPORT_SYMBOL(pci_read_config_word);
@@ -905,18 +913,26 @@ 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)) {
+		*val = ~0;
+		return -ENODEV;
+	}
 	return pci_bus_read_config_dword(dev->bus, dev->devfn, where, val);
 }
 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))
+		return -ENODEV;
 	return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val);
 }
 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))
+		return -ENODEV;
 	return pci_bus_write_config_word(dev->bus, dev->devfn, where, val);
 }
 EXPORT_SYMBOL(pci_write_config_word);
@@ -924,6 +940,8 @@ 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))
+		return -ENODEV;
 	return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
 }
 EXPORT_SYMBOL(pci_write_config_dword);
-- 
2.7.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCHv6 4/5] pci/msix: Skip disabling disconnected devices
  2017-02-07 19:32 [PATCHv6 0/5] Limiting PCI access Keith Busch
                   ` (2 preceding siblings ...)
  2017-02-07 19:32 ` [PATCHv6 3/5] pci: No config access for disconnected devices Keith Busch
@ 2017-02-07 19:32 ` Keith Busch
  2017-02-07 19:32 ` [PATCHv6 5/5] pci: Quick return for pci_device_is_present Keith Busch
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Keith Busch @ 2017-02-07 19:32 UTC (permalink / raw)
  To: linux-pci, Bjorn Helgaas
  Cc: Greg Kroah-Hartman, Lukas Wunner, Wei Zhang, Austin Bolen,
	Christoph Hellwig, Keith Busch

This patch checks the device connected state prior to executing device
shutdown operations or writing msi messages so that tear down on
disconnected devices completes quicker.

Signed-off-by: Keith Busch <keith.busch@intel.com>
Reviewed-by: Christoph Hellwig <hch@lst.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 50c5003..17807eb3 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -317,7 +317,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 || pci_dev_is_disconnected(dev)) {
 		/* Don't touch the hardware now */
 	} else if (entry->msi_attrib.is_msix) {
 		void __iomem *base = pci_msix_desc_addr(entry);
@@ -1020,6 +1020,11 @@ void pci_msix_shutdown(struct pci_dev *dev)
 	if (!pci_msi_enable || !dev || !dev->msix_enabled)
 		return;
 
+	if (pci_dev_is_disconnected(dev)) {
+		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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCHv6 5/5] pci: Quick return for pci_device_is_present
  2017-02-07 19:32 [PATCHv6 0/5] Limiting PCI access Keith Busch
                   ` (3 preceding siblings ...)
  2017-02-07 19:32 ` [PATCHv6 4/5] pci/msix: Skip disabling " Keith Busch
@ 2017-02-07 19:32 ` Keith Busch
  2017-03-29 18:01 ` [PATCHv6 0/5] Limiting PCI access Wei Zhang
  2017-03-30  3:56 ` Bjorn Helgaas
  6 siblings, 0 replies; 13+ messages in thread
From: Keith Busch @ 2017-02-07 19:32 UTC (permalink / raw)
  To: linux-pci, Bjorn Helgaas
  Cc: Greg Kroah-Hartman, Lukas Wunner, Wei Zhang, Austin Bolen,
	Christoph Hellwig, Keith Busch

If the pci device is disconnected, return false immediately for checking
presence. We can't use the early return in pci device config accessors
since this function uses the bus accessors.

Signed-off-by: Keith Busch <keith.busch@intel.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a881c0d..b8d37e7 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4932,6 +4932,8 @@ bool pci_device_is_present(struct pci_dev *pdev)
 {
 	u32 v;
 
+	if (pci_dev_is_disconnected(pdev))
+		return false;
 	return pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0);
 }
 EXPORT_SYMBOL_GPL(pci_device_is_present);
-- 
2.7.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCHv6 3/5] pci: No config access for disconnected devices
  2017-02-07 19:32 ` [PATCHv6 3/5] pci: No config access for disconnected devices Keith Busch
@ 2017-02-08  6:04   ` Lukas Wunner
  2017-02-10 18:38     ` Keith Busch
  0 siblings, 1 reply; 13+ messages in thread
From: Lukas Wunner @ 2017-02-08  6:04 UTC (permalink / raw)
  To: Keith Busch
  Cc: linux-pci, Bjorn Helgaas, Greg Kroah-Hartman, Wei Zhang,
	Austin Bolen, Christoph Hellwig

On Tue, Feb 07, 2017 at 02:32:35PM -0500, Keith Busch wrote:
> If we've  detected the PCI device is disconnected, there is no need to
> attempt to access its config space since we know the operation will
> fail. This patch has all the config reads and writes return -ENODEV
> error immediately when in such a state.
> 
> If a config read is sent to a disconnected device, the value 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>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/pci/access.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index d37b2ed..d63e9dd 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -892,12 +892,20 @@ 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)) {

You used to have unlikely() here up until v4 but dropped it in v5.
Why?  Seemed sensible to me.

Thanks,

Lukas

> +		*val = ~0;
> +		return -ENODEV;
> +	}
>  	return pci_bus_read_config_byte(dev->bus, dev->devfn, where, val);
>  }
>  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)) {
> +		*val = ~0;
> +		return -ENODEV;
> +	}
>  	return pci_bus_read_config_word(dev->bus, dev->devfn, where, val);
>  }
>  EXPORT_SYMBOL(pci_read_config_word);
> @@ -905,18 +913,26 @@ 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)) {
> +		*val = ~0;
> +		return -ENODEV;
> +	}
>  	return pci_bus_read_config_dword(dev->bus, dev->devfn, where, val);
>  }
>  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))
> +		return -ENODEV;
>  	return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val);
>  }
>  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))
> +		return -ENODEV;
>  	return pci_bus_write_config_word(dev->bus, dev->devfn, where, val);
>  }
>  EXPORT_SYMBOL(pci_write_config_word);
> @@ -924,6 +940,8 @@ 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))
> +		return -ENODEV;
>  	return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
>  }
>  EXPORT_SYMBOL(pci_write_config_dword);
> -- 
> 2.7.2
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv6 3/5] pci: No config access for disconnected devices
  2017-02-08  6:04   ` Lukas Wunner
@ 2017-02-10 18:38     ` Keith Busch
  2017-02-10 19:54       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 13+ messages in thread
From: Keith Busch @ 2017-02-10 18:38 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: linux-pci, Bjorn Helgaas, Greg Kroah-Hartman, Wei Zhang,
	Austin Bolen, Christoph Hellwig

On Wed, Feb 08, 2017 at 07:04:10AM +0100, Lukas Wunner wrote:
> On Tue, Feb 07, 2017 at 02:32:35PM -0500, Keith Busch wrote:
> > If we've  detected the PCI device is disconnected, there is no need to
> > attempt to access its config space since we know the operation will
> > fail. This patch has all the config reads and writes return -ENODEV
> > error immediately when in such a state.
> > 
> > If a config read is sent to a disconnected device, the value 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>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  drivers/pci/access.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > index d37b2ed..d63e9dd 100644
> > --- a/drivers/pci/access.c
> > +++ b/drivers/pci/access.c
> > @@ -892,12 +892,20 @@ 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)) {
> 
> You used to have unlikely() here up until v4 but dropped it in v5.
> Why?  Seemed sensible to me.

Didn't mean to remove the unlikely. The micro-optimization isn't in a
performance path, but I'll build it back in.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv6 3/5] pci: No config access for disconnected devices
  2017-02-10 18:38     ` Keith Busch
@ 2017-02-10 19:54       ` Greg Kroah-Hartman
  2017-02-10 20:54         ` Keith Busch
  0 siblings, 1 reply; 13+ messages in thread
From: Greg Kroah-Hartman @ 2017-02-10 19:54 UTC (permalink / raw)
  To: Keith Busch
  Cc: Lukas Wunner, linux-pci, Bjorn Helgaas, Wei Zhang, Austin Bolen,
	Christoph Hellwig

On Fri, Feb 10, 2017 at 01:38:52PM -0500, Keith Busch wrote:
> On Wed, Feb 08, 2017 at 07:04:10AM +0100, Lukas Wunner wrote:
> > On Tue, Feb 07, 2017 at 02:32:35PM -0500, Keith Busch wrote:
> > > If we've  detected the PCI device is disconnected, there is no need to
> > > attempt to access its config space since we know the operation will
> > > fail. This patch has all the config reads and writes return -ENODEV
> > > error immediately when in such a state.
> > > 
> > > If a config read is sent to a disconnected device, the value 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>
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  drivers/pci/access.c | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > > 
> > > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > > index d37b2ed..d63e9dd 100644
> > > --- a/drivers/pci/access.c
> > > +++ b/drivers/pci/access.c
> > > @@ -892,12 +892,20 @@ 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)) {
> > 
> > You used to have unlikely() here up until v4 but dropped it in v5.
> > Why?  Seemed sensible to me.
> 
> Didn't mean to remove the unlikely. The micro-optimization isn't in a
> performance path, but I'll build it back in.

Only do so if you can measure the difference, otherwise it is useless,
and can actually hurt.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv6 3/5] pci: No config access for disconnected devices
  2017-02-10 19:54       ` Greg Kroah-Hartman
@ 2017-02-10 20:54         ` Keith Busch
  0 siblings, 0 replies; 13+ messages in thread
From: Keith Busch @ 2017-02-10 20:54 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Lukas Wunner, linux-pci, Bjorn Helgaas, Wei Zhang, Austin Bolen,
	Christoph Hellwig

On Fri, Feb 10, 2017 at 08:54:13PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 10, 2017 at 01:38:52PM -0500, Keith Busch wrote:
> > Didn't mean to remove the unlikely. The micro-optimization isn't in a
> > performance path, but I'll build it back in.
> 
> Only do so if you can measure the difference, otherwise it is useless,
> and can actually hurt.

Okay, that sounds good to me then, and I can't measure a difference here
in either case.

Bjorn,
What do you think? Is this series okay or any other conerncs I may
help address?

Thanks,
Keith

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv6 0/5] Limiting PCI access
  2017-02-07 19:32 [PATCHv6 0/5] Limiting PCI access Keith Busch
                   ` (4 preceding siblings ...)
  2017-02-07 19:32 ` [PATCHv6 5/5] pci: Quick return for pci_device_is_present Keith Busch
@ 2017-03-29 18:01 ` Wei Zhang
  2017-03-30  3:56 ` Bjorn Helgaas
  6 siblings, 0 replies; 13+ messages in thread
From: Wei Zhang @ 2017-03-29 18:01 UTC (permalink / raw)
  To: Keith Busch, linux-pci, Bjorn Helgaas
  Cc: Greg Kroah-Hartman, Lukas Wunner, Austin Bolen,
	Christoph Hellwig, Jens Axboe, Chris Petersen, Krishna Dhulipala

SGkgRm9sa3MsDQoNClRoaXMgc2VyaWVzIG9mIHBhdGNoZXMgaXMgY3JpdGljYWwgaW4gc3VwcG9y
dGluZyB0aGUgTlZNZSBTU0QgaG90IHBsdWcgb24gdGhlIEZsYXNoIEpCT0YgcGxhdGZvcm1zIHN1
Y2ggRmFjZWJvb2svT0NQIExpZ2h0bmluZy4gIFRoZSBQQ0llIGhvdHBsdWcgaW1wcm92ZW1lbnRz
IGhhdmUgYmVlbiBwcmVzZW50ZWQgaW4gbXVsdGlwbGUgY29uZmVyZW5jZXMgKG5hbWVseSBPQ1Ag
U3VtbWl0IDE3IGluIFNhbnRhIENsYXJhIGFuZCBMaW51eCBWYXVsdCAxNyBpbiBDYW1icmlkZ2Up
IGluIHRoZSBwYXN0IG1vbnRoLiAgVGhlc2UgY2hhbmdlcyBoYXZlIGJlZW4gdmFsaWRhdGVkIGJ5
IHRoZSBGYWNlYm9vayBpbnRlcm5hbCB0ZXN0aW5nIGFzIHdlbGwgYXMgbXVsdGlwbGUgT0RNIHZl
bmRvcnMgdGVzdGluZyB0ZWFtcy4gIA0KDQpBcmUgdGhlcmUgYWRkaXRpb25hbCBjb21tZW50cyBv
biB0aGlzPyAgSSB0YWxrZWQgdG8gS2VpdGggYXQgdGhlIFZhdWx0IGxhc3Qgd2Vlay4gIEhlIG1l
bnRpb25lZCB0aGVyZSB3YXMgbm8gb3V0c3RhbmRpbmcgcmV2aWV3IGZlZWRiYWNrIHRvIGJlIGFk
ZHJlc3NlZC4gICBXZSB3b3VsZCBhcHByZWNpYXRlIGlmIHdlIGNhbiB1cHN0cmVhbSB0aGlzIHBh
dGNoIEFTQVAhDQoNClRlc3RlZC1ieTogS3Jpc2huYSBEaHVsaXBhbGEgPGtyaXNobmFkQGZiLmNv
bT4NClJldmlld2VkLWJ5OiBXZWkgWmhhbmcgPHd6aGFuZ0BmYi5jb20+DQoNClRoYW5rcyENCi1X
ZWkNCg0KLS0NCndlaSB6aGFuZyB8IHNvZnR3YXJlIGVuZ2luZWVyIHwgZmFjZWJvb2sNCnd6aGFu
Z0BmYi5jb20gfCAoNDA4KSA0NjAtNDgwMw0KDQpPbiAyLzcvMTcsIDExOjMyIEFNLCAiS2VpdGgg
QnVzY2giIDxrZWl0aC5idXNjaEBpbnRlbC5jb20+IHdyb3RlOg0KDQogICAgVGhlIGNvZGUgaXMg
ZnVuY3Rpb25hbGx5IGVxdWl2YWxlbnQgdG8gdjUuIEkndmUgZml4ZWQgdGhlIHN0eWxlDQogICAg
cG9pbnRzIG1lbnRpb25lZCBpbiByZXZpZXcgb24gbGluZSB3YXJwcGluZywgYWRqdXN0ZWQgdGhl
IGNoYW5nZSBsb2cNCiAgICBvbiB0aGUgbXNpL21zaXggc2tpcHBhZ2UgdG8gbm90IGV4Y2x1ZGUg
bWVudGlvbmluZyBtc2ksIGFuZCBhZGRlZCB0aGUNCiAgICBhY2N1bXVsYXRlZCBSZXZpZXdlZC1i
eSdzLg0KICAgIA0KICAgIEtlaXRoIEJ1c2NoICg1KToNCiAgICAgIHBjaTogRXhwb3J0IHBjaSBk
ZXZpY2UgY29uZmlnIGFjY2Vzc29ycw0KICAgICAgcGNpOiBBZGQgZGV2aWNlIGRpc2Nvbm5lY3Rl
ZCBzdGF0ZQ0KICAgICAgcGNpOiBObyBjb25maWcgYWNjZXNzIGZvciBkaXNjb25uZWN0ZWQgZGV2
aWNlcw0KICAgICAgcGNpL21zaXg6IFNraXAgZGlzYWJsaW5nIGRpc2Nvbm5lY3RlZCBkZXZpY2Vz
DQogICAgICBwY2k6IFF1aWNrIHJldHVybiBmb3IgcGNpX2RldmljZV9pc19wcmVzZW50DQogICAg
DQogICAgIGRyaXZlcnMvcGNpL2FjY2Vzcy5jICAgICAgICAgICAgIHwgNTYgKysrKysrKysrKysr
KysrKysrKysrKysrKysrKysrKysrKysrKysrKw0KICAgICBkcml2ZXJzL3BjaS9ob3RwbHVnL3Bj
aWVocF9wY2kuYyB8ICA2ICsrKysrDQogICAgIGRyaXZlcnMvcGNpL21zaS5jICAgICAgICAgICAg
ICAgIHwgIDcgKysrKy0NCiAgICAgZHJpdmVycy9wY2kvcGNpLmMgICAgICAgICAgICAgICAgfCAg
MiArKw0KICAgICBkcml2ZXJzL3BjaS9wY2kuaCAgICAgICAgICAgICAgICB8IDE0ICsrKysrKysr
KysNCiAgICAgZHJpdmVycy9wY2kvcGNpZS9wY2llLWRwYy5jICAgICAgfCAgNSArKysrDQogICAg
IGluY2x1ZGUvbGludXgvcGNpLmggICAgICAgICAgICAgIHwgMzQgKysrKysrLS0tLS0tLS0tLS0t
LS0tLS0tDQogICAgIDcgZmlsZXMgY2hhbmdlZCwgOTcgaW5zZXJ0aW9ucygrKSwgMjcgZGVsZXRp
b25zKC0pDQogICAgDQogICAgLS0gDQogICAgMi43LjINCiAgICANCiAgICANCg0K

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv6 0/5] Limiting PCI access
  2017-02-07 19:32 [PATCHv6 0/5] Limiting PCI access Keith Busch
                   ` (5 preceding siblings ...)
  2017-03-29 18:01 ` [PATCHv6 0/5] Limiting PCI access Wei Zhang
@ 2017-03-30  3:56 ` Bjorn Helgaas
  2017-03-30  4:10   ` Wei Zhang
  6 siblings, 1 reply; 13+ messages in thread
From: Bjorn Helgaas @ 2017-03-30  3:56 UTC (permalink / raw)
  To: Keith Busch
  Cc: linux-pci, Bjorn Helgaas, Greg Kroah-Hartman, Lukas Wunner,
	Wei Zhang, Austin Bolen, Christoph Hellwig

On Tue, Feb 07, 2017 at 02:32:32PM -0500, Keith Busch wrote:
> The code is functionally equivalent to v5. I've fixed the style
> points mentioned in review on line warpping, adjusted the change log
> on the msi/msix skippage to not exclude mentioning msi, and added the
> accumulated Reviewed-by's.
> 
> Keith Busch (5):
>   pci: Export pci device config accessors
>   pci: Add device disconnected state
>   pci: No config access for disconnected devices
>   pci/msix: Skip disabling disconnected devices
>   pci: Quick return for pci_device_is_present
> 
>  drivers/pci/access.c             | 56 ++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/hotplug/pciehp_pci.c |  6 +++++
>  drivers/pci/msi.c                |  7 ++++-
>  drivers/pci/pci.c                |  2 ++
>  drivers/pci/pci.h                | 14 ++++++++++
>  drivers/pci/pcie/pcie-dpc.c      |  5 ++++
>  include/linux/pci.h              | 34 ++++++------------------
>  7 files changed, 97 insertions(+), 27 deletions(-)

Applied to pci/enumeration for v4.12 with

  Tested-by: Krishna Dhulipala <krishnad@fb.com>
  Reviewed-by: Wei Zhang <wzhang@fb.com>

Thanks!

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv6 0/5] Limiting PCI access
  2017-03-30  3:56 ` Bjorn Helgaas
@ 2017-03-30  4:10   ` Wei Zhang
  0 siblings, 0 replies; 13+ messages in thread
From: Wei Zhang @ 2017-03-30  4:10 UTC (permalink / raw)
  To: Bjorn Helgaas, Keith Busch
  Cc: linux-pci, Bjorn Helgaas, Greg Kroah-Hartman, Lukas Wunner,
	Austin Bolen, Christoph Hellwig

VGhpcyBpcyBncmVhdCEgIFRoYW5rcyBCam9ybiBhbmQgS2VpdGghICAtV2VpDQoNCi0tDQp3ZWkg
emhhbmcgfCBzb2Z0d2FyZSBlbmdpbmVlciB8IGZhY2Vib29rDQp3emhhbmdAZmIuY29tIHwgKDQw
OCkgNDYwLTQ4MDMNCg0KT24gMy8yOS8xNywgODo1NiBQTSwgIkJqb3JuIEhlbGdhYXMiIDxoZWxn
YWFzQGtlcm5lbC5vcmc+IHdyb3RlOg0KDQogICAgT24gVHVlLCBGZWIgMDcsIDIwMTcgYXQgMDI6
MzI6MzJQTSAtMDUwMCwgS2VpdGggQnVzY2ggd3JvdGU6DQogICAgPiBUaGUgY29kZSBpcyBmdW5j
dGlvbmFsbHkgZXF1aXZhbGVudCB0byB2NS4gSSd2ZSBmaXhlZCB0aGUgc3R5bGUNCiAgICA+IHBv
aW50cyBtZW50aW9uZWQgaW4gcmV2aWV3IG9uIGxpbmUgd2FycHBpbmcsIGFkanVzdGVkIHRoZSBj
aGFuZ2UgbG9nDQogICAgPiBvbiB0aGUgbXNpL21zaXggc2tpcHBhZ2UgdG8gbm90IGV4Y2x1ZGUg
bWVudGlvbmluZyBtc2ksIGFuZCBhZGRlZCB0aGUNCiAgICA+IGFjY3VtdWxhdGVkIFJldmlld2Vk
LWJ5J3MuDQogICAgPiANCiAgICA+IEtlaXRoIEJ1c2NoICg1KToNCiAgICA+ICAgcGNpOiBFeHBv
cnQgcGNpIGRldmljZSBjb25maWcgYWNjZXNzb3JzDQogICAgPiAgIHBjaTogQWRkIGRldmljZSBk
aXNjb25uZWN0ZWQgc3RhdGUNCiAgICA+ICAgcGNpOiBObyBjb25maWcgYWNjZXNzIGZvciBkaXNj
b25uZWN0ZWQgZGV2aWNlcw0KICAgID4gICBwY2kvbXNpeDogU2tpcCBkaXNhYmxpbmcgZGlzY29u
bmVjdGVkIGRldmljZXMNCiAgICA+ICAgcGNpOiBRdWljayByZXR1cm4gZm9yIHBjaV9kZXZpY2Vf
aXNfcHJlc2VudA0KICAgID4gDQogICAgPiAgZHJpdmVycy9wY2kvYWNjZXNzLmMgICAgICAgICAg
ICAgfCA1NiArKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrDQogICAgPiAg
ZHJpdmVycy9wY2kvaG90cGx1Zy9wY2llaHBfcGNpLmMgfCAgNiArKysrKw0KICAgID4gIGRyaXZl
cnMvcGNpL21zaS5jICAgICAgICAgICAgICAgIHwgIDcgKysrKy0NCiAgICA+ICBkcml2ZXJzL3Bj
aS9wY2kuYyAgICAgICAgICAgICAgICB8ICAyICsrDQogICAgPiAgZHJpdmVycy9wY2kvcGNpLmgg
ICAgICAgICAgICAgICAgfCAxNCArKysrKysrKysrDQogICAgPiAgZHJpdmVycy9wY2kvcGNpZS9w
Y2llLWRwYy5jICAgICAgfCAgNSArKysrDQogICAgPiAgaW5jbHVkZS9saW51eC9wY2kuaCAgICAg
ICAgICAgICAgfCAzNCArKysrKystLS0tLS0tLS0tLS0tLS0tLS0NCiAgICA+ICA3IGZpbGVzIGNo
YW5nZWQsIDk3IGluc2VydGlvbnMoKyksIDI3IGRlbGV0aW9ucygtKQ0KICAgIA0KICAgIEFwcGxp
ZWQgdG8gcGNpL2VudW1lcmF0aW9uIGZvciB2NC4xMiB3aXRoDQogICAgDQogICAgICBUZXN0ZWQt
Ynk6IEtyaXNobmEgRGh1bGlwYWxhIDxrcmlzaG5hZEBmYi5jb20+DQogICAgICBSZXZpZXdlZC1i
eTogV2VpIFpoYW5nIDx3emhhbmdAZmIuY29tPg0KICAgIA0KICAgIFRoYW5rcyENCiAgICANCg0K

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2017-03-30  4:10 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-07 19:32 [PATCHv6 0/5] Limiting PCI access Keith Busch
2017-02-07 19:32 ` [PATCHv6 1/5] pci: Export pci device config accessors Keith Busch
2017-02-07 19:32 ` [PATCHv6 2/5] pci: Add device disconnected state Keith Busch
2017-02-07 19:32 ` [PATCHv6 3/5] pci: No config access for disconnected devices Keith Busch
2017-02-08  6:04   ` Lukas Wunner
2017-02-10 18:38     ` Keith Busch
2017-02-10 19:54       ` Greg Kroah-Hartman
2017-02-10 20:54         ` Keith Busch
2017-02-07 19:32 ` [PATCHv6 4/5] pci/msix: Skip disabling " Keith Busch
2017-02-07 19:32 ` [PATCHv6 5/5] pci: Quick return for pci_device_is_present Keith Busch
2017-03-29 18:01 ` [PATCHv6 0/5] Limiting PCI access Wei Zhang
2017-03-30  3:56 ` Bjorn Helgaas
2017-03-30  4:10   ` Wei Zhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).