On 6/23/20 4:25 AM, Niklas Cassel wrote:
On Tue, Jun 23, 2020 at 01:53:47AM -0700, Sagi Grimberg wrote:
On 6/22/20 9:25 AM, Keith Busch wrote:
From: Niklas Cassel <niklas.cassel@xxxxxxx>
Implements support for the I/O Command Sets command set. The command set
introduces a method to enumerate multiple command sets per namespace. If
the command set is exposed, this method for enumeration will be used
instead of the traditional method that uses the CC.CSS register command
set register for command set identification.
For namespaces where the Command Set Identifier is not supported or
recognized, the specific namespace will not be created.
Reviewed-by: Javier González <javier.gonz@xxxxxxxxxxx>
Reviewed-by: Martin K. Petersen <martin.petersen@xxxxxxxxxx>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@xxxxxxx>
Reviewed-by: Matias Bjørling <matias.bjorling@xxxxxxx>
Reviewed-by: Daniel Wagner <dwagner@xxxxxxx>
Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
---
drivers/nvme/host/core.c | 48 +++++++++++++++++++++++++++++++++-------
drivers/nvme/host/nvme.h | 1 +
include/linux/nvme.h | 19 ++++++++++++++--
3 files changed, 58 insertions(+), 10 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9491dbcfe81a..45a3cb5a35bd 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1056,8 +1056,13 @@ static int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
return error;
}
+static bool nvme_multi_css(struct nvme_ctrl *ctrl)
+{
+ return (ctrl->ctrl_config & NVME_CC_CSS_MASK) == NVME_CC_CSS_CSI;
+}
+
static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
- struct nvme_ns_id_desc *cur)
+ struct nvme_ns_id_desc *cur, bool *csi_seen)
{
const char *warn_str = "ctrl returned bogus length:";
void *data = cur;
@@ -1087,6 +1092,15 @@ static int nvme_process_ns_desc(struct nvme_ctrl *ctrl, struct nvme_ns_ids *ids,
}
uuid_copy(&ids->uuid, data + sizeof(*cur));
return NVME_NIDT_UUID_LEN;
+ case NVME_NIDT_CSI:
+ if (cur->nidl != NVME_NIDT_CSI_LEN) {
+ dev_warn(ctrl->device, "%s %d for NVME_NIDT_CSI\n",
+ warn_str, cur->nidl);
+ return -1;
+ }
+ memcpy(&ids->csi, data + sizeof(*cur), NVME_NIDT_CSI_LEN);
+ *csi_seen = true;
+ return NVME_NIDT_CSI_LEN;
default:
/* Skip unknown types */
return cur->nidl;
@@ -1097,10 +1111,9 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
struct nvme_ns_ids *ids)
{
struct nvme_command c = { };
- int status;
+ bool csi_seen = false;
+ int status, pos, len;
void *data;
- int pos;
- int len;
c.identify.opcode = nvme_admin_identify;
c.identify.nsid = cpu_to_le32(nsid);
@@ -1130,13 +1143,19 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
if (cur->nidl == 0)
break;
- len = nvme_process_ns_desc(ctrl, ids, cur);
+ len = nvme_process_ns_desc(ctrl, ids, cur, &csi_seen);
if (len < 0)
goto free_data;
len += sizeof(*cur);
}
free_data:
+ if (!status && nvme_multi_css(ctrl) && !csi_seen) {
We will clear the status if we detect a path error, that is to
avoid needlessly removing the ns for path failures, so you should
check at the goto site.
The problem is that this check has to be done after checking all the ns descs,
so this check to be done as the final thing, at least after processing all the
ns descs. No matter if nvme_process_ns_desc() returned an error, or if
simply NVME_NIDT_CSI wasn't part of the ns desc list, so the loop reached the
end without error.
Even if the nvme command failed and the status was cleared:
if (status > 0 && !(status & NVME_SC_DNR))
status = 0;
we still need to return an error, if (nvme_multi_css(ctrl) && !csi_seen).
(Not reporting a CSI when nvme_multi_css() is enabled, is fatal.)
That is why the code looks like it does.
I guess we could do something like this, which does the same thing,
but perhaps is a bit clearer:
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e95f0c498a6b..bef687b9a277 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1160,8 +1160,10 @@ static int nvme_identify_ns_descs(struct nvme_ctrl *ctrl, unsigned nsid,
* Don't treat an error as fatal, as we potentially already
* have a NGUID or EUI-64.
*/
- if (status > 0 && !(status & NVME_SC_DNR))
+ if (status > 0 && !(status & NVME_SC_DNR)) {
status = 0;
+ goto csi_check;
+ }
I think its the opposite. If we failed with DNR, you should assume
that either the controller wants the host to retry, or its a
path/transport error. So we need to leave this namespace alone and
assume that when the host is connected back to the controller, the
scan_work will revalidate again.
So you should fail the csi_check only if you see a DNR error status.
