LinuxLists.cc - [PATCH v4 0/8] Implement NVMe Namespace Descriptor Identification

[permalink] [raw]

Subject: [PATCH v4 1/8] nvme: introduce NVMe Namespace Identification Descriptor structures

Signed-off-by: Johannes Thumshirn <[email protected]>
Reviewed-by: Max Gurtovoy <[email protected]>
---
include/linux/nvme.h | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index b625bacf37ef..afa6ef484e50 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -288,6 +288,7 @@ enum {
NVME_ID_CNS_NS = 0x00,
NVME_ID_CNS_CTRL = 0x01,
NVME_ID_CNS_NS_ACTIVE_LIST = 0x02,
+ NVME_ID_CNS_NS_DESC_LIST = 0x03,
NVME_ID_CNS_NS_PRESENT_LIST = 0x10,
NVME_ID_CNS_NS_PRESENT = 0x11,
NVME_ID_CNS_CTRL_NS_LIST = 0x12,
@@ -314,6 +315,22 @@ enum {
NVME_NS_DPS_PI_TYPE3 = 3,
};

+struct nvme_ns_identifier_hdr {
+ __u8 nidt;
+ __u8 nidl;
+ __le16 reserved;
+};
+
+#define NVME_NIDT_EUI64_LEN 8
+#define NVME_NIDT_NGUID_LEN 16
+#define NVME_NIDT_UUID_LEN 16
+
+enum {
+ NVME_NIDT_EUI64 = 0x01,
+ NVME_NIDT_NGUID = 0x02,
+ NVME_NIDT_UUID = 0x03,
+};
+
struct nvme_smart_log {
__u8 critical_warning;
__u8 temperature[2];
@@ -658,6 +675,8 @@ struct nvme_identify {
__u32 rsvd11[5];
};

+#define NVME_IDENTIFY_DATA_SIZE 4096
+
struct nvme_features {
__u8 opcode;
__u8 flags;
--
2.12.0

2017-06-04 10:37:13

[permalink] [raw]

Subject: [PATCH v4 4/8] nvmet: add uuid field to nvme_ns and populate via configfs

Add the UUID field from the NVMe Namespace Identification Descriptor
to the nvmet_ns structure and allow it's population via configfs.

Signed-off-by: Johannes Thumshirn <[email protected]>
---
drivers/nvme/target/configfs.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index be8c800078e2..16f9f6e3a084 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -305,11 +305,41 @@ static ssize_t nvmet_ns_device_path_store(struct config_item *item,

CONFIGFS_ATTR(nvmet_ns_, device_path);

+static ssize_t nvmet_ns_device_uuid_show(struct config_item *item, char *page)
+{
+ return sprintf(page, "%pUb\n", &to_nvmet_ns(item)->uuid);
+}
+
+static ssize_t nvmet_ns_device_uuid_store(struct config_item *item,
+ const char *page, size_t count)
+{
+ struct nvmet_ns *ns = to_nvmet_ns(item);
+ struct nvmet_subsys *subsys = ns->subsys;
+ int ret = 0;
+
+
+ mutex_lock(&subsys->lock);
+ if (ns->enabled) {
+ ret = -EBUSY;
+ goto out_unlock;
+ }
+
+
+ if (uuid_be_to_bin(page, &ns->uuid))
+ ret = -EINVAL;
+
+out_unlock:
+ mutex_unlock(&subsys->lock);
+ return ret ? ret : count;
+}
+
static ssize_t nvmet_ns_device_nguid_show(struct config_item *item, char *page)
{
return sprintf(page, "%pUb\n", &to_nvmet_ns(item)->nguid);
}

+CONFIGFS_ATTR(nvmet_ns_, device_uuid);
+
static ssize_t nvmet_ns_device_nguid_store(struct config_item *item,
const char *page, size_t count)
{
@@ -379,6 +409,7 @@ CONFIGFS_ATTR(nvmet_ns_, enable);
static struct configfs_attribute *nvmet_ns_attrs[] = {
&nvmet_ns_attr_device_path,
&nvmet_ns_attr_device_nguid,
+ &nvmet_ns_attr_device_uuid,
&nvmet_ns_attr_enable,
NULL,
};
--
2.12.0

2017-06-04 10:37:14

[permalink] [raw]

Subject: [PATCH v4 7/8] nvmet: allow overriding the NVMe VS via configfs

Allow overriding the announced NVMe Version of a via configfs.

This is particularly helpful when debugging new features for the host
or target side without bumping the hard coded version (as the target
might not be fully compliant to the announced version yet).

Signed-off-by: Johannes Thumshirn <[email protected]>
---
drivers/nvme/target/configfs.c | 34 ++++++++++++++++++++++++++++++++++
include/linux/nvme.h | 4 ++++
2 files changed, 38 insertions(+)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 16f9f6e3a084..45421d4308a4 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -650,8 +650,42 @@ static ssize_t nvmet_subsys_attr_allow_any_host_store(struct config_item *item,

CONFIGFS_ATTR(nvmet_subsys_, attr_allow_any_host);

+static ssize_t nvmet_subsys_version_show(struct config_item *item,
+ char *page)
+{
+ struct nvmet_subsys *subsys = to_subsys(item);
+ int major, minor, tertiary;
+ u32 ver;
+
+ ver = subsys->ver;
+ major = NVME_MAJOR(ver);
+ minor = NVME_MINOR(ver);
+ tertiary = NVME_TERRIARY(ver);
+
+ return snprintf(page, PAGE_SIZE, "%d %d %d\n", major, minor, tertiary);
+}
+
+static ssize_t nvmet_subsys_version_store(struct config_item *item,
+ const char *page, size_t count)
+{
+ struct nvmet_subsys *subsys = to_subsys(item);
+ int major, minor, tertiary;
+ int ret;
+
+
+ ret = sscanf(page, "%d %d %d\n", &major, &minor, &tertiary);
+ if (ret != 3)
+ return -EINVAL;
+
+ subsys->ver = NVME_VS(major, minor, tertiary);
+
+ return count;
+}
+CONFIGFS_ATTR(nvmet_subsys_, version);
+
static struct configfs_attribute *nvmet_subsys_attrs[] = {
&nvmet_subsys_attr_attr_allow_any_host,
+ &nvmet_subsys_attr_version,
NULL,
};

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index afa6ef484e50..0d6e307a7aa4 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1069,4 +1069,8 @@ struct nvme_completion {
#define NVME_VS(major, minor, tertiary) \
(((major) << 16) | ((minor) << 8) | (tertiary))

+#define NVME_MAJOR(ver) ((ver) >> 16)
+#define NVME_MINOR(ver) (((ver) >> 8) & 0xff)
+#define NVME_TERRIARY(ver) ((ver) & 0xff)
+
#endif /* _LINUX_NVME_H */
--
2.12.0

2017-06-04 10:37:10

[permalink] [raw]

Subject: [PATCH v4 2/8] nvme: rename uuid to nguid in nvme_ns

The uuid field in the nvme_ns structure represents the nguid field
from the identify namespace command. And as NVMe 1.3 introduced an
UUID in the NVMe Namespace Identification Descriptor this will
collide.

So rename the uuid to nguid to prevent any further
confusion. Unfortunately we export the nguid to sysfs in the uuid
sysfs attribute, but this can't be changed anymore without possibly
breaking existing userspace.

Signed-off-by: Johannes Thumshirn <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Reviewed-by: Hannes Reinecke <[email protected]>
---
drivers/nvme/host/core.c | 10 +++++-----
drivers/nvme/host/nvme.h | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a60926410438..7b254be16887 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1016,7 +1016,7 @@ static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id)
if (ns->ctrl->vs >= NVME_VS(1, 1, 0))
memcpy(ns->eui, (*id)->eui64, sizeof(ns->eui));
if (ns->ctrl->vs >= NVME_VS(1, 2, 0))
- memcpy(ns->uuid, (*id)->nguid, sizeof(ns->uuid));
+ memcpy(ns->nguid, (*id)->nguid, sizeof(ns->nguid));

return 0;
}
@@ -1784,8 +1784,8 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
int serial_len = sizeof(ctrl->serial);
int model_len = sizeof(ctrl->model);

- if (memchr_inv(ns->uuid, 0, sizeof(ns->uuid)))
- return sprintf(buf, "eui.%16phN\n", ns->uuid);
+ if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
+ return sprintf(buf, "eui.%16phN\n", ns->nguid);

if (memchr_inv(ns->eui, 0, sizeof(ns->eui)))
return sprintf(buf, "eui.%8phN\n", ns->eui);
@@ -1804,7 +1804,7 @@ static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
- return sprintf(buf, "%pU\n", ns->uuid);
+ return sprintf(buf, "%pU\n", ns->nguid);
}
static DEVICE_ATTR(uuid, S_IRUGO, uuid_show, NULL);

@@ -1839,7 +1839,7 @@ static umode_t nvme_ns_attrs_are_visible(struct kobject *kobj,
struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

if (a == &dev_attr_uuid.attr) {
- if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid)))
+ if (!memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
return 0;
}
if (a == &dev_attr_eui.attr) {
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 9d6a070d4391..5004f0c41397 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -189,7 +189,7 @@ struct nvme_ns {
int instance;

u8 eui[8];
- u8 uuid[16];
+ u8 nguid[16];

unsigned ns_id;
int lba_shift;
--
2.12.0

2017-06-04 10:38:03

[permalink] [raw]

Subject: [PATCH v4 8/8] nvmet: use NVME_IDENTIFY_DATA_SIZE

Use NVME_IDENTIFY_DATA_SIZE define instead of hard coding the magic
4096 value.

Signed-off-by: Johannes Thumshirn <[email protected]>
---
drivers/nvme/target/admin-cmd.c | 4 ++--
drivers/nvme/target/discovery.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 833b5ef935c3..def6cde1e714 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -336,7 +336,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)

static void nvmet_execute_identify_nslist(struct nvmet_req *req)
{
- static const int buf_size = 4096;
+ static const int buf_size = NVME_IDENTIFY_DATA_SIZE;
struct nvmet_ctrl *ctrl = req->sq->ctrl;
struct nvmet_ns *ns;
u32 min_nsid = le32_to_cpu(req->cmd->identify.nsid);
@@ -554,7 +554,7 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
}
break;
case nvme_admin_identify:
- req->data_len = 4096;
+ req->data_len = NVME_IDENTIFY_DATA_SIZE;
switch (cmd->identify.cns) {
case NVME_ID_CNS_NS:
req->execute = nvmet_execute_identify_ns;
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index 1aaf597e81fc..c7a90384dd75 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -185,7 +185,7 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
}
case nvme_admin_identify:
- req->data_len = 4096;
+ req->data_len = NVME_IDENTIFY_DATA_SIZE;
switch (cmd->identify.cns) {
case NVME_ID_CNS_CTRL:
req->execute =
--
2.12.0

2017-06-04 10:38:06

[permalink] [raw]

Subject: [PATCH v4 6/8] nvme: provide UUID value to userspace

Now that we have a way for getting the UUID from a target, provide it
to userspace as well.

Unfortunately there is already a sysfs attribute called UUID which is
a misnomer as it holds the NGUID value. So instead of creating yet
another wrong name, create a new 'nguid' sysfs attribute for the
NGUID. For the UUID attribute add a check wheter the namespace has a
UUID assigned to it and return this or return the NGUID to maintain
backwards compatibility. This should give userspace a chance to catch
up.

Signed-off-by: Johannes Thumshirn <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Reviewed-by: Hannes Reinecke <[email protected]>
---
drivers/nvme/host/core.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 37047841da0e..3aa5b12680e5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1887,11 +1887,28 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
}
static DEVICE_ATTR(wwid, S_IRUGO, wwid_show, NULL);

+static ssize_t nguid_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
+ return sprintf(buf, "%pU\n", ns->nguid);
+}
+static DEVICE_ATTR(nguid, S_IRUGO, nguid_show, NULL);
+
static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
- return sprintf(buf, "%pU\n", ns->nguid);
+
+ /* For backward compatibility expose the NGUID to userspace if
+ * we have no UUID set
+ */
+ if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid))) {
+ printk_ratelimited(KERN_WARNING
+ "No UUID available providing old NGUID\n");
+ return sprintf(buf, "%pU\n", ns->nguid);
+ }
+ return sprintf(buf, "%pU\n", ns->uuid);
}
static DEVICE_ATTR(uuid, S_IRUGO, uuid_show, NULL);

@@ -1914,6 +1931,7 @@ static DEVICE_ATTR(nsid, S_IRUGO, nsid_show, NULL);
static struct attribute *nvme_ns_attrs[] = {
&dev_attr_wwid.attr,
&dev_attr_uuid.attr,
+ &dev_attr_nguid.attr,
&dev_attr_eui.attr,
&dev_attr_nsid.attr,
NULL,
@@ -1926,6 +1944,11 @@ static umode_t nvme_ns_attrs_are_visible(struct kobject *kobj,
struct nvme_ns *ns = nvme_get_ns_from_dev(dev);

if (a == &dev_attr_uuid.attr) {
+ if (!memchr_inv(ns->uuid, 0, sizeof(ns->uuid)) ||
+ !memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
+ return 0;
+ }
+ if (a == &dev_attr_nguid.attr) {
if (!memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
return 0;
}
--
2.12.0

2017-06-04 10:38:28

[permalink] [raw]

Subject: [PATCH v4 5/8] nvme: get list of namespace descriptors

If a target identifies itself as NVMe 1.3 compliant, try to get the
list of Namespace Identification Descriptors and populate the UUID,
NGUID and EUI64 fileds in the NVMe namespace structure with these
values.

Signed-off-by: Johannes Thumshirn <[email protected]>
---
drivers/nvme/host/core.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++
drivers/nvme/host/nvme.h | 1 +
2 files changed, 88 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 7b254be16887..37047841da0e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -643,6 +643,85 @@ int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
return error;
}

+static void nvme_parse_ns_descs(struct nvme_ns *ns, void *data)
+{
+ int pos;
+ int len;
+
+ for (pos = 0; pos < NVME_IDENTIFY_DATA_SIZE; pos += len) {
+ struct nvme_ns_identifier_hdr *cur = data + pos;
+
+ if (cur->nidl == 0)
+ break;
+
+ switch (cur->nidt) {
+ case NVME_NIDT_EUI64:
+ if (cur->nidl != NVME_NIDT_EUI64_LEN) {
+ dev_warn(ns->ctrl->dev,
+ "Target returned bogus length: %d for NVME_NIDT_EUI64\n",
+ cur->nidl);
+ return;
+ }
+ len = NVME_NIDT_EUI64_LEN;
+ memcpy(ns->eui, data + pos + sizeof(*cur), len);
+ break;
+ case NVME_NIDT_NGUID:
+ if (cur->nidl != NVME_NIDT_NGUID_LEN) {
+ dev_warn(ns->ctrl->dev,
+ "Target returned bogus length: %d for NVME_NIDT_NGUID\n",
+ cur->nidl);
+ return;
+ }
+ len = NVME_NIDT_NGUID_LEN;
+ memcpy(ns->nguid, data + pos + sizeof(*cur), len);
+ break;
+ case NVME_NIDT_UUID:
+ if (cur->nidl != NVME_NIDT_UUID_LEN) {
+ dev_warn(ns->ctrl->dev,
+ "Target returned bogus length: %d for NVME_NIDT_UUID\n",
+ cur->nidl);
+ return;
+ }
+ len = NVME_NIDT_UUID_LEN;
+ memcpy(ns->uuid, data + pos + sizeof(*cur), len);
+ break;
+ default:
+ dev_warn(ns->ctrl->dev,
+ "Invalid Namespace Identification Descriptor Type: %d\n",
+ cur->nidt);
+ return;
+ }
+
+ len += sizeof(*cur);
+ }
+}
+
+static int nvme_identify_ns_descs(struct nvme_ns *ns, unsigned nsid)
+{
+ struct nvme_command c = { };
+ int status;
+ void *data;
+
+ c.identify.opcode = nvme_admin_identify;
+ c.identify.nsid = cpu_to_le32(nsid);
+ c.identify.cns = NVME_ID_CNS_NS_DESC_LIST;
+
+ data = kzalloc(NVME_IDENTIFY_DATA_SIZE, GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ status = nvme_submit_sync_cmd(ns->ctrl->admin_q, &c, data,
+ NVME_IDENTIFY_DATA_SIZE);
+ if (status)
+ goto free_data;
+
+ nvme_parse_ns_descs(ns, data);
+
+free_data:
+ kfree(data);
+ return status;
+}
+
static int nvme_identify_ns_list(struct nvme_ctrl *dev, unsigned nsid, __le32 *ns_list)
{
struct nvme_command c = { };
@@ -1017,6 +1096,14 @@ static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id)
memcpy(ns->eui, (*id)->eui64, sizeof(ns->eui));
if (ns->ctrl->vs >= NVME_VS(1, 2, 0))
memcpy(ns->nguid, (*id)->nguid, sizeof(ns->nguid));
+ if (ns->ctrl->vs >= NVME_VS(1, 3, 0)) {
+ /* Don't treat error as fatal we potentially
+ * already have a NGUID or EUI-64
+ */
+ if (nvme_identify_ns_descs(ns, ns->ns_id))
+ dev_warn(ns->ctrl->dev,
+ "%s: Identify Descriptors failed\n", __func__);
+ }

return 0;
}
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 5004f0c41397..7007521e8194 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -190,6 +190,7 @@ struct nvme_ns {

u8 eui[8];
u8 nguid[16];
+ u8 uuid[16];

unsigned ns_id;
int lba_shift;
--
2.12.0

2017-06-04 10:38:37

[permalink] [raw]

Subject: [PATCH v4 3/8] nvmet: implement namespace identify descriptor list

A NVMe Identify NS command with a CNS value of '3' is expecting a list
of Namespace Identification Descriptor structures to be returned to
the host for the namespace requested in the namespace identify
command.

This Namespace Identification Descriptor structure consists of the
type of the namespace identifier, the length of the identifier and the
actual identifier.

Valid types are NGUID and UUID which we have saved in our nvme_ns
structure if they have been configured via configfs. If no value has
been assigened to one of these we return an "invalid opcode" back to
the host to maintain backward compatibiliy with older implementations
without Namespace Identify Descriptor list support.

Also as the Namespace Identify Descriptor list is the only mandatory
feature change between 1.2.1 and 1.3.0 we can bump the advertised
version as well.

Signed-off-by: Johannes Thumshirn <[email protected]>
---
drivers/nvme/target/admin-cmd.c | 53 +++++++++++++++++++++++++++++++++++++++++
drivers/nvme/target/core.c | 3 ++-
drivers/nvme/target/nvmet.h | 1 +
3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index ff1f97006322..833b5ef935c3 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -367,6 +367,56 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req)
nvmet_req_complete(req, status);
}

+static void nvmet_execute_identify_desclist(struct nvmet_req *req)
+{
+ struct nvmet_ns *ns;
+ struct nvme_ns_identifier_hdr hdr;
+ u16 status = 0;
+ off_t off = 0;
+
+ ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid);
+ if (!ns) {
+ status = NVME_SC_INVALID_NS | NVME_SC_DNR;
+ goto out;
+ }
+
+ if (memchr_inv(&ns->uuid, 0, sizeof(ns->uuid))) {
+ memset(&hdr, 0, sizeof(hdr));
+ hdr.nidt = NVME_NIDT_UUID;
+ hdr.nidl = NVME_NIDT_UUID_LEN;
+ status = nvmet_copy_to_sgl(req, off, &hdr, sizeof(hdr));
+ if (status)
+ goto out_put_ns;
+ off += sizeof(hdr);
+
+ status = nvmet_copy_to_sgl(req, off, &ns->uuid,
+ sizeof(ns->uuid));
+ if (status)
+ goto out_put_ns;
+ off += sizeof(ns->uuid);
+ }
+ if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid))) {
+ memset(&hdr, 0, sizeof(hdr));
+ hdr.nidt = NVME_NIDT_NGUID;
+ hdr.nidl = NVME_NIDT_NGUID_LEN;
+ status = nvmet_copy_to_sgl(req, off, &hdr, sizeof(hdr));
+ if (status)
+ goto out_put_ns;
+ off += sizeof(hdr);
+
+ status = nvmet_copy_to_sgl(req, off, &ns->nguid,
+ sizeof(ns->nguid));
+ if (status)
+ goto out_put_ns;
+ off += sizeof(ns->nguid);
+ }
+
+out_put_ns:
+ nvmet_put_namespace(ns);
+out:
+ nvmet_req_complete(req, status);
+}
+
/*
* A "mimimum viable" abort implementation: the command is mandatory in the
* spec, but we are not required to do any useful work. We couldn't really
@@ -515,6 +565,9 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
case NVME_ID_CNS_NS_ACTIVE_LIST:
req->execute = nvmet_execute_identify_nslist;
return 0;
+ case NVME_ID_CNS_NS_DESC_LIST:
+ req->execute = nvmet_execute_identify_desclist;
+ return 0;
}
break;
case nvme_admin_abort_cmd:
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index eb9399ac97cf..1a9c17c7dc59 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -380,6 +380,7 @@ struct nvmet_ns *nvmet_ns_alloc(struct nvmet_subsys *subsys, u32 nsid)

ns->nsid = nsid;
ns->subsys = subsys;
+ uuid_be_gen(&ns->uuid);

return ns;
}
@@ -926,7 +927,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
if (!subsys)
return NULL;

- subsys->ver = NVME_VS(1, 2, 1); /* NVMe 1.2.1 */
+ subsys->ver = NVME_VS(1, 3, 0); /* NVMe 1.3.0 */

switch (type) {
case NVME_NQN_NVME:
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index cfc5c7fb0ab7..4c6cb5ea1186 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -46,6 +46,7 @@ struct nvmet_ns {
u32 blksize_shift;
loff_t size;
u8 nguid[16];
+ uuid_be uuid;

bool enabled;
struct nvmet_subsys *subsys;
--
2.12.0

2017-06-04 11:46:31

by Max Gurtovoy

[permalink] [raw]

Subject: Re: [PATCH v4 4/8] nvmet: add uuid field to nvme_ns and populate via configfs

On 6/4/2017 1:36 PM, Johannes Thumshirn wrote:
> Add the UUID field from the NVMe Namespace Identification Descriptor
> to the nvmet_ns structure and allow it's population via configfs.
>
> Signed-off-by: Johannes Thumshirn <[email protected]>

Looks good,

Reviewed-by: Max Gurtovoy <[email protected]>

2017-06-04 14:16:18

by Max Gurtovoy

[permalink] [raw]

Subject: Re: [PATCH v4 8/8] nvmet: use NVME_IDENTIFY_DATA_SIZE

Looks good,

Reviewed-by: Max Gurtovoy <[email protected]>

2017-06-04 14:43:21

[permalink] [raw]

Subject: Re: [PATCH v4 1/8] nvme: introduce NVMe Namespace Identification Descriptor structures

Looks good Johannes,

Reviewed-by: Sagi Grimberg <[email protected]>

2017-06-04 14:43:44

[permalink] [raw]

Subject: Re: [PATCH v4 2/8] nvme: rename uuid to nguid in nvme_ns

Looks good Johannes,

Reviewed-by: Sagi Grimberg <[email protected]>

2017-06-04 14:59:15

[permalink] [raw]

Subject: Re: [PATCH v4 1/8] nvme: introduce NVMe Namespace Identification Descriptor structures

On 04/06/17 13:36, Johannes Thumshirn wrote:
> Signed-off-by: Johannes Thumshirn <[email protected]>
> Reviewed-by: Max Gurtovoy <[email protected]>
> ---
> include/linux/nvme.h | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index b625bacf37ef..afa6ef484e50 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -288,6 +288,7 @@ enum {
> NVME_ID_CNS_NS = 0x00,
> NVME_ID_CNS_CTRL = 0x01,
> NVME_ID_CNS_NS_ACTIVE_LIST = 0x02,
> + NVME_ID_CNS_NS_DESC_LIST = 0x03,
> NVME_ID_CNS_NS_PRESENT_LIST = 0x10,
> NVME_ID_CNS_NS_PRESENT = 0x11,
> NVME_ID_CNS_CTRL_NS_LIST = 0x12,
> @@ -314,6 +315,22 @@ enum {
> NVME_NS_DPS_PI_TYPE3 = 3,
> };
>
> +struct nvme_ns_identifier_hdr {
> + __u8 nidt;
> + __u8 nidl;
> + __le16 reserved;
> +};

Nit: _hdr is usually associated with a message or alike.

Maybe nvme_ns_id_desc is more consistent with the spec language.

2017-06-04 15:00:45

[permalink] [raw]

Subject: Re: [PATCH v4 3/8] nvmet: implement namespace identify descriptor list

> +static void nvmet_execute_identify_desclist(struct nvmet_req *req)
> +{
> + struct nvmet_ns *ns;
> + struct nvme_ns_identifier_hdr hdr;
> + u16 status = 0;
> + off_t off = 0;
> +
> + ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid);
> + if (!ns) {
> + status = NVME_SC_INVALID_NS | NVME_SC_DNR;
> + goto out;
> + }
> +
> + if (memchr_inv(&ns->uuid, 0, sizeof(ns->uuid))) {
> + memset(&hdr, 0, sizeof(hdr));
> + hdr.nidt = NVME_NIDT_UUID;
> + hdr.nidl = NVME_NIDT_UUID_LEN;
> + status = nvmet_copy_to_sgl(req, off, &hdr, sizeof(hdr));
> + if (status)
> + goto out_put_ns;
> + off += sizeof(hdr);
> +
> + status = nvmet_copy_to_sgl(req, off, &ns->uuid,
> + sizeof(ns->uuid));
> + if (status)
> + goto out_put_ns;
> + off += sizeof(ns->uuid);
> + }
> + if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid))) {

Plain override in case a namespace is fed with both uuid and nguid?

Perhaps we should try to help the user a bit here and either fail
the user trying to populate both in configfs or pick one here.

> + memset(&hdr, 0, sizeof(hdr));
> + hdr.nidt = NVME_NIDT_NGUID;
> + hdr.nidl = NVME_NIDT_NGUID_LEN;
> + status = nvmet_copy_to_sgl(req, off, &hdr, sizeof(hdr));
> + if (status)
> + goto out_put_ns;
> + off += sizeof(hdr);
> +
> + status = nvmet_copy_to_sgl(req, off, &ns->nguid,
> + sizeof(ns->nguid));
> + if (status)
> + goto out_put_ns;
> + off += sizeof(ns->nguid);
> + }

If none exist we return whatever the the stack contains in hdr...
Lets initialize hdr with {} at the declaration.

2017-06-04 15:01:17

[permalink] [raw]

Subject: Re: [PATCH v4 4/8] nvmet: add uuid field to nvme_ns and populate via configfs

Looks good Johannes,

Reviewed-by: Sagi Grimberg <[email protected]>

2017-06-04 15:06:12

[permalink] [raw]

Subject: Re: [PATCH v4 5/8] nvme: get list of namespace descriptors

On 04/06/17 13:36, Johannes Thumshirn wrote:
> If a target identifies itself as NVMe 1.3 compliant, try to get the
> list of Namespace Identification Descriptors and populate the UUID,
> NGUID and EUI64 fileds in the NVMe namespace structure with these
> values.
>
> Signed-off-by: Johannes Thumshirn <[email protected]>
> ---
> drivers/nvme/host/core.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/nvme/host/nvme.h | 1 +
> 2 files changed, 88 insertions(+)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 7b254be16887..37047841da0e 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -643,6 +643,85 @@ int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
> return error;
> }
>
> +static void nvme_parse_ns_descs(struct nvme_ns *ns, void *data)
> +{
> + int pos;
> + int len;
> +
> + for (pos = 0; pos < NVME_IDENTIFY_DATA_SIZE; pos += len) {
> + struct nvme_ns_identifier_hdr *cur = data + pos;
> +
> + if (cur->nidl == 0)
> + break;
> +
> + switch (cur->nidt) {
> + case NVME_NIDT_EUI64:
> + if (cur->nidl != NVME_NIDT_EUI64_LEN) {
> + dev_warn(ns->ctrl->dev,
> + "Target returned bogus length: %d for NVME_NIDT_EUI64\n",
> + cur->nidl);

Hmm, "target" is not a spec'd entity in NVMe AFAIR, and we try to avoid
using this language in the host too, lets call it "ctrl".

2017-06-04 15:06:56

[permalink] [raw]

Subject: Re: [PATCH v4 6/8] nvme: provide UUID value to userspace

Looks good,

Reviewed-by: Sagi Grimberg <[email protected]>

2017-06-04 15:08:52

[permalink] [raw]

Subject: Re: [PATCH v4 7/8] nvmet: allow overriding the NVMe VS via configfs

On 04/06/17 13:36, Johannes Thumshirn wrote:
> Allow overriding the announced NVMe Version of a via configfs.
>
> This is particularly helpful when debugging new features for the host
> or target side without bumping the hard coded version (as the target
> might not be fully compliant to the announced version yet).
>
> Signed-off-by: Johannes Thumshirn <[email protected]>
> ---
> drivers/nvme/target/configfs.c | 34 ++++++++++++++++++++++++++++++++++
> include/linux/nvme.h | 4 ++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 16f9f6e3a084..45421d4308a4 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -650,8 +650,42 @@ static ssize_t nvmet_subsys_attr_allow_any_host_store(struct config_item *item,
>
> CONFIGFS_ATTR(nvmet_subsys_, attr_allow_any_host);
>
> +static ssize_t nvmet_subsys_version_show(struct config_item *item,
> + char *page)
> +{
> + struct nvmet_subsys *subsys = to_subsys(item);
> + int major, minor, tertiary;
> + u32 ver;
> +
> + ver = subsys->ver;
> + major = NVME_MAJOR(ver);
> + minor = NVME_MINOR(ver);
> + tertiary = NVME_TERRIARY(ver);
> +
> + return snprintf(page, PAGE_SIZE, "%d %d %d\n", major, minor, tertiary);

Nit: maybe a dot separator would be better? e.g. "1.3.0" rather than
"1 3 0"

Otherwise looks good,

Reviewed-by: Sagi Grimberg <[email protected]>

2017-06-04 15:09:53

[permalink] [raw]

Subject: Re: [PATCH v4 8/8] nvmet: use NVME_IDENTIFY_DATA_SIZE

Looks fine,

Reviewed-by: Sagi Grimberg <[email protected]>

2017-06-05 05:32:05

[permalink] [raw]

Subject: Re: [PATCH v4 0/8] Implement NVMe Namespace Descriptor Identification

On Sun, Jun 04, 2017 at 12:36:41PM +0200, Johannes Thumshirn wrote:
> The Namespace Identification Descriptor list is the only way a target
> can identify itself via the newly introduced UUID to the host (instead
> of the EUI-64 or NGUID).

s/instead/in addition/

> While I was already touching the relevant code paths, I decided to
> also include the EUI-64 in the 'Identify Namespace' command response.

Not anymore :)

2017-06-05 05:33:03

[permalink] [raw]

Subject: Re: [PATCH v4 1/8] nvme: introduce NVMe Namespace Identification Descriptor structures

On Sun, Jun 04, 2017 at 05:59:06PM +0300, Sagi Grimberg wrote:
>> +struct nvme_ns_identifier_hdr {
>> + __u8 nidt;
>> + __u8 nidl;
>> + __le16 reserved;
>> +};
>
> Nit: _hdr is usually associated with a message or alike.
>
> Maybe nvme_ns_id_desc is more consistent with the spec language.

The descriptor itself includes the actual value, that's why
I suggested adding the _hdr prefix. But I'm fine going either way.

2017-06-05 05:35:49

[permalink] [raw]

Subject: Re: [PATCH v4 3/8] nvmet: implement namespace identify descriptor list

I think this needs to zero the remainder of the 4k
buffer, quoting the spec:

"The controller may return any number of variable length Namespace
Identification Descriptor structures that fit into the 4096 byte Identify
payload. All remaining bytes after the namespace identification descriptor
structures should be cleared to 0h, and the host shall interpret a
Namespace Identifier Descriptor Length (NIDL) value of 0h as the end of
the list. If the hosts sees an unknown descriptor type it should
continue parsing the structure."

It could be easily done by copying the zero page to userspace.
Eventually I want to add a helper to zero parts of a sglist to
lib/scatterlist.c, but I don't want to burden that on you.

2017-06-05 05:38:29

[permalink] [raw]

Subject: Re: [PATCH v4 5/8] nvme: get list of namespace descriptors

> + }
> + len = NVME_NIDT_UUID_LEN;
> + memcpy(ns->uuid, data + pos + sizeof(*cur), len);
> + break;
> + default:
> + dev_warn(ns->ctrl->dev,
> + "Invalid Namespace Identification Descriptor Type: %d\n",
> + cur->nidt);
> + return;

Please drop the warning and return, the spec says hosts should ignore
unknown types. This is important to future proof for new types that
could be added.

> +static int nvme_identify_ns_descs(struct nvme_ns *ns, unsigned nsid)
> +{
> + struct nvme_command c = { };
> + int status;
> + void *data;
> +
> + c.identify.opcode = nvme_admin_identify;
> + c.identify.nsid = cpu_to_le32(nsid);
> + c.identify.cns = NVME_ID_CNS_NS_DESC_LIST;
> +
> + data = kzalloc(NVME_IDENTIFY_DATA_SIZE, GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + status = nvme_submit_sync_cmd(ns->ctrl->admin_q, &c, data,
> + NVME_IDENTIFY_DATA_SIZE);
> + if (status)
> + goto free_data;
> +
> + nvme_parse_ns_descs(ns, data);

Just merge nvme_parse_ns_descs into the caller?

2017-06-05 05:45:14

[permalink] [raw]

Subject: Re: [PATCH v4 7/8] nvmet: allow overriding the NVMe VS via configfs

On Sun, Jun 04, 2017 at 12:36:48PM +0200, Johannes Thumshirn wrote:
> Allow overriding the announced NVMe Version of a via configfs.
>
> This is particularly helpful when debugging new features for the host
> or target side without bumping the hard coded version (as the target
> might not be fully compliant to the announced version yet).
>
> Signed-off-by: Johannes Thumshirn <[email protected]>
> ---
> drivers/nvme/target/configfs.c | 34 ++++++++++++++++++++++++++++++++++
> include/linux/nvme.h | 4 ++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 16f9f6e3a084..45421d4308a4 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -650,8 +650,42 @@ static ssize_t nvmet_subsys_attr_allow_any_host_store(struct config_item *item,
>
> CONFIGFS_ATTR(nvmet_subsys_, attr_allow_any_host);
>
> +static ssize_t nvmet_subsys_version_show(struct config_item *item,
> + char *page)
> +{
> + struct nvmet_subsys *subsys = to_subsys(item);
> + int major, minor, tertiary;
> + u32 ver;
> +
> + ver = subsys->ver;
> + major = NVME_MAJOR(ver);
> + minor = NVME_MINOR(ver);
> + tertiary = NVME_TERRIARY(ver);
> +
> + return snprintf(page, PAGE_SIZE, "%d %d %d\n", major, minor, tertiary);

Nit Dop we really need all these variables? Why not:

return snprintf(page, PAGE_SIZE, "%d %d %d\n", NVME_MAJOR(subsys->ver),
NVME_MINOR(subsys->ver), NVME_TERRIARY(subsys->ver));

?

Also except for the 1.2.1 oddbackk NVMe versions usually have two
components, and should be printed as such if the tertiary version is
0.

> +static ssize_t nvmet_subsys_version_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct nvmet_subsys *subsys = to_subsys(item);
> + int major, minor, tertiary;
> + int ret;
> +
> +
> + ret = sscanf(page, "%d %d %d\n", &major, &minor, &tertiary);
> + if (ret != 3)
> + return -EINVAL;

Same issue here. I also have to say I'm a bit sceptical about
just being able to set versions as there are various dependencies
on it. E.g. for 1.0 the version field in identify namespace doesn't
even exists and should be cleared to 0.

> +
> + subsys->ver = NVME_VS(major, minor, tertiary);

locking?

> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index afa6ef484e50..0d6e307a7aa4 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -1069,4 +1069,8 @@ struct nvme_completion {
> #define NVME_VS(major, minor, tertiary) \
> (((major) << 16) | ((minor) << 8) | (tertiary))
>
> +#define NVME_MAJOR(ver) ((ver) >> 16)
> +#define NVME_MINOR(ver) (((ver) >> 8) & 0xff)
> +#define NVME_TERRIARY(ver) ((ver) & 0xff)

I think TERRIARY should be TERTIARY.

2017-06-05 08:37:16

[permalink] [raw]

Subject: Re: [PATCH v4 8/8] nvmet: use NVME_IDENTIFY_DATA_SIZE

On Sun, Jun 04, 2017 at 12:36:49PM +0200, Johannes Thumshirn wrote:
> Use NVME_IDENTIFY_DATA_SIZE define instead of hard coding the magic
> 4096 value.

Can you bump this to the front of the series?

2017-06-06 06:23:29

[permalink] [raw]

Subject: Re: [PATCH v4 1/8] nvme: introduce NVMe Namespace Identification Descriptor structures

On 06/04/2017 12:36 PM, Johannes Thumshirn wrote:
> Signed-off-by: Johannes Thumshirn <[email protected]>
> Reviewed-by: Max Gurtovoy <[email protected]>
> ---
> include/linux/nvme.h | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
Reviewed-by: Hannes Reinecke <[email protected]>

Cheers,

Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

2017-06-06 06:23:59

[permalink] [raw]

Subject: Re: [PATCH v4 3/8] nvmet: implement namespace identify descriptor list

On 06/04/2017 12:36 PM, Johannes Thumshirn wrote:
> A NVMe Identify NS command with a CNS value of '3' is expecting a list
> of Namespace Identification Descriptor structures to be returned to
> the host for the namespace requested in the namespace identify
> command.
>
> This Namespace Identification Descriptor structure consists of the
> type of the namespace identifier, the length of the identifier and the
> actual identifier.
>
> Valid types are NGUID and UUID which we have saved in our nvme_ns
> structure if they have been configured via configfs. If no value has
> been assigened to one of these we return an "invalid opcode" back to
> the host to maintain backward compatibiliy with older implementations
> without Namespace Identify Descriptor list support.
>
> Also as the Namespace Identify Descriptor list is the only mandatory
> feature change between 1.2.1 and 1.3.0 we can bump the advertised
> version as well.
>
> Signed-off-by: Johannes Thumshirn <[email protected]>
> ---
> drivers/nvme/target/admin-cmd.c | 53 +++++++++++++++++++++++++++++++++++++++++
> drivers/nvme/target/core.c | 3 ++-
> drivers/nvme/target/nvmet.h | 1 +
> 3 files changed, 56 insertions(+), 1 deletion(-)
>
Reviewed-by: Hannes Reinecke <[email protected]>

Cheers,

Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

2017-06-06 06:24:25

[permalink] [raw]

Subject: Re: [PATCH v4 4/8] nvmet: add uuid field to nvme_ns and populate via configfs

On 06/04/2017 12:36 PM, Johannes Thumshirn wrote:
> Add the UUID field from the NVMe Namespace Identification Descriptor
> to the nvmet_ns structure and allow it's population via configfs.
>
> Signed-off-by: Johannes Thumshirn <[email protected]>
> ---
> drivers/nvme/target/configfs.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
Reviewed-by: Hannes Reinecke <[email protected]>

Cheers,

Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

2017-06-06 06:25:02

[permalink] [raw]

Subject: Re: [PATCH v4 5/8] nvme: get list of namespace descriptors

On 06/04/2017 12:36 PM, Johannes Thumshirn wrote:
> If a target identifies itself as NVMe 1.3 compliant, try to get the
> list of Namespace Identification Descriptors and populate the UUID,
> NGUID and EUI64 fileds in the NVMe namespace structure with these
> values.
>
> Signed-off-by: Johannes Thumshirn <[email protected]>
> ---
> drivers/nvme/host/core.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/nvme/host/nvme.h | 1 +
> 2 files changed, 88 insertions(+)
>
Reviewed-by: Hannes Reinecke <[email protected]>

Cheers,

Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

2017-06-06 06:25:44

[permalink] [raw]

Subject: Re: [PATCH v4 7/8] nvmet: allow overriding the NVMe VS via configfs

On 06/04/2017 12:36 PM, Johannes Thumshirn wrote:
> Allow overriding the announced NVMe Version of a via configfs.
>
> This is particularly helpful when debugging new features for the host
> or target side without bumping the hard coded version (as the target
> might not be fully compliant to the announced version yet).
>
> Signed-off-by: Johannes Thumshirn <[email protected]>
> ---
> drivers/nvme/target/configfs.c | 34 ++++++++++++++++++++++++++++++++++
> include/linux/nvme.h | 4 ++++
> 2 files changed, 38 insertions(+)
>
Reviewed-by: Hannes Reinecke <[email protected]>

Cheers,

Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

2017-06-06 06:26:09

[permalink] [raw]

Subject: Re: [PATCH v4 8/8] nvmet: use NVME_IDENTIFY_DATA_SIZE

On 06/04/2017 12:36 PM, Johannes Thumshirn wrote:
> Use NVME_IDENTIFY_DATA_SIZE define instead of hard coding the magic
> 4096 value.
>
> Signed-off-by: Johannes Thumshirn <[email protected]>
> ---
> drivers/nvme/target/admin-cmd.c | 4 ++--
> drivers/nvme/target/discovery.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
Reviewed-by: Hannes Reinecke <[email protected]>

Cheers,

Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

2017-06-06 07:16:01

[permalink] [raw]

Subject: Re: [PATCH v4 5/8] nvme: get list of namespace descriptors

On 05/06/17 08:38, Christoph Hellwig wrote:
>> + }
>> + len = NVME_NIDT_UUID_LEN;
>> + memcpy(ns->uuid, data + pos + sizeof(*cur), len);
>> + break;
>> + default:
>> + dev_warn(ns->ctrl->dev,
>> + "Invalid Namespace Identification Descriptor Type: %d\n",
>> + cur->nidt);
>> + return;
>
> Please drop the warning and return, the spec says hosts should ignore
> unknown types. This is important to future proof for new types that
> could be added.

Also, please stay consistent with the rest of the driver by logging the
correct device prefix using ctrl->device and not ctrl->dev.

(if we have some ctrl->dev left-overs, can you also send a patch to fix?)

2017-06-06 12:04:23