From mboxrd@z Thu Jan 1 00:00:00 1970 From: minwoo.im.dev@gmail.com (Minwoo Im) Date: Wed, 25 Jul 2018 04:53:18 +0900 Subject: [PATCH v3 2/3] PCI: Samsung SM961/PM961 NVMe disable before FLR quirk In-Reply-To: <20180724161440.2729.89835.stgit@gimli.home> References: <20180724160440.2729.75178.stgit@gimli.home> <20180724161440.2729.89835.stgit@gimli.home> Message-ID: <1532461998.20066.5.camel@gmail.com> Hi Alex, On Tue, 2018-07-24@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 > --- > ?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 > ?#include > ?#include > +#include > ?#include /* 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 at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-nvme From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Message-ID: <1532461998.20066.5.camel@gmail.com> Subject: Re: [PATCH v3 2/3] PCI: Samsung SM961/PM961 NVMe disable before FLR quirk From: Minwoo Im To: Alex Williamson , linux-pci@vger.kernel.org Date: Wed, 25 Jul 2018 04:53:18 +0900 In-Reply-To: <20180724161440.2729.89835.stgit@gimli.home> References: <20180724160440.2729.75178.stgit@gimli.home> <20180724161440.2729.89835.stgit@gimli.home> Mime-Version: 1.0 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org Content-Type: text/plain; charset="utf-8" Sender: "Linux-nvme" Errors-To: linux-nvme-bounces+bjorn=helgaas.com@lists.infradead.org List-ID: SGkgQWxleCwKCk9uIFR1ZSwgMjAxOC0wNy0yNCBhdCAxMDoxNCAtMDYwMCwgQWxleCBXaWxsaWFt c29uIHdyb3RlOgo+IFRoZSBTYW1zdW5nIFNNOTYxL1BNOTYxICg5NjAgRVZPKSBzb21ldGltZXMg ZmFpbHMgdG8gcmV0dXJuIGZyb20gRkxSCj4gd2l0aCB0aGUgUENJIGNvbmZpZyBzcGFjZSByZWFk aW5nIGJhY2sgYXMgLTEuwqDCoEEgcmVwcm9kdWNpYmxlIGluc3RhbmNlCj4gb2YgdGhpcyBiZWhh dmlvciBpcyByZXNvbHZlZCBieSBjbGVhcmluZyB0aGUgZW5hYmxlIGJpdCBpbiB0aGUgTlZNZQo+ IGNvbmZpZ3VyYXRpb24gcmVnaXN0ZXIgYW5kIHdhaXRpbmcgZm9yIHRoZSByZWFkeSBzdGF0dXMg dG8gY2xlYXIKPiAoZGlzYWJsaW5nIHRoZSBOVk1lIGNvbnRyb2xsZXIpIHByaW9yIHRvIEZMUi4K PiAKPiBTaWduZWQtb2ZmLWJ5OiBBbGV4IFdpbGxpYW1zb24gPGFsZXgud2lsbGlhbXNvbkByZWRo YXQuY29tPgo+IC0tLQo+IMKgZHJpdmVycy9wY2kvcXVpcmtzLmMgfMKgwqDCoDgzCj4gKysrKysr KysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysKPiDCoDEgZmlsZSBj aGFuZ2VkLCA4MyBpbnNlcnRpb25zKCspCj4gCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvcGNpL3F1 aXJrcy5jIGIvZHJpdmVycy9wY2kvcXVpcmtzLmMKPiBpbmRleCBlNzJjODc0MmFhZmEuLjM4OTlj ZGQyNTE0YiAxMDA2NDQKPiAtLS0gYS9kcml2ZXJzL3BjaS9xdWlya3MuYwo+ICsrKyBiL2RyaXZl cnMvcGNpL3F1aXJrcy5jCj4gQEAgLTI4LDYgKzI4LDcgQEAKPiDCoCNpbmNsdWRlIDxsaW51eC9w bGF0Zm9ybV9kYXRhL3g4Ni9hcHBsZS5oPgo+IMKgI2luY2x1ZGUgPGxpbnV4L3BtX3J1bnRpbWUu aD4KPiDCoCNpbmNsdWRlIDxsaW51eC9zd2l0Y2h0ZWMuaD4KPiArI2luY2x1ZGUgPGxpbnV4L252 bWUuaD4KPiDCoCNpbmNsdWRlIDxhc20vZG1hLmg+CS8qIGlzYV9kbWFfYnJpZGdlX2J1Z2d5ICov Cj4gwqAjaW5jbHVkZSAicGNpLmgiCj4gwqAKPiBAQCAtMzY2OSw2ICszNjcwLDg3IEBAIHN0YXRp YyBpbnQgcmVzZXRfY2hlbHNpb19nZW5lcmljX2RldihzdHJ1Y3QgcGNpX2Rldgo+ICpkZXYsIGlu dCBwcm9iZSkKPiDCoCNkZWZpbmUgUENJX0RFVklDRV9JRF9JTlRFTF9JVkJfTV9WR0HCoMKgwqDC oMKgwqAweDAxNTYKPiDCoCNkZWZpbmUgUENJX0RFVklDRV9JRF9JTlRFTF9JVkJfTTJfVkdBwqDC oMKgwqDCoDB4MDE2Ngo+IMKgCj4gKy8qCj4gKyAqIFRoZSBTYW1zdW5nIFNNOTYxL1BNOTYxIGNv bnRyb2xsZXIgY2FuIHNvbWV0aW1lcyBlbnRlciBhIGZhdGFsIHN0YXRlIGFmdGVyCj4gKyAqIEZM UiB3aGVyZSBjb25maWcgc3BhY2UgcmVhZHMgZnJvbSB0aGUgZGV2aWNlIHJldHVybiAtMS7CoMKg V2Ugc2VlbSB0byBiZQo+ICsgKiBhYmxlIHRvIGF2b2lkIHRoaXMgY29uZGl0aW9uIGlmIHdlIGRp c2FibGUgdGhlIE5WTWUgY29udHJvbGxlciBwcmlvciB0bwo+ICsgKiBGTFIuwqDCoFRoaXMgcXVp cmsgaXMgZ2VuZXJpYyBmb3IgYW55IE5WTWUgY2xhc3MgZGV2aWNlIHJlcXVpcmluZyBzaW1pbGFy Cj4gKyAqIGFzc2lzdGFuY2UgdG8gcXVpZXNjZSB0aGUgZGV2aWNlIHByaW9yIHRvIEZMUi4KPiAr ICoKPiArICogTlZNZSBzcGVjaWZpY2F0aW9uOiBodHRwczovL252bWV4cHJlc3Mub3JnL3Jlc291 cmNlcy9zcGVjaWZpY2F0aW9ucy8KPiArICogUmV2aXNpb24gMS4wZToKCkl0IHNlZW1zIHRvbyBv bGQgdmVyc2lvbiBvZiBOVk1lIHNwZWNpZmljYXRpb24uIMKgRG8geW91IGhhdmUgYW55IHNwZWNp YWwgcmVhc29uCnRvIGNvbW1lbnQgdGhlIHNwZWNpZmllZCAxLjAgdmVyc2lvbiBpbnN0ZWFkIG9m IDEuMyBvciBzb21ldGhpbmcgbmV3ZXI/Cgo+ICsgKsKgwqDCoMKgQ2hhcHRlciAyOiBSZXF1aXJl ZCBhbmQgb3B0aW9uYWwgUENJIGNvbmZpZyByZWdpc3RlcnMKPiArICrCoMKgwqDCoENoYXB0ZXIg MzogTlZNZSBjb250cm9sIHJlZ2lzdGVycwo+ICsgKsKgwqDCoMKgQ2hhcHRlciA3LjM6IFJlc2V0 IGJlaGF2aW9yCj4gKyAqLwo+ICtzdGF0aWMgaW50IG52bWVfZGlzYWJsZV9hbmRfZmxyKHN0cnVj dCBwY2lfZGV2ICpkZXYsIGludCBwcm9iZSkKClRoZSBuYW1lIG9mIHRoaXMgZnVuY3Rpb24gc2Vl bXMgYWJsZSB0byBiZSBzdGFydGVkIHdpdGggJ3Jlc2V0XycgcHJlZml4IGp1c3QKbGlrZSBvdGhl ciBxdWlya3MgZm9yIHJlc2V0LgpXaGF0IGFib3V0IHJlc2V0X3NhbXN1bmdfcG05NjEgb3Igc29t ZXRoaW5nPwoKPiArewo+ICsJdm9pZCBfX2lvbWVtICpiYXI7Cj4gKwl1MTYgY21kOwo+ICsJdTMy IGNmZzsKPiArCj4gKwlpZiAoZGV2LT5jbGFzcyAhPSBQQ0lfQ0xBU1NfU1RPUkFHRV9FWFBSRVNT IHx8Cj4gKwnCoMKgwqDCoCFwY2llX2hhc19mbHIoZGV2KSB8fCAhcGNpX3Jlc291cmNlX3N0YXJ0 KGRldiwgMCkpCj4gKwkJcmV0dXJuIC1FTk9UVFk7Cj4gKwo+ICsJaWYgKHByb2JlKQo+ICsJCXJl dHVybiAwOwo+ICsKPiArCWJhciA9IHBjaV9pb21hcChkZXYsIDAsIE5WTUVfUkVHX0NDICsgc2l6 ZW9mKGNmZykpOwo+ICsJaWYgKCFiYXIpCj4gKwkJcmV0dXJuIC1FTk9UVFk7Cj4gKwo+ICsJcGNp X3JlYWRfY29uZmlnX3dvcmQoZGV2LCBQQ0lfQ09NTUFORCwgJmNtZCk7Cj4gKwlwY2lfd3JpdGVf Y29uZmlnX3dvcmQoZGV2LCBQQ0lfQ09NTUFORCwgY21kIHwgUENJX0NPTU1BTkRfTUVNT1JZKTsK PiArCj4gKwljZmcgPSByZWFkbChiYXIgKyBOVk1FX1JFR19DQyk7Cj4gKwo+ICsJLyogRGlzYWJs ZSBjb250cm9sbGVyIGlmIGVuYWJsZWQgKi8KPiArCWlmIChjZmcgJiBOVk1FX0NDX0VOQUJMRSkg ewo+ICsJCXU2NCBjYXAgPSByZWFkcShiYXIgKyBOVk1FX1JFR19DQVApOwo+ICsJCXVuc2lnbmVk IGxvbmcgdGltZW91dDsKPiArCj4gKwkJLyoKPiArCQnCoCogUGVyIG52bWVfZGlzYWJsZV9jdHJs KCkgc2tpcCBzaHV0ZG93biBub3RpZmljYXRpb24gYXMgaXQKPiArCQnCoCogY291bGQgY29tcGxl dGUgY29tbWFuZHMgdG8gdGhlIGFkbWluIHF1ZXVlLsKgwqBXZSBvbmx5Cj4gaW50ZW5kCj4gKwkJ wqAqIHRvIHF1aWVzY2UgdGhlIGRldmljZSBiZWZvcmUgcmVzZXQuCj4gKwkJwqAqLwo+ICsJCWNm ZyAmPSB+KE5WTUVfQ0NfU0hOX01BU0sgfCBOVk1FX0NDX0VOQUJMRSk7Cj4gKwo+ICsJCXdyaXRl bChjZmcsIGJhciArIE5WTUVfUkVHX0NDKTsKPiArCj4gKwkJLyoKPiArCQnCoCogU29tZSBjb250 cm9sbGVycyByZXF1aXJlIGFuIGFkZGl0aW9uYWwgZGVsYXkgaGVyZSwgc2VlCj4gKwkJwqAqIE5W TUVfUVVJUktfREVMQVlfQkVGT1JFX0NIS19SRFkuwqDCoE5vbmUgb2YgdGhvc2UgYXJlIHlldAo+ ICsJCcKgKiBzdXBwb3J0ZWQgYnkgdGhpcyBxdWlyay4KPiArCQnCoCovCj4gKwo+ICsJCS8qIENh cCByZWdpc3RlciBwcm92aWRlcyBtYXggdGltZW91dCBpbiA1MDBtcyBpbmNyZW1lbnRzICovCj4g KwkJdGltZW91dCA9ICgoTlZNRV9DQVBfVElNRU9VVChjYXApICsgMSkgKiBIWiAvIDIpICsgamlm ZmllczsKPiArCj4gKwkJZm9yICg7Oykgewo+ICsJCQl1MzIgc3RhdHVzID0gcmVhZGwoYmFyICsg TlZNRV9SRUdfQ1NUUyk7Cj4gKwo+ICsJCQkvKiBSZWFkeSBzdGF0dXMgYmVjb21lcyB6ZXJvIG9u IGRpc2FibGUgY29tcGxldGUgKi8KPiArCQkJaWYgKCEoc3RhdHVzICYgTlZNRV9DU1RTX1JEWSkp Cj4gKwkJCQlicmVhazsKPiArCj4gKwkJCW1zbGVlcCgxMDApOwo+ICsKPiArCQkJaWYgKHRpbWVf YWZ0ZXIoamlmZmllcywgdGltZW91dCkpIHsKPiArCQkJCXBjaV93YXJuKGRldiwgIlRpbWVvdXQg d2FpdGluZyBmb3IgTlZNZSByZWFkeQo+IHN0YXR1cyB0byBjbGVhciBhZnRlciBkaXNhYmxlXG4i KTsKPiArCQkJCWJyZWFrOwo+ICsJCQl9Cj4gKwkJfQo+ICsJfQo+ICsKPiArCXBjaV9pb3VubWFw KGRldiwgYmFyKTsKPiArCj4gKwlwY2llX2ZscihkZXYpOwo+ICsKPiArCXJldHVybiAwOwo+ICt9 Cj4gKwo+IMKgc3RhdGljIGNvbnN0IHN0cnVjdCBwY2lfZGV2X3Jlc2V0X21ldGhvZHMgcGNpX2Rl dl9yZXNldF9tZXRob2RzW10gPSB7Cj4gwqAJeyBQQ0lfVkVORE9SX0lEX0lOVEVMLCBQQ0lfREVW SUNFX0lEX0lOVEVMXzgyNTk5X1NGUF9WRiwKPiDCoAkJwqByZXNldF9pbnRlbF84MjU5OV9zZnBf dmlydGZuIH0sCj4gQEAgLTM2NzYsNiArMzc1OCw3IEBAIHN0YXRpYyBjb25zdCBzdHJ1Y3QgcGNp X2Rldl9yZXNldF9tZXRob2RzCj4gcGNpX2Rldl9yZXNldF9tZXRob2RzW10gPSB7Cj4gwqAJCXJl c2V0X2l2Yl9pZ2QgfSwKPiDCoAl7IFBDSV9WRU5ET1JfSURfSU5URUwsIFBDSV9ERVZJQ0VfSURf SU5URUxfSVZCX00yX1ZHQSwKPiDCoAkJcmVzZXRfaXZiX2lnZCB9LAo+ICsJeyBQQ0lfVkVORE9S X0lEX1NBTVNVTkcsIDB4YTgwNCwgbnZtZV9kaXNhYmxlX2FuZF9mbHIgfSwKCldoeSBkb24ndCB3 ZSBqdXN0IGRlZmluZSBhIG1hY3JvIGp1c3QgbGlrZSBvdGhlciBERVZJQ0VfSURzLiAoZS5nLgpQ Q0lFX0RFVklDRV9JRF9JTlRFTF84MjU5OV9TRlBfVkYpLgoKI2RlZmluZSBQQ0lfREVWSUNFX0lE X1NBTVNVTkdfUE05NjEgwqAweGE4MDQKCj4gwqAJeyBQQ0lfVkVORE9SX0lEX0NIRUxTSU8sIFBD SV9BTllfSUQsCj4gwqAJCXJlc2V0X2NoZWxzaW9fZ2VuZXJpY19kZXYgfSwKPiDCoAl7IDAgfQo+ IAo+IAo+IF9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCj4g TGludXgtbnZtZSBtYWlsaW5nIGxpc3QKPiBMaW51eC1udm1lQGxpc3RzLmluZnJhZGVhZC5vcmcK PiBodHRwOi8vbGlzdHMuaW5mcmFkZWFkLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2xpbnV4LW52bWUK Cl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCkxpbnV4LW52 bWUgbWFpbGluZyBsaXN0CkxpbnV4LW52bWVAbGlzdHMuaW5mcmFkZWFkLm9yZwpodHRwOi8vbGlz dHMuaW5mcmFkZWFkLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2xpbnV4LW52bWUK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1AE43C6778F for ; Tue, 24 Jul 2018 19:53:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id BFE2420874 for ; Tue, 24 Jul 2018 19:53:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="T+5gY9EK" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BFE2420874 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388733AbeGXVB0 (ORCPT ); Tue, 24 Jul 2018 17:01:26 -0400 Received: from mail-pl0-f67.google.com ([209.85.160.67]:46673 "EHLO mail-pl0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2388587AbeGXVB0 (ORCPT ); Tue, 24 Jul 2018 17:01:26 -0400 Received: by mail-pl0-f67.google.com with SMTP id t17-v6so2214197ply.13; Tue, 24 Jul 2018 12:53:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=message-id:subject:from:to:cc:date:in-reply-to:references :mime-version:content-transfer-encoding; bh=cLhvLS5EoQ8CaU1yIPsPgI0PXjzuWGZnxf3ed/tRZVA=; b=T+5gY9EKulmN3i+sM+w8CxeDGats7tAVqz990bW6co44aGJnAHAwngZqgco0H2HDuV Hsp15fdbKv48/Sv1vR/9Y4ZzLluwRmkjfZ0neo5R/tCWXaGwARW9vTIAP4f61zF9cvfr +ZoBU4QhjEA/6glCU02bH0R1vLPOTlYGmCkpB6/2gLUR7kAOLYMFfqN3RUSnmlqF3yEv UDNjWQOF8GfY1JQ5ii8WT3Y99RhF5zmmYLse1Ebyc0z8C+g1HJWBMYk6MFAAXQ/TaeA4 DOGzMw//Lc6Dhf5JVVtySRL96cJ/1gbKZLpSh5wL5YjcsmtC+LmXq5YpbPOloJlxJcQX 1Tdg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:mime-version:content-transfer-encoding; bh=cLhvLS5EoQ8CaU1yIPsPgI0PXjzuWGZnxf3ed/tRZVA=; b=P/w8L4c3JH4IQxDOG7q8fXLgKJTvylaZxdkTkwiEW3F/3/WJx7WoJBoBq+XzYErYgO Hvxaq0KkopFHJ5ledRi8OfhPmpJNYG3/b9jps7CTZ7bPkx9thfI3JkeV71RSU9ESWpy3 YFQMJY4mB48FqTIeaa7xA0msez4Fu5jb0RCKqbQWjy/0bN4Bc4NT+PBsrBjH1vFxZD4s FSM1A7p7KEKk7yTmlFyIgYYYeaFE/pmxrew7GOQF/50A6sS6sS7tfjlER+BiHa17zJ3w ffxLgHc1L04iTkGCCE+dAhb2bp4CDaoZ6XqePINKi8w4RIxmv76BbLJLK11fduImYwiq VNAA== X-Gm-Message-State: AOUpUlFJmxQHUgQOUfPdDmhDhuuEp4+zTpIDaWsGeIS1HOLuWSJxodPM z0hOXFgOaDJkTiagfgTEMCs= X-Google-Smtp-Source: AAOMgpeQtDUvvce9BP4YXvS06qQbYur6XHj6RknWPGeINwEMP1jwdOvWl9ZpwVgehwqpMgyfG/9haQ== X-Received: by 2002:a17:902:d807:: with SMTP id a7-v6mr18097775plz.214.1532462003832; Tue, 24 Jul 2018 12:53:23 -0700 (PDT) Received: from minwooim-desktop ([1.237.242.62]) by smtp.googlemail.com with ESMTPSA id b62-v6sm34562141pfm.97.2018.07.24.12.53.20 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 24 Jul 2018 12:53:22 -0700 (PDT) Message-ID: <1532461998.20066.5.camel@gmail.com> Subject: Re: [PATCH v3 2/3] PCI: Samsung SM961/PM961 NVMe disable before FLR quirk From: Minwoo Im To: Alex Williamson , linux-pci@vger.kernel.org Cc: linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org Date: Wed, 25 Jul 2018 04:53:18 +0900 In-Reply-To: <20180724161440.2729.89835.stgit@gimli.home> References: <20180724160440.2729.75178.stgit@gimli.home> <20180724161440.2729.89835.stgit@gimli.home> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.5.2-0ubuntu3.2 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- >  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 >  #include >  #include > +#include >  #include /* 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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-nvme