[PATCH 0/4] PCI: Reset exclusions

[PATCH 0/4] PCI: Reset exclusions

[Alex Williamson]

This is really 2 sets of 2 patches, but they both add bits to
dev_flags so are included together.

This fixes two problems we've seen with resets.  The first is for
devices that advertise a PM reset mechanism, but it doesn't appear to
do anything.  We add a quirk and flags bit to indicate the PM reset
mechanism isn't viable.  This happens on some AMD GPUs and causes
vfio-pci to assume the PM reset was successful when it really did
nothing and we should have escalated to a PCI bus reset.  Alex
Deucher confirms that PM reset isn't used by graphics drivers.  The
exclusion here is for users of pci_reset_function(), which mostly
only includes pci-sysfs and drivers like legacy KVM device assignment
and VFIO.

The second issue is a problem identified with an Atheros wifi chip
where in performing a bus reset of the device, we not only permanently
lose access to the device, but it introduces host stability issues if
we try to access it.  I've been unsuccessful in finding any way to
make the device behave or in finding anyone with access to hardware
documentation and errata for this device, so it seems like the most
appropriate path is to blacklist bus resets for topologies including
this device.  The second two patches add infrastructure and quirks to
do this.  Thanks,

Alex

---

Alex Williamson (4):
      PCI: quirk Atheros AR93xx to avoid bus reset
      PCI: Allow device quirks to exclude bus reset
      PCI: quirk AMD/ATI VGA cards to avoid PM reset
      PCI: Allow device quirks to exclude D3->D0 PM reset


 drivers/pci/pci.c    |   42 +++++++++++++++++++++++++++++++++++++-----
 drivers/pci/quirks.c |   35 +++++++++++++++++++++++++++++++++++
 include/linux/pci.h  |    4 ++++
 3 files changed, 76 insertions(+), 5 deletions(-)

[PATCH 1/4] PCI: Allow device quirks to exclude D3->D0 PM reset

[Alex Williamson]

The PCI PM spec indicates that when No_Soft_Reset is clear
(NoSoftRst-) that the device performs an internal reset when
transitioning From D3hot to D0.  Configuration context is lost and
the device requires a full reinitialization sequence.

Unfortunately the definition of "internal reset", beyond the
application of the configuration context, is largely left to the
interpretation of the specific device.  For some devices, setting
NoSoftRst- appears arbitrary and no obvious "internal reset" occurs
on D3hot to D0 transition.

We still need to honor the PCI specification and restore PCI config
context in the event that we do a PM reset, so we don't cache and
modify the PCI_PM_CTRL_NO_SOFT_RESET bit for the device, but for
interfaces where the intention is to reset the device, like
pci_reset_function(), we need a mechanism to flag that PM reset
doesn't perform any significant "internal reset" of the device.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 drivers/pci/pci.c   |    2 +-
 include/linux/pci.h |    2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 625a4ac..ba54a5a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3206,7 +3206,7 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
 {
 	u16 csr;
 
-	if (!dev->pm_cap)
+	if (!dev->pm_cap || dev->dev_flags & PCI_DEV_FLAGS_NO_PM_RESET)
 		return -ENOTTY;
 
 	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &csr);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5be8db4..aea347d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -175,6 +175,8 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_DMA_ALIAS_DEVFN = (__force pci_dev_flags_t) (1 << 4),
 	/* Use a PCIe-to-PCI bridge alias even if !pci_is_pcie */
 	PCI_DEV_FLAG_PCIE_BRIDGE_ALIAS = (__force pci_dev_flags_t) (1 << 5),
+	/* Do not use PM reset even if device advertises NoSoftRst- */
+	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 6),
 };
 
 enum pci_irq_reroute_variant {

[PATCH 2/4] PCI: quirk AMD/ATI VGA cards to avoid PM reset

[Alex Williamson]

Some AMD/ATI GPUs report that they support PM reset (NoSoftRst-) but
initiating such a reset has no apparent affect on the device.  The
monitor remains sync'd, the framebuffer contents are retained, etc.
Callers of pci_reset_function() don't necessarily have a way to
validate whether a reset was effective, so we really want to avoid
making use of a known non-effective reset.  Returning an error in
such cases appears to be the better option.  For users like vfio-pci,
this allows the driver to escalate to the bus reset interfaces.

If a device lives on the root bus, there's really no further
escalation path, so we exempt PM reset as potentially better than
nothing.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
---

 drivers/pci/quirks.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 90acb32..561e10d 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3008,6 +3008,27 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_REALTEK, 0x8169,
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MELLANOX, PCI_ANY_ID,
 			 quirk_broken_intx_masking);
 
+static void quirk_no_pm_reset(struct pci_dev *dev)
+{
+	/*
+	 * A non-effective PM reset may be better than nothing
+	 * if we can't do a bus reset
+	 */
+	if (!pci_is_root_bus(dev->bus))
+		dev->dev_flags |= PCI_DEV_FLAGS_NO_PM_RESET;
+}
+
+/*
+ * Some AMD/ATI GPUS (HD8570 - Oland) report supporting PM reset via D3->D0
+ * transition (NoSoftRst-).  This reset mechanims seems to have no effect
+ * whatsoever on the device, even retaining the framebuffer contents and
+ * monitor sync.  Advertising this support makes other layers, like VFIO
+ * assume pci_reset_function() is viable for this device.  Mark it as
+ * unavailable to skip it when testing reset methods.
+ */
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
+			       PCI_CLASS_DISPLAY_VGA, 8, quirk_no_pm_reset);
+
 #ifdef CONFIG_ACPI
 /*
  * Apple: Shutdown Cactus Ridge Thunderbolt controller.

[PATCH 3/4] PCI: Allow device quirks to exclude bus reset

[Alex Williamson]

Enable a mechanism for devices to quirk that they do not behave when
doing a PCI bus reset.  We require a modest level of spec compliant
behavior in order to do a reset, for instance the device should come
out of reset without throwing errors and PCI config space should be
accessible after reset.  This is too much to ask for some devices.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 drivers/pci/pci.c   |   40 ++++++++++++++++++++++++++++++++++++----
 include/linux/pci.h |    2 ++
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ba54a5a..7d6f87e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3280,7 +3280,8 @@ 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)
+	if (pci_is_root_bus(dev->bus) || dev->subordinate ||
+	    !dev->bus->self || dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET)
 		return -ENOTTY;
 
 	list_for_each_entry(pdev, &dev->bus->devices, bus_list)
@@ -3314,7 +3315,8 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
 {
 	struct pci_dev *pdev;
 
-	if (dev->subordinate || !dev->slot)
+	if (dev->subordinate || !dev->slot ||
+	    dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET)
 		return -ENOTTY;
 
 	list_for_each_entry(pdev, &dev->bus->devices, bus_list)
@@ -3566,6 +3568,20 @@ int pci_try_reset_function(struct pci_dev *dev)
 }
 EXPORT_SYMBOL_GPL(pci_try_reset_function);
 
+/* Do any devices on or below this bus prevent a bus reset */
+static bool pci_bus_resetable(struct pci_bus *bus)
+{
+	struct pci_dev *dev;
+
+	list_for_each_entry(dev, &bus->devices, bus_list) {
+		if (dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET ||
+		    (dev->subordinate && !pci_bus_resetable(dev->subordinate)))
+			return false;
+	}
+
+	return true;
+}
+
 /* Lock devices from the top of the tree down */
 static void pci_bus_lock(struct pci_bus *bus)
 {
@@ -3616,6 +3632,22 @@ unlock:
 	return 0;
 }
 
+/* Do any devices on or below this slot prevent a bus reset */
+static bool pci_slot_resetable(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->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET ||
+		    (dev->subordinate && !pci_bus_resetable(dev->subordinate)))
+			return false;
+	}
+
+	return true;
+}
+
 /* Lock devices from the top of the tree down */
 static void pci_slot_lock(struct pci_slot *slot)
 {
@@ -3737,7 +3769,7 @@ static int pci_slot_reset(struct pci_slot *slot, int probe)
 {
 	int rc;
 
-	if (!slot)
+	if (!slot || !pci_slot_resetable(slot))
 		return -ENOTTY;
 
 	if (!probe)
@@ -3829,7 +3861,7 @@ EXPORT_SYMBOL_GPL(pci_try_reset_slot);
 
 static int pci_bus_reset(struct pci_bus *bus, int probe)
 {
-	if (!bus->self)
+	if (!bus->self || !pci_bus_resetable(bus))
 		return -ENOTTY;
 
 	if (probe)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index aea347d..c9a8904 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -177,6 +177,8 @@ enum pci_dev_flags {
 	PCI_DEV_FLAG_PCIE_BRIDGE_ALIAS = (__force pci_dev_flags_t) (1 << 5),
 	/* Do not use PM reset even if device advertises NoSoftRst- */
 	PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 6),
+	/* Do not use bus resets for device */
+	PCI_DEV_FLAGS_NO_BUS_RESET = (__force pci_dev_flags_t) (1 << 7),
 };
 
 enum pci_irq_reroute_variant {

[PATCH 4/4] PCI: quirk Atheros AR93xx to avoid bus reset

[Alex Williamson]

Reports against the TL-WDN4800 card indicate that PCI bus reset of
this Atheros device cause system lock-ups and resets.  I've also
been able to confirm this behavior on multiple systems.  The device
never returns from reset and attempts to access config space of the
device after reset result in hangs.  Blacklist bus reset for the
device to avoid this issue.

Reported-by: Andreas Hartmann <andihartmann@freenet.de>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Tested-by: Andreas Hartmann <andihartmann@freenet.de>
---

 drivers/pci/quirks.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 561e10d..ebbd5b4 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3029,6 +3029,20 @@ static void quirk_no_pm_reset(struct pci_dev *dev)
 DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
 			       PCI_CLASS_DISPLAY_VGA, 8, quirk_no_pm_reset);
 
+static void quirk_no_bus_reset(struct pci_dev *dev)
+{
+	dev->dev_flags |= PCI_DEV_FLAGS_NO_BUS_RESET;
+}
+
+/*
+ * Atheros AR93xx chips do not behave after a bus reset.  The device will
+ * throw a Link Down error on AER capable system and regardless of AER,
+ * config space of the device is never accessible again and typically
+ * causes the system to hang or reset when access is attempted.
+ * http://www.spinics.net/lists/linux-pci/msg34797.html
+ */
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0030, quirk_no_bus_reset);
+
 #ifdef CONFIG_ACPI
 /*
  * Apple: Shutdown Cactus Ridge Thunderbolt controller.

RE: [PATCH 2/4] PCI: quirk AMD/ATI VGA cards to avoid PM reset

[Deucher, Alexander]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2720 bytes --]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Friday, November 21, 2014 1:24 PM
> To: bhelgaas@google.com
> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; Alex
> Williamson; Deucher, Alexander
> Subject: [PATCH 2/4] PCI: quirk AMD/ATI VGA cards to avoid PM reset
> 
> Some AMD/ATI GPUs report that they support PM reset (NoSoftRst-) but
> initiating such a reset has no apparent affect on the device.  The
> monitor remains sync'd, the framebuffer contents are retained, etc.
> Callers of pci_reset_function() don't necessarily have a way to
> validate whether a reset was effective, so we really want to avoid
> making use of a known non-effective reset.  Returning an error in
> such cases appears to be the better option.  For users like vfio-pci,
> this allows the driver to escalate to the bus reset interfaces.
> 
> If a device lives on the root bus, there's really no further
> escalation path, so we exempt PM reset as potentially better than
> nothing.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
> 
>  drivers/pci/quirks.c |   21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 90acb32..561e10d 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3008,6 +3008,27 @@
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_REALTEK, 0x8169,
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MELLANOX, PCI_ANY_ID,
>  			 quirk_broken_intx_masking);
> 
> +static void quirk_no_pm_reset(struct pci_dev *dev)
> +{
> +	/*
> +	 * A non-effective PM reset may be better than nothing
> +	 * if we can't do a bus reset
> +	 */
> +	if (!pci_is_root_bus(dev->bus))
> +		dev->dev_flags |= PCI_DEV_FLAGS_NO_PM_RESET;
> +}
> +
> +/*
> + * Some AMD/ATI GPUS (HD8570 - Oland) report supporting PM reset via
> D3->D0
> + * transition (NoSoftRst-).  This reset mechanims seems to have no effect
> + * whatsoever on the device, even retaining the framebuffer contents and
> + * monitor sync.  Advertising this support makes other layers, like VFIO
> + * assume pci_reset_function() is viable for this device.  Mark it as
> + * unavailable to skip it when testing reset methods.
> + */
> +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
> +			       PCI_CLASS_DISPLAY_VGA, 8,
> quirk_no_pm_reset);
> +
>  #ifdef CONFIG_ACPI
>  /*
>   * Apple: Shutdown Cactus Ridge Thunderbolt controller.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 4/4] PCI: quirk Atheros AR93xx to avoid bus reset

[Andreas Hartmann]

Hello Bjorn,

I'm running this patch and the corresponding "[PATCH 3/4] PCI: Allow
device quirks to exclude bus reset" patch meanwhile since a month w/
kernel 3.14.x and couldn't find any problem. Would it be possible to
apply these patches to main kernel? Or even to lt-kernel 3.14?


Thanks.
kind regards,
Andreas Hartmann


Alex Williamson wrote:
> Reports against the TL-WDN4800 card indicate that PCI bus reset of
> this Atheros device cause system lock-ups and resets.  I've also
> been able to confirm this behavior on multiple systems.  The device
> never returns from reset and attempts to access config space of the
> device after reset result in hangs.  Blacklist bus reset for the
> device to avoid this issue.
> 
> Reported-by: Andreas Hartmann <andihartmann@freenet.de>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> Tested-by: Andreas Hartmann <andihartmann@freenet.de>
> ---
> 
>  drivers/pci/quirks.c |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 561e10d..ebbd5b4 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3029,6 +3029,20 @@ static void quirk_no_pm_reset(struct pci_dev *dev)
>  DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
>  			       PCI_CLASS_DISPLAY_VGA, 8, quirk_no_pm_reset);
>  
> +static void quirk_no_bus_reset(struct pci_dev *dev)
> +{
> +	dev->dev_flags |= PCI_DEV_FLAGS_NO_BUS_RESET;
> +}
> +
> +/*
> + * Atheros AR93xx chips do not behave after a bus reset.  The device will
> + * throw a Link Down error on AER capable system and regardless of AER,
> + * config space of the device is never accessible again and typically
> + * causes the system to hang or reset when access is attempted.
> + * http://www.spinics.net/lists/linux-pci/msg34797.html
> + */
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0030, quirk_no_bus_reset);
> +
>  #ifdef CONFIG_ACPI
>  /*
>   * Apple: Shutdown Cactus Ridge Thunderbolt controller.
> 

Re: [PATCH 4/4] PCI: quirk Atheros AR93xx to avoid bus reset

[Bjorn Helgaas]

On Fri, Nov 21, 2014 at 11:24:27AM -0700, Alex Williamson wrote:
> Reports against the TL-WDN4800 card indicate that PCI bus reset of
> this Atheros device cause system lock-ups and resets.  I've also
> been able to confirm this behavior on multiple systems.  The device
> never returns from reset and attempts to access config space of the
> device after reset result in hangs.  Blacklist bus reset for the
> device to avoid this issue.
> 
> Reported-by: Andreas Hartmann <andihartmann@freenet.de>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> Tested-by: Andreas Hartmann <andihartmann@freenet.de>

If I understand correctly, these two (patches 3 & 4) fix a v3.14 regression
caused by 425c1b223dac ("PCI: Add Virtual Channel to save/restore support").

If so, these should go to for-linus for v3.19.  What about patches 1 & 2?
Do they fix a regression?  Is there a pointer to a bugzilla or problem
report about that issue?

I don't understand the connection between 425c1b223dac and
PCI_DEV_FLAGS_NO_BUS_RESET, because 425c1b223dac doesn't seem to do any
resets.  Is that the wrong commit, or can you outline the connection for
me?

Bjorn

> ---
> 
>  drivers/pci/quirks.c |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 561e10d..ebbd5b4 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3029,6 +3029,20 @@ static void quirk_no_pm_reset(struct pci_dev *dev)
>  DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
>  			       PCI_CLASS_DISPLAY_VGA, 8, quirk_no_pm_reset);
>  
> +static void quirk_no_bus_reset(struct pci_dev *dev)
> +{
> +	dev->dev_flags |= PCI_DEV_FLAGS_NO_BUS_RESET;
> +}
> +
> +/*
> + * Atheros AR93xx chips do not behave after a bus reset.  The device will
> + * throw a Link Down error on AER capable system and regardless of AER,
> + * config space of the device is never accessible again and typically
> + * causes the system to hang or reset when access is attempted.
> + * http://www.spinics.net/lists/linux-pci/msg34797.html
> + */
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0030, quirk_no_bus_reset);
> +
>  #ifdef CONFIG_ACPI
>  /*
>   * Apple: Shutdown Cactus Ridge Thunderbolt controller.
> 

Re: [PATCH 4/4] PCI: quirk Atheros AR93xx to avoid bus reset

[Alex Williamson]

On Thu, 2015-01-08 at 09:07 -0700, Bjorn Helgaas wrote:
> On Fri, Nov 21, 2014 at 11:24:27AM -0700, Alex Williamson wrote:
> > Reports against the TL-WDN4800 card indicate that PCI bus reset of
> > this Atheros device cause system lock-ups and resets.  I've also
> > been able to confirm this behavior on multiple systems.  The device
> > never returns from reset and attempts to access config space of the
> > device after reset result in hangs.  Blacklist bus reset for the
> > device to avoid this issue.
> > 
> > Reported-by: Andreas Hartmann <andihartmann@freenet.de>
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > Tested-by: Andreas Hartmann <andihartmann@freenet.de>
> 
> If I understand correctly, these two (patches 3 & 4) fix a v3.14 regression
> caused by 425c1b223dac ("PCI: Add Virtual Channel to save/restore support").
> 
> If so, these should go to for-linus for v3.19.  What about patches 1 & 2?
> Do they fix a regression?  Is there a pointer to a bugzilla or problem
> report about that issue?
> 
> I don't understand the connection between 425c1b223dac and
> PCI_DEV_FLAGS_NO_BUS_RESET, because 425c1b223dac doesn't seem to do any
> resets.  Is that the wrong commit, or can you outline the connection for
> me?

TBH, I don't have a lot of faith in associating this to 425c1b223dac,
I'm not sure how Andreas' bisect landed there.  IME, this device cannot,
and has never been able to handle a bus reset.  A simple setpci
experiment on the commandline can confirm this.  What I think happened
is that with the PCI bus reset infrastructure we added, we switched QEMU
to prefer PCI bus resets over things like PM D3hot->D0 resets.  So it's
just more prolific use of bus resets by userspace.

There's also no regression in 1 & 2, PM reset has never done anything
useful on those devices.  Thanks,

Alex

> > ---
> > 
> >  drivers/pci/quirks.c |   14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 561e10d..ebbd5b4 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -3029,6 +3029,20 @@ static void quirk_no_pm_reset(struct pci_dev *dev)
> >  DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
> >  			       PCI_CLASS_DISPLAY_VGA, 8, quirk_no_pm_reset);
> >  
> > +static void quirk_no_bus_reset(struct pci_dev *dev)
> > +{
> > +	dev->dev_flags |= PCI_DEV_FLAGS_NO_BUS_RESET;
> > +}
> > +
> > +/*
> > + * Atheros AR93xx chips do not behave after a bus reset.  The device will
> > + * throw a Link Down error on AER capable system and regardless of AER,
> > + * config space of the device is never accessible again and typically
> > + * causes the system to hang or reset when access is attempted.
> > + * http://www.spinics.net/lists/linux-pci/msg34797.html
> > + */
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0030, quirk_no_bus_reset);
> > +
> >  #ifdef CONFIG_ACPI
> >  /*
> >   * Apple: Shutdown Cactus Ridge Thunderbolt controller.
> > 

Re: [PATCH 4/4] PCI: quirk Atheros AR93xx to avoid bus reset

[Bjorn Helgaas]

On Thu, Jan 8, 2015 at 1:30 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Thu, 2015-01-08 at 09:07 -0700, Bjorn Helgaas wrote:
>> On Fri, Nov 21, 2014 at 11:24:27AM -0700, Alex Williamson wrote:
>> > Reports against the TL-WDN4800 card indicate that PCI bus reset of
>> > this Atheros device cause system lock-ups and resets.  I've also
>> > been able to confirm this behavior on multiple systems.  The device
>> > never returns from reset and attempts to access config space of the
>> > device after reset result in hangs.  Blacklist bus reset for the
>> > device to avoid this issue.
>> >
>> > Reported-by: Andreas Hartmann <andihartmann@freenet.de>
>> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>> > Tested-by: Andreas Hartmann <andihartmann@freenet.de>
>>
>> If I understand correctly, these two (patches 3 & 4) fix a v3.14 regression
>> caused by 425c1b223dac ("PCI: Add Virtual Channel to save/restore support").
>>
>> If so, these should go to for-linus for v3.19.  What about patches 1 & 2?
>> Do they fix a regression?  Is there a pointer to a bugzilla or problem
>> report about that issue?
>>
>> I don't understand the connection between 425c1b223dac and
>> PCI_DEV_FLAGS_NO_BUS_RESET, because 425c1b223dac doesn't seem to do any
>> resets.  Is that the wrong commit, or can you outline the connection for
>> me?
>
> TBH, I don't have a lot of faith in associating this to 425c1b223dac,
> I'm not sure how Andreas' bisect landed there.  IME, this device cannot,
> and has never been able to handle a bus reset.  A simple setpci
> experiment on the commandline can confirm this.  What I think happened
> is that with the PCI bus reset infrastructure we added, we switched QEMU
> to prefer PCI bus resets over things like PM D3hot->D0 resets.  So it's
> just more prolific use of bus resets by userspace.

Maybe it's not 425c1b223dac, but it seems pretty clear that it really
is a regression from the user point of view.  I'm sure Andreas was
using the same user-space for v3.13 and v3.14.

Testing 425c1b223dac^ (the predecessor of 425c1b223dac) and
425c1b223dac would confirm the bisect results.

If it really turns out that 425c1b223dac^ works and 425c1b223dac
fails, maybe we could learn something interesting by adding debug
output to the various reset flavors and comparing the output?

> There's also no regression in 1 & 2, PM reset has never done anything
> useful on those devices.  Thanks,

Since 1 & 2 don't fix a regression, I think what I should do is apply
3 & 4 to for-linus for v3.19, then apply 1 & 2 on top as v3.20
material.

Bjorn

>> > ---
>> >
>> >  drivers/pci/quirks.c |   14 ++++++++++++++
>> >  1 file changed, 14 insertions(+)
>> >
>> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> > index 561e10d..ebbd5b4 100644
>> > --- a/drivers/pci/quirks.c
>> > +++ b/drivers/pci/quirks.c
>> > @@ -3029,6 +3029,20 @@ static void quirk_no_pm_reset(struct pci_dev *dev)
>> >  DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
>> >                            PCI_CLASS_DISPLAY_VGA, 8, quirk_no_pm_reset);
>> >
>> > +static void quirk_no_bus_reset(struct pci_dev *dev)
>> > +{
>> > +   dev->dev_flags |= PCI_DEV_FLAGS_NO_BUS_RESET;
>> > +}
>> > +
>> > +/*
>> > + * Atheros AR93xx chips do not behave after a bus reset.  The device will
>> > + * throw a Link Down error on AER capable system and regardless of AER,
>> > + * config space of the device is never accessible again and typically
>> > + * causes the system to hang or reset when access is attempted.
>> > + * http://www.spinics.net/lists/linux-pci/msg34797.html
>> > + */
>> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0030, quirk_no_bus_reset);
>> > +
>> >  #ifdef CONFIG_ACPI
>> >  /*
>> >   * Apple: Shutdown Cactus Ridge Thunderbolt controller.
>> >
>
>
>

Re: [PATCH 4/4] PCI: quirk Atheros AR93xx to avoid bus reset

[Andreas Hartmann]

Alex Williamson wrote:
> On Thu, 2015-01-08 at 09:07 -0700, Bjorn Helgaas wrote:
>> On Fri, Nov 21, 2014 at 11:24:27AM -0700, Alex Williamson wrote:
>>> Reports against the TL-WDN4800 card indicate that PCI bus reset of
>>> this Atheros device cause system lock-ups and resets.  I've also
>>> been able to confirm this behavior on multiple systems.  The device
>>> never returns from reset and attempts to access config space of the
>>> device after reset result in hangs.  Blacklist bus reset for the
>>> device to avoid this issue.
>>>
>>> Reported-by: Andreas Hartmann <andihartmann@freenet.de>
>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>> Tested-by: Andreas Hartmann <andihartmann@freenet.de>
>>
>> If I understand correctly, these two (patches 3 & 4) fix a v3.14 regression
>> caused by 425c1b223dac ("PCI: Add Virtual Channel to save/restore support").
>>
>> If so, these should go to for-linus for v3.19.  What about patches 1 & 2?
>> Do they fix a regression?  Is there a pointer to a bugzilla or problem
>> report about that issue?
>>
>> I don't understand the connection between 425c1b223dac and
>> PCI_DEV_FLAGS_NO_BUS_RESET, because 425c1b223dac doesn't seem to do any
>> resets.  Is that the wrong commit, or can you outline the connection for
>> me?
> 
> TBH, I don't have a lot of faith in associating this to 425c1b223dac,
> I'm not sure how Andreas' bisect landed there. 

Because removing this patch made it working again :-)

And too:
http://thread.gmane.org/gmane.linux.kernel.pci/35170/focus=35984

Kernel 2.10. and 2.12. and 2.13. did work fine for me. 2.14 is the first
kernel, which hangs the machine at startup of the VM. The userland
(qemu) didn't change in between.

Therefore: from my point of view, it is a regression, because things
have been working < 2.14.

Besides that: It is undoubted, that there is a problem with resetting
this card. But the difference between >= 3.14 and < 3.14 is, that < 3.14
has been working nevertheless. The patch
425c1b223dac456d00a61fd6b451b6d1cf00d065 obviously changed something
which I can't say and I don't know off. Therefore, the quirk-patch is
definitely required, because things work completely fine again w/ this
patch.

"Working" means for me here: I was able to start (and use) the VM w/o
crashing the machine and this isn't possible w/ unpatched 2.14+ any
more. Yes, w/ 2.12, I wasn't able to restart the VM (it then crashed the
machine), but w/ 2.10 even this was possible.


> IME, this device cannot,
> and has never been able to handle a bus reset.  A simple setpci
> experiment on the commandline can confirm this.  What I think happened
> is that with the PCI bus reset infrastructure we added, we switched QEMU
> to prefer PCI bus resets over things like PM D3hot->D0 resets.  So it's
> just more prolific use of bus resets by userspace.
> 
> There's also no regression in 1 & 2, PM reset has never done anything
> useful on those devices.  Thanks,
> 
> Alex
> 
>>> ---
>>>
>>>  drivers/pci/quirks.c |   14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>>> index 561e10d..ebbd5b4 100644
>>> --- a/drivers/pci/quirks.c
>>> +++ b/drivers/pci/quirks.c
>>> @@ -3029,6 +3029,20 @@ static void quirk_no_pm_reset(struct pci_dev *dev)
>>>  DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
>>>  			       PCI_CLASS_DISPLAY_VGA, 8, quirk_no_pm_reset);
>>>  
>>> +static void quirk_no_bus_reset(struct pci_dev *dev)
>>> +{
>>> +	dev->dev_flags |= PCI_DEV_FLAGS_NO_BUS_RESET;
>>> +}
>>> +
>>> +/*
>>> + * Atheros AR93xx chips do not behave after a bus reset.  The device will
>>> + * throw a Link Down error on AER capable system and regardless of AER,
>>> + * config space of the device is never accessible again and typically
>>> + * causes the system to hang or reset when access is attempted.
>>> + * http://www.spinics.net/lists/linux-pci/msg34797.html
>>> + */
>>> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0030, quirk_no_bus_reset);
>>> +
>>>  #ifdef CONFIG_ACPI
>>>  /*
>>>   * Apple: Shutdown Cactus Ridge Thunderbolt controller.
>>>
> 
> 
> 
> --
> 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
> 

Re: [PATCH 4/4] PCI: quirk Atheros AR93xx to avoid bus reset

[Alex Williamson]

On Mon, 2015-01-12 at 16:20 +0100, Andreas Hartmann wrote:
> Alex Williamson wrote:
> > On Thu, 2015-01-08 at 09:07 -0700, Bjorn Helgaas wrote:
> >> On Fri, Nov 21, 2014 at 11:24:27AM -0700, Alex Williamson wrote:
> >>> Reports against the TL-WDN4800 card indicate that PCI bus reset of
> >>> this Atheros device cause system lock-ups and resets.  I've also
> >>> been able to confirm this behavior on multiple systems.  The device
> >>> never returns from reset and attempts to access config space of the
> >>> device after reset result in hangs.  Blacklist bus reset for the
> >>> device to avoid this issue.
> >>>
> >>> Reported-by: Andreas Hartmann <andihartmann@freenet.de>
> >>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >>> Tested-by: Andreas Hartmann <andihartmann@freenet.de>
> >>
> >> If I understand correctly, these two (patches 3 & 4) fix a v3.14 regression
> >> caused by 425c1b223dac ("PCI: Add Virtual Channel to save/restore support").
> >>
> >> If so, these should go to for-linus for v3.19.  What about patches 1 & 2?
> >> Do they fix a regression?  Is there a pointer to a bugzilla or problem
> >> report about that issue?
> >>
> >> I don't understand the connection between 425c1b223dac and
> >> PCI_DEV_FLAGS_NO_BUS_RESET, because 425c1b223dac doesn't seem to do any
> >> resets.  Is that the wrong commit, or can you outline the connection for
> >> me?
> > 
> > TBH, I don't have a lot of faith in associating this to 425c1b223dac,
> > I'm not sure how Andreas' bisect landed there. 
> 
> Because removing this patch made it working again :-)
> 
> And too:
> http://thread.gmane.org/gmane.linux.kernel.pci/35170/focus=35984
> 
> Kernel 2.10. and 2.12. and 2.13. did work fine for me. 2.14 is the first
> kernel, which hangs the machine at startup of the VM. The userland
> (qemu) didn't change in between.

s/2\./3\./

Ok, so what about VC save/restore (425c1b223dac) is the problem then?
When we tried to determine that, you found that if we continue from the
top of the save loop, everything works (ie. no VC state saved), but if
you continue after the variable declaration of the same loop (ie. still
no VC state saved), it breaks:

http://www.spinics.net/lists/linux-pci/msg36166.html

So, please forgive me if I don't have a whole lot of faith that
425c1b223dac is involved.

We also both independently determined that this particular device never
recovers from a PCI bus reset, even when done from userspace with setpci
and absolutely no save/restore wrappers.  Config space on the device is
never accessible after the reset.  Therefore, how could any sort of bus
reset with save/restore ever work for this device?

> Therefore: from my point of view, it is a regression, because things
> have been working < 2.14.
> 
> Besides that: It is undoubted, that there is a problem with resetting
> this card. But the difference between >= 3.14 and < 3.14 is, that < 3.14
> has been working nevertheless. The patch
> 425c1b223dac456d00a61fd6b451b6d1cf00d065 obviously changed something
> which I can't say and I don't know off. Therefore, the quirk-patch is
> definitely required, because things work completely fine again w/ this
> patch.
> 
> "Working" means for me here: I was able to start (and use) the VM w/o
> crashing the machine and this isn't possible w/ unpatched 2.14+ any
> more. Yes, w/ 2.12, I wasn't able to restart the VM (it then crashed the
> machine), but w/ 2.10 even this was possible.

What?!  So v3.12 still had a machine crash when assigning this device.
The vfio hot reset interface was added in v3.12, so v3.10 didn't have
any way to do a reset other than what pci_reset_function() decided to
do.  That all seems to associate the machine crash to the ability to do
a bus reset on the device.  I'm not sure why the behavior changed
between v3.14 and v3.12 (maybe the try-reset addition), but there's some
sort of pre-existing issue before we even got to 425c1b223dac.

I'm perfectly happy tagging this for stable, but it seems like a
hardware bug exposed by allowing userspace the ability to select a bus
reset.  Whether or not that's a kernel regression isn't exactly clear to
me ("new functionality exposes broken hardware, news at 11").  Thanks,

Alex

> > IME, this device cannot,
> > and has never been able to handle a bus reset.  A simple setpci
> > experiment on the commandline can confirm this.  What I think happened
> > is that with the PCI bus reset infrastructure we added, we switched QEMU
> > to prefer PCI bus resets over things like PM D3hot->D0 resets.  So it's
> > just more prolific use of bus resets by userspace.
> > 
> > There's also no regression in 1 & 2, PM reset has never done anything
> > useful on those devices.  Thanks,
> > 
> > Alex
> > 
> >>> ---
> >>>
> >>>  drivers/pci/quirks.c |   14 ++++++++++++++
> >>>  1 file changed, 14 insertions(+)
> >>>
> >>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >>> index 561e10d..ebbd5b4 100644
> >>> --- a/drivers/pci/quirks.c
> >>> +++ b/drivers/pci/quirks.c
> >>> @@ -3029,6 +3029,20 @@ static void quirk_no_pm_reset(struct pci_dev *dev)
> >>>  DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
> >>>  			       PCI_CLASS_DISPLAY_VGA, 8, quirk_no_pm_reset);
> >>>  
> >>> +static void quirk_no_bus_reset(struct pci_dev *dev)
> >>> +{
> >>> +	dev->dev_flags |= PCI_DEV_FLAGS_NO_BUS_RESET;
> >>> +}
> >>> +
> >>> +/*
> >>> + * Atheros AR93xx chips do not behave after a bus reset.  The device will
> >>> + * throw a Link Down error on AER capable system and regardless of AER,
> >>> + * config space of the device is never accessible again and typically
> >>> + * causes the system to hang or reset when access is attempted.
> >>> + * http://www.spinics.net/lists/linux-pci/msg34797.html
> >>> + */
> >>> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0030, quirk_no_bus_reset);
> >>> +
> >>>  #ifdef CONFIG_ACPI
> >>>  /*
> >>>   * Apple: Shutdown Cactus Ridge Thunderbolt controller.
> >>>
> > 
> > 
> > 
> > --
> > 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
> > 
> 

Re: [PATCH 4/4] PCI: quirk Atheros AR93xx to avoid bus reset

[Andreas Hartmann]

Hello Alex!

Alex Williamson wrote:
> On Mon, 2015-01-12 at 16:20 +0100, Andreas Hartmann wrote:
>> Alex Williamson wrote:
>>> On Thu, 2015-01-08 at 09:07 -0700, Bjorn Helgaas wrote:
>>>> On Fri, Nov 21, 2014 at 11:24:27AM -0700, Alex Williamson wrote:
>>>>> Reports against the TL-WDN4800 card indicate that PCI bus reset of
>>>>> this Atheros device cause system lock-ups and resets.  I've also
>>>>> been able to confirm this behavior on multiple systems.  The device
>>>>> never returns from reset and attempts to access config space of the
>>>>> device after reset result in hangs.  Blacklist bus reset for the
>>>>> device to avoid this issue.
>>>>>
>>>>> Reported-by: Andreas Hartmann <andihartmann@freenet.de>
>>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>>>> Tested-by: Andreas Hartmann <andihartmann@freenet.de>
>>>>
>>>> If I understand correctly, these two (patches 3 & 4) fix a v3.14 regression
>>>> caused by 425c1b223dac ("PCI: Add Virtual Channel to save/restore support").
>>>>
>>>> If so, these should go to for-linus for v3.19.  What about patches 1 & 2?
>>>> Do they fix a regression?  Is there a pointer to a bugzilla or problem
>>>> report about that issue?
>>>>
>>>> I don't understand the connection between 425c1b223dac and
>>>> PCI_DEV_FLAGS_NO_BUS_RESET, because 425c1b223dac doesn't seem to do any
>>>> resets.  Is that the wrong commit, or can you outline the connection for
>>>> me?
>>>
>>> TBH, I don't have a lot of faith in associating this to 425c1b223dac,
>>> I'm not sure how Andreas' bisect landed there. 
>>
>> Because removing this patch made it working again :-)
>>
>> And too:
>> http://thread.gmane.org/gmane.linux.kernel.pci/35170/focus=35984
>>
>> Kernel 2.10. and 2.12. and 2.13. did work fine for me. 2.14 is the first
>> kernel, which hangs the machine at startup of the VM. The userland
>> (qemu) didn't change in between.
> 
> s/2\./3\./

Thanks :-) It seems I don't like the number 3 :-)

> Ok, so what about VC save/restore (425c1b223dac) is the problem then?
> When we tried to determine that, you found that if we continue from the
> top of the save loop, everything works (ie. no VC state saved), but if
> you continue after the variable declaration of the same loop (ie. still
> no VC state saved), it breaks:
> 
> http://www.spinics.net/lists/linux-pci/msg36166.html
> 
> So, please forgive me if I don't have a whole lot of faith that
> 425c1b223dac is involved.

It's hard for me, too. Really. It's kind of mystique.

> We also both independently determined that this particular device never
> recovers from a PCI bus reset, even when done from userspace with setpci
> and absolutely no save/restore wrappers.

Yes.

>  Config space on the device is
> never accessible after the reset.

Yes.

>  Therefore, how could any sort of bus
> reset with save/restore ever work for this device?

I can't say. What I definitely can say, is that I never had problems
with running VMs w/ qemu until 3.14 came up. Do you think I'm lying? I
used 3.10. and 3.12. for long time w/o (known!) problems (3.12 only on
first start of VM). Otherwise I would have been here long time before :-))).

>> Therefore: from my point of view, it is a regression, because things
>> have been working < 2.14.
>>
>> Besides that: It is undoubted, that there is a problem with resetting
>> this card. But the difference between >= 3.14 and < 3.14 is, that < 3.14
>> has been working nevertheless. The patch
>> 425c1b223dac456d00a61fd6b451b6d1cf00d065 obviously changed something
>> which I can't say and I don't know off. Therefore, the quirk-patch is
>> definitely required, because things work completely fine again w/ this
>> patch.
>>
>> "Working" means for me here: I was able to start (and use) the VM w/o
>> crashing the machine and this isn't possible w/ unpatched 2.14+ any
>> more. Yes, w/ 2.12, I wasn't able to restart the VM (it then crashed the
>> machine), but w/ 2.10 even this was possible.
> 
> What?!  So v3.12 still had a machine crash when assigning this device.

Yes. If you *re*start the VM (long time, I didn't knew that fact at all
- I just discovered it during testing while analyzing the problem :-)).
The first start (after reboot) was not a problem. This was the usual use
case here :-)).

Believe me, I'm really convinced that this card does have a problem with
resets. I'm just wondering why it had worked for me until 3.13. That's all.

> The vfio hot reset interface was added in v3.12, so v3.10 didn't have
> any way to do a reset other than what pci_reset_function() decided to
> do.  That all seems to associate the machine crash to the ability to do
> a bus reset on the device.  I'm not sure why the behavior changed
> between v3.14 and v3.12 (maybe the try-reset addition), but there's some
> sort of pre-existing issue before we even got to 425c1b223dac.

Most probably.

> I'm perfectly happy tagging this for stable,

Thanks!! I'm really very comfortable with your patch and your support!
Really! Thanks a lot! It's just odd for me, why it partly worked (first
start of VM worked) w/ 3.12 and 3.13 and 3.14 suddenly no more at all.

You have been accidentally the sufferer - most probably it could have
hit any other change, too. Sorry for that :-(. Therefore: kudos for
anyway fixing the problem. This is really not a matter of course at all!

> but it seems like a
> hardware bug exposed by allowing userspace the ability to select a bus
> reset.  Whether or not that's a kernel regression isn't exactly clear to
> me ("new functionality exposes broken hardware, news at 11").  Thanks,
> 
> Alex


Kind regards,
Andreas

>>> IME, this device cannot,
>>> and has never been able to handle a bus reset.  A simple setpci
>>> experiment on the commandline can confirm this.  What I think happened
>>> is that with the PCI bus reset infrastructure we added, we switched QEMU
>>> to prefer PCI bus resets over things like PM D3hot->D0 resets.  So it's
>>> just more prolific use of bus resets by userspace.
>>>
>>> There's also no regression in 1 & 2, PM reset has never done anything
>>> useful on those devices.  Thanks,
>>>
>>> Alex
>>>
>>>>> ---
>>>>>
>>>>>  drivers/pci/quirks.c |   14 ++++++++++++++
>>>>>  1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>>>>> index 561e10d..ebbd5b4 100644
>>>>> --- a/drivers/pci/quirks.c
>>>>> +++ b/drivers/pci/quirks.c
>>>>> @@ -3029,6 +3029,20 @@ static void quirk_no_pm_reset(struct pci_dev *dev)
>>>>>  DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
>>>>>  			       PCI_CLASS_DISPLAY_VGA, 8, quirk_no_pm_reset);
>>>>>  
>>>>> +static void quirk_no_bus_reset(struct pci_dev *dev)
>>>>> +{
>>>>> +	dev->dev_flags |= PCI_DEV_FLAGS_NO_BUS_RESET;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Atheros AR93xx chips do not behave after a bus reset.  The device will
>>>>> + * throw a Link Down error on AER capable system and regardless of AER,
>>>>> + * config space of the device is never accessible again and typically
>>>>> + * causes the system to hang or reset when access is attempted.
>>>>> + * http://www.spinics.net/lists/linux-pci/msg34797.html
>>>>> + */
>>>>> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0030, quirk_no_bus_reset);
>>>>> +
>>>>>  #ifdef CONFIG_ACPI
>>>>>  /*
>>>>>   * Apple: Shutdown Cactus Ridge Thunderbolt controller.
>>>>>
>>>
>>>
>>>
>>> --
>>> 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
>>>
>>
> 
> 
> 

Re: [PATCH 4/4] PCI: quirk Atheros AR93xx to avoid bus reset

[Bjorn Helgaas]

On Mon, Jan 12, 2015 at 1:15 PM, Andreas Hartmann
<andihartmann@freenet.de> wrote:
> Hello Alex!
>
> Alex Williamson wrote:
>> On Mon, 2015-01-12 at 16:20 +0100, Andreas Hartmann wrote:
>>> Alex Williamson wrote:
>>>> On Thu, 2015-01-08 at 09:07 -0700, Bjorn Helgaas wrote:
>>>>> On Fri, Nov 21, 2014 at 11:24:27AM -0700, Alex Williamson wrote:
>>>>>> Reports against the TL-WDN4800 card indicate that PCI bus reset of
>>>>>> this Atheros device cause system lock-ups and resets.  I've also
>>>>>> been able to confirm this behavior on multiple systems.  The device
>>>>>> never returns from reset and attempts to access config space of the
>>>>>> device after reset result in hangs.  Blacklist bus reset for the
>>>>>> device to avoid this issue.
>>>>>>
>>>>>> Reported-by: Andreas Hartmann <andihartmann@freenet.de>
>>>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>>>>> Tested-by: Andreas Hartmann <andihartmann@freenet.de>
>>>>>
>>>>> If I understand correctly, these two (patches 3 & 4) fix a v3.14 regression
>>>>> caused by 425c1b223dac ("PCI: Add Virtual Channel to save/restore support").
>>>>>
>>>>> If so, these should go to for-linus for v3.19.  What about patches 1 & 2?
>>>>> Do they fix a regression?  Is there a pointer to a bugzilla or problem
>>>>> report about that issue?
>>>>>
>>>>> I don't understand the connection between 425c1b223dac and
>>>>> PCI_DEV_FLAGS_NO_BUS_RESET, because 425c1b223dac doesn't seem to do any
>>>>> resets.  Is that the wrong commit, or can you outline the connection for
>>>>> me?
>>>>
>>>> TBH, I don't have a lot of faith in associating this to 425c1b223dac,
>>>> I'm not sure how Andreas' bisect landed there.
>>>
>>> Because removing this patch made it working again :-)
>>>
>>> And too:
>>> http://thread.gmane.org/gmane.linux.kernel.pci/35170/focus=35984
>>>
>>> Kernel 2.10. and 2.12. and 2.13. did work fine for me. 2.14 is the first
>>> kernel, which hangs the machine at startup of the VM. The userland
>>> (qemu) didn't change in between.
>>
>> s/2\./3\./
>
> Thanks :-) It seems I don't like the number 3 :-)
>
>> Ok, so what about VC save/restore (425c1b223dac) is the problem then?
>> When we tried to determine that, you found that if we continue from the
>> top of the save loop, everything works (ie. no VC state saved), but if
>> you continue after the variable declaration of the same loop (ie. still
>> no VC state saved), it breaks:
>>
>> http://www.spinics.net/lists/linux-pci/msg36166.html
>>
>> So, please forgive me if I don't have a whole lot of faith that
>> 425c1b223dac is involved.
>
> It's hard for me, too. Really. It's kind of mystique.
>
>> We also both independently determined that this particular device never
>> recovers from a PCI bus reset, even when done from userspace with setpci
>> and absolutely no save/restore wrappers.
>
> Yes.
>
>>  Config space on the device is
>> never accessible after the reset.
>
> Yes.
>
>>  Therefore, how could any sort of bus
>> reset with save/restore ever work for this device?
>
> I can't say. What I definitely can say, is that I never had problems
> with running VMs w/ qemu until 3.14 came up. Do you think I'm lying? I
> used 3.10. and 3.12. for long time w/o (known!) problems (3.12 only on
> first start of VM). Otherwise I would have been here long time before :-))).
>
>>> Therefore: from my point of view, it is a regression, because things
>>> have been working < 2.14.
>>>
>>> Besides that: It is undoubted, that there is a problem with resetting
>>> this card. But the difference between >= 3.14 and < 3.14 is, that < 3.14
>>> has been working nevertheless. The patch
>>> 425c1b223dac456d00a61fd6b451b6d1cf00d065 obviously changed something
>>> which I can't say and I don't know off. Therefore, the quirk-patch is
>>> definitely required, because things work completely fine again w/ this
>>> patch.
>>>
>>> "Working" means for me here: I was able to start (and use) the VM w/o
>>> crashing the machine and this isn't possible w/ unpatched 2.14+ any
>>> more. Yes, w/ 2.12, I wasn't able to restart the VM (it then crashed the
>>> machine), but w/ 2.10 even this was possible.
>>
>> What?!  So v3.12 still had a machine crash when assigning this device.
>
> Yes. If you *re*start the VM (long time, I didn't knew that fact at all
> - I just discovered it during testing while analyzing the problem :-)).
> The first start (after reboot) was not a problem. This was the usual use
> case here :-)).
>
> Believe me, I'm really convinced that this card does have a problem with
> resets. I'm just wondering why it had worked for me until 3.13. That's all.
>
>> The vfio hot reset interface was added in v3.12, so v3.10 didn't have
>> any way to do a reset other than what pci_reset_function() decided to
>> do.  That all seems to associate the machine crash to the ability to do
>> a bus reset on the device.  I'm not sure why the behavior changed
>> between v3.14 and v3.12 (maybe the try-reset addition), but there's some
>> sort of pre-existing issue before we even got to 425c1b223dac.
>
> Most probably.
>
>> I'm perfectly happy tagging this for stable,
>
> Thanks!! I'm really very comfortable with your patch and your support!
> Really! Thanks a lot! It's just odd for me, why it partly worked (first
> start of VM worked) w/ 3.12 and 3.13 and 3.14 suddenly no more at all.
>
> You have been accidentally the sufferer - most probably it could have
> hit any other change, too. Sorry for that :-(. Therefore: kudos for
> anyway fixing the problem. This is really not a matter of course at all!

So we should be able to add instrumentation to the reset paths in
425c1b223dac and 425c1b223dac^ and see some difference in how those
paths are exercised.  Right?

It still feels like there's some magic we don't understand here, and
that niggles at me.

Bjorn

>> but it seems like a
>> hardware bug exposed by allowing userspace the ability to select a bus
>> reset.  Whether or not that's a kernel regression isn't exactly clear to
>> me ("new functionality exposes broken hardware, news at 11").  Thanks,
>>
>> Alex
>
>
> Kind regards,
> Andreas
>
>>>> IME, this device cannot,
>>>> and has never been able to handle a bus reset.  A simple setpci
>>>> experiment on the commandline can confirm this.  What I think happened
>>>> is that with the PCI bus reset infrastructure we added, we switched QEMU
>>>> to prefer PCI bus resets over things like PM D3hot->D0 resets.  So it's
>>>> just more prolific use of bus resets by userspace.
>>>>
>>>> There's also no regression in 1 & 2, PM reset has never done anything
>>>> useful on those devices.  Thanks,
>>>>
>>>> Alex
>>>>
>>>>>> ---
>>>>>>
>>>>>>  drivers/pci/quirks.c |   14 ++++++++++++++
>>>>>>  1 file changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>>>>>> index 561e10d..ebbd5b4 100644
>>>>>> --- a/drivers/pci/quirks.c
>>>>>> +++ b/drivers/pci/quirks.c
>>>>>> @@ -3029,6 +3029,20 @@ static void quirk_no_pm_reset(struct pci_dev *dev)
>>>>>>  DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
>>>>>>                          PCI_CLASS_DISPLAY_VGA, 8, quirk_no_pm_reset);
>>>>>>
>>>>>> +static void quirk_no_bus_reset(struct pci_dev *dev)
>>>>>> +{
>>>>>> + dev->dev_flags |= PCI_DEV_FLAGS_NO_BUS_RESET;
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * Atheros AR93xx chips do not behave after a bus reset.  The device will
>>>>>> + * throw a Link Down error on AER capable system and regardless of AER,
>>>>>> + * config space of the device is never accessible again and typically
>>>>>> + * causes the system to hang or reset when access is attempted.
>>>>>> + * http://www.spinics.net/lists/linux-pci/msg34797.html
>>>>>> + */
>>>>>> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0030, quirk_no_bus_reset);
>>>>>> +
>>>>>>  #ifdef CONFIG_ACPI
>>>>>>  /*
>>>>>>   * Apple: Shutdown Cactus Ridge Thunderbolt controller.
>>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> 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
>>>>
>>>
>>
>>
>>
>

Re: [PATCH 0/4] PCI: Reset exclusions

[Bjorn Helgaas]

On Fri, Nov 21, 2014 at 11:24:02AM -0700, Alex Williamson wrote:
> This is really 2 sets of 2 patches, but they both add bits to
> dev_flags so are included together.
> 
> This fixes two problems we've seen with resets.  The first is for
> devices that advertise a PM reset mechanism, but it doesn't appear to
> do anything.  We add a quirk and flags bit to indicate the PM reset
> mechanism isn't viable.  This happens on some AMD GPUs and causes
> vfio-pci to assume the PM reset was successful when it really did
> nothing and we should have escalated to a PCI bus reset.  Alex
> Deucher confirms that PM reset isn't used by graphics drivers.  The
> exclusion here is for users of pci_reset_function(), which mostly
> only includes pci-sysfs and drivers like legacy KVM device assignment
> and VFIO.
> 
> The second issue is a problem identified with an Atheros wifi chip
> where in performing a bus reset of the device, we not only permanently
> lose access to the device, but it introduces host stability issues if
> we try to access it.  I've been unsuccessful in finding any way to
> make the device behave or in finding anyone with access to hardware
> documentation and errata for this device, so it seems like the most
> appropriate path is to blacklist bus resets for topologies including
> this device.  The second two patches add infrastructure and quirks to
> do this.  Thanks,
> 
> Alex
> 
> ---
> 
> Alex Williamson (4):
>       PCI: quirk Atheros AR93xx to avoid bus reset
>       PCI: Allow device quirks to exclude bus reset

I applied these two (above) to for-linus for v3.19, thanks.

>       PCI: quirk AMD/ATI VGA cards to avoid PM reset
>       PCI: Allow device quirks to exclude D3->D0 PM reset
> 
> 
>  drivers/pci/pci.c    |   42 +++++++++++++++++++++++++++++++++++++-----
>  drivers/pci/quirks.c |   35 +++++++++++++++++++++++++++++++++++
>  include/linux/pci.h  |    4 ++++
>  3 files changed, 76 insertions(+), 5 deletions(-)

Re: [PATCH 0/4] PCI: Reset exclusions

[Bjorn Helgaas]

On Thu, Jan 15, 2015 at 06:28:02PM -0600, Bjorn Helgaas wrote:
> On Fri, Nov 21, 2014 at 11:24:02AM -0700, Alex Williamson wrote:
> > This is really 2 sets of 2 patches, but they both add bits to
> > dev_flags so are included together.
> > 
> > This fixes two problems we've seen with resets.  The first is for
> > devices that advertise a PM reset mechanism, but it doesn't appear to
> > do anything.  We add a quirk and flags bit to indicate the PM reset
> > mechanism isn't viable.  This happens on some AMD GPUs and causes
> > vfio-pci to assume the PM reset was successful when it really did
> > nothing and we should have escalated to a PCI bus reset.  Alex
> > Deucher confirms that PM reset isn't used by graphics drivers.  The
> > exclusion here is for users of pci_reset_function(), which mostly
> > only includes pci-sysfs and drivers like legacy KVM device assignment
> > and VFIO.
> > 
> > The second issue is a problem identified with an Atheros wifi chip
> > where in performing a bus reset of the device, we not only permanently
> > lose access to the device, but it introduces host stability issues if
> > we try to access it.  I've been unsuccessful in finding any way to
> > make the device behave or in finding anyone with access to hardware
> > documentation and errata for this device, so it seems like the most
> > appropriate path is to blacklist bus resets for topologies including
> > this device.  The second two patches add infrastructure and quirks to
> > do this.  Thanks,
> > 
> > Alex
> > 
> > ---
> > 
> > Alex Williamson (4):
> >       PCI: quirk Atheros AR93xx to avoid bus reset
> >       PCI: Allow device quirks to exclude bus reset
> 
> I applied these two (above) to for-linus for v3.19, thanks.
> 
> >       PCI: quirk AMD/ATI VGA cards to avoid PM reset
> >       PCI: Allow device quirks to exclude D3->D0 PM reset

And I applied these PM reset patches to pci/virtualization for v3.20 (with
Alex's ack on the latter), thanks!

> > 
> >  drivers/pci/pci.c    |   42 +++++++++++++++++++++++++++++++++++++-----
> >  drivers/pci/quirks.c |   35 +++++++++++++++++++++++++++++++++++
> >  include/linux/pci.h  |    4 ++++
> >  3 files changed, 76 insertions(+), 5 deletions(-)