* [PATCH v5 1/9] pci: Create pci_reset_bridge_secondary_bus()
2013-08-08 20:09 [PATCH v5 0/9] pci: bus and slot reset interfaces Alex Williamson
@ 2013-08-08 20:09 ` Alex Williamson
2013-08-08 20:09 ` [PATCH v5 2/9] pci: Add hotplug_slot_ops.reset_slot() Alex Williamson
` (8 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Alex Williamson @ 2013-08-08 20:09 UTC (permalink / raw)
To: bhelgaas, linux-pci; +Cc: alexander.h.duyck, ddutile, indou.takao, linux-kernel
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
and elsewhere.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
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 e37fea6..d468608 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3215,9 +3215,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)
@@ -3230,14 +3251,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 0fd1f15..35c1bc4 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -924,6 +924,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);
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v5 2/9] pci: Add hotplug_slot_ops.reset_slot()
2013-08-08 20:09 [PATCH v5 0/9] pci: bus and slot reset interfaces Alex Williamson
2013-08-08 20:09 ` [PATCH v5 1/9] pci: Create pci_reset_bridge_secondary_bus() Alex Williamson
@ 2013-08-08 20:09 ` Alex Williamson
2013-08-08 20:09 ` [PATCH v5 3/9] pci: Implement reset_slot for pciehp Alex Williamson
` (7 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Alex Williamson @ 2013-08-08 20:09 UTC (permalink / raw)
To: bhelgaas, linux-pci; +Cc: alexander.h.duyck, ddutile, indou.takao, linux-kernel
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 <alex.williamson@redhat.com>
---
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 8db71dc..bd32109 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);
};
/**
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v5 3/9] pci: Implement reset_slot for pciehp
2013-08-08 20:09 [PATCH v5 0/9] pci: bus and slot reset interfaces Alex Williamson
2013-08-08 20:09 ` [PATCH v5 1/9] pci: Create pci_reset_bridge_secondary_bus() Alex Williamson
2013-08-08 20:09 ` [PATCH v5 2/9] pci: Add hotplug_slot_ops.reset_slot() Alex Williamson
@ 2013-08-08 20:09 ` Alex Williamson
2013-08-08 20:09 ` [PATCH v5 4/9] pci: Add slot reset option to pci_dev_reset Alex Williamson
` (6 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Alex Williamson @ 2013-08-08 20:09 UTC (permalink / raw)
To: bhelgaas, linux-pci; +Cc: alexander.h.duyck, ddutile, indou.takao, linux-kernel
PCIe hotplug has a bus per slot, so we can just use a normal
secondary bus reset. However, if a slot supports surprise removal
then a bus reset can be seen as a presence detection change triggering
a hot-remove followed by a hot-add. Disable presence detection from
triggering an interrupt or being polled around the bus reset.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
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 7fb3269..541bbe6 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 b225573..51f56ef 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))
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v5 4/9] pci: Add slot reset option to pci_dev_reset
2013-08-08 20:09 [PATCH v5 0/9] pci: bus and slot reset interfaces Alex Williamson
` (2 preceding siblings ...)
2013-08-08 20:09 ` [PATCH v5 3/9] pci: Implement reset_slot for pciehp Alex Williamson
@ 2013-08-08 20:09 ` Alex Williamson
2013-08-14 21:22 ` Bjorn Helgaas
2013-08-08 20:09 ` [PATCH v5 5/9] pci: Split out pci_dev lock/unlock and save/restore Alex Williamson
` (5 subsequent siblings)
9 siblings, 1 reply; 15+ messages in thread
From: Alex Williamson @ 2013-08-08 20:09 UTC (permalink / raw)
To: bhelgaas, linux-pci; +Cc: alexander.h.duyck, ddutile, indou.takao, linux-kernel
If the hotplug controller provides a way to reset a slot, use that
before a direct parent bus reset. Like the bus reset option, this is
only available when a single pci_dev occupies the slot.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
drivers/pci/pci.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d468608..9407aab 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"
@@ -3256,6 +3257,35 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
return 0;
}
+static int pci_reset_hotplug_slot(struct hotplug_slot *hotplug, int probe)
+{
+ int rc = -ENOTTY;
+
+ if (!hotplug || !try_module_get(hotplug->ops->owner))
+ return rc;
+
+ if (hotplug->ops->reset_slot)
+ rc = hotplug->ops->reset_slot(hotplug, probe);
+
+ module_put(hotplug->ops->owner);
+
+ return rc;
+}
+
+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_reset_hotplug_slot(dev->slot->hotplug, probe);
+}
+
static int __pci_dev_reset(struct pci_dev *dev, int probe)
{
int rc;
@@ -3278,6 +3308,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;
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v5 4/9] pci: Add slot reset option to pci_dev_reset
2013-08-08 20:09 ` [PATCH v5 4/9] pci: Add slot reset option to pci_dev_reset Alex Williamson
@ 2013-08-14 21:22 ` Bjorn Helgaas
2013-08-15 23:24 ` rui wang
0 siblings, 1 reply; 15+ messages in thread
From: Bjorn Helgaas @ 2013-08-14 21:22 UTC (permalink / raw)
To: Alex Williamson
Cc: linux-pci, Alex Duyck, Don Dutile, Takao Indoh, linux-kernel
On Thu, Aug 8, 2013 at 2:09 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> If the hotplug controller provides a way to reset a slot, use that
> before a direct parent bus reset. Like the bus reset option, this is
> only available when a single pci_dev occupies the slot.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> drivers/pci/pci.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d468608..9407aab 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"
> @@ -3256,6 +3257,35 @@ static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
> return 0;
> }
>
> +static int pci_reset_hotplug_slot(struct hotplug_slot *hotplug, int probe)
> +{
> + int rc = -ENOTTY;
> +
> + if (!hotplug || !try_module_get(hotplug->ops->owner))
> + return rc;
> +
> + if (hotplug->ops->reset_slot)
> + rc = hotplug->ops->reset_slot(hotplug, probe);
> +
> + module_put(hotplug->ops->owner);
> +
> + return rc;
> +}
> +
> +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_reset_hotplug_slot(dev->slot->hotplug, probe);
I don't think this is hotplug-safe. It looks like pci_bus_sem might
be the right semaphore to hold while verifying that we won't reset any
unintended devices. But I think most users of the bus->devices list
are unprotected, so there's no point in trying to fix just this one.
> +}
> +
> static int __pci_dev_reset(struct pci_dev *dev, int probe)
> {
> int rc;
> @@ -3278,6 +3308,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;
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v5 4/9] pci: Add slot reset option to pci_dev_reset
2013-08-14 21:22 ` Bjorn Helgaas
@ 2013-08-15 23:24 ` rui wang
0 siblings, 0 replies; 15+ messages in thread
From: rui wang @ 2013-08-15 23:24 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Alex Williamson, linux-pci, Alex Duyck, Don Dutile, Takao Indoh,
linux-kernel
On 8/15/13, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Aug 8, 2013 at 2:09 PM, Alex Williamson
<snip>
> I don't think this is hotplug-safe. It looks like pci_bus_sem might
> be the right semaphore to hold while verifying that we won't reset any
> unintended devices.
Something like pci_bus_sem locks all the buses, which may pose a big
overhead. So it doesn't seem like an ideal solution. A better approach
is to lock only one parent to protect only a sub-tree. At least that's
what I've seen in some other operating systems dealing with PCI
hotplug. But that requires a significant amount of change.
Thanks
Rui
But I think most users of the bus->devices list
> are unprotected, so there's no point in trying to fix just this one.
>
>> +}
>> +
>> static int __pci_dev_reset(struct pci_dev *dev, int probe)
>> {
>> int rc;
>> @@ -3278,6 +3308,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;
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5 5/9] pci: Split out pci_dev lock/unlock and save/restore
2013-08-08 20:09 [PATCH v5 0/9] pci: bus and slot reset interfaces Alex Williamson
` (3 preceding siblings ...)
2013-08-08 20:09 ` [PATCH v5 4/9] pci: Add slot reset option to pci_dev_reset Alex Williamson
@ 2013-08-08 20:09 ` Alex Williamson
2013-08-08 20:09 ` [PATCH v5 6/9] pci: Add slot and bus reset interfaces Alex Williamson
` (4 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Alex Williamson @ 2013-08-08 20:09 UTC (permalink / raw)
To: bhelgaas, linux-pci; +Cc: alexander.h.duyck, ddutile, indou.takao, linux-kernel
Only cosmetic code changes to existing paths. Expand the comment in
the new pci_dev_save_and_disable() function since there's a lot
hidden in that Command register write.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
drivers/pci/pci.c | 55 +++++++++++++++++++++++++++++++++++++----------------
1 file changed, 38 insertions(+), 17 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9407aab..4a0275c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3317,22 +3317,49 @@ 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_and_disable(struct pci_dev *dev)
+{
+ pci_save_state(dev);
+ /*
+ * Disable the device by clearing the Command register, except for
+ * INTx-disable which is set. This not only disable MMIO and I/O port
+ * BARs, but also prevents the device from being Bus Master, preventing
+ * DMA from the device including MSI/MSI-X interrupts. For PCI 2.3
+ * compliant devices, INTx-disable prevents legacy interrupts.
+ */
+ 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;
}
/**
@@ -3423,17 +3450,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_and_disable(dev);
rc = pci_dev_reset(dev, 0);
- pci_restore_state(dev);
+ pci_dev_restore(dev);
return rc;
}
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v5 6/9] pci: Add slot and bus reset interfaces
2013-08-08 20:09 [PATCH v5 0/9] pci: bus and slot reset interfaces Alex Williamson
` (4 preceding siblings ...)
2013-08-08 20:09 ` [PATCH v5 5/9] pci: Split out pci_dev lock/unlock and save/restore Alex Williamson
@ 2013-08-08 20:09 ` Alex Williamson
2013-08-14 21:24 ` Bjorn Helgaas
2013-08-08 20:10 ` [PATCH v5 7/9] pci: Wake-up devices before save for reset Alex Williamson
` (3 subsequent siblings)
9 siblings, 1 reply; 15+ messages in thread
From: Alex Williamson @ 2013-08-08 20:09 UTC (permalink / raw)
To: bhelgaas, linux-pci; +Cc: alexander.h.duyck, ddutile, indou.takao, linux-kernel
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 <alex.williamson@redhat.com>
---
drivers/pci/pci.c | 209 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 2
2 files changed, 211 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 4a0275c..1dba7dd 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3460,6 +3460,215 @@ int pci_reset_function(struct pci_dev *dev)
}
EXPORT_SYMBOL_GPL(pci_reset_function);
+/* Lock devices from the top of the tree down */
+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);
+ }
+}
+
+/* Unlock devices from the bottom of the tree up */
+static void pci_bus_unlock(struct pci_bus *bus)
+{
+ struct pci_dev *dev;
+
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ if (dev->subordinate)
+ pci_bus_unlock(dev->subordinate);
+ pci_dev_unlock(dev);
+ }
+}
+
+/* Lock devices from the top of the tree down */
+static void pci_slot_lock(struct pci_slot *slot)
+{
+ struct pci_dev *dev;
+
+ 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);
+ }
+}
+
+/* Unlock devices from the bottom of the tree up */
+static void pci_slot_unlock(struct pci_slot *slot)
+{
+ struct pci_dev *dev;
+
+ list_for_each_entry(dev, &slot->bus->devices, bus_list) {
+ if (!dev->slot || dev->slot != slot)
+ continue;
+ if (dev->subordinate)
+ pci_bus_unlock(dev->subordinate);
+ pci_dev_unlock(dev);
+ }
+}
+
+/* Save and disable devices from the top of the tree down */
+static void pci_bus_save_and_disable(struct pci_bus *bus)
+{
+ struct pci_dev *dev;
+
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ pci_dev_save_and_disable(dev);
+ if (dev->subordinate)
+ pci_bus_save_and_disable(dev->subordinate);
+ }
+}
+
+/*
+ * Restore devices from top of the tree down - parent bridges need to be
+ * restored before we can get to subordinate devices.
+ */
+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);
+ }
+}
+
+/* Save and disable devices from the top of the tree down */
+static void pci_slot_save_and_disable(struct pci_slot *slot)
+{
+ struct pci_dev *dev;
+
+ list_for_each_entry(dev, &slot->bus->devices, bus_list) {
+ if (!dev->slot || dev->slot != slot)
+ continue;
+ pci_dev_save_and_disable(dev);
+ if (dev->subordinate)
+ pci_bus_save_and_disable(dev->subordinate);
+ }
+}
+
+/*
+ * Restore devices from top of the tree down - parent bridges need to be
+ * restored before we can get to subordinate devices.
+ */
+static void pci_slot_restore(struct pci_slot *slot)
+{
+ struct pci_dev *dev;
+
+ 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_reset_hotplug_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_and_disable(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_and_disable(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 35c1bc4..1a8fd34 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -924,6 +924,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);
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v5 6/9] pci: Add slot and bus reset interfaces
2013-08-08 20:09 ` [PATCH v5 6/9] pci: Add slot and bus reset interfaces Alex Williamson
@ 2013-08-14 21:24 ` Bjorn Helgaas
2013-08-14 22:00 ` Alex Williamson
0 siblings, 1 reply; 15+ messages in thread
From: Bjorn Helgaas @ 2013-08-14 21:24 UTC (permalink / raw)
To: Alex Williamson
Cc: linux-pci, Alex Duyck, Don Dutile, Takao Indoh, linux-kernel
On Thu, Aug 8, 2013 at 2:09 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> 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.
How does the caller check or ensure this? If there's a bridge
involved, I'm not sure the caller *can* ensure this, because the
caller can't hold pci_bus_sem or whatever we use to protect hotplug
changes.
> 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 <alex.williamson@redhat.com>
> ---
> drivers/pci/pci.c | 209 +++++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/pci.h | 2
> 2 files changed, 211 insertions(+)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 4a0275c..1dba7dd 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3460,6 +3460,215 @@ int pci_reset_function(struct pci_dev *dev)
> }
> EXPORT_SYMBOL_GPL(pci_reset_function);
>
> +/* Lock devices from the top of the tree down */
> +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);
> + }
> +}
> +
> +/* Unlock devices from the bottom of the tree up */
> +static void pci_bus_unlock(struct pci_bus *bus)
> +{
> + struct pci_dev *dev;
> +
> + list_for_each_entry(dev, &bus->devices, bus_list) {
> + if (dev->subordinate)
> + pci_bus_unlock(dev->subordinate);
> + pci_dev_unlock(dev);
> + }
> +}
> +
> +/* Lock devices from the top of the tree down */
> +static void pci_slot_lock(struct pci_slot *slot)
> +{
> + struct pci_dev *dev;
> +
> + 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);
> + }
> +}
> +
> +/* Unlock devices from the bottom of the tree up */
> +static void pci_slot_unlock(struct pci_slot *slot)
> +{
> + struct pci_dev *dev;
> +
> + list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> + if (!dev->slot || dev->slot != slot)
> + continue;
> + if (dev->subordinate)
> + pci_bus_unlock(dev->subordinate);
> + pci_dev_unlock(dev);
> + }
> +}
> +
> +/* Save and disable devices from the top of the tree down */
> +static void pci_bus_save_and_disable(struct pci_bus *bus)
> +{
> + struct pci_dev *dev;
> +
> + list_for_each_entry(dev, &bus->devices, bus_list) {
> + pci_dev_save_and_disable(dev);
> + if (dev->subordinate)
> + pci_bus_save_and_disable(dev->subordinate);
> + }
> +}
> +
> +/*
> + * Restore devices from top of the tree down - parent bridges need to be
> + * restored before we can get to subordinate devices.
> + */
> +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);
> + }
> +}
> +
> +/* Save and disable devices from the top of the tree down */
> +static void pci_slot_save_and_disable(struct pci_slot *slot)
> +{
> + struct pci_dev *dev;
> +
> + list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> + if (!dev->slot || dev->slot != slot)
> + continue;
> + pci_dev_save_and_disable(dev);
> + if (dev->subordinate)
> + pci_bus_save_and_disable(dev->subordinate);
> + }
> +}
> +
> +/*
> + * Restore devices from top of the tree down - parent bridges need to be
> + * restored before we can get to subordinate devices.
> + */
> +static void pci_slot_restore(struct pci_slot *slot)
> +{
> + struct pci_dev *dev;
> +
> + 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_reset_hotplug_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_and_disable(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_and_disable(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 35c1bc4..1a8fd34 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -924,6 +924,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);
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v5 6/9] pci: Add slot and bus reset interfaces
2013-08-14 21:24 ` Bjorn Helgaas
@ 2013-08-14 22:00 ` Alex Williamson
0 siblings, 0 replies; 15+ messages in thread
From: Alex Williamson @ 2013-08-14 22:00 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, Alex Duyck, Don Dutile, Takao Indoh, linux-kernel
On Wed, 2013-08-14 at 15:24 -0600, Bjorn Helgaas wrote:
> On Thu, Aug 8, 2013 at 2:09 PM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > 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.
>
> How does the caller check or ensure this? If there's a bridge
> involved, I'm not sure the caller *can* ensure this, because the
> caller can't hold pci_bus_sem or whatever we use to protect hotplug
> changes.
I'm not sure how effectively we can combat every hotplug race given our
current infrastructure. However, in the case of vfio, ownership is
based on iommu groups. If all of the devices in the group are bound to
vfio drivers and the user can open the group and setup iommu protection,
then they have ownership of all those devices. It's then a matter of
figuring out what iommu groups are affected by a bus/slot reset and
making the user prove that they own all the necessary vfio groups to
cover the full set of affected devices. The vfio-pci patch cc'd to the
list does this. A hotplug while we're walking bus->devices_list would
be bad, but I do try to build in protection against the list changing
between ioctls and where we can between each step in the ioctl. Thanks,
Alex
> > 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 <alex.williamson@redhat.com>
> > ---
> > drivers/pci/pci.c | 209 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/pci.h | 2
> > 2 files changed, 211 insertions(+)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 4a0275c..1dba7dd 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3460,6 +3460,215 @@ int pci_reset_function(struct pci_dev *dev)
> > }
> > EXPORT_SYMBOL_GPL(pci_reset_function);
> >
> > +/* Lock devices from the top of the tree down */
> > +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);
> > + }
> > +}
> > +
> > +/* Unlock devices from the bottom of the tree up */
> > +static void pci_bus_unlock(struct pci_bus *bus)
> > +{
> > + struct pci_dev *dev;
> > +
> > + list_for_each_entry(dev, &bus->devices, bus_list) {
> > + if (dev->subordinate)
> > + pci_bus_unlock(dev->subordinate);
> > + pci_dev_unlock(dev);
> > + }
> > +}
> > +
> > +/* Lock devices from the top of the tree down */
> > +static void pci_slot_lock(struct pci_slot *slot)
> > +{
> > + struct pci_dev *dev;
> > +
> > + 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);
> > + }
> > +}
> > +
> > +/* Unlock devices from the bottom of the tree up */
> > +static void pci_slot_unlock(struct pci_slot *slot)
> > +{
> > + struct pci_dev *dev;
> > +
> > + list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> > + if (!dev->slot || dev->slot != slot)
> > + continue;
> > + if (dev->subordinate)
> > + pci_bus_unlock(dev->subordinate);
> > + pci_dev_unlock(dev);
> > + }
> > +}
> > +
> > +/* Save and disable devices from the top of the tree down */
> > +static void pci_bus_save_and_disable(struct pci_bus *bus)
> > +{
> > + struct pci_dev *dev;
> > +
> > + list_for_each_entry(dev, &bus->devices, bus_list) {
> > + pci_dev_save_and_disable(dev);
> > + if (dev->subordinate)
> > + pci_bus_save_and_disable(dev->subordinate);
> > + }
> > +}
> > +
> > +/*
> > + * Restore devices from top of the tree down - parent bridges need to be
> > + * restored before we can get to subordinate devices.
> > + */
> > +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);
> > + }
> > +}
> > +
> > +/* Save and disable devices from the top of the tree down */
> > +static void pci_slot_save_and_disable(struct pci_slot *slot)
> > +{
> > + struct pci_dev *dev;
> > +
> > + list_for_each_entry(dev, &slot->bus->devices, bus_list) {
> > + if (!dev->slot || dev->slot != slot)
> > + continue;
> > + pci_dev_save_and_disable(dev);
> > + if (dev->subordinate)
> > + pci_bus_save_and_disable(dev->subordinate);
> > + }
> > +}
> > +
> > +/*
> > + * Restore devices from top of the tree down - parent bridges need to be
> > + * restored before we can get to subordinate devices.
> > + */
> > +static void pci_slot_restore(struct pci_slot *slot)
> > +{
> > + struct pci_dev *dev;
> > +
> > + 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_reset_hotplug_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_and_disable(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_and_disable(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 35c1bc4..1a8fd34 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -924,6 +924,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);
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5 7/9] pci: Wake-up devices before save for reset
2013-08-08 20:09 [PATCH v5 0/9] pci: bus and slot reset interfaces Alex Williamson
` (5 preceding siblings ...)
2013-08-08 20:09 ` [PATCH v5 6/9] pci: Add slot and bus reset interfaces Alex Williamson
@ 2013-08-08 20:10 ` Alex Williamson
2013-08-08 20:10 ` [PATCH v5 8/9] pci: Tune secondary bus reset timing Alex Williamson
` (2 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Alex Williamson @ 2013-08-08 20:10 UTC (permalink / raw)
To: bhelgaas, linux-pci; +Cc: alexander.h.duyck, ddutile, indou.takao, linux-kernel
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 <alex.williamson@redhat.com>
---
drivers/pci/pci.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1dba7dd..b204206 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3332,6 +3332,13 @@ static void pci_dev_unlock(struct pci_dev *dev)
static void pci_dev_save_and_disable(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);
/*
* Disable the device by clearing the Command register, except for
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v5 8/9] pci: Tune secondary bus reset timing
2013-08-08 20:09 [PATCH v5 0/9] pci: bus and slot reset interfaces Alex Williamson
` (6 preceding siblings ...)
2013-08-08 20:10 ` [PATCH v5 7/9] pci: Wake-up devices before save for reset Alex Williamson
@ 2013-08-08 20:10 ` Alex Williamson
2013-08-08 20:10 ` [PATCH v5 9/9] pci: Remove aer_do_secondary_bus_reset() Alex Williamson
2013-08-15 20:48 ` [PATCH v5 0/9] pci: bus and slot reset interfaces Bjorn Helgaas
9 siblings, 0 replies; 15+ messages in thread
From: Alex Williamson @ 2013-08-08 20:10 UTC (permalink / raw)
To: bhelgaas, linux-pci; +Cc: alexander.h.duyck, ddutile, indou.takao, linux-kernel
The PCI spec indicates that with stable power, reset needs to be
asserted for a minimum of 1ms (Trst). We should be able to assume
stable power for a Hot Reset, but we add another millisecond as
a fudge factor to make sure the reset is seen on the bus for at least
a full 1ms.
After reset is de-asserted we must wait for devices to complete
initialization. The specs refer to this as "recovery time" (Trhfa).
For PCI this is 2^25 clock cycles or 2^26 for PCI-X. For minimum
bus speeds, both of those come to 1s. PCIe "softens" this
requirement with the Configuration Request Retry Status (CRS)
completion status. Theoretically we could use CRS to shorten the
wait time. We don't make use of that here, using a fixed 1s delay
to allow devices to re-initialize.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
drivers/pci/pci.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b204206..ba68451 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3230,11 +3230,23 @@ void pci_reset_bridge_secondary_bus(struct pci_dev *dev)
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);
+ /*
+ * PCI spec v3.0 7.6.4.2 requires minimum Trst of 1ms. Double
+ * this to 2ms to ensure that we meet the minium requirement.
+ */
+ msleep(2);
ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);
- msleep(100);
+
+ /*
+ * Trhfa for conventional PCI is 2^25 clock cycles.
+ * Assuming a minimum 33MHz clock this results in a 1s
+ * delay before we can consider subordinate devices to
+ * be re-initialized. PCIe has some ways to shorten this,
+ * but we don't make use of them yet.
+ */
+ ssleep(1);
}
EXPORT_SYMBOL_GPL(pci_reset_bridge_secondary_bus);
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v5 9/9] pci: Remove aer_do_secondary_bus_reset()
2013-08-08 20:09 [PATCH v5 0/9] pci: bus and slot reset interfaces Alex Williamson
` (7 preceding siblings ...)
2013-08-08 20:10 ` [PATCH v5 8/9] pci: Tune secondary bus reset timing Alex Williamson
@ 2013-08-08 20:10 ` Alex Williamson
2013-08-15 20:48 ` [PATCH v5 0/9] pci: bus and slot reset interfaces Bjorn Helgaas
9 siblings, 0 replies; 15+ messages in thread
From: Alex Williamson @ 2013-08-08 20:10 UTC (permalink / raw)
To: bhelgaas, linux-pci; +Cc: alexander.h.duyck, ddutile, indou.takao, linux-kernel
One PCI bus reset function to rule them all.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
drivers/pci/pcie/aer/aerdrv.c | 2 +-
drivers/pci/pcie/aer/aerdrv.h | 1 -
drivers/pci/pcie/aer/aerdrv_core.c | 35 +----------------------------------
3 files changed, 2 insertions(+), 36 deletions(-)
diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
index 76ef634..0bf82a2 100644
--- a/drivers/pci/pcie/aer/aerdrv.c
+++ b/drivers/pci/pcie/aer/aerdrv.c
@@ -352,7 +352,7 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, reg32);
- aer_do_secondary_bus_reset(dev);
+ pci_reset_bridge_secondary_bus(dev);
dev_printk(KERN_DEBUG, &dev->dev, "Root Port link has been reset\n");
/* Clear Root Error Status */
diff --git a/drivers/pci/pcie/aer/aerdrv.h b/drivers/pci/pcie/aer/aerdrv.h
index 90ea3e8..84420b7 100644
--- a/drivers/pci/pcie/aer/aerdrv.h
+++ b/drivers/pci/pcie/aer/aerdrv.h
@@ -106,7 +106,6 @@ static inline pci_ers_result_t merge_result(enum pci_ers_result orig,
}
extern struct bus_type pcie_port_bus_type;
-void aer_do_secondary_bus_reset(struct pci_dev *dev);
int aer_init(struct pcie_device *dev);
void aer_isr(struct work_struct *work);
void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
index 8b68ae5..85ca36f 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -367,39 +367,6 @@ static pci_ers_result_t broadcast_error_message(struct pci_dev *dev,
}
/**
- * aer_do_secondary_bus_reset - perform secondary bus reset
- * @dev: pointer to bridge's pci_dev data structure
- *
- * Invoked when performing link reset at Root Port or Downstream Port.
- */
-void aer_do_secondary_bus_reset(struct pci_dev *dev)
-{
- u16 p2p_ctrl;
-
- /* Assert Secondary Bus Reset */
- pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &p2p_ctrl);
- p2p_ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
- pci_write_config_word(dev, PCI_BRIDGE_CONTROL, p2p_ctrl);
-
- /*
- * we should send hot reset message for 2ms to allow it time to
- * propagate to all downstream ports
- */
- msleep(2);
-
- /* De-assert Secondary Bus Reset */
- p2p_ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
- pci_write_config_word(dev, PCI_BRIDGE_CONTROL, p2p_ctrl);
-
- /*
- * System software must wait for at least 100ms from the end
- * of a reset of one or more device before it is permitted
- * to issue Configuration Requests to those devices.
- */
- msleep(200);
-}
-
-/**
* default_reset_link - default reset function
* @dev: pointer to pci_dev data structure
*
@@ -408,7 +375,7 @@ void aer_do_secondary_bus_reset(struct pci_dev *dev)
*/
static pci_ers_result_t default_reset_link(struct pci_dev *dev)
{
- aer_do_secondary_bus_reset(dev);
+ pci_reset_bridge_secondary_bus(dev);
dev_printk(KERN_DEBUG, &dev->dev, "downstream link has been reset\n");
return PCI_ERS_RESULT_RECOVERED;
}
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v5 0/9] pci: bus and slot reset interfaces
2013-08-08 20:09 [PATCH v5 0/9] pci: bus and slot reset interfaces Alex Williamson
` (8 preceding siblings ...)
2013-08-08 20:10 ` [PATCH v5 9/9] pci: Remove aer_do_secondary_bus_reset() Alex Williamson
@ 2013-08-15 20:48 ` Bjorn Helgaas
9 siblings, 0 replies; 15+ messages in thread
From: Bjorn Helgaas @ 2013-08-15 20:48 UTC (permalink / raw)
To: Alex Williamson
Cc: linux-pci, Alex Duyck, Don Dutile, Takao Indoh, linux-kernel
On Thu, Aug 8, 2013 at 2:09 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> v5: Increase length of time we assert reset in the final common
> secondary bus reset function to 2ms and document that this is
> to ensure that reset on the bus is seen for at least a full
> 1ms (per converstation with Alex Duyck)
>
> Thanks,
> Alex
>
> This series adds PCI bus and slot reset interfaces to the already
> existing function reset interface. I need this for two reasons, the
> first is that not all devices support function level reset. Even
> some of those that we detect as supporting a PM reset on D3hot->D0
> transition actually don't do any reset. Others have no reset
> capability at all. We currently implement a secondary bus reset
> escalation from the function reset path, but only when there is a
> single devfn on the bus. Drivers like vfio can have ownership of
> all of the devices on a bus and should therefore have a path to
> initiate a secondary bus reset with multiple devices. This is
> particularly required for use of GPUs by userspace, where none of
> the predominant GPUs implement a useful function level reset.
>
> The second reason is that even the current function reset escalating
> to a secondary bus reset can cause problems with hotplug controllers.
> If a root port supports PCIe HP with suprise removal, a bus reset
> can trigger a presence detection change, which results in an attempt
> to remove the struct device. By having a slot reset interface, we
> can involve the hotplug controllers to allow for a controlled bus
> reset and avoid this spurious removal attempt.
>
> ---
>
> Alex Williamson (9):
> pci: Create pci_reset_bridge_secondary_bus()
> pci: Add hotplug_slot_ops.reset_slot()
> pci: Implement reset_slot for pciehp
> pci: Add slot reset 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
> pci: Tune secondary bus reset timing
> pci: Remove aer_do_secondary_bus_reset()
>
>
> drivers/pci/hotplug/pciehp.h | 1
> drivers/pci/hotplug/pciehp_core.c | 12 +
> drivers/pci/hotplug/pciehp_hpc.c | 31 +++
> drivers/pci/pci.c | 349 +++++++++++++++++++++++++++++++++---
> drivers/pci/pcie/aer/aerdrv.c | 2
> drivers/pci/pcie/aer/aerdrv.h | 1
> drivers/pci/pcie/aer/aerdrv_core.c | 35 ----
> include/linux/pci.h | 3
> include/linux/pci_hotplug.h | 4
> 9 files changed, 376 insertions(+), 62 deletions(-)
Merged to the PCI "next" branch for v3.12, thanks!
^ permalink raw reply [flat|nested] 15+ messages in thread