v2: Sorry, v1 somehow didn't cc linux-pci. No code change
We currently support reset of individual functions via the
pci_reset_function() interface. If a device supports one of the FLR
mechanisms, this works great. If not, things get a little shady. We
can do a D3hot->D0 PM reset if the device claims that does a soft
reset, but the definition of a soft reset seems to be left to
interpretation. We can do a secondary bus reset if the device is
behind a bridge, but this is a function level reset interface, so if
there's anything else on that bus, we're out of luck. Speaking of out
of luck, guess what happens when we do a secondary bus reset on a port
that supports pciehp surprise removal... we tickle the presence
detection logic and start a hot remove, but the device is locked, so
it doesn't get removed, then it gets added again, but it already
exists... ugh, what a mess.
That seems like an indication that we want to create an interface for
hotplug controllers to participate in a bus reset and prevent such
craziness. But we don't hotplug a bus, we hotplug a slot. A bus can
support multiple slots, so depending on the hotplug controller
capabilities, we can potentially do a reset on a slot granularity and
we can avoid spurious hotplug events. Thus, pci_reset_slot.
We're not always going to have a hotplug controller, in fact pciehp is
the only one I know how to implement a reset_slot interface for (and
it actually has a 1:1 slot:bus interface), so we can try pci_reset_slot
and fall through to pci_reset_bus if needed.
Once we have these we can export them to drivers, because sometimes
those shady parts of pci_reset_function just aren't good enough and we
understand that a reset can affect multiple devices. VFIO is a driver
that can take advantage of such an interface. A VFIO user can be in
control of multiple devices. If they control all of the devices on a
bus or a slot, a bus or slot reset mechanism should be available.
Where we particularly need this is for exposing VGA devices to
userspace. These fall into the shady area where they claim to support
a PM function reset, but it doesn't actually do anything. They're
also often not alone on the bus as they support a separate function
for the audio controller. With this series we can expose a bus reset
interface and get repeatability.
Comments welcome. Thanks,
Alex
---
Alex Williamson (8):
pci: Create pci_reset_bridge_secondary_bus()
pci: Add hotplug_slot_ops.reset_slot()
pci: Add pci_hp_reset_slot
pci: Implement reset_slot for pciehp
pci: Add reset_slot option to pci_dev_reset
pci: Split out pci_dev lock/unlock and save/restore
pci: Add slot and bus reset interfaces
pci: Wake-up devices before save for reset
drivers/pci/hotplug/pci_hotplug_core.c | 24 ++
drivers/pci/hotplug/pciehp.h | 1
drivers/pci/hotplug/pciehp_core.c | 12 +
drivers/pci/hotplug/pciehp_hpc.c | 31 +++
drivers/pci/pci.c | 313 +++++++++++++++++++++++++++++---
include/linux/pci.h | 3
include/linux/pci_hotplug.h | 12 +
7 files changed, 370 insertions(+), 26 deletions(-)
Move the secondary bus reset code from pci_parent_bus_reset() into its own
function. Export it as we'll later be calling it from hotplug controllers.
Signed-off-by: Alex Williamson <[email protected]>
---
drivers/pci/pci.c | 32 +++++++++++++++++++++++---------
include/linux/pci.h | 1 +
2 files changed, 24 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b099e00..d0f313f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3210,9 +3210,30 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
return 0;
}
-static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
+/**
+ * pci_reset_bridge_secondary_bus - Reset the secondary bus on a PCI bridge.
+ * @dev: Bridge device
+ *
+ * Use the bridge control register to assert reset on the secondary bus.
+ * Devices on the secondary bus are left in power-on state.
+ */
+void pci_reset_bridge_secondary_bus(struct pci_dev *dev)
{
u16 ctrl;
+
+ pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &ctrl);
+ ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
+ pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);
+ msleep(100);
+
+ ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
+ pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);
+ msleep(100);
+}
+EXPORT_SYMBOL_GPL(pci_reset_bridge_secondary_bus);
+
+static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
+{
struct pci_dev *pdev;
if (pci_is_root_bus(dev->bus) || dev->subordinate || !dev->bus->self)
@@ -3225,14 +3246,7 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
if (probe)
return 0;
- pci_read_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, &ctrl);
- ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
- pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
- msleep(100);
-
- ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
- pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
- msleep(100);
+ pci_reset_bridge_secondary_bus(dev->bus->self);
return 0;
}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 710067f..695620c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -905,6 +905,7 @@ int pcie_set_mps(struct pci_dev *dev, int mps);
int __pci_reset_function(struct pci_dev *dev);
int __pci_reset_function_locked(struct pci_dev *dev);
int pci_reset_function(struct pci_dev *dev);
+void pci_reset_bridge_secondary_bus(struct pci_dev *dev);
void pci_update_resource(struct pci_dev *dev, int resno);
int __must_check pci_assign_resource(struct pci_dev *dev, int i);
int __must_check pci_reassign_resource(struct pci_dev *dev, int i, resource_size_t add_size, resource_size_t align);
This optional callback allows htoplug controllers to perform slot
specific resets. These may be necessary in cases where a normal
secondary bus reset can interact with controller logic and expose
spurious hotplugs.
Signed-off-by: Alex Williamson <[email protected]>
---
include/linux/pci_hotplug.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index 45fc162..0d3a604 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -63,6 +63,9 @@ enum pcie_link_width {
* @get_adapter_status: Called to get see if an adapter is present in the slot or not.
* If this field is NULL, the value passed in the struct hotplug_slot_info
* will be used when this value is requested by a user.
+ * @reset_slot: Optional interface to allow override of a bus reset for the
+ * slot for cases where a secondary bus reset can result in spurious
+ * hotplug events or where a slot can be reset independent of the bus.
*
* The table of function pointers that is passed to the hotplug pci core by a
* hotplug pci driver. These functions are called by the hotplug pci core when
@@ -80,6 +83,7 @@ struct hotplug_slot_ops {
int (*get_attention_status) (struct hotplug_slot *slot, u8 *value);
int (*get_latch_status) (struct hotplug_slot *slot, u8 *value);
int (*get_adapter_status) (struct hotplug_slot *slot, u8 *value);
+ int (*reset_slot) (struct hotplug_slot *slot, int probe);
};
/**
Disable notification around secondary bus reset.
Signed-off-by: Alex Williamson <[email protected]>
---
drivers/pci/hotplug/pciehp.h | 1 +
drivers/pci/hotplug/pciehp_core.c | 12 ++++++++++++
drivers/pci/hotplug/pciehp_hpc.c | 31 +++++++++++++++++++++++++++++++
3 files changed, 44 insertions(+)
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index 2c113de..6c9e674 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -155,6 +155,7 @@ void pciehp_green_led_off(struct slot *slot);
void pciehp_green_led_blink(struct slot *slot);
int pciehp_check_link_status(struct controller *ctrl);
void pciehp_release_ctrl(struct controller *ctrl);
+int pciehp_reset_slot(struct slot *slot, int probe);
static inline const char *slot_name(struct slot *slot)
{
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 7d72c5e..f4a18f5 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -69,6 +69,7 @@ static int get_power_status (struct hotplug_slot *slot, u8 *value);
static int get_attention_status (struct hotplug_slot *slot, u8 *value);
static int get_latch_status (struct hotplug_slot *slot, u8 *value);
static int get_adapter_status (struct hotplug_slot *slot, u8 *value);
+static int reset_slot (struct hotplug_slot *slot, int probe);
/**
* release_slot - free up the memory used by a slot
@@ -111,6 +112,7 @@ static int init_slot(struct controller *ctrl)
ops->disable_slot = disable_slot;
ops->get_power_status = get_power_status;
ops->get_adapter_status = get_adapter_status;
+ ops->reset_slot = reset_slot;
if (MRL_SENS(ctrl))
ops->get_latch_status = get_latch_status;
if (ATTN_LED(ctrl)) {
@@ -223,6 +225,16 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
return pciehp_get_adapter_status(slot, value);
}
+static int reset_slot(struct hotplug_slot *hotplug_slot, int probe)
+{
+ struct slot *slot = hotplug_slot->private;
+
+ ctrl_dbg(slot->ctrl, "%s: physical_slot = %s\n",
+ __func__, slot_name(slot));
+
+ return pciehp_reset_slot(slot, probe);
+}
+
static int pciehp_probe(struct pcie_device *dev)
{
int rc;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 5127f3f..b01ed53 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -749,6 +749,37 @@ static void pcie_disable_notification(struct controller *ctrl)
ctrl_warn(ctrl, "Cannot disable software notification\n");
}
+/*
+ * pciehp has a 1:1 bus:slot relationship so we ultimately want a secondary
+ * bus reset of the bridge, but if the slot supports surprise removal we need
+ * to disable presence detection around the bus reset and clear any spurious
+ * events after.
+ */
+int pciehp_reset_slot(struct slot *slot, int probe)
+{
+ struct controller *ctrl = slot->ctrl;
+
+ if (probe)
+ return 0;
+
+ if (HP_SUPR_RM(ctrl)) {
+ pcie_write_cmd(ctrl, 0, PCI_EXP_SLTCTL_PDCE);
+ if (pciehp_poll_mode)
+ del_timer_sync(&ctrl->poll_timer);
+ }
+
+ pci_reset_bridge_secondary_bus(ctrl->pcie->port);
+
+ if (HP_SUPR_RM(ctrl)) {
+ pciehp_writew(ctrl, PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_PDC);
+ pcie_write_cmd(ctrl, PCI_EXP_SLTCTL_PDCE, PCI_EXP_SLTCTL_PDCE);
+ if (pciehp_poll_mode)
+ int_poll_timeout(ctrl->poll_timer.data);
+ }
+
+ return 0;
+}
+
int pcie_init_notification(struct controller *ctrl)
{
if (pciehp_request_irq(ctrl))
Provide an interface to get to hotplug controller reset_slot callback
Signed-off-by: Alex Williamson <[email protected]>
---
drivers/pci/hotplug/pci_hotplug_core.c | 24 ++++++++++++++++++++++++
include/linux/pci_hotplug.h | 8 ++++++++
2 files changed, 32 insertions(+)
diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
index 202f4a9..1d0fee4 100644
--- a/drivers/pci/hotplug/pci_hotplug_core.c
+++ b/drivers/pci/hotplug/pci_hotplug_core.c
@@ -537,6 +537,29 @@ int __must_check pci_hp_change_slot_info(struct hotplug_slot *hotplug,
return 0;
}
+/**
+ * pci_hp_reset_slot - reset slot
+ *
+ * @hotplug: pointer to hotplug slot to reset
+ * @probe: reset slot (0) or just probe
+ *
+ * Returns 0 if successful, anything else for an error.
+ */
+int pci_hp_reset_slot(struct hotplug_slot *hotplug, int probe)
+{
+ int result = -ENOTTY;
+
+ if (!hotplug || !try_module_get(hotplug->ops->owner))
+ return result;
+
+ if (hotplug->ops->reset_slot)
+ result = hotplug->ops->reset_slot(hotplug, probe);
+
+ module_put(hotplug->ops->owner);
+
+ return result;
+}
+
static int __init pci_hotplug_init (void)
{
int result;
@@ -570,3 +593,4 @@ MODULE_PARM_DESC(debug, "Debugging mode enabled or not");
EXPORT_SYMBOL_GPL(__pci_hp_register);
EXPORT_SYMBOL_GPL(pci_hp_deregister);
EXPORT_SYMBOL_GPL(pci_hp_change_slot_info);
+EXPORT_SYMBOL_GPL(pci_hp_reset_slot);
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index 0d3a604..d771cee 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -135,6 +135,14 @@ extern int __pci_hp_register(struct hotplug_slot *slot, struct pci_bus *pbus,
extern int pci_hp_deregister(struct hotplug_slot *slot);
extern int __must_check pci_hp_change_slot_info (struct hotplug_slot *slot,
struct hotplug_slot_info *info);
+#ifdef CONFIG_HOTPLUG_PCI
+int pci_hp_reset_slot(struct hotplug_slot *slot, int probe);
+#else
+static inline int pci_hp_reset_slot(struct hotplug_slot *slot, int probe)
+{
+ return -ENOTTY;
+}
+#endif
/* use a define to avoid include chaining to get THIS_MODULE & friends */
#define pci_hp_register(slot, pbus, devnr, name) \
If the hotplug controller provides a way to reset a slot, use that
before a direct parent bus reset.
Signed-off-by: Alex Williamson <[email protected]>
---
drivers/pci/pci.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d0f313f..f5035f5 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -22,6 +22,7 @@
#include <linux/interrupt.h>
#include <linux/device.h>
#include <linux/pm_runtime.h>
+#include <linux/pci_hotplug.h>
#include <asm-generic/pci-bridge.h>
#include <asm/setup.h>
#include "pci.h"
@@ -3251,6 +3252,20 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
return 0;
}
+static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
+{
+ struct pci_dev *pdev;
+
+ if (dev->subordinate || !dev->slot)
+ return -ENOTTY;
+
+ list_for_each_entry(pdev, &dev->bus->devices, bus_list)
+ if (pdev != dev && pdev->slot == dev->slot)
+ return -ENOTTY;
+
+ return pci_hp_reset_slot(dev->slot->hotplug, probe);
+}
+
static int __pci_dev_reset(struct pci_dev *dev, int probe)
{
int rc;
@@ -3273,6 +3288,10 @@ static int __pci_dev_reset(struct pci_dev *dev, int probe)
if (rc != -ENOTTY)
goto done;
+ rc = pci_dev_reset_slot_function(dev, probe);
+ if (rc != -ENOTTY)
+ goto done;
+
rc = pci_parent_bus_reset(dev, probe);
done:
return rc;
Sometimes pci_reset_function is not sufficient. We have cases where
devices do not support any kind of reset, but there might be multiple
functions on the bus preventing pci_reset_function from doing a
secondary bus reset. We also have cases where a device will advertise
that it supports a PM reset, but really does nothing on D3hot->D0
(graphics cards are notorious for this). These devices often also
have more than one function, so even blacklisting PM reset for them
wouldn't allow a secondary bus reset through pci_reset_function.
If a driver supports multiple devices it should have the ability to
induce a bus reset when it needs to. This patch provides that ability
through pci_reset_slot and pci_reset_bus. It's the caller's
responsibility when using these interfaces to understand that all of
the devices in or below the slot (or on or below the bus) will be
reset and therefore should be under control of the caller. PCI state
of all the affected devices is saved and restored around these resets,
but internal state of all of the affected devices is reset (which
should be the intention).
Signed-off-by: Alex Williamson <[email protected]>
---
drivers/pci/pci.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 2 +
2 files changed, 205 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ee3cb18..8647998 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3437,6 +3437,209 @@ int pci_reset_function(struct pci_dev *dev)
}
EXPORT_SYMBOL_GPL(pci_reset_function);
+static void pci_bus_lock(struct pci_bus *bus)
+{
+ struct pci_dev *dev;
+
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ pci_dev_lock(dev);
+ if (dev->subordinate)
+ pci_bus_lock(dev->subordinate);
+ }
+}
+
+static void pci_bus_unlock(struct pci_bus *bus)
+{
+ struct pci_dev *dev;
+
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ pci_dev_unlock(dev);
+ if (dev->subordinate)
+ pci_bus_unlock(dev->subordinate);
+ }
+}
+
+static void pci_slot_lock(struct pci_slot *slot)
+{
+ struct pci_dev *dev;
+
+ BUG_ON(!slot);
+
+ list_for_each_entry(dev, &slot->bus->devices, bus_list) {
+ if (!dev->slot || dev->slot != slot)
+ continue;
+ pci_dev_lock(dev);
+ if (dev->subordinate)
+ pci_bus_lock(dev->subordinate);
+ }
+}
+
+static void pci_slot_unlock(struct pci_slot *slot)
+{
+ struct pci_dev *dev;
+
+ BUG_ON(!slot);
+
+ list_for_each_entry(dev, &slot->bus->devices, bus_list) {
+ if (!dev->slot || dev->slot != slot)
+ continue;
+ pci_dev_unlock(dev);
+ if (dev->subordinate)
+ pci_bus_unlock(dev->subordinate);
+ }
+}
+
+static void pci_bus_save(struct pci_bus *bus)
+{
+ struct pci_dev *dev;
+
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ pci_dev_save(dev);
+ if (dev->subordinate)
+ pci_bus_save(dev->subordinate);
+ }
+}
+
+static void pci_bus_restore(struct pci_bus *bus)
+{
+ struct pci_dev *dev;
+
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ pci_dev_restore(dev);
+ if (dev->subordinate)
+ pci_bus_restore(dev->subordinate);
+ }
+}
+
+static void pci_slot_save(struct pci_slot *slot)
+{
+ struct pci_dev *dev;
+
+ BUG_ON(!slot);
+
+ list_for_each_entry(dev, &slot->bus->devices, bus_list) {
+ if (!dev->slot || dev->slot != slot)
+ continue;
+ pci_dev_save(dev);
+ if (dev->subordinate)
+ pci_bus_save(dev->subordinate);
+ }
+}
+
+static void pci_slot_restore(struct pci_slot *slot)
+{
+ struct pci_dev *dev;
+
+ BUG_ON(!slot);
+
+ list_for_each_entry(dev, &slot->bus->devices, bus_list) {
+ if (!dev->slot || dev->slot != slot)
+ continue;
+ pci_dev_restore(dev);
+ if (dev->subordinate)
+ pci_bus_restore(dev->subordinate);
+ }
+}
+
+static int pci_slot_reset(struct pci_slot *slot, int probe)
+{
+ int rc;
+
+ if (!slot)
+ return -ENOTTY;
+
+ if (!probe)
+ pci_slot_lock(slot);
+
+ might_sleep();
+
+ rc = pci_hp_reset_slot(slot->hotplug, probe);
+
+ if (!probe)
+ pci_slot_unlock(slot);
+
+ return rc;
+}
+
+/**
+ * pci_reset_slot - reset a PCI slot
+ * @slot: PCI slot to reset
+ *
+ * A PCI bus may host multiple slots, each slot may support a reset mechanism
+ * independent of other slots. For instance, some slots may support slot power
+ * control. In the case of a 1:1 bus to slot architecture, this function may
+ * wrap the bus reset to avoid spurious slot related events such as hotplug.
+ * Generally a slot reset should be attempted before a bus reset. All of the
+ * function of the slot and any subordinate buses behind the slot are reset
+ * through this function. PCI config space of all devices in the slot and
+ * behind the slot is saved before and restored after reset.
+ *
+ * Return 0 on success, non-zero on error.
+ */
+int pci_reset_slot(struct pci_slot *slot)
+{
+ int rc;
+
+ rc = pci_slot_reset(slot, 1);
+ if (rc)
+ return rc;
+
+ pci_slot_save(slot);
+
+ rc = pci_slot_reset(slot, 0);
+
+ pci_slot_restore(slot);
+
+ return rc;
+}
+EXPORT_SYMBOL_GPL(pci_reset_slot);
+
+static int pci_bus_reset(struct pci_bus *bus, int probe)
+{
+ if (!bus->self)
+ return -ENOTTY;
+
+ if (probe)
+ return 0;
+
+ pci_bus_lock(bus);
+
+ might_sleep();
+
+ pci_reset_bridge_secondary_bus(bus->self);
+
+ pci_bus_unlock(bus);
+
+ return 0;
+}
+
+/**
+ * pci_reset_bus - reset a PCI bus
+ * @bus: top level PCI bus to reset
+ *
+ * Do a bus reset on the given bus and any subordinate buses, saving
+ * and restoring state of all devices.
+ *
+ * Return 0 on success, non-zero on error.
+ */
+int pci_reset_bus(struct pci_bus *bus)
+{
+ int rc;
+
+ rc = pci_bus_reset(bus, 1);
+ if (rc)
+ return rc;
+
+ pci_bus_save(bus);
+
+ rc = pci_bus_reset(bus, 0);
+
+ pci_bus_restore(bus);
+
+ return rc;
+}
+EXPORT_SYMBOL_GPL(pci_reset_bus);
+
/**
* pcix_get_max_mmrbc - get PCI-X maximum designed memory read byte count
* @dev: PCI device to query
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 695620c..bd8ec30 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -905,6 +905,8 @@ int pcie_set_mps(struct pci_dev *dev, int mps);
int __pci_reset_function(struct pci_dev *dev);
int __pci_reset_function_locked(struct pci_dev *dev);
int pci_reset_function(struct pci_dev *dev);
+int pci_reset_slot(struct pci_slot *slot);
+int pci_reset_bus(struct pci_bus *bus);
void pci_reset_bridge_secondary_bus(struct pci_dev *dev);
void pci_update_resource(struct pci_dev *dev, int resno);
int __must_check pci_assign_resource(struct pci_dev *dev, int i);
Only cosmetic changes to existing paths.
Signed-off-by: Alex Williamson <[email protected]>
---
drivers/pci/pci.c | 52 +++++++++++++++++++++++++++++++++++-----------------
1 file changed, 35 insertions(+), 17 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f5035f5..ee3cb18 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3297,22 +3297,46 @@ done:
return rc;
}
+static void pci_dev_lock(struct pci_dev *dev)
+{
+ pci_cfg_access_lock(dev);
+ /* block PM suspend, driver probe, etc. */
+ device_lock(&dev->dev);
+}
+
+static void pci_dev_unlock(struct pci_dev *dev)
+{
+ device_unlock(&dev->dev);
+ pci_cfg_access_unlock(dev);
+}
+
+static void pci_dev_save(struct pci_dev *dev)
+{
+ pci_save_state(dev);
+ /*
+ * both INTx and MSI are disabled after the Interrupt Disable bit
+ * is set and the Bus Master bit is cleared.
+ */
+ pci_write_config_word(dev, PCI_COMMAND, PCI_COMMAND_INTX_DISABLE);
+}
+
+static void pci_dev_restore(struct pci_dev *dev)
+{
+ pci_restore_state(dev);
+}
+
static int pci_dev_reset(struct pci_dev *dev, int probe)
{
int rc;
- if (!probe) {
- pci_cfg_access_lock(dev);
- /* block PM suspend, driver probe, etc. */
- device_lock(&dev->dev);
- }
+ if (!probe)
+ pci_dev_lock(dev);
rc = __pci_dev_reset(dev, probe);
- if (!probe) {
- device_unlock(&dev->dev);
- pci_cfg_access_unlock(dev);
- }
+ if (!probe)
+ pci_dev_unlock(dev);
+
return rc;
}
/**
@@ -3403,17 +3427,11 @@ int pci_reset_function(struct pci_dev *dev)
if (rc)
return rc;
- pci_save_state(dev);
-
- /*
- * both INTx and MSI are disabled after the Interrupt Disable bit
- * is set and the Bus Master bit is cleared.
- */
- pci_write_config_word(dev, PCI_COMMAND, PCI_COMMAND_INTX_DISABLE);
+ pci_dev_save(dev);
rc = pci_dev_reset(dev, 0);
- pci_restore_state(dev);
+ pci_dev_restore(dev);
return rc;
}
Devices come out of reset in D0. Restoring a device to a different
post-reset state takes more smarts than our simple config space
restore, which can leave devices in an inconsistent state. For
example, if a device is reset in D3, but the restore doesn't
successfully return the device to D3, then the actual state of the
device and dev->current_state are contradictory. Put everything
in D0 going into the reset, then we don't need to do anything
special on the way out.
Signed-off-by: Alex Williamson <[email protected]>
---
drivers/pci/pci.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8647998..98372ec 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3312,6 +3312,13 @@ static void pci_dev_unlock(struct pci_dev *dev)
static void pci_dev_save(struct pci_dev *dev)
{
+ /*
+ * Wake-up device prior to save. PM registers default to D0 after
+ * reset and a simple register restore doesn't reliably return
+ * to a non-D0 state anyway.
+ */
+ pci_set_power_state(dev, PCI_D0);
+
pci_save_state(dev);
/*
* both INTx and MSI are disabled after the Interrupt Disable bit
On 05/07/2013 10:57 PM, Alex Williamson wrote:
> Move the secondary bus reset code from pci_parent_bus_reset() into its own
> function. Export it as we'll later be calling it from hotplug controllers.
>
> Signed-off-by: Alex Williamson<[email protected]>
> ---
> drivers/pci/pci.c | 32 +++++++++++++++++++++++---------
> include/linux/pci.h | 1 +
> 2 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b099e00..d0f313f 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3210,9 +3210,30 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
> return 0;
> }
>
> -static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
> +/**
> + * pci_reset_bridge_secondary_bus - Reset the secondary bus on a PCI bridge.
> + * @dev: Bridge device
> + *
> + * Use the bridge control register to assert reset on the secondary bus.
> + * Devices on the secondary bus are left in power-on state.
> + */
> +void pci_reset_bridge_secondary_bus(struct pci_dev *dev)
see recommended api change below...
> {
> u16 ctrl;
> +
> + pci_read_config_word(dev, PCI_BRIDGE_CONTROL,&ctrl);
> + ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
> + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);
> + msleep(100);
> +
> + ctrl&= ~PCI_BRIDGE_CTL_BUS_RESET;
> + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);
> + msleep(100);
> +}
> +EXPORT_SYMBOL_GPL(pci_reset_bridge_secondary_bus);
> +
> +static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
> +{
> struct pci_dev *pdev;
>
> if (pci_is_root_bus(dev->bus) || dev->subordinate || !dev->bus->self)
> @@ -3225,14 +3246,7 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
> if (probe)
> return 0;
>
> - pci_read_config_word(dev->bus->self, PCI_BRIDGE_CONTROL,&ctrl);
> - ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
> - pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
> - msleep(100);
> -
> - ctrl&= ~PCI_BRIDGE_CTL_BUS_RESET;
> - pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
> - msleep(100);
> + pci_reset_bridge_secondary_bus(dev->bus->self);
>
It makes more sense to pass the pci-bus struct of the bus you want to reset,
instead of the pdev of the bridge that bus is sourced by.
i.e, dev->bus, and above, extract the pdev from the bus-struct.
That way, devices asking for bus-reset, don't have to know the bus-struct details
to make the call right... just their own pdev->bus reference.
(more object-like/clean).
> return 0;
> }
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 710067f..695620c 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -905,6 +905,7 @@ int pcie_set_mps(struct pci_dev *dev, int mps);
> int __pci_reset_function(struct pci_dev *dev);
> int __pci_reset_function_locked(struct pci_dev *dev);
> int pci_reset_function(struct pci_dev *dev);
> +void pci_reset_bridge_secondary_bus(struct pci_dev *dev);
and final tweak.
> void pci_update_resource(struct pci_dev *dev, int resno);
> int __must_check pci_assign_resource(struct pci_dev *dev, int i);
> int __must_check pci_reassign_resource(struct pci_dev *dev, int i, resource_size_t add_size, resource_size_t align);
>
> --
> 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
On Wed, 2013-05-08 at 15:14 -0400, Don Dutile wrote:
> On 05/07/2013 10:57 PM, Alex Williamson wrote:
> > Move the secondary bus reset code from pci_parent_bus_reset() into its own
> > function. Export it as we'll later be calling it from hotplug controllers.
> >
> > Signed-off-by: Alex Williamson<[email protected]>
> > ---
> > drivers/pci/pci.c | 32 +++++++++++++++++++++++---------
> > include/linux/pci.h | 1 +
> > 2 files changed, 24 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index b099e00..d0f313f 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3210,9 +3210,30 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
> > return 0;
> > }
> >
> > -static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
> > +/**
> > + * pci_reset_bridge_secondary_bus - Reset the secondary bus on a PCI bridge.
> > + * @dev: Bridge device
> > + *
> > + * Use the bridge control register to assert reset on the secondary bus.
> > + * Devices on the secondary bus are left in power-on state.
> > + */
> > +void pci_reset_bridge_secondary_bus(struct pci_dev *dev)
> see recommended api change below...
>
> > {
> > u16 ctrl;
> > +
> > + pci_read_config_word(dev, PCI_BRIDGE_CONTROL,&ctrl);
> > + ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
> > + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);
> > + msleep(100);
> > +
> > + ctrl&= ~PCI_BRIDGE_CTL_BUS_RESET;
> > + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);
> > + msleep(100);
> > +}
> > +EXPORT_SYMBOL_GPL(pci_reset_bridge_secondary_bus);
> > +
> > +static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
> > +{
> > struct pci_dev *pdev;
> >
> > if (pci_is_root_bus(dev->bus) || dev->subordinate || !dev->bus->self)
> > @@ -3225,14 +3246,7 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
> > if (probe)
> > return 0;
> >
> > - pci_read_config_word(dev->bus->self, PCI_BRIDGE_CONTROL,&ctrl);
> > - ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
> > - pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
> > - msleep(100);
> > -
> > - ctrl&= ~PCI_BRIDGE_CTL_BUS_RESET;
> > - pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
> > - msleep(100);
> > + pci_reset_bridge_secondary_bus(dev->bus->self);
> >
> It makes more sense to pass the pci-bus struct of the bus you want to reset,
> instead of the pdev of the bridge that bus is sourced by.
> i.e, dev->bus, and above, extract the pdev from the bus-struct.
> That way, devices asking for bus-reset, don't have to know the bus-struct details
> to make the call right... just their own pdev->bus reference.
> (more object-like/clean).
Have you read patch 7/8 or does this comment take that patch into
account? For the bus reset and slot reset interfaces the API I'm
proposing does require passing a pci_bus and pci_slot respectively.
Here we have a bridge (pci_dev) and we want to reset the secondary bus
on that bridge and I think the function name indicates that. Thanks,
Alex
> > return 0;
> > }
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 710067f..695620c 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -905,6 +905,7 @@ int pcie_set_mps(struct pci_dev *dev, int mps);
> > int __pci_reset_function(struct pci_dev *dev);
> > int __pci_reset_function_locked(struct pci_dev *dev);
> > int pci_reset_function(struct pci_dev *dev);
> > +void pci_reset_bridge_secondary_bus(struct pci_dev *dev);
> and final tweak.
>
> > void pci_update_resource(struct pci_dev *dev, int resno);
> > int __must_check pci_assign_resource(struct pci_dev *dev, int i);
> > int __must_check pci_reassign_resource(struct pci_dev *dev, int i, resource_size_t add_size, resource_size_t align);
> >
> > --
> > 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
>
On Tue, 2013-05-07 at 20:57 -0600, Alex Williamson wrote:
> Provide an interface to get to hotplug controller reset_slot callback
>
> Signed-off-by: Alex Williamson <[email protected]>
> ---
> drivers/pci/hotplug/pci_hotplug_core.c | 24 ++++++++++++++++++++++++
> include/linux/pci_hotplug.h | 8 ++++++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
> index 202f4a9..1d0fee4 100644
> --- a/drivers/pci/hotplug/pci_hotplug_core.c
> +++ b/drivers/pci/hotplug/pci_hotplug_core.c
> @@ -537,6 +537,29 @@ int __must_check pci_hp_change_slot_info(struct hotplug_slot *hotplug,
> return 0;
> }
>
> +/**
> + * pci_hp_reset_slot - reset slot
> + *
> + * @hotplug: pointer to hotplug slot to reset
> + * @probe: reset slot (0) or just probe
> + *
> + * Returns 0 if successful, anything else for an error.
> + */
> +int pci_hp_reset_slot(struct hotplug_slot *hotplug, int probe)
> +{
> + int result = -ENOTTY;
> +
> + if (!hotplug || !try_module_get(hotplug->ops->owner))
> + return result;
> +
> + if (hotplug->ops->reset_slot)
> + result = hotplug->ops->reset_slot(hotplug, probe);
> +
> + module_put(hotplug->ops->owner);
> +
> + return result;
> +}
> +
> static int __init pci_hotplug_init (void)
> {
> int result;
> @@ -570,3 +593,4 @@ MODULE_PARM_DESC(debug, "Debugging mode enabled or not");
> EXPORT_SYMBOL_GPL(__pci_hp_register);
> EXPORT_SYMBOL_GPL(pci_hp_deregister);
> EXPORT_SYMBOL_GPL(pci_hp_change_slot_info);
> +EXPORT_SYMBOL_GPL(pci_hp_reset_slot);
> diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
> index 0d3a604..d771cee 100644
> --- a/include/linux/pci_hotplug.h
> +++ b/include/linux/pci_hotplug.h
> @@ -135,6 +135,14 @@ extern int __pci_hp_register(struct hotplug_slot *slot, struct pci_bus *pbus,
> extern int pci_hp_deregister(struct hotplug_slot *slot);
> extern int __must_check pci_hp_change_slot_info (struct hotplug_slot *slot,
> struct hotplug_slot_info *info);
> +#ifdef CONFIG_HOTPLUG_PCI
> +int pci_hp_reset_slot(struct hotplug_slot *slot, int probe);
> +#else
> +static inline int pci_hp_reset_slot(struct hotplug_slot *slot, int probe)
> +{
> + return -ENOTTY;
> +}
> +#endif
>
> /* use a define to avoid include chaining to get THIS_MODULE & friends */
> #define pci_hp_register(slot, pbus, devnr, name) \
>
There's a bug here when CONFIG_HOTPLUG_PCI=m. I'll fix and repost.
self NAK. Thanks,
Alex