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

[PATCH v2 0/2] 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 (2):
  nvmet: expose IEEE OUI to configfs
  nvmet: expose firmware revision to configfs

 drivers/nvme/target/admin-cmd.c |   9 +--
 drivers/nvme/target/configfs.c  | 112 ++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c      |  17 ++++-
 drivers/nvme/target/nvmet.h     |   3 +
 4 files changed, 133 insertions(+), 8 deletions(-)

-- 
2.38.1

[PATCH v2 1/2] 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 |  7 ++---
 drivers/nvme/target/configfs.c  | 49 +++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c      |  2 ++
 drivers/nvme/target/nvmet.h     |  1 +
 4 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index c8a061ce3ee5..48a2f587f38a 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -372,6 +372,8 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	memcpy_and_pad(id->fr, sizeof(id->fr),
 		       UTS_RELEASE, strlen(UTS_RELEASE), ' ');
 
+	put_unaligned_le24(subsys->ieee_oui, id->ieee);
+
 	id->rab = 6;
 
 	if (nvmet_is_disc_subsys(ctrl->subsys))
@@ -379,11 +381,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.38.1

[PATCH v2 2/2] 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 48a2f587f38a..6b46f90a63cf 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), ' ');
 
 	put_unaligned_le24(subsys->ieee_oui, id->ieee);
 
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.38.1

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

[Sagi Grimberg]

> 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 |  7 ++---
>   drivers/nvme/target/configfs.c  | 49 +++++++++++++++++++++++++++++++++
>   drivers/nvme/target/core.c      |  2 ++
>   drivers/nvme/target/nvmet.h     |  1 +
>   4 files changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index c8a061ce3ee5..48a2f587f38a 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -372,6 +372,8 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
>   	memcpy_and_pad(id->fr, sizeof(id->fr),
>   		       UTS_RELEASE, strlen(UTS_RELEASE), ' ');
>   
> +	put_unaligned_le24(subsys->ieee_oui, id->ieee);
> +

Any particular reason why it is placed here and not where the comment
was?

>   	id->rab = 6;
>   
>   	if (nvmet_is_disc_subsys(ctrl->subsys))
> @@ -379,11 +381,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;

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

[Christoph Hellwig]

On Tue, Nov 15, 2022 at 04:05:14PM +0200, Sagi Grimberg wrote:
>> index c8a061ce3ee5..48a2f587f38a 100644
>> --- a/drivers/nvme/target/admin-cmd.c
>> +++ b/drivers/nvme/target/admin-cmd.c
>> @@ -372,6 +372,8 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
>>   	memcpy_and_pad(id->fr, sizeof(id->fr),
>>   		       UTS_RELEASE, strlen(UTS_RELEASE), ' ');
>>   +	put_unaligned_le24(subsys->ieee_oui, id->ieee);
>> +
>
> Any particular reason why it is placed here and not where the comment
> was?

Keeping the ID assignment together seems pretty reasonable to me.

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

[Aleksandr Miloserdov]

On 16.11.2022, 09:58, "Christoph Hellwig" <hch@lst.de> wrote:
>    On Tue, Nov 15, 2022 at 04:05:14PM +0200, Sagi Grimberg wrote:
>    >> index c8a061ce3ee5..48a2f587f38a 100644
>    >> --- a/drivers/nvme/target/admin-cmd.c
>    >> +++ b/drivers/nvme/target/admin-cmd.c
>    >> @@ -372,6 +372,8 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
>    >>      memcpy_and_pad(id->fr, sizeof(id->fr),
>    >>                     UTS_RELEASE, strlen(UTS_RELEASE), ' ');
>    >>   +  put_unaligned_le24(subsys->ieee_oui, id->ieee);
>    >> +
>    >
>    > Any particular reason why it is placed here and not where the comment
>    > was?
>
>    Keeping the ID assignment together seems pretty reasonable to me.

Also assigning adjacent struct members in one chunk looks neat to me.

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

[Sagi Grimberg]

>>     On Tue, Nov 15, 2022 at 04:05:14PM +0200, Sagi Grimberg wrote:
>>     >> index c8a061ce3ee5..48a2f587f38a 100644
>>     >> --- a/drivers/nvme/target/admin-cmd.c
>>     >> +++ b/drivers/nvme/target/admin-cmd.c
>>     >> @@ -372,6 +372,8 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
>>     >>      memcpy_and_pad(id->fr, sizeof(id->fr),
>>     >>                     UTS_RELEASE, strlen(UTS_RELEASE), ' ');
>>     >>   +  put_unaligned_le24(subsys->ieee_oui, id->ieee);
>>     >> +
>>     >
>>     > Any particular reason why it is placed here and not where the comment
>>     > was?
>>
>>     Keeping the ID assignment together seems pretty reasonable to me.
> 
> Also assigning adjacent struct members in one chunk looks neat to me.
> 

That's fine, just wanted to make sure.
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>