Re: [PATCH v3 2/3] PCI: Samsung SM961/PM961 NVMe disable before FLR quirk

[Minwoo Im]

Hi Alex,

On Tue, 2018-07-24 at 10:14 -0600, Alex Williamson wrote:
> 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 NVMe
> configuration register and waiting for the ready status to clear
> (disabling the NVMe controller) prior to FLR.
> 
> Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> ---
> Âdrivers/pci/quirks.c |ÂÂÂ83
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> Â1 file changed, 83 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index e72c8742aafa..3899cdd2514b 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,87 @@ 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
> Â
> +/*
> + * The Samsung SM961/PM961 controller can sometimes enter a fatal state after
> + * FLR where config space reads from the device return -1.ÂÂWe seem to be
> + * able to avoid this condition if we disable the NVMe controller prior to
> + * FLR.ÂÂThis quirk is generic for any NVMe class device requiring similar
> + * assistance to quiesce the device prior to FLR.
> + *
> + * NVMe specification: https://nvmexpress.org/resources/specifications/
> + * Revision 1.0e:

It seems too old version of NVMe specification. ÂDo you have any special reason
to comment the specified 1.0 version instead of 1.3 or something newer?

> + *ÂÂÂÂChapter 2: Required and optional PCI config registers
> + *ÂÂÂÂChapter 3: NVMe control registers
> + *ÂÂÂÂChapter 7.3: Reset behavior
> + */
> +static int nvme_disable_and_flr(struct pci_dev *dev, int probe)

The name of this function seems able to be started with 'reset_' prefix just
like other quirks for reset.
What about reset_samsung_pm961 or something?

> +{
> +	void __iomem *bar;
> +	u16 cmd;
> +	u32 cfg;
> +
> +	if (dev->class != PCI_CLASS_STORAGE_EXPRESS ||
> +	ÂÂÂÂ!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);
> +
> +		/*
> +		Â* Some controllers require an additional delay here, see
> +		Â* NVME_QUIRK_DELAY_BEFORE_CHK_RDY.ÂÂNone of those are yet
> +		Â* supported by this quirk.
> +		Â*/
> +
> +		/* 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);
> +
> +	pcie_flr(dev);
> +
> +	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 },
> @@ -3676,6 +3758,7 @@ static const struct pci_dev_reset_methods
> pci_dev_reset_methods[] = {
> Â		reset_ivb_igd },
> Â	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_IVB_M2_VGA,
> Â		reset_ivb_igd },
> +	{ PCI_VENDOR_ID_SAMSUNG, 0xa804, nvme_disable_and_flr },

Why don't we just define a macro just like other DEVICE_IDs. (e.g.
PCIE_DEVICE_ID_INTEL_82599_SFP_VF).

#define PCI_DEVICE_ID_SAMSUNG_PM961 Â0xa804

> Â	{ PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
> Â		reset_chelsio_generic_dev },
> Â	{ 0 }
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-nvme