[PATCHv5 0/5] Limiting PCI access

[PATCHv5 0/5] Limiting PCI access

[Keith Busch]

Version 5 of attempting to limit accessing PCI devices we know to be
disconnected. The motivation for this is that we do not need or want to
start an operation that we know will fail, and this has demonstrated an
improvement in time to complete device removal.

v4 -> v5:

Greg was concerned about the method for observing the disconnected state
being visible to device drivers. We do not want device drivers relying
on this to handle their device specific operations when a device is
suddenly disconnected. Since the intended usage is internal to the PCI
driver itself, this series privatizes the state. As a consequence, I had
to move the inline pci device config accessors to exported functions so
they could access the private API for the disconnected state.

I've also split the short-cut for pci_device_is_present from the
pci_dev_config_<read|write>_<byte|word|dword> access short cut into two
different patches since they're not really the same thing.

This is otherwise the same as before.

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 |  5 ++++
 drivers/pci/msi.c                |  7 ++++-
 drivers/pci/pci.c                |  2 ++
 drivers/pci/pci.h                | 14 ++++++++++
 drivers/pci/pcie/pcie-dpc.c      |  4 +++
 include/linux/pci.h              | 34 ++++++------------------
 7 files changed, 95 insertions(+), 27 deletions(-)

-- 
2.7.2

[PATCHv5 1/5] pci: Export pci device config accessors

[Keith Busch]

This patch removes 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>
---
 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

[PATCHv5 2/5] pci: Add device disconnected state

[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>
---
 drivers/pci/hotplug/pciehp_pci.c |  5 +++++
 drivers/pci/pci.h                | 14 ++++++++++++++
 drivers/pci/pcie/pcie-dpc.c      |  4 ++++
 include/linux/pci.h              |  2 ++
 4 files changed, 25 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
index 9e69403..9b2f41d 100644
--- a/drivers/pci/hotplug/pciehp_pci.c
+++ b/drivers/pci/hotplug/pciehp_pci.c
@@ -109,6 +109,11 @@ 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..d91e538 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,9 @@ 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

[PATCHv5 3/5] pci: No config access for disconnected devices

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

[PATCHv5 4/5] pci/msix: Skip disabling disconnected devices

[Keith Busch]

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

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 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

[PATCHv5 5/5] pci: Quick return for pci_device_is_present

[Keith Busch]

If the pci device is disconnected, return false immediately for checking
presence. We can't rely on the pci device config accessors since
this function uses the bus accessors, so this patch has the explicit
disconnected check first.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 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

Re: [PATCHv5 2/5] pci: Add device disconnected state

[Greg Kroah-Hartman]

On Fri, Feb 03, 2017 at 12:55:52PM -0500, Keith Busch wrote:
> 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>
> ---
>  drivers/pci/hotplug/pciehp_pci.c |  5 +++++
>  drivers/pci/pci.h                | 14 ++++++++++++++
>  drivers/pci/pcie/pcie-dpc.c      |  4 ++++
>  include/linux/pci.h              |  2 ++
>  4 files changed, 25 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
> index 9e69403..9b2f41d 100644
> --- a/drivers/pci/hotplug/pciehp_pci.c
> +++ b/drivers/pci/hotplug/pciehp_pci.c
> @@ -109,6 +109,11 @@ 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);

Why a single bit?  Are you trying to do this without locking or
something?  And if you want a bit, what's wrong with a C bit type?  Or
just a boolean?

I applaud your attempt to make this "private" to the pci core, just
don't know if it really will work, or if it is worth it entirely...

Anyway, I'll let Bjorn judge this feature being worth it or not :)

thanks,

greg k-h

Re: [PATCHv5 1/5] pci: Export pci device config accessors

[Christoph Hellwig]

On Fri, Feb 03, 2017 at 12:55:51PM -0500, Keith Busch wrote:
> This patch removes 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>

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv5 2/5] pci: Add device disconnected state

[Christoph Hellwig]

On Fri, Feb 03, 2017 at 09:53:30PM +0100, Greg Kroah-Hartman wrote:
> > +/* 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);
> 
> Why a single bit?  Are you trying to do this without locking or
> something?  And if you want a bit, what's wrong with a C bit type?  Or
> just a boolean?

What's a C bit type?  set_bit and friends defintively are the standard
for being able to set individual bits without worrying for RMW races.
So while a bool would work fine here for now, this seems much easier
to extent for any flag in the future if we need one.

Re: [PATCHv5 2/5] pci: Add device disconnected state

[Christoph Hellwig]

On Fri, Feb 03, 2017 at 12:55:52PM -0500, Keith Busch wrote:
> 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>
> ---
>  drivers/pci/hotplug/pciehp_pci.c |  5 +++++
>  drivers/pci/pci.h                | 14 ++++++++++++++
>  drivers/pci/pcie/pcie-dpc.c      |  4 ++++
>  include/linux/pci.h              |  2 ++
>  4 files changed, 25 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
> index 9e69403..9b2f41d 100644
> --- a/drivers/pci/hotplug/pciehp_pci.c
> +++ b/drivers/pci/hotplug/pciehp_pci.c
> @@ -109,6 +109,11 @@ 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);

overlong line here.

> +static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
> +{
> +	set_bit(PCI_DEV_DISCONNECTED, &dev->priv_flags);
> +	return 0;
> +}

Return void here?

> +			pci_walk_bus(dev->subordinate, pci_dev_set_disconnected, NULL);

Antother too long line.

Except for these nitpicks this patch looks fine to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv5 3/5] pci: No config access for disconnected devices

[Christoph Hellwig]

Looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Although a comment in the code explaining why return both an error
and set the returned value to all bits set might be helpful for
future readers of the code.

Re: [PATCHv5 4/5] pci/msix: Skip disabling disconnected devices

[Christoph Hellwig]

> @@ -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)) {

It also skips any MSI message writes as far as I cann tell from the
above.  Which seems sensible, but should be mentioned in the changelog.

Otherwise looks fine to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv5 5/5] pci: Quick return for pci_device_is_present

[Christoph Hellwig]

On Fri, Feb 03, 2017 at 12:55:55PM -0500, Keith Busch wrote:
> If the pci device is disconnected, return false immediately for checking
> presence. We can't rely on the pci device config accessors since
> this function uses the bus accessors, so this patch has the explicit
> disconnected check first.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv5 2/5] pci: Add device disconnected state

[Greg Kroah-Hartman]

On Mon, Feb 06, 2017 at 09:32:02AM -0800, Christoph Hellwig wrote:
> On Fri, Feb 03, 2017 at 09:53:30PM +0100, Greg Kroah-Hartman wrote:
> > > +/* 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);
> > 
> > Why a single bit?  Are you trying to do this without locking or
> > something?  And if you want a bit, what's wrong with a C bit type?  Or
> > just a boolean?
> 
> What's a C bit type?

Sorry, was thinking of:
	unsigned foo:2;
don't know what the "real" name for that is...

thanks,

greg k-h

Re: [PATCHv5 2/5] pci: Add device disconnected state

[Christoph Hellwig]

On Mon, Feb 06, 2017 at 06:40:46PM +0100, Greg Kroah-Hartman wrote:
> Sorry, was thinking of:
> 	unsigned foo:2;
> don't know what the "real" name for that is...

They're bitfield.  Same issue with RMW applies as for bool.

Re: [PATCHv5 2/5] pci: Add device disconnected state

[Keith Busch]

On Mon, Feb 06, 2017 at 06:40:46PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Feb 06, 2017 at 09:32:02AM -0800, Christoph Hellwig wrote:
> > On Fri, Feb 03, 2017 at 09:53:30PM +0100, Greg Kroah-Hartman wrote:
> > > > +/* 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);
> > > 
> > > Why a single bit?  Are you trying to do this without locking or
> > > something?  And if you want a bit, what's wrong with a C bit type?  Or
> > > just a boolean?
> > 
> > What's a C bit type?
> 
> Sorry, was thinking of:
> 	unsigned foo:2;
> don't know what the "real" name for that is...

The C standard calls that a 'bit field'. I originally used that for this
flag, but Bjorn was concerned about the atomicity of getting this bit set,
conflicting with setting adjacent bits. It's unlikely that would happen
considering the pci driver's current usage, but he's correct that it's
possible without additional locking.

A bool would work just as well for the intended usage, though we may
eventually want all the currently defined bit fields to use the set/test
bit APIs, so this gets us started in that direction.

Re: [PATCHv5 2/5] pci: Add device disconnected state

[Keith Busch]

On Mon, Feb 06, 2017 at 09:33:00AM -0800, Christoph Hellwig wrote:
> On Fri, Feb 03, 2017 at 12:55:52PM -0500, Keith Busch wrote:
> > +static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
> > +{
> > +	set_bit(PCI_DEV_DISCONNECTED, &dev->priv_flags);
> > +	return 0;
> > +}
> 
> Return void here?

This is used as the callback to pci_walk_bus for setting downstream device
disconnect state in case a switch was hot removed, so I have to respect
the prototype's 'int' return and the "unused" data parameter.

I'll fix up the long lines.