| Message ID | 1486144555-5526-3-git-send-email-keith.busch@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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
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.
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>
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
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.
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.
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.
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)
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(+)