* [PATCH 0/2] PCI: NVMe reset quirk
@ 2018-07-23 22:24 Alex Williamson
2018-07-23 22:24 ` [PATCH 1/2] PCI: Export pcie_has_flr() Alex Williamson
2018-07-23 22:24 ` [PATCH 2/2] PCI: NVMe device specific reset quirk Alex Williamson
0 siblings, 2 replies; 7+ messages in thread
From: Alex Williamson @ 2018-07-23 22:24 UTC (permalink / raw)
To: linux-pci; +Cc: linux-kernel, linux-nvme
As discussed in the 2nd patch, at least one NVMe controller sometimes
doesn't like being reset while enabled and another will timeout during
a subsequent re-enable if it happens too quickly after reset.
Introduce a device specific reset quirk for all NVMe class devices so
that we can try to get reliable behavior from them for device
assignment and any other users of the PCI subsystem reset interface.
Patches against current PCI next branch. Thanks,
Alex
---
Alex Williamson (2):
PCI: Export pcie_has_flr()
PCI: NVMe device specific reset quirk
drivers/pci/pci.c | 3 +
drivers/pci/quirks.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 1
3 files changed, 115 insertions(+), 1 deletion(-)
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/2] PCI: Export pcie_has_flr() 2018-07-23 22:24 [PATCH 0/2] PCI: NVMe reset quirk Alex Williamson @ 2018-07-23 22:24 ` Alex Williamson 2018-07-23 22:24 ` [PATCH 2/2] PCI: NVMe device specific reset quirk Alex Williamson 1 sibling, 0 replies; 7+ messages in thread From: Alex Williamson @ 2018-07-23 22:24 UTC (permalink / raw) To: linux-pci; +Cc: linux-kernel, linux-nvme pcie_flr() suggests pcie_has_flr() to ensure that PCIe FLR support is present prior to calling. pcie_flr() is exported while pcie_has_flr() is not. Resolve this. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- drivers/pci/pci.c | 3 ++- include/linux/pci.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 2bec76c9d9a7..52fe2d72a99c 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4071,7 +4071,7 @@ static int pci_dev_wait(struct pci_dev *dev, char *reset_type, int timeout) * Returns true if the device advertises support for PCIe function level * resets. */ -static bool pcie_has_flr(struct pci_dev *dev) +bool pcie_has_flr(struct pci_dev *dev) { u32 cap; @@ -4081,6 +4081,7 @@ static bool pcie_has_flr(struct pci_dev *dev) pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); return cap & PCI_EXP_DEVCAP_FLR; } +EXPORT_SYMBOL_GPL(pcie_has_flr); /** * pcie_flr - initiate a PCIe function level reset diff --git a/include/linux/pci.h b/include/linux/pci.h index 04c7ea6ed67b..bbe030d7814f 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1092,6 +1092,7 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev, enum pci_bus_speed *speed, enum pcie_link_width *width); void pcie_print_link_status(struct pci_dev *dev); +bool pcie_has_flr(struct pci_dev *dev); int pcie_flr(struct pci_dev *dev); int __pci_reset_function_locked(struct pci_dev *dev); int pci_reset_function(struct pci_dev *dev); _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] PCI: NVMe device specific reset quirk 2018-07-23 22:24 [PATCH 0/2] PCI: NVMe reset quirk Alex Williamson 2018-07-23 22:24 ` [PATCH 1/2] PCI: Export pcie_has_flr() Alex Williamson @ 2018-07-23 22:24 ` Alex Williamson 2018-07-23 22:45 ` Keith Busch 2018-07-23 22:45 ` Bjorn Helgaas 1 sibling, 2 replies; 7+ messages in thread From: Alex Williamson @ 2018-07-23 22:24 UTC (permalink / raw) To: linux-pci; +Cc: linux-kernel, linux-nvme Take advantage of NVMe devices using a standard interface to quiesce the controller prior to reset, including device specific delays before and after that reset. This resolves several NVMe device assignment scenarios with two different vendors. The Intel DC P3700 controller has been shown to only work as a VM boot device on the initial VM startup, failing after reset or reboot, and also fails to initialize after hot-plug into a VM. Adding a delay after FLR resolves these cases. The Samsung SM961/PM961 (960 EVO) sometimes fails to return from FLR with the PCI config space reading back as -1. A reproducible instance of this behavior is resolved by clearing the enable bit in the configuration register and waiting for the ready status to clear (disabling the NVMe controller) prior to FLR. As all NVMe devices make use of this standard interface and the NVMe specification also requires PCIe FLR support, we can apply this quirk to all devices with matching class code. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- drivers/pci/quirks.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index e72c8742aafa..83853562f220 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -28,6 +28,7 @@ #include <linux/platform_data/x86/apple.h> #include <linux/pm_runtime.h> #include <linux/switchtec.h> +#include <linux/nvme.h> #include <asm/dma.h> /* isa_dma_bridge_buggy */ #include "pci.h" @@ -3669,6 +3670,116 @@ static int reset_chelsio_generic_dev(struct pci_dev *dev, int probe) #define PCI_DEVICE_ID_INTEL_IVB_M_VGA 0x0156 #define PCI_DEVICE_ID_INTEL_IVB_M2_VGA 0x0166 +/* NVMe controller needs delay before testing ready status */ +#define NVME_QUIRK_CHK_RDY_DELAY (1 << 0) +/* NVMe controller needs post-FLR delay */ +#define NVME_QUIRK_POST_FLR_DELAY (1 << 1) + +static const struct pci_device_id nvme_reset_tbl[] = { + { PCI_DEVICE(0x1bb1, 0x0100), /* Seagate Nytro Flash Storage */ + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, + { PCI_DEVICE(0x1c58, 0x0003), /* HGST adapter */ + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, + { PCI_DEVICE(0x1c58, 0x0023), /* WDC SN200 adapter */ + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, + { PCI_DEVICE(0x1c5f, 0x0540), /* Memblaze Pblaze4 adapter */ + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, + { PCI_DEVICE(0x144d, 0xa821), /* Samsung PM1725 */ + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, + { PCI_DEVICE(0x144d, 0xa822), /* Samsung PM1725a */ + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x0953), /* Intel DC P3700 */ + .driver_data = NVME_QUIRK_POST_FLR_DELAY, }, + { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) }, + { 0 } +}; + +/* + * The NVMe specification requires that controllers support PCIe FLR, but + * but some Samsung SM961/PM961 controllers fail to recover after FLR (-1 + * config space) unless the device is quiesced prior to FLR. Do this for + * all NVMe devices by disabling the controller before reset. Some Intel + * controllers also require an additional post-FLR delay or else attempts + * to re-enable will timeout, do that here as well with heuristically + * determined delay value. Also maintain the delay between disabling and + * checking ready status as used by the native NVMe driver. + */ +static int reset_nvme(struct pci_dev *dev, int probe) +{ + const struct pci_device_id *id; + void __iomem *bar; + u16 cmd; + u32 cfg; + + id = pci_match_id(nvme_reset_tbl, dev); + if (!id || !pcie_has_flr(dev) || !pci_resource_start(dev, 0)) + return -ENOTTY; + + if (probe) + return 0; + + bar = pci_iomap(dev, 0, NVME_REG_CC + sizeof(cfg)); + if (!bar) + return -ENOTTY; + + pci_read_config_word(dev, PCI_COMMAND, &cmd); + pci_write_config_word(dev, PCI_COMMAND, cmd | PCI_COMMAND_MEMORY); + + cfg = readl(bar + NVME_REG_CC); + + /* Disable controller if enabled */ + if (cfg & NVME_CC_ENABLE) { + u64 cap = readq(bar + NVME_REG_CAP); + unsigned long timeout; + + /* + * Per nvme_disable_ctrl() skip shutdown notification as it + * could complete commands to the admin queue. We only intend + * to quiesce the device before reset. + */ + cfg &= ~(NVME_CC_SHN_MASK | NVME_CC_ENABLE); + + writel(cfg, bar + NVME_REG_CC); + + /* A heuristic value, matches NVME_QUIRK_DELAY_AMOUNT */ + if (id->driver_data & NVME_QUIRK_CHK_RDY_DELAY) + msleep(2300); + + /* Cap register provides max timeout in 500ms increments */ + timeout = ((NVME_CAP_TIMEOUT(cap) + 1) * HZ / 2) + jiffies; + + for (;;) { + u32 status = readl(bar + NVME_REG_CSTS); + + /* Ready status becomes zero on disable complete */ + if (!(status & NVME_CSTS_RDY)) + break; + + msleep(100); + + if (time_after(jiffies, timeout)) { + pci_warn(dev, "Timeout waiting for NVMe ready status to clear after disable\n"); + break; + } + } + } + + pci_iounmap(dev, bar); + + /* + * We could use the optional NVM Subsystem Reset here, hardware + * supporting this is simply unavailable at the time of this code + * to validate in comparison to PCIe FLR. NVMe spec dictates that + * NVMe devices shall implement PCIe FLR. + */ + pcie_flr(dev); + + if (id->driver_data & NVME_QUIRK_POST_FLR_DELAY) + msleep(250); /* Heuristic based on Intel DC P3700 */ + + return 0; +} + static const struct pci_dev_reset_methods pci_dev_reset_methods[] = { { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF, reset_intel_82599_sfp_virtfn }, @@ -3678,6 +3789,7 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = { reset_ivb_igd }, { PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID, reset_chelsio_generic_dev }, + { PCI_ANY_ID, PCI_ANY_ID, reset_nvme }, { 0 } }; _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] PCI: NVMe device specific reset quirk 2018-07-23 22:24 ` [PATCH 2/2] PCI: NVMe device specific reset quirk Alex Williamson @ 2018-07-23 22:45 ` Keith Busch 2018-07-23 23:08 ` Alex Williamson 2018-07-23 22:45 ` Bjorn Helgaas 1 sibling, 1 reply; 7+ messages in thread From: Keith Busch @ 2018-07-23 22:45 UTC (permalink / raw) To: Alex Williamson; +Cc: linux-pci, linux-kernel, linux-nvme On Mon, Jul 23, 2018 at 04:24:31PM -0600, Alex Williamson wrote: > Take advantage of NVMe devices using a standard interface to quiesce > the controller prior to reset, including device specific delays before > and after that reset. This resolves several NVMe device assignment > scenarios with two different vendors. The Intel DC P3700 controller > has been shown to only work as a VM boot device on the initial VM > startup, failing after reset or reboot, and also fails to initialize > after hot-plug into a VM. Adding a delay after FLR resolves these > cases. The Samsung SM961/PM961 (960 EVO) sometimes fails to return > from FLR with the PCI config space reading back as -1. A reproducible > instance of this behavior is resolved by clearing the enable bit in > the configuration register and waiting for the ready status to clear > (disabling the NVMe controller) prior to FLR. > > As all NVMe devices make use of this standard interface and the NVMe > specification also requires PCIe FLR support, we can apply this quirk > to all devices with matching class code. Shouldn't this go in the nvme driver's reset_prepare/reset_done callbacks? _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] PCI: NVMe device specific reset quirk 2018-07-23 22:45 ` Keith Busch @ 2018-07-23 23:08 ` Alex Williamson 0 siblings, 0 replies; 7+ messages in thread From: Alex Williamson @ 2018-07-23 23:08 UTC (permalink / raw) To: Keith Busch; +Cc: linux-pci, linux-kernel, linux-nvme On Mon, 23 Jul 2018 16:45:08 -0600 Keith Busch <keith.busch@intel.com> wrote: > On Mon, Jul 23, 2018 at 04:24:31PM -0600, Alex Williamson wrote: > > Take advantage of NVMe devices using a standard interface to quiesce > > the controller prior to reset, including device specific delays before > > and after that reset. This resolves several NVMe device assignment > > scenarios with two different vendors. The Intel DC P3700 controller > > has been shown to only work as a VM boot device on the initial VM > > startup, failing after reset or reboot, and also fails to initialize > > after hot-plug into a VM. Adding a delay after FLR resolves these > > cases. The Samsung SM961/PM961 (960 EVO) sometimes fails to return > > from FLR with the PCI config space reading back as -1. A reproducible > > instance of this behavior is resolved by clearing the enable bit in > > the configuration register and waiting for the ready status to clear > > (disabling the NVMe controller) prior to FLR. > > > > As all NVMe devices make use of this standard interface and the NVMe > > specification also requires PCIe FLR support, we can apply this quirk > > to all devices with matching class code. > > Shouldn't this go in the nvme driver's reset_prepare/reset_done callbacks? The scenario I'm trying to fix is device assignment, the nvme driver isn't in play there. The device is bound to the vfio-pci driver at the time of these resets. Thanks, Alex _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] PCI: NVMe device specific reset quirk 2018-07-23 22:24 ` [PATCH 2/2] PCI: NVMe device specific reset quirk Alex Williamson 2018-07-23 22:45 ` Keith Busch @ 2018-07-23 22:45 ` Bjorn Helgaas 2018-07-24 0:11 ` Alex Williamson 1 sibling, 1 reply; 7+ messages in thread From: Bjorn Helgaas @ 2018-07-23 22:45 UTC (permalink / raw) To: Alex Williamson; +Cc: linux-pci, linux-kernel, linux-nvme On Mon, Jul 23, 2018 at 04:24:31PM -0600, Alex Williamson wrote: > Take advantage of NVMe devices using a standard interface to quiesce > the controller prior to reset, including device specific delays before > and after that reset. This resolves several NVMe device assignment > scenarios with two different vendors. The Intel DC P3700 controller > has been shown to only work as a VM boot device on the initial VM > startup, failing after reset or reboot, and also fails to initialize > after hot-plug into a VM. Adding a delay after FLR resolves these > cases. The Samsung SM961/PM961 (960 EVO) sometimes fails to return > from FLR with the PCI config space reading back as -1. A reproducible > instance of this behavior is resolved by clearing the enable bit in > the configuration register and waiting for the ready status to clear > (disabling the NVMe controller) prior to FLR. > > As all NVMe devices make use of this standard interface and the NVMe > specification also requires PCIe FLR support, we can apply this quirk > to all devices with matching class code. Do you have any pointers to problem reports or bugzilla entries that we could include here? > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > drivers/pci/quirks.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 112 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index e72c8742aafa..83853562f220 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -28,6 +28,7 @@ > #include <linux/platform_data/x86/apple.h> > #include <linux/pm_runtime.h> > #include <linux/switchtec.h> > +#include <linux/nvme.h> > #include <asm/dma.h> /* isa_dma_bridge_buggy */ > #include "pci.h" > > @@ -3669,6 +3670,116 @@ static int reset_chelsio_generic_dev(struct pci_dev *dev, int probe) > #define PCI_DEVICE_ID_INTEL_IVB_M_VGA 0x0156 > #define PCI_DEVICE_ID_INTEL_IVB_M2_VGA 0x0166 > > +/* NVMe controller needs delay before testing ready status */ > +#define NVME_QUIRK_CHK_RDY_DELAY (1 << 0) > +/* NVMe controller needs post-FLR delay */ > +#define NVME_QUIRK_POST_FLR_DELAY (1 << 1) > + > +static const struct pci_device_id nvme_reset_tbl[] = { > + { PCI_DEVICE(0x1bb1, 0x0100), /* Seagate Nytro Flash Storage */ > + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, > + { PCI_DEVICE(0x1c58, 0x0003), /* HGST adapter */ > + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, > + { PCI_DEVICE(0x1c58, 0x0023), /* WDC SN200 adapter */ > + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, > + { PCI_DEVICE(0x1c5f, 0x0540), /* Memblaze Pblaze4 adapter */ > + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, > + { PCI_DEVICE(0x144d, 0xa821), /* Samsung PM1725 */ We do have PCI_VENDOR_ID_SAMSUNG if you want to use it here. I don't see Seagate, HGST, etc. > + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, > + { PCI_DEVICE(0x144d, 0xa822), /* Samsung PM1725a */ > + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x0953), /* Intel DC P3700 */ > + .driver_data = NVME_QUIRK_POST_FLR_DELAY, }, > + { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) }, > + { 0 } > +}; > + > +/* > + * The NVMe specification requires that controllers support PCIe FLR, but > + * but some Samsung SM961/PM961 controllers fail to recover after FLR (-1 > + * config space) unless the device is quiesced prior to FLR. Do this for > + * all NVMe devices by disabling the controller before reset. Some Intel > + * controllers also require an additional post-FLR delay or else attempts > + * to re-enable will timeout, do that here as well with heuristically > + * determined delay value. Also maintain the delay between disabling and > + * checking ready status as used by the native NVMe driver. > + */ > +static int reset_nvme(struct pci_dev *dev, int probe) > +{ > + const struct pci_device_id *id; > + void __iomem *bar; > + u16 cmd; > + u32 cfg; > + > + id = pci_match_id(nvme_reset_tbl, dev); > + if (!id || !pcie_has_flr(dev) || !pci_resource_start(dev, 0)) > + return -ENOTTY; > + > + if (probe) > + return 0; > + > + bar = pci_iomap(dev, 0, NVME_REG_CC + sizeof(cfg)); > + if (!bar) > + return -ENOTTY; > + > + pci_read_config_word(dev, PCI_COMMAND, &cmd); > + pci_write_config_word(dev, PCI_COMMAND, cmd | PCI_COMMAND_MEMORY); > + > + cfg = readl(bar + NVME_REG_CC); Apparently this is part of some NVMe spec and all controllers support this? Is there a public reference you could cite for the details? > + > + /* Disable controller if enabled */ > + if (cfg & NVME_CC_ENABLE) { > + u64 cap = readq(bar + NVME_REG_CAP); > + unsigned long timeout; > + > + /* > + * Per nvme_disable_ctrl() skip shutdown notification as it > + * could complete commands to the admin queue. We only intend > + * to quiesce the device before reset. > + */ > + cfg &= ~(NVME_CC_SHN_MASK | NVME_CC_ENABLE); > + > + writel(cfg, bar + NVME_REG_CC); > + > + /* A heuristic value, matches NVME_QUIRK_DELAY_AMOUNT */ > + if (id->driver_data & NVME_QUIRK_CHK_RDY_DELAY) > + msleep(2300); > + > + /* Cap register provides max timeout in 500ms increments */ > + timeout = ((NVME_CAP_TIMEOUT(cap) + 1) * HZ / 2) + jiffies; > + > + for (;;) { > + u32 status = readl(bar + NVME_REG_CSTS); > + > + /* Ready status becomes zero on disable complete */ > + if (!(status & NVME_CSTS_RDY)) > + break; > + > + msleep(100); > + > + if (time_after(jiffies, timeout)) { > + pci_warn(dev, "Timeout waiting for NVMe ready status to clear after disable\n"); > + break; > + } > + } > + } > + > + pci_iounmap(dev, bar); > + > + /* > + * We could use the optional NVM Subsystem Reset here, hardware > + * supporting this is simply unavailable at the time of this code > + * to validate in comparison to PCIe FLR. NVMe spec dictates that > + * NVMe devices shall implement PCIe FLR. > + */ > + pcie_flr(dev); > + > + if (id->driver_data & NVME_QUIRK_POST_FLR_DELAY) > + msleep(250); /* Heuristic based on Intel DC P3700 */ > + > + return 0; > +} > + > static const struct pci_dev_reset_methods pci_dev_reset_methods[] = { > { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF, > reset_intel_82599_sfp_virtfn }, > @@ -3678,6 +3789,7 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = { > reset_ivb_igd }, > { PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID, > reset_chelsio_generic_dev }, > + { PCI_ANY_ID, PCI_ANY_ID, reset_nvme }, > { 0 } > }; > > > > _______________________________________________ > Linux-nvme mailing list > Linux-nvme@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-nvme _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] PCI: NVMe device specific reset quirk 2018-07-23 22:45 ` Bjorn Helgaas @ 2018-07-24 0:11 ` Alex Williamson 0 siblings, 0 replies; 7+ messages in thread From: Alex Williamson @ 2018-07-24 0:11 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-pci, linux-kernel, linux-nvme On Mon, 23 Jul 2018 17:45:33 -0500 Bjorn Helgaas <helgaas@kernel.org> wrote: > On Mon, Jul 23, 2018 at 04:24:31PM -0600, Alex Williamson wrote: > > Take advantage of NVMe devices using a standard interface to quiesce > > the controller prior to reset, including device specific delays before > > and after that reset. This resolves several NVMe device assignment > > scenarios with two different vendors. The Intel DC P3700 controller > > has been shown to only work as a VM boot device on the initial VM > > startup, failing after reset or reboot, and also fails to initialize > > after hot-plug into a VM. Adding a delay after FLR resolves these > > cases. The Samsung SM961/PM961 (960 EVO) sometimes fails to return > > from FLR with the PCI config space reading back as -1. A reproducible > > instance of this behavior is resolved by clearing the enable bit in > > the configuration register and waiting for the ready status to clear > > (disabling the NVMe controller) prior to FLR. > > > > As all NVMe devices make use of this standard interface and the NVMe > > specification also requires PCIe FLR support, we can apply this quirk > > to all devices with matching class code. > > Do you have any pointers to problem reports or bugzilla entries that > we could include here? Yes, https://bugzilla.redhat.com/show_bug.cgi?id=1592654 This only covers the Intel P3700 issue. The Samsung issue has been reported via a couple bugs, but was not reproducible until recently. Those bugs were previously closed due to lack of information. I'll add the above link. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > --- > > drivers/pci/quirks.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 112 insertions(+) > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > index e72c8742aafa..83853562f220 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -28,6 +28,7 @@ > > #include <linux/platform_data/x86/apple.h> > > #include <linux/pm_runtime.h> > > #include <linux/switchtec.h> > > +#include <linux/nvme.h> > > #include <asm/dma.h> /* isa_dma_bridge_buggy */ > > #include "pci.h" > > > > @@ -3669,6 +3670,116 @@ static int reset_chelsio_generic_dev(struct pci_dev *dev, int probe) > > #define PCI_DEVICE_ID_INTEL_IVB_M_VGA 0x0156 > > #define PCI_DEVICE_ID_INTEL_IVB_M2_VGA 0x0166 > > > > +/* NVMe controller needs delay before testing ready status */ > > +#define NVME_QUIRK_CHK_RDY_DELAY (1 << 0) > > +/* NVMe controller needs post-FLR delay */ > > +#define NVME_QUIRK_POST_FLR_DELAY (1 << 1) > > + > > +static const struct pci_device_id nvme_reset_tbl[] = { > > + { PCI_DEVICE(0x1bb1, 0x0100), /* Seagate Nytro Flash Storage */ > > + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, > > + { PCI_DEVICE(0x1c58, 0x0003), /* HGST adapter */ > > + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, > > + { PCI_DEVICE(0x1c58, 0x0023), /* WDC SN200 adapter */ > > + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, > > + { PCI_DEVICE(0x1c5f, 0x0540), /* Memblaze Pblaze4 adapter */ > > + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, > > + { PCI_DEVICE(0x144d, 0xa821), /* Samsung PM1725 */ > > We do have PCI_VENDOR_ID_SAMSUNG if you want to use it here. I > don't see Seagate, HGST, etc. Oops, cut and pasted those from the nvme driver, I'll use the Samsung macro. > > + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, > > + { PCI_DEVICE(0x144d, 0xa822), /* Samsung PM1725a */ > > + .driver_data = NVME_QUIRK_CHK_RDY_DELAY, }, > > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x0953), /* Intel DC P3700 */ > > + .driver_data = NVME_QUIRK_POST_FLR_DELAY, }, > > + { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) }, > > + { 0 } > > +}; > > + > > +/* > > + * The NVMe specification requires that controllers support PCIe FLR, but > > + * but some Samsung SM961/PM961 controllers fail to recover after FLR (-1 > > + * config space) unless the device is quiesced prior to FLR. Do this for > > + * all NVMe devices by disabling the controller before reset. Some Intel > > + * controllers also require an additional post-FLR delay or else attempts > > + * to re-enable will timeout, do that here as well with heuristically > > + * determined delay value. Also maintain the delay between disabling and > > + * checking ready status as used by the native NVMe driver. > > + */ > > +static int reset_nvme(struct pci_dev *dev, int probe) > > +{ > > + const struct pci_device_id *id; > > + void __iomem *bar; > > + u16 cmd; > > + u32 cfg; > > + > > + id = pci_match_id(nvme_reset_tbl, dev); > > + if (!id || !pcie_has_flr(dev) || !pci_resource_start(dev, 0)) > > + return -ENOTTY; > > + > > + if (probe) > > + return 0; > > + > > + bar = pci_iomap(dev, 0, NVME_REG_CC + sizeof(cfg)); > > + if (!bar) > > + return -ENOTTY; > > + > > + pci_read_config_word(dev, PCI_COMMAND, &cmd); > > + pci_write_config_word(dev, PCI_COMMAND, cmd | PCI_COMMAND_MEMORY); > > + > > + cfg = readl(bar + NVME_REG_CC); > > Apparently this is part of some NVMe spec and all controllers support > this? Is there a public reference you could cite for the details? Yep, I'll add a link in the comments, it's all public. Thanks, Alex > > + > > + /* Disable controller if enabled */ > > + if (cfg & NVME_CC_ENABLE) { > > + u64 cap = readq(bar + NVME_REG_CAP); > > + unsigned long timeout; > > + > > + /* > > + * Per nvme_disable_ctrl() skip shutdown notification as it > > + * could complete commands to the admin queue. We only intend > > + * to quiesce the device before reset. > > + */ > > + cfg &= ~(NVME_CC_SHN_MASK | NVME_CC_ENABLE); > > + > > + writel(cfg, bar + NVME_REG_CC); > > + > > + /* A heuristic value, matches NVME_QUIRK_DELAY_AMOUNT */ > > + if (id->driver_data & NVME_QUIRK_CHK_RDY_DELAY) > > + msleep(2300); > > + > > + /* Cap register provides max timeout in 500ms increments */ > > + timeout = ((NVME_CAP_TIMEOUT(cap) + 1) * HZ / 2) + jiffies; > > + > > + for (;;) { > > + u32 status = readl(bar + NVME_REG_CSTS); > > + > > + /* Ready status becomes zero on disable complete */ > > + if (!(status & NVME_CSTS_RDY)) > > + break; > > + > > + msleep(100); > > + > > + if (time_after(jiffies, timeout)) { > > + pci_warn(dev, "Timeout waiting for NVMe ready status to clear after disable\n"); > > + break; > > + } > > + } > > + } > > + > > + pci_iounmap(dev, bar); > > + > > + /* > > + * We could use the optional NVM Subsystem Reset here, hardware > > + * supporting this is simply unavailable at the time of this code > > + * to validate in comparison to PCIe FLR. NVMe spec dictates that > > + * NVMe devices shall implement PCIe FLR. > > + */ > > + pcie_flr(dev); > > + > > + if (id->driver_data & NVME_QUIRK_POST_FLR_DELAY) > > + msleep(250); /* Heuristic based on Intel DC P3700 */ > > + > > + return 0; > > +} > > + > > static const struct pci_dev_reset_methods pci_dev_reset_methods[] = { > > { PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF, > > reset_intel_82599_sfp_virtfn }, > > @@ -3678,6 +3789,7 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = { > > reset_ivb_igd }, > > { PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID, > > reset_chelsio_generic_dev }, > > + { PCI_ANY_ID, PCI_ANY_ID, reset_nvme }, > > { 0 } > > }; > > > > > > > > _______________________________________________ > > Linux-nvme mailing list > > Linux-nvme@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-07-24 0:11 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-07-23 22:24 [PATCH 0/2] PCI: NVMe reset quirk Alex Williamson 2018-07-23 22:24 ` [PATCH 1/2] PCI: Export pcie_has_flr() Alex Williamson 2018-07-23 22:24 ` [PATCH 2/2] PCI: NVMe device specific reset quirk Alex Williamson 2018-07-23 22:45 ` Keith Busch 2018-07-23 23:08 ` Alex Williamson 2018-07-23 22:45 ` Bjorn Helgaas 2018-07-24 0:11 ` Alex Williamson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).