* [PATCH] nvme: Namepace identification descriptor list is optional @ 2019-12-02 15:56 Keith Busch 2019-12-02 16:15 ` Christoph Hellwig 2020-07-10 8:22 ` Ingo Brunberg 0 siblings, 2 replies; 18+ messages in thread From: Keith Busch @ 2019-12-02 15:56 UTC (permalink / raw) To: linux-nvme; +Cc: Keith Busch, Ingo Brunberg, hch, stable, Sagi Grimberg Despite NVM Express specification 1.3 requires a controller claiming to be 1.3 or higher implement Identify CNS 03h (Namespace Identification Descriptor list), the driver doesn't really need this identification in order to use a namespace. The code had already documented in comments that we're not to consider an error to this command. Return success if the controller provided any response to an namespace identification descriptors command. Fixes: 538af88ea7d9de24 ("nvme: make nvme_report_ns_ids propagate error back") Reported-by: Ingo Brunberg <ingo_brunberg@web.de> Cc: Sagi Grimberg <sagi@grimberg.me> Cc: stable@vger.kernel.org # 5.4+ Signed-off-by: Keith Busch <kbusch@kernel.org> --- drivers/nvme/host/core.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index e6ee34376c5e..2a84e1402244 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1735,6 +1735,8 @@ static int nvme_report_ns_ids(struct nvme_ctrl *ctrl, unsigned int nsid, if (ret) dev_warn(ctrl->device, "Identify Descriptors failed (%d)\n", ret); + if (ret > 0) + ret = 0; } return ret; } -- 2.21.0 _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: Namepace identification descriptor list is optional 2019-12-02 15:56 [PATCH] nvme: Namepace identification descriptor list is optional Keith Busch @ 2019-12-02 16:15 ` Christoph Hellwig 2019-12-02 16:22 ` Keith Busch 2020-07-10 8:22 ` Ingo Brunberg 1 sibling, 1 reply; 18+ messages in thread From: Christoph Hellwig @ 2019-12-02 16:15 UTC (permalink / raw) To: Keith Busch; +Cc: stable, Ingo Brunberg, hch, linux-nvme, Sagi Grimberg On Tue, Dec 03, 2019 at 12:56:11AM +0900, Keith Busch wrote: > Despite NVM Express specification 1.3 requires a controller claiming to > be 1.3 or higher implement Identify CNS 03h (Namespace Identification > Descriptor list), the driver doesn't really need this identification in > order to use a namespace. The code had already documented in comments > that we're not to consider an error to this command. > > Return success if the controller provided any response to an > namespace identification descriptors command. > > Fixes: 538af88ea7d9de24 ("nvme: make nvme_report_ns_ids propagate error back") > Reported-by: Ingo Brunberg <ingo_brunberg@web.de> Why would we ignore the error? Do you have a buggy controller messing this up? _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: Namepace identification descriptor list is optional 2019-12-02 16:15 ` Christoph Hellwig @ 2019-12-02 16:22 ` Keith Busch 2019-12-02 16:27 ` Nadolski, Edmund 0 siblings, 1 reply; 18+ messages in thread From: Keith Busch @ 2019-12-02 16:22 UTC (permalink / raw) To: Christoph Hellwig; +Cc: stable, Ingo Brunberg, Sagi Grimberg, linux-nvme On Mon, Dec 02, 2019 at 05:15:45PM +0100, Christoph Hellwig wrote: > On Tue, Dec 03, 2019 at 12:56:11AM +0900, Keith Busch wrote: > > Despite NVM Express specification 1.3 requires a controller claiming to > > be 1.3 or higher implement Identify CNS 03h (Namespace Identification > > Descriptor list), the driver doesn't really need this identification in > > order to use a namespace. The code had already documented in comments > > that we're not to consider an error to this command. > > > > Return success if the controller provided any response to an > > namespace identification descriptors command. > > > > Fixes: 538af88ea7d9de24 ("nvme: make nvme_report_ns_ids propagate error back") > > Reported-by: Ingo Brunberg <ingo_brunberg@web.de> > > Why would we ignore the error? Do you have a buggy controller messing > this up? I don't have such a controller, but many apparently do. The regression was reported here: http://lists.infradead.org/pipermail/linux-nvme/2019-December/028223.html And of course it's the SMI controller ... _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: Namepace identification descriptor list is optional 2019-12-02 16:22 ` Keith Busch @ 2019-12-02 16:27 ` Nadolski, Edmund 2019-12-02 16:29 ` Christoph Hellwig 0 siblings, 1 reply; 18+ messages in thread From: Nadolski, Edmund @ 2019-12-02 16:27 UTC (permalink / raw) To: Keith Busch, Christoph Hellwig Cc: Ingo Brunberg, linux-nvme, Sagi Grimberg, stable On 12/2/2019 9:22 AM, Keith Busch wrote: > On Mon, Dec 02, 2019 at 05:15:45PM +0100, Christoph Hellwig wrote: >> On Tue, Dec 03, 2019 at 12:56:11AM +0900, Keith Busch wrote: >> > Despite NVM Express specification 1.3 requires a controller claiming to >> > be 1.3 or higher implement Identify CNS 03h (Namespace Identification >> > Descriptor list), the driver doesn't really need this identification in >> > order to use a namespace. The code had already documented in comments >> > that we're not to consider an error to this command. >> > >> > Return success if the controller provided any response to an >> > namespace identification descriptors command. >> > >> > Fixes: 538af88ea7d9de24 ("nvme: make nvme_report_ns_ids propagate error back") >> > Reported-by: Ingo Brunberg <ingo_brunberg@web.de> >> >> Why would we ignore the error? Do you have a buggy controller messing >> this up? > > I don't have such a controller, but many apparently do. The regression > was reported here: > > http://lists.infradead.org/pipermail/linux-nvme/2019-December/028223.html > > And of course it's the SMI controller ... Does 5.4 show the exact error code? Perhaps we should selectively allow just for that case? Ed _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: Namepace identification descriptor list is optional 2019-12-02 16:27 ` Nadolski, Edmund @ 2019-12-02 16:29 ` Christoph Hellwig 2019-12-02 16:45 ` Nadolski, Edmund 2019-12-02 16:49 ` Keith Busch 0 siblings, 2 replies; 18+ messages in thread From: Christoph Hellwig @ 2019-12-02 16:29 UTC (permalink / raw) To: Nadolski, Edmund Cc: Sagi Grimberg, linux-nvme, Ingo Brunberg, stable, Keith Busch, Christoph Hellwig On Mon, Dec 02, 2019 at 09:27:14AM -0700, Nadolski, Edmund wrote: >> I don't have such a controller, but many apparently do. The regression >> was reported here: >> >> http://lists.infradead.org/pipermail/linux-nvme/2019-December/028223.html >> >> And of course it's the SMI controller ... > > Does 5.4 show the exact error code? Perhaps we should selectively allow > just for that case? They'll find other ways to f***ck up. Looks like at least the controller in the bug report also doesn't have an subnqn and the nguid/eui64 are bogus. I wonder if we actually do users a favour by allowing that.. _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: Namepace identification descriptor list is optional 2019-12-02 16:29 ` Christoph Hellwig @ 2019-12-02 16:45 ` Nadolski, Edmund 2019-12-02 16:49 ` Keith Busch 1 sibling, 0 replies; 18+ messages in thread From: Nadolski, Edmund @ 2019-12-02 16:45 UTC (permalink / raw) To: Christoph Hellwig, Nadolski, Edmund Cc: Keith Busch, stable, Ingo Brunberg, Sagi Grimberg, linux-nvme On 12/2/2019 9:29 AM, Christoph Hellwig wrote: > On Mon, Dec 02, 2019 at 09:27:14AM -0700, Nadolski, Edmund wrote: >>> I don't have such a controller, but many apparently do. The regression >>> was reported here: >>> >>> http://lists.infradead.org/pipermail/linux-nvme/2019-December/028223.html >>> >>> And of course it's the SMI controller ... >> >> Does 5.4 show the exact error code? Perhaps we should selectively allow >> just for that case? > > They'll find other ways to f***ck up. Looks like at least the controller > in the bug report also doesn't have an subnqn and the nguid/eui64 are > bogus. I wonder if we actually do users a favour by allowing that.. Agreed, tho since it looks like a regression, does it make sense to treat as a quirk? Ed _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: Namepace identification descriptor list is optional 2019-12-02 16:29 ` Christoph Hellwig 2019-12-02 16:45 ` Nadolski, Edmund @ 2019-12-02 16:49 ` Keith Busch 2019-12-02 17:34 ` Christoph Hellwig 1 sibling, 1 reply; 18+ messages in thread From: Keith Busch @ 2019-12-02 16:49 UTC (permalink / raw) To: Christoph Hellwig Cc: Ingo Brunberg, linux-nvme, Nadolski, Edmund, Sagi Grimberg, stable On Mon, Dec 02, 2019 at 05:29:05PM +0100, Christoph Hellwig wrote: > On Mon, Dec 02, 2019 at 09:27:14AM -0700, Nadolski, Edmund wrote: > >> I don't have such a controller, but many apparently do. The regression > >> was reported here: > >> > >> http://lists.infradead.org/pipermail/linux-nvme/2019-December/028223.html > >> > >> And of course it's the SMI controller ... > > > > Does 5.4 show the exact error code? Perhaps we should selectively allow > > just for that case? > > They'll find other ways to f***ck up. Looks like at least the controller > in the bug report also doesn't have an subnqn and the nguid/eui64 are > bogus. Indeed, they will find creative ways to break it. Customer or OEM requirments are poorly written, like "Must report NVMe version 1.3". Nobody bothers to mention that it must also be compliant to that version, or even realize they never cared for those features in the first place. Compliance testing like from UNH should have caught this before shipping with such a device, but it's a cheap device, so maybe they skip that step. > I wonder if we actually do users a favour by allowing that.. I think it's too late now. We did successfully use such namespaces before 5.4, even if they're fundamentally broken. Johannes also commented *not* to consider these errors when this identification was originally implemented, so either he knew vendors screwed this up, or had the forethought to know they would. _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: Namepace identification descriptor list is optional 2019-12-02 16:49 ` Keith Busch @ 2019-12-02 17:34 ` Christoph Hellwig 2019-12-02 21:21 ` Keith Busch 0 siblings, 1 reply; 18+ messages in thread From: Christoph Hellwig @ 2019-12-02 17:34 UTC (permalink / raw) To: Keith Busch Cc: Sagi Grimberg, linux-nvme, Ingo Brunberg, Nadolski, Edmund, stable, Christoph Hellwig On Tue, Dec 03, 2019 at 01:49:03AM +0900, Keith Busch wrote: > Customer or OEM requirments are poorly written, like "Must report NVMe > version 1.3". Nobody bothers to mention that it must also be compliant > to that version, or even realize they never cared for those features in > the first place. > > Compliance testing like from UNH should have caught this before shipping > with such a device, but it's a cheap device, so maybe they skip that step. > > > I wonder if we actually do users a favour by allowing that.. > > I think it's too late now. We did successfully use such namespaces > before 5.4, even if they're fundamentally broken. > > Johannes also commented *not* to consider these errors when this > identification was originally implemented, so either he knew vendors > screwed this up, or had the forethought to know they would. Yes. I guess your patch is the best thing for now: Reviewed-by: Christoph Hellwig <hch@lst.de> But I think we might need a new kernel tain flag or something like it for devices that are so obviously broken in their identifiers. _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: Namepace identification descriptor list is optional 2019-12-02 17:34 ` Christoph Hellwig @ 2019-12-02 21:21 ` Keith Busch 0 siblings, 0 replies; 18+ messages in thread From: Keith Busch @ 2019-12-02 21:21 UTC (permalink / raw) To: Christoph Hellwig Cc: Ingo Brunberg, linux-nvme, Nadolski, Edmund, Sagi Grimberg, stable On Mon, Dec 02, 2019 at 06:34:14PM +0100, Christoph Hellwig wrote: > Yes. I guess your patch is the best thing for now: > > Reviewed-by: Christoph Hellwig <hch@lst.de> Thanks, applied for-5.5 > But I think we might need a new kernel tain flag or something like > it for devices that are so obviously broken in their identifiers. That's fine with me. We currently just log a warning when an error is returned here, we can add_taint() too. Which flag do you think? TAINT_FIRMWARE_WORKAROUND, TAINT_CRAP, or something else? _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: Namepace identification descriptor list is optional 2019-12-02 15:56 [PATCH] nvme: Namepace identification descriptor list is optional Keith Busch 2019-12-02 16:15 ` Christoph Hellwig @ 2020-07-10 8:22 ` Ingo Brunberg 2020-07-22 23:23 ` Sagi Grimberg 1 sibling, 1 reply; 18+ messages in thread From: Ingo Brunberg @ 2020-07-10 8:22 UTC (permalink / raw) To: linux-nvme With commit ea43d97, appearing now in kernel 5.7.8, you made my cheap SSD disfunctional again. For reference, see bug 205679. Maybe you remember the discussion. I do not know if this was intentional or not. Maybe you should introduce a quirk for that particular controller. Regards Ingo Brunberg _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: Namepace identification descriptor list is optional 2020-07-10 8:22 ` Ingo Brunberg @ 2020-07-22 23:23 ` Sagi Grimberg 2020-07-23 1:23 ` Ingo Brunberg 0 siblings, 1 reply; 18+ messages in thread From: Sagi Grimberg @ 2020-07-22 23:23 UTC (permalink / raw) To: Ingo Brunberg, linux-nvme > With commit ea43d97, appearing now in kernel 5.7.8, you made my cheap > SSD disfunctional again. For reference, see bug 205679. Maybe you > remember the discussion. > > I do not know if this was intentional or not. Maybe you should introduce > a quirk for that particular controller. Either we quirk this controller, or we are back to test specific errors... What is the pci id of the device to test a quirk? _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: Namepace identification descriptor list is optional 2020-07-22 23:23 ` Sagi Grimberg @ 2020-07-23 1:23 ` Ingo Brunberg 2020-07-28 11:08 ` Christoph Hellwig 0 siblings, 1 reply; 18+ messages in thread From: Ingo Brunberg @ 2020-07-23 1:23 UTC (permalink / raw) To: Sagi Grimberg; +Cc: linux-nvme Sagi Grimberg <sagi@grimberg.me> writes: >> With commit ea43d97, appearing now in kernel 5.7.8, you made my cheap >> SSD disfunctional again. For reference, see bug 205679. Maybe you >> remember the discussion. >> >> I do not know if this was intentional or not. Maybe you should introduce >> a quirk for that particular controller. > > Either we quirk this controller, or we are back to test specific errors... > > What is the pci id of the device to test a quirk? The PCI ID is 126f:2263 Related old and new bugs on bugzilla.kernel.org with some actual drives listed are: 205679, 205645, 208583, 208585 _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: Namepace identification descriptor list is optional 2020-07-23 1:23 ` Ingo Brunberg @ 2020-07-28 11:08 ` Christoph Hellwig 2020-07-28 14:41 ` Ingo Brunberg 0 siblings, 1 reply; 18+ messages in thread From: Christoph Hellwig @ 2020-07-28 11:08 UTC (permalink / raw) To: Ingo Brunberg; +Cc: Sagi Grimberg, linux-nvme On Thu, Jul 23, 2020 at 03:23:28AM +0200, Ingo Brunberg wrote: > Sagi Grimberg <sagi@grimberg.me> writes: > > >> With commit ea43d97, appearing now in kernel 5.7.8, you made my cheap > >> SSD disfunctional again. For reference, see bug 205679. Maybe you > >> remember the discussion. > >> > >> I do not know if this was intentional or not. Maybe you should introduce > >> a quirk for that particular controller. > > > > Either we quirk this controller, or we are back to test specific errors... > > > > What is the pci id of the device to test a quirk? > > The PCI ID is 126f:2263 Please try this patch: diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 05aa568a60af62..0602e0f3e77de9 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1287,18 +1287,6 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid, if (status) { dev_warn(ctrl->device, "Identify Descriptors failed (%d)\n", status); - /* - * Don't treat non-retryable errors as fatal, as we potentially - * already have a NGUID or EUI-64. If we failed with DNR set, - * we want to silently ignore the error as we can still - * identify the device, but if the status has DNR set, we want - * to propagate the error back specifically for the disk - * revalidation flow to make sure we don't abandon the - * device just because of a temporal retry-able error (such - * as path of transport errors). - */ - if (status > 0 && (status & NVME_SC_DNR) && !nvme_multi_css(ctrl)) - status = 0; goto free_data; } @@ -1888,7 +1876,9 @@ static int nvme_report_ns_ids(struct nvme_ctrl *ctrl, unsigned int nsid, memcpy(ids->eui64, id->eui64, sizeof(id->eui64)); if (ctrl->vs >= NVME_VS(1, 2, 0)) memcpy(ids->nguid, id->nguid, sizeof(id->nguid)); - if (ctrl->vs >= NVME_VS(1, 3, 0) || nvme_multi_css(ctrl)) + if (nvme_multi_css(ctrl) || + (ctrl->vs >= NVME_VS(1, 3, 0) && + !(ctrl->quirks & NVME_QUIRK_NO_NS_DESC_LIST))) return nvme_identify_ns_descs(ctrl, nsid, ids); return 0; } diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index c5c1bac797aa5a..4cadaea9034ae4 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -137,6 +137,13 @@ enum nvme_quirks { * Don't change the value of the temperature threshold feature */ NVME_QUIRK_NO_TEMP_THRESH_CHANGE = (1 << 14), + + /* + * The controller doesn't handle the Identify Namespace + * Identification Descriptor list subcommand despite claiming + * NVMe 1.3 compliance. + */ + NVME_QUIRK_NO_NS_DESC_LIST = (1 << 15), }; /* diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 61e612d52d61d6..bac3a3f9c79e0d 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -3181,6 +3181,8 @@ static const struct pci_device_id nvme_id_table[] = { { PCI_DEVICE(0x1cc1, 0x8201), /* ADATA SX8200PNP 512GB */ .driver_data = NVME_QUIRK_NO_DEEPEST_PS | NVME_QUIRK_IGNORE_DEV_SUBNQN, }, + { PCI_DEVICE(0x126f, 0x2263), + .driver_data = NVME_QUIRK_NO_NS_DESC_LIST, }, { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) }, { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001), .driver_data = NVME_QUIRK_SINGLE_VECTOR }, _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: Namepace identification descriptor list is optional 2020-07-28 11:08 ` Christoph Hellwig @ 2020-07-28 14:41 ` Ingo Brunberg 2020-07-28 16:01 ` Sagi Grimberg 0 siblings, 1 reply; 18+ messages in thread From: Ingo Brunberg @ 2020-07-28 14:41 UTC (permalink / raw) To: Christoph Hellwig; +Cc: sagi, linux-nvme Christoph Hellwig <hch@infradead.org> writes: > Please try this patch: > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 05aa568a60af62..0602e0f3e77de9 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -1287,18 +1287,6 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid, > if (status) { > dev_warn(ctrl->device, > "Identify Descriptors failed (%d)\n", status); > - /* > - * Don't treat non-retryable errors as fatal, as we potentially > - * already have a NGUID or EUI-64. If we failed with DNR set, > - * we want to silently ignore the error as we can still > - * identify the device, but if the status has DNR set, we want > - * to propagate the error back specifically for the disk > - * revalidation flow to make sure we don't abandon the > - * device just because of a temporal retry-able error (such > - * as path of transport errors). > - */ > - if (status > 0 && (status & NVME_SC_DNR) && !nvme_multi_css(ctrl)) > - status = 0; > goto free_data; > } > > @@ -1888,7 +1876,9 @@ static int nvme_report_ns_ids(struct nvme_ctrl *ctrl, unsigned int nsid, > memcpy(ids->eui64, id->eui64, sizeof(id->eui64)); > if (ctrl->vs >= NVME_VS(1, 2, 0)) > memcpy(ids->nguid, id->nguid, sizeof(id->nguid)); > - if (ctrl->vs >= NVME_VS(1, 3, 0) || nvme_multi_css(ctrl)) > + if (nvme_multi_css(ctrl) || > + (ctrl->vs >= NVME_VS(1, 3, 0) && > + !(ctrl->quirks & NVME_QUIRK_NO_NS_DESC_LIST))) > return nvme_identify_ns_descs(ctrl, nsid, ids); > return 0; > } > diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h > index c5c1bac797aa5a..4cadaea9034ae4 100644 > --- a/drivers/nvme/host/nvme.h > +++ b/drivers/nvme/host/nvme.h > @@ -137,6 +137,13 @@ enum nvme_quirks { > * Don't change the value of the temperature threshold feature > */ > NVME_QUIRK_NO_TEMP_THRESH_CHANGE = (1 << 14), > + > + /* > + * The controller doesn't handle the Identify Namespace > + * Identification Descriptor list subcommand despite claiming > + * NVMe 1.3 compliance. > + */ > + NVME_QUIRK_NO_NS_DESC_LIST = (1 << 15), > }; > > /* > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index 61e612d52d61d6..bac3a3f9c79e0d 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -3181,6 +3181,8 @@ static const struct pci_device_id nvme_id_table[] = { > { PCI_DEVICE(0x1cc1, 0x8201), /* ADATA SX8200PNP 512GB */ > .driver_data = NVME_QUIRK_NO_DEEPEST_PS | > NVME_QUIRK_IGNORE_DEV_SUBNQN, }, > + { PCI_DEVICE(0x126f, 0x2263), > + .driver_data = NVME_QUIRK_NO_NS_DESC_LIST, }, > { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) }, > { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001), > .driver_data = NVME_QUIRK_SINGLE_VECTOR }, Thanks, the patch works. Maybe it is not too late to make its way into kernel 5.8. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: Namepace identification descriptor list is optional 2020-07-28 14:41 ` Ingo Brunberg @ 2020-07-28 16:01 ` Sagi Grimberg 0 siblings, 0 replies; 18+ messages in thread From: Sagi Grimberg @ 2020-07-28 16:01 UTC (permalink / raw) To: Ingo Brunberg, Christoph Hellwig; +Cc: linux-nvme >> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c >> index 05aa568a60af62..0602e0f3e77de9 100644 >> --- a/drivers/nvme/host/core.c >> +++ b/drivers/nvme/host/core.c >> @@ -1287,18 +1287,6 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid, >> if (status) { >> dev_warn(ctrl->device, >> "Identify Descriptors failed (%d)\n", status); >> - /* >> - * Don't treat non-retryable errors as fatal, as we potentially >> - * already have a NGUID or EUI-64. If we failed with DNR set, >> - * we want to silently ignore the error as we can still >> - * identify the device, but if the status has DNR set, we want >> - * to propagate the error back specifically for the disk >> - * revalidation flow to make sure we don't abandon the >> - * device just because of a temporal retry-able error (such >> - * as path of transport errors). >> - */ >> - if (status > 0 && (status & NVME_SC_DNR) && !nvme_multi_css(ctrl)) >> - status = 0; >> goto free_data; >> } >> >> @@ -1888,7 +1876,9 @@ static int nvme_report_ns_ids(struct nvme_ctrl *ctrl, unsigned int nsid, >> memcpy(ids->eui64, id->eui64, sizeof(id->eui64)); >> if (ctrl->vs >= NVME_VS(1, 2, 0)) >> memcpy(ids->nguid, id->nguid, sizeof(id->nguid)); >> - if (ctrl->vs >= NVME_VS(1, 3, 0) || nvme_multi_css(ctrl)) >> + if (nvme_multi_css(ctrl) || >> + (ctrl->vs >= NVME_VS(1, 3, 0) && >> + !(ctrl->quirks & NVME_QUIRK_NO_NS_DESC_LIST))) >> return nvme_identify_ns_descs(ctrl, nsid, ids); >> return 0; >> } >> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h >> index c5c1bac797aa5a..4cadaea9034ae4 100644 >> --- a/drivers/nvme/host/nvme.h >> +++ b/drivers/nvme/host/nvme.h >> @@ -137,6 +137,13 @@ enum nvme_quirks { >> * Don't change the value of the temperature threshold feature >> */ >> NVME_QUIRK_NO_TEMP_THRESH_CHANGE = (1 << 14), >> + >> + /* >> + * The controller doesn't handle the Identify Namespace >> + * Identification Descriptor list subcommand despite claiming >> + * NVMe 1.3 compliance. >> + */ >> + NVME_QUIRK_NO_NS_DESC_LIST = (1 << 15), >> }; >> >> /* >> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c >> index 61e612d52d61d6..bac3a3f9c79e0d 100644 >> --- a/drivers/nvme/host/pci.c >> +++ b/drivers/nvme/host/pci.c >> @@ -3181,6 +3181,8 @@ static const struct pci_device_id nvme_id_table[] = { >> { PCI_DEVICE(0x1cc1, 0x8201), /* ADATA SX8200PNP 512GB */ >> .driver_data = NVME_QUIRK_NO_DEEPEST_PS | >> NVME_QUIRK_IGNORE_DEV_SUBNQN, }, >> + { PCI_DEVICE(0x126f, 0x2263), >> + .driver_data = NVME_QUIRK_NO_NS_DESC_LIST, }, >> { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) }, >> { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001), >> .driver_data = NVME_QUIRK_SINGLE_VECTOR }, > > Thanks, the patch works. Maybe it is not too late to make its way into > kernel 5.8. > Patch looks good, you can add my Reviewed-by: Sagi Grimberg <sagi@grimberg.me> on the formal patch. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <d163890a-a469-e71c-45b7-f63e1efee04e@intel.com>]
* Re: [PATCH] nvme: Namepace identification descriptor list is optional [not found] <d163890a-a469-e71c-45b7-f63e1efee04e@intel.com> @ 2020-07-12 9:31 ` Ingo Brunberg 2020-07-12 17:03 ` Rajashekar, Revanth 2020-07-14 15:21 ` Keith Busch 0 siblings, 2 replies; 18+ messages in thread From: Ingo Brunberg @ 2020-07-12 9:31 UTC (permalink / raw) To: Rajashekar, Revanth; +Cc: kbusch, chaitanya.kulkarni, linux-nvme, sagi "Rajashekar, Revanth" <revanth.rajashekar@intel.com> writes: > Hi, > > On 7/10/2020 2:22 AM, Ingo Brunberg wrote: > > With commit ea43d97, appearing now in kernel 5.7.8, you made my > cheap > SSD disfunctional again. For reference, see bug 205679. Maybe you > remember the discussion. > > Shouldn't the logic used in commit ea43d97, be applied to the line 'if (ret == -ENOMEM || (ret > 0 && > !(ret & NVME_SC_DNR)))' in function '_nvme_revalidate_disk' ? I was wondering the same. But that does not make my drive work. Something about that logic is not quite right. If the condition is met in nvme_identify_ns_descs, 0 is returned. So there is no point in repeating the same test in nvme_revalidate_disk as that code path is not reached anyway. If I got it right, NVME_SC_DNR is not set for my drive, so this test is not the right solution for that problematic controller. Regards Ingo Brunberg _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: Namepace identification descriptor list is optional 2020-07-12 9:31 ` Ingo Brunberg @ 2020-07-12 17:03 ` Rajashekar, Revanth 2020-07-14 15:21 ` Keith Busch 1 sibling, 0 replies; 18+ messages in thread From: Rajashekar, Revanth @ 2020-07-12 17:03 UTC (permalink / raw) To: Ingo Brunberg; +Cc: kbusch, chaitanya.kulkarni, linux-nvme, sagi On 7/12/2020 3:31 AM, Ingo Brunberg wrote: > "Rajashekar, Revanth" <revanth.rajashekar@intel.com> writes: > >> Hi, >> >> On 7/10/2020 2:22 AM, Ingo Brunberg wrote: >> >> With commit ea43d97, appearing now in kernel 5.7.8, you made my >> cheap >> SSD disfunctional again. For reference, see bug 205679. Maybe you >> remember the discussion. >> >> Shouldn't the logic used in commit ea43d97, be applied to the line 'if (ret == -ENOMEM || (ret > 0 && >> !(ret & NVME_SC_DNR)))' in function '_nvme_revalidate_disk' ? > I was wondering the same. But that does not make my drive > work. Something about that logic is not quite right. If the condition is > met in nvme_identify_ns_descs, 0 is returned. So there is no point in > repeating the same test in nvme_revalidate_disk as that code path is not > reached anyway. I agree with that... My concern is in case if the status had the DNR bit cleared. In that case this line of the code is reached and the status is silently suppressed to 0 because of the condition !(ret & NVME_SC_DNR) The intention of the commit ea43d97 was to navigate back the error in-case if it was a retryable error (DNR cleared to 0). > > If I got it right, NVME_SC_DNR is not set for my drive, so this test is > not the right solution for that problematic controller. > > Regards > Ingo Brunberg _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] nvme: Namepace identification descriptor list is optional 2020-07-12 9:31 ` Ingo Brunberg 2020-07-12 17:03 ` Rajashekar, Revanth @ 2020-07-14 15:21 ` Keith Busch 1 sibling, 0 replies; 18+ messages in thread From: Keith Busch @ 2020-07-14 15:21 UTC (permalink / raw) To: Ingo Brunberg; +Cc: Rajashekar, Revanth, chaitanya.kulkarni, linux-nvme, sagi On Sun, Jul 12, 2020 at 11:31:18AM +0200, Ingo Brunberg wrote: > If I got it right, NVME_SC_DNR is not set for my drive, so this test is > not the right solution for that problematic controller. So your controller claims v1.3 spec compliance, but does not implement a required command type, then tells the host that we should retry that command... I did mention the DNR reliance seemed a bit fragile considering the check was supposed to be for non-compliant controllers. The check as it exists is fixing real issues, though. I don't have an idea off the top of my head for a generic solution that will work with your controller and the issue the existing code addresses, but will think about it a bit more. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-07-28 16:01 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-02 15:56 [PATCH] nvme: Namepace identification descriptor list is optional Keith Busch 2019-12-02 16:15 ` Christoph Hellwig 2019-12-02 16:22 ` Keith Busch 2019-12-02 16:27 ` Nadolski, Edmund 2019-12-02 16:29 ` Christoph Hellwig 2019-12-02 16:45 ` Nadolski, Edmund 2019-12-02 16:49 ` Keith Busch 2019-12-02 17:34 ` Christoph Hellwig 2019-12-02 21:21 ` Keith Busch 2020-07-10 8:22 ` Ingo Brunberg 2020-07-22 23:23 ` Sagi Grimberg 2020-07-23 1:23 ` Ingo Brunberg 2020-07-28 11:08 ` Christoph Hellwig 2020-07-28 14:41 ` Ingo Brunberg 2020-07-28 16:01 ` Sagi Grimberg [not found] <d163890a-a469-e71c-45b7-f63e1efee04e@intel.com> 2020-07-12 9:31 ` Ingo Brunberg 2020-07-12 17:03 ` Rajashekar, Revanth 2020-07-14 15:21 ` Keith Busch
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).