Re: [PATCHv3 3/5] nvme: implement I/O Command Sets Command Set support
From: Sagi Grimberg <sagi@grimberg.me>
Date: 2020-06-24 18:28:44
Also in:
linux-nvme
Subsystem:
nvm express driver, the rest · Maintainers:
Keith Busch, Jens Axboe, Christoph Hellwig, Sagi Grimberg, Linus Torvalds
quoted
quoted
And what if it is a DNR error? For example, the controller simply doesn't support this CNS value. While the controller should support it, we definitely don't need it for NVM command set namespaces.Maybe I mis-undersatnd the comment, but if you see a DNR error, it means that the controller replied an error and its final, so then you can have extra checks.If the controller does not support the CNS value, it may return Invalid Field with DNR set. That error currently gets propogated back to nvme_init_ns_head(), which then abandons the namespace. Just as the code coments say, we had been historically been clearing such errors because we have other ways to identify the namespace, but now we're not clearing that error.
I don't understand what you are saying Keith.
My comment was for this block:
--
if (!status && nvme_multi_css(ctrl) && !csi_seen) {
dev_warn(ctrl->device, "Command set not reported for nsid:%d\n",
nsid);
status = -EINVAL;
}
--
I was saying that !status doesn't necessarily mean success, but it can
also mean that its an retry-able error status (due to transport or
controller). If we see a retry-able error we should still clear the
status so we don't abandon the namespace.
This for example would achieve that:
--diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ba512c45b40f..46d8a8379aff 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c@@ -1126,12 +1126,21 @@ 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 an error as fatal, as we potentially already
- * have a NGUID or EUI-64.
- */
- if (status > 0 && !(status & NVME_SC_DNR))
- status = 0;
+
+ if (status > 0 && !(status & NVME_SC_DNR)) {
+ if (nvme_multi_css(ctrl) && !csi_seen) {
+ dev_warn(ctrl->device,
+ "Command set not reported for
nsid:%d\n",
+ nsid);
+ status = -EINVAL;
+ } else {
+ /*
+ * Don't treat an error as fatal, as we
+ * potentially already have a NGUID or
EUI-64.
+ */
+ status = 0;
+ }
+ }
goto free_data;
}
--