[PATCH 0/3] Allow user to set nvmet firmware revision and IEEE OUI

[PATCH 0/3] Allow user to set nvmet firmware revision and IEEE OUI

[Aleksandr Miloserdov]

This patch series introduces changes that allow user to set nvmet
firmware revision and IEEE OUI via configFS.

Signed-off-by: Aleksandr Miloserdov <a.miloserdov@yadro.com>

Aleksandr Miloserdov (3):
  nvmet: fix memory leak in configfs
  nvmet: expose IEEE OUI to configfs
  nvmet: expose firmware revision to configfs

 drivers/nvme/target/admin-cmd.c |  11 ++-
 drivers/nvme/target/configfs.c  | 122 +++++++++++++++++++++++++++++++-
 drivers/nvme/target/core.c      |  17 ++++-
 drivers/nvme/target/nvmet.h     |   3 +
 4 files changed, 143 insertions(+), 10 deletions(-)

-- 
2.37.0

[PATCH 1/3] nvmet: fix memory leak in configfs

[Aleksandr Miloserdov]

Since model_number is allocated before it needs to be freed before kmemdump_nul.

Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
Reviewed-by: Dmitriy Bogdanov <d.bogdanov@yadro.com>
Signed-off-by: Aleksandr Miloserdov <a.miloserdov@yadro.com>
---
 drivers/nvme/target/configfs.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index e34a2896fedb..89242b0f8614 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1215,6 +1215,7 @@ static ssize_t nvmet_subsys_attr_model_store_locked(struct nvmet_subsys *subsys,
 		const char *page, size_t count)
 {
 	int pos = 0, len;
+	char *val;
 
 	if (subsys->subsys_discovered) {
 		pr_err("Can't set model number. %s is already assigned\n",
@@ -1237,9 +1238,14 @@ static ssize_t nvmet_subsys_attr_model_store_locked(struct nvmet_subsys *subsys,
 			return -EINVAL;
 	}
 
-	subsys->model_number = kmemdup_nul(page, len, GFP_KERNEL);
-	if (!subsys->model_number)
+	val = kmemdup_nul(page, len, GFP_KERNEL);
+	if (!val)
 		return -ENOMEM;
+
+	kfree(subsys->model_number);
+
+	subsys->model_number = val;
+
 	return count;
 }
 
-- 
2.37.0

[PATCH 2/3] nvmet: expose IEEE OUI to configfs

[Aleksandr Miloserdov]

Allow user to set OUI for the controller vendor.

Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
Reviewed-by: Dmitriy Bogdanov <d.bogdanov@yadro.com>
Signed-off-by: Aleksandr Miloserdov <a.miloserdov@yadro.com>
---
 drivers/nvme/target/admin-cmd.c |  9 +++---
 drivers/nvme/target/configfs.c  | 49 +++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c      |  2 ++
 drivers/nvme/target/nvmet.h     |  1 +
 4 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index c8a061ce3ee5..0b415335bb5f 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -372,6 +372,10 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	memcpy_and_pad(id->fr, sizeof(id->fr),
 		       UTS_RELEASE, strlen(UTS_RELEASE), ' ');
 
+	id->ieee[0] = (subsys->ieee_oui) & 0xff;
+	id->ieee[1] = (subsys->ieee_oui >> 8) & 0xff;
+	id->ieee[2] = (subsys->ieee_oui >> 16) & 0xff;
+
 	id->rab = 6;
 
 	if (nvmet_is_disc_subsys(ctrl->subsys))
@@ -379,11 +383,6 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	else
 		id->cntrltype = NVME_CTRL_IO;
 
-	/*
-	 * XXX: figure out how we can assign a IEEE OUI, but until then
-	 * the safest is to leave it as zeroes.
-	 */
-
 	/* we support multiple ports, multiples hosts and ANA: */
 	id->cmic = NVME_CTRL_CMIC_MULTI_PORT | NVME_CTRL_CMIC_MULTI_CTRL |
 		NVME_CTRL_CMIC_ANA;
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 89242b0f8614..ad62e36ec997 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1265,6 +1265,54 @@ static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_model);
 
+static ssize_t nvmet_subsys_attr_ieee_oui_show(struct config_item *item,
+					    char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	return sysfs_emit(page, "0x%06x\n", subsys->ieee_oui);
+}
+
+static ssize_t nvmet_subsys_attr_ieee_oui_store_locked(struct nvmet_subsys *subsys,
+		const char *page, size_t count)
+{
+	uint32_t val = 0;
+	int ret;
+
+	if (subsys->subsys_discovered) {
+		pr_err("Can't set IEEE OUI. 0x%06x is already assigned\n",
+		      subsys->ieee_oui);
+		return -EINVAL;
+	}
+
+	ret = kstrtou32(page, 0, &val);
+	if (ret < 0)
+		return ret;
+
+	if (val >= 0x1000000)
+		return -EINVAL;
+
+	subsys->ieee_oui = val;
+
+	return count;
+}
+
+static ssize_t nvmet_subsys_attr_ieee_oui_store(struct config_item *item,
+					     const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+	ssize_t ret;
+
+	down_write(&nvmet_config_sem);
+	mutex_lock(&subsys->lock);
+	ret = nvmet_subsys_attr_ieee_oui_store_locked(subsys, page, count);
+	mutex_unlock(&subsys->lock);
+	up_write(&nvmet_config_sem);
+
+	return ret;
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_ieee_oui);
+
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 static ssize_t nvmet_subsys_attr_pi_enable_show(struct config_item *item,
 						char *page)
@@ -1323,6 +1371,7 @@ static struct configfs_attribute *nvmet_subsys_attrs[] = {
 	&nvmet_subsys_attr_attr_cntlid_max,
 	&nvmet_subsys_attr_attr_model,
 	&nvmet_subsys_attr_attr_qid_max,
+	&nvmet_subsys_attr_attr_ieee_oui,
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 	&nvmet_subsys_attr_attr_pi_enable,
 #endif
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index aecb5853f8da..ec697ceeac56 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1561,6 +1561,8 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 		goto free_subsys;
 	}
 
+	subsys->ieee_oui = 0;
+
 	switch (type) {
 	case NVME_NQN_NVME:
 		subsys->max_qid = NVMET_NR_QUEUES;
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index dfe3894205aa..1dc0ff8b2f3f 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -264,6 +264,7 @@ struct nvmet_subsys {
 	struct config_group	allowed_hosts_group;
 
 	char			*model_number;
+	u32			ieee_oui;
 
 #ifdef CONFIG_NVME_TARGET_PASSTHRU
 	struct nvme_ctrl	*passthru_ctrl;
-- 
2.37.0

[PATCH 3/3] nvmet: expose firmware revision to configfs

[Aleksandr Miloserdov]

Allow user to set currently active firmware revision

Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
Reviewed-by: Dmitriy Bogdanov <d.bogdanov@yadro.com>
Signed-off-by: Aleksandr Miloserdov <a.miloserdov@yadro.com>
---
 drivers/nvme/target/admin-cmd.c |  2 +-
 drivers/nvme/target/configfs.c  | 63 +++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c      | 15 ++++++--
 drivers/nvme/target/nvmet.h     |  2 ++
 4 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 0b415335bb5f..92098032c85d 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -370,7 +370,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	memcpy_and_pad(id->mn, sizeof(id->mn), subsys->model_number,
 		       strlen(subsys->model_number), ' ');
 	memcpy_and_pad(id->fr, sizeof(id->fr),
-		       UTS_RELEASE, strlen(UTS_RELEASE), ' ');
+		       subsys->firmware_rev, strlen(subsys->firmware_rev), ' ');
 
 	id->ieee[0] = (subsys->ieee_oui) & 0xff;
 	id->ieee[1] = (subsys->ieee_oui >> 8) & 0xff;
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index ad62e36ec997..1862b9479347 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1313,6 +1313,68 @@ static ssize_t nvmet_subsys_attr_ieee_oui_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_ieee_oui);
 
+static ssize_t nvmet_subsys_attr_firmware_show(struct config_item *item,
+					    char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	return sysfs_emit(page, "%s\n", subsys->firmware_rev);
+}
+
+static ssize_t nvmet_subsys_attr_firmware_store_locked(struct nvmet_subsys *subsys,
+		const char *page, size_t count)
+{
+	int pos = 0, len;
+	char *val;
+
+	if (subsys->subsys_discovered) {
+		pr_err("Can't set firmware revision. %s is already assigned\n",
+		       subsys->firmware_rev);
+		return -EINVAL;
+	}
+
+	len = strcspn(page, "\n");
+	if (!len)
+		return -EINVAL;
+
+	if (len > NVMET_FR_MAX_SIZE) {
+		pr_err("Firmware revision size can not exceed %d Bytes\n",
+		       NVMET_FR_MAX_SIZE);
+		return -EINVAL;
+	}
+
+	for (pos = 0; pos < len; pos++) {
+		if (!nvmet_is_ascii(page[pos]))
+			return -EINVAL;
+	}
+
+	val = kmemdup_nul(page, len, GFP_KERNEL);
+	if (!val)
+		return -ENOMEM;
+
+	kfree(subsys->firmware_rev);
+
+	subsys->firmware_rev = val;
+
+	return count;
+}
+
+static ssize_t nvmet_subsys_attr_firmware_store(struct config_item *item,
+					     const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+	ssize_t ret;
+
+	down_write(&nvmet_config_sem);
+	mutex_lock(&subsys->lock);
+	ret = nvmet_subsys_attr_firmware_store_locked(subsys, page, count);
+	mutex_unlock(&subsys->lock);
+	up_write(&nvmet_config_sem);
+
+	return ret;
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_firmware);
+
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 static ssize_t nvmet_subsys_attr_pi_enable_show(struct config_item *item,
 						char *page)
@@ -1372,6 +1434,7 @@ static struct configfs_attribute *nvmet_subsys_attrs[] = {
 	&nvmet_subsys_attr_attr_model,
 	&nvmet_subsys_attr_attr_qid_max,
 	&nvmet_subsys_attr_attr_ieee_oui,
+	&nvmet_subsys_attr_attr_firmware,
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 	&nvmet_subsys_attr_attr_pi_enable,
 #endif
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index ec697ceeac56..98d41bbd3f72 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -10,6 +10,8 @@
 #include <linux/pci-p2pdma.h>
 #include <linux/scatterlist.h>
 
+#include <generated/utsrelease.h>
+
 #define CREATE_TRACE_POINTS
 #include "trace.h"
 
@@ -1563,6 +1565,12 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 
 	subsys->ieee_oui = 0;
 
+	subsys->firmware_rev = kstrndup(UTS_RELEASE, NVMET_FR_MAX_SIZE, GFP_KERNEL);
+	if (!subsys->firmware_rev) {
+		ret = -ENOMEM;
+		goto free_mn;
+	}
+
 	switch (type) {
 	case NVME_NQN_NVME:
 		subsys->max_qid = NVMET_NR_QUEUES;
@@ -1574,14 +1582,14 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 	default:
 		pr_err("%s: Unknown Subsystem type - %d\n", __func__, type);
 		ret = -EINVAL;
-		goto free_mn;
+		goto free_fr;
 	}
 	subsys->type = type;
 	subsys->subsysnqn = kstrndup(subsysnqn, NVMF_NQN_SIZE,
 			GFP_KERNEL);
 	if (!subsys->subsysnqn) {
 		ret = -ENOMEM;
-		goto free_mn;
+		goto free_fr;
 	}
 	subsys->cntlid_min = NVME_CNTLID_MIN;
 	subsys->cntlid_max = NVME_CNTLID_MAX;
@@ -1594,6 +1602,8 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 
 	return subsys;
 
+free_fr:
+	kfree(subsys->firmware_rev);
 free_mn:
 	kfree(subsys->model_number);
 free_subsys:
@@ -1613,6 +1623,7 @@ static void nvmet_subsys_free(struct kref *ref)
 
 	kfree(subsys->subsysnqn);
 	kfree(subsys->model_number);
+	kfree(subsys->firmware_rev);
 	kfree(subsys);
 }
 
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 1dc0ff8b2f3f..dd8985b54a84 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -29,6 +29,7 @@
 #define NVMET_DEFAULT_CTRL_MODEL	"Linux"
 #define NVMET_MN_MAX_SIZE		40
 #define NVMET_SN_MAX_SIZE		20
+#define NVMET_FR_MAX_SIZE		8
 
 /*
  * Supported optional AENs:
@@ -265,6 +266,7 @@ struct nvmet_subsys {
 
 	char			*model_number;
 	u32			ieee_oui;
+	char			*firmware_rev;
 
 #ifdef CONFIG_NVME_TARGET_PASSTHRU
 	struct nvme_ctrl	*passthru_ctrl;
-- 
2.37.0

Re: [PATCH 1/3] nvmet: fix memory leak in configfs

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 2/3] nvmet: expose IEEE OUI to configfs

[Sagi Grimberg]

On 10/26/22 11:31, Aleksandr Miloserdov wrote:
> Allow user to set OUI for the controller vendor.
> 
> Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
> Reviewed-by: Dmitriy Bogdanov <d.bogdanov@yadro.com>
> Signed-off-by: Aleksandr Miloserdov <a.miloserdov@yadro.com>
> ---
>   drivers/nvme/target/admin-cmd.c |  9 +++---
>   drivers/nvme/target/configfs.c  | 49 +++++++++++++++++++++++++++++++++
>   drivers/nvme/target/core.c      |  2 ++
>   drivers/nvme/target/nvmet.h     |  1 +
>   4 files changed, 56 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index c8a061ce3ee5..0b415335bb5f 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -372,6 +372,10 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
>   	memcpy_and_pad(id->fr, sizeof(id->fr),
>   		       UTS_RELEASE, strlen(UTS_RELEASE), ' ');
>   
> +	id->ieee[0] = (subsys->ieee_oui) & 0xff;
> +	id->ieee[1] = (subsys->ieee_oui >> 8) & 0xff;
> +	id->ieee[2] = (subsys->ieee_oui >> 16) & 0xff;
> +

put_unaligned_24 ?

Re: [PATCH 2/3] nvmet: expose IEEE OUI to configfs

[Aleksandr Miloserdov]

> On 26.10.2022, 14:49, "Sagi Grimberg" <sagi@grimberg.me> wrote:
>    On 10/26/22 11:31, Aleksandr Miloserdov wrote:
> > Allow user to set OUI for the controller vendor.
> >
> > Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
> > Reviewed-by: Dmitriy Bogdanov <d.bogdanov@yadro.com>
> > Signed-off-by: Aleksandr Miloserdov <a.miloserdov@yadro.com>
> > ---
> >   drivers/nvme/target/admin-cmd.c |  9 +++---
> >   drivers/nvme/target/configfs.c  | 49 +++++++++++++++++++++++++++++++++
> >   drivers/nvme/target/core.c      |  2 ++
> >   drivers/nvme/target/nvmet.h     |  1 +
> >   4 files changed, 56 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> > index c8a061ce3ee5..0b415335bb5f 100644
> > --- a/drivers/nvme/target/admin-cmd.c
> > +++ b/drivers/nvme/target/admin-cmd.c
> > @@ -372,6 +372,10 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
> >       memcpy_and_pad(id->fr, sizeof(id->fr),
> >                      UTS_RELEASE, strlen(UTS_RELEASE), ' ');
> >
> > +     id->ieee[0] = (subsys->ieee_oui) & 0xff;
> > +     id->ieee[1] = (subsys->ieee_oui >> 8) & 0xff;
> > +     id->ieee[2] = (subsys->ieee_oui >> 16) & 0xff;
> > +
>
>    put_unaligned_24 ?

Seems like a good idea, I'll add it in the second version of the patch.
Did you happen to check out the third patch from the series?

Re: [PATCH 2/3] nvmet: expose IEEE OUI to configfs

[Konstantin Shelekhin]

On Wed, Oct 26, 2022 at 01:49:17PM +0300, Sagi Grimberg wrote:
> On 10/26/22 11:31, Aleksandr Miloserdov wrote:
> > Allow user to set OUI for the controller vendor.
> > 
> > Reviewed-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
> > Reviewed-by: Dmitriy Bogdanov <d.bogdanov@yadro.com>
> > Signed-off-by: Aleksandr Miloserdov <a.miloserdov@yadro.com>
> > ---
> >   drivers/nvme/target/admin-cmd.c |  9 +++---
> >   drivers/nvme/target/configfs.c  | 49 +++++++++++++++++++++++++++++++++
> >   drivers/nvme/target/core.c      |  2 ++
> >   drivers/nvme/target/nvmet.h     |  1 +
> >   4 files changed, 56 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> > index c8a061ce3ee5..0b415335bb5f 100644
> > --- a/drivers/nvme/target/admin-cmd.c
> > +++ b/drivers/nvme/target/admin-cmd.c
> > @@ -372,6 +372,10 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
> >       memcpy_and_pad(id->fr, sizeof(id->fr),
> >                      UTS_RELEASE, strlen(UTS_RELEASE), ' ');
> > 
> > +     id->ieee[0] = (subsys->ieee_oui) & 0xff;
> > +     id->ieee[1] = (subsys->ieee_oui >> 8) & 0xff;
> > +     id->ieee[2] = (subsys->ieee_oui >> 16) & 0xff;
> > +
> 
> put_unaligned_24 ?

While there: we have a little internal bikeshedding on how to actually
name the ConfigFS parameter :D

Weirdly in this part of the spec it's called IEEE instead of OUI,
however in other parts (like EUI64 generation) it's referenced as OUI.
To the end user it makes more sense to call it simply OUI. So basically
we have:

  attr_ieee
  attr_oui
  attr_ieee_oui

Purely a style nitpicking, but what's your stance?

Re: [PATCH 2/3] nvmet: expose IEEE OUI to configfs

[Christoph Hellwig]

On Fri, Oct 28, 2022 at 02:33:35PM +0300, Konstantin Shelekhin wrote:
> While there: we have a little internal bikeshedding on how to actually
> name the ConfigFS parameter :D
> 
> Weirdly in this part of the spec it's called IEEE instead of OUI,
> however in other parts (like EUI64 generation) it's referenced as OUI.
> To the end user it makes more sense to call it simply OUI. So basically
> we have:
> 
>   attr_ieee
>   attr_oui
>   attr_ieee_oui
> 
> Purely a style nitpicking, but what's your stance?

The NVMe naming is indeed rather odd.  I'd be tempted to just follow
it anyway.

Re: [PATCH 1/3] nvmet: fix memory leak in configfs

[Christoph Hellwig]

Thanks,

I've applied this bug fix to the nvme-6.1 branch.