linux-nvme.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Mark Ruijter <MRuijter@onestopsystems.com>
To: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>,
	"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>
Cc: "hch@lst.de" <hch@lst.de>, "sagi@grimberg.me" <sagi@grimberg.me>
Subject: Re: [PATCH] nvmet: make ctrl model configurable
Date: Mon, 11 Nov 2019 11:09:18 +0000	[thread overview]
Message-ID: <6A0D52BF-0ADC-4C82-9AF1-A04067E32663@onestopsystems.com> (raw)
In-Reply-To: <20191101080855.17970-1-chaitanya.kulkarni@wdc.com>

[-- Attachment #1: Type: text/plain, Size: 8815 bytes --]

Hi Chaitanya,

I just removed my original patch from my test system that I use and replaced it with the patch that you submitted a while ago.
When trying to use it I noticed it is flawed.

Any attempt to change the model will return 'invalid argument'.
The ret variable is initialized as '-EINVAL' by default and never set to 0 when the input was valid.

The minimum change required to solve the problem would be :
diff -u configfs.c configfs.c-orig 
--- configfs.c	2019-11-11 10:16:14.970374663 +0100
+++ configfs.c-orig	2019-11-11 10:05:27.754635151 +0100
@@ -874,7 +874,7 @@
 					     const char *page, size_t count)
 {
 	struct nvmet_subsys *subsys = to_subsys(item);
-	int ret = 0, pos, len;
+	int ret = -EINVAL, pos, len;
 	char c;
 
 	down_write(&nvmet_config_sem);

Looking at this code once more I would probably simplify it like this:

diff --git a/configfs.c-orig b/configfs.c
index 1d64059..d2e7935 100644
--- a/configfs.c-orig
+++ b/configfs.c
@@ -879,28 +879,26 @@ static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
 
        down_write(&nvmet_config_sem);
        len = strcspn(page, "\n");
-       if (!len) {
-               ret = -EINVAL;
+       if (!len)
                goto out_unlock;
-       }
 
        /* Only 20h (space) until 7eh (~) is allowed */
        for (pos = 0; pos < len; pos++) {
                c = page[pos];
-               if (c < 0x20 || c > 0x7e) {
-                       ret = -EINVAL;
+               if (c < 0x20 || c > 0x7e)
                        goto out_unlock;
-               }
        }
 
        kfree(subsys->model);
        subsys->model = kstrndup(page, len, GFP_KERNEL);
        if (!subsys->model)
                ret = -ENOMEM;
+       else
+               ret = count;
 
 out_unlock:
        up_write(&nvmet_config_sem);
-       return ret ? ret : count;
+       return ret;
 }
 
 CONFIGFS_ATTR(nvmet_subsys_, attr_model);

---

This shortens the routine with 2 lines.
I tested both changes and they work:

dev:/sys/kernel/config/nvmet/subsystems/loop # echo -e "\t model" > attr_model 
-bash: echo: write error: Invalid argument
dev:/sys/kernel/config/nvmet/subsystems/loop # echo -e "newmodel" > attr_model

I have attached the tested patch with the shortened routine to this email.

Thanks,

Mark



This email message is for the sole use of the intended recipient(s) and contains confidential information.  Any unauthorized use, disclosure or distribution is prohibited.  If you are not an intended recipient, please contact the sender by reply email and destroy all copies of the original message.

Op 01-11-19 09:09 heeft Linux-nvme namens Chaitanya Kulkarni <linux-nvme-bounces@lists.infradead.org namens chaitanya.kulkarni@wdc.com> geschreven:

    From: Mark Ruijter <MRuijter@onestopsystems.com>
    
    This patch adds a new target subsys attribute which allows user to
    optionally specify model name which then used in the
    nvmet_execute_identify_ctrl() to fill up the nvme_id_ctrl structure.
    
    The default value for the model is set to "Linux" for backward
    compatibility.
    
    Signed-off-by: Mark Ruijter <MRuijter@onestopsystems.com>
    Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
    ---
    Hi,
    
    This patch is originally posted by Mark and later same day by me with
    the same concept. I've merged changes from both patch into one and
    cleaned the code to fit the kernel coding standards.
    
    -Regards,
    Chaitanya
    ---
     drivers/nvme/target/admin-cmd.c |  4 +--
     drivers/nvme/target/configfs.c  | 45 +++++++++++++++++++++++++++++++++
     drivers/nvme/target/core.c      |  7 +++++
     drivers/nvme/target/nvmet.h     |  2 ++
     4 files changed, 56 insertions(+), 2 deletions(-)
    
    diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
    index 56c21b501185..917d8f3fded7 100644
    --- a/drivers/nvme/target/admin-cmd.c
    +++ b/drivers/nvme/target/admin-cmd.c
    @@ -315,9 +315,9 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
     static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
     {
     	struct nvmet_ctrl *ctrl = req->sq->ctrl;
    +	const char *model = ctrl->subsys->model;
     	struct nvme_id_ctrl *id;
     	u16 status = 0;
    -	const char model[] = "Linux";
     
     	id = kzalloc(sizeof(*id), GFP_KERNEL);
     	if (!id) {
    @@ -332,7 +332,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
     	memset(id->sn, ' ', sizeof(id->sn));
     	bin2hex(id->sn, &ctrl->subsys->serial,
     		min(sizeof(ctrl->subsys->serial), sizeof(id->sn) / 2));
    -	memcpy_and_pad(id->mn, sizeof(id->mn), model, sizeof(model) - 1, ' ');
    +	memcpy_and_pad(id->mn, sizeof(id->mn), model, strlen(model), ' ');
     	memcpy_and_pad(id->fr, sizeof(id->fr),
     		       UTS_RELEASE, strlen(UTS_RELEASE), ' ');
     
    diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
    index 98613a45bd3b..faf34b15b975 100644
    --- a/drivers/nvme/target/configfs.c
    +++ b/drivers/nvme/target/configfs.c
    @@ -862,10 +862,54 @@ static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
     }
     CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
     
    +static ssize_t nvmet_subsys_attr_model_show(struct config_item *item,
    +					    char *page)
    +{
    +	struct nvmet_subsys *subsys = to_subsys(item);
    +
    +	return snprintf(page, PAGE_SIZE, "%s\n", subsys->model);
    +}
    +
    +static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
    +					     const char *page, size_t count)
    +{
    +	struct nvmet_subsys *subsys = to_subsys(item);
    +	int ret = -EINVAL, pos, len;
    +	char c;
    +
    +	down_write(&nvmet_config_sem);
    +	len = strcspn(page, "\n");
    +	if (!len) {
    +		ret = -EINVAL;
    +		goto out_unlock;
    +	}
    +
    +	/* Only 20h (space) until 7eh (~) is allowed */
    +	for (pos = 0; pos < len; pos++) {
    +		c = page[pos];
    +		if (c < 0x20 || c > 0x7e) {
    +			ret = -EINVAL;
    +			goto out_unlock;
    +		}
    +	}
    +
    +	kfree(subsys->model);
    +	subsys->model = kstrndup(page, len, GFP_KERNEL);
    +	if (!subsys->model)
    +		ret = -ENOMEM;
    +
    +out_unlock:
    +	up_write(&nvmet_config_sem);
    +	return ret ? ret : count;
    +}
    +
    +CONFIGFS_ATTR(nvmet_subsys_, attr_model);
    +
     static struct configfs_attribute *nvmet_subsys_attrs[] = {
     	&nvmet_subsys_attr_attr_allow_any_host,
     	&nvmet_subsys_attr_attr_version,
     	&nvmet_subsys_attr_attr_serial,
    +	&nvmet_subsys_attr_attr_model,
     	NULL,
     };
     
    @@ -901,6 +945,7 @@ static struct config_group *nvmet_subsys_make(struct config_group *group,
     	}
     
     	subsys = nvmet_subsys_alloc(name, NVME_NQN_NVME);
    +	subsys->model = kstrdup(NVMET_DEFAULT_CTRL_MODEL, GFP_KERNEL);
     	if (IS_ERR(subsys))
     		return ERR_CAST(subsys);
     
    diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
    index 28438b833c1b..adc1eb667601 100644
    --- a/drivers/nvme/target/core.c
    +++ b/drivers/nvme/target/core.c
    @@ -1416,6 +1416,12 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
     		kfree(subsys);
     		return ERR_PTR(-ENOMEM);
     	}
    +	subsys->model = kstrdup(NVMET_DEFAULT_CTRL_MODEL, GFP_KERNEL);
    +	if (!subsys->model) {
    +		kfree(subsys->subsysnqn);
    +		kfree(subsys);
    +		return ERR_PTR(-ENOMEM);
    +	}
     
     	kref_init(&subsys->ref);
     
    @@ -1435,6 +1441,7 @@ static void nvmet_subsys_free(struct kref *ref)
     	WARN_ON_ONCE(!list_empty(&subsys->namespaces));
     
     	kfree(subsys->subsysnqn);
    +	kfree(subsys->model);
     	kfree(subsys);
     }
     
    diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
    index 46df45e837c9..afb6116de892 100644
    --- a/drivers/nvme/target/nvmet.h
    +++ b/drivers/nvme/target/nvmet.h
    @@ -23,6 +23,7 @@
     #define NVMET_ASYNC_EVENTS		4
     #define NVMET_ERROR_LOG_SLOTS		128
     #define NVMET_NO_ERROR_LOC		((u16)-1)
    +#define NVMET_DEFAULT_CTRL_MODEL       "Linux"
     
     /*
      * Supported optional AENs:
    @@ -222,6 +223,7 @@ struct nvmet_subsys {
     	u64			ver;
     	u64			serial;
     	char			*subsysnqn;
    +	char			*model;
     
     	struct config_group	group;
     
    -- 
    2.22.1
    
    
    _______________________________________________
    Linux-nvme mailing list
    Linux-nvme@lists.infradead.org
    http://lists.infradead.org/mailman/listinfo/linux-nvme
    


[-- Attachment #2: model_patch_patched.diff --]
[-- Type: application/octet-stream, Size: 3903 bytes --]

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 51800a9..1f6f7d9 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -287,9 +287,9 @@ out:
 static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
+	const char *model = ctrl->subsys->model;
 	struct nvme_id_ctrl *id;
 	u16 status = 0;
-	const char model[] = "Linux";
 
 	id = kzalloc(sizeof(*id), GFP_KERNEL);
 	if (!id) {
@@ -304,7 +304,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	memset(id->sn, ' ', sizeof(id->sn));
 	bin2hex(id->sn, &ctrl->subsys->serial,
 		min(sizeof(ctrl->subsys->serial), sizeof(id->sn) / 2));
-	memcpy_and_pad(id->mn, sizeof(id->mn), model, sizeof(model) - 1, ' ');
+	memcpy_and_pad(id->mn, sizeof(id->mn), model, strlen(model), ' ');
 	memcpy_and_pad(id->fr, sizeof(id->fr),
 		       UTS_RELEASE, strlen(UTS_RELEASE), ' ');
 
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 98613a4..ef62853 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -862,10 +862,52 @@ static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
 
+static ssize_t nvmet_subsys_attr_model_show(struct config_item *item,
+					    char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	return snprintf(page, PAGE_SIZE, "%s\n", subsys->model);
+}
+
+static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
+					     const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+	int ret = -EINVAL, pos, len;
+	char c;
+
+	down_write(&nvmet_config_sem);
+	len = strcspn(page, "\n");
+	if (!len)
+		goto out_unlock;
+
+	/* Only 20h (space) until 7eh (~) is allowed */
+	for (pos = 0; pos < len; pos++) {
+		c = page[pos];
+		if (c < 0x20 || c > 0x7e)
+			goto out_unlock;
+	}
+
+	kfree(subsys->model);
+	subsys->model = kstrndup(page, len, GFP_KERNEL);
+	if (!subsys->model)
+		ret = -ENOMEM;
+	else
+		ret = count;
+
+out_unlock:
+	up_write(&nvmet_config_sem);
+	return ret;
+}
+
+CONFIGFS_ATTR(nvmet_subsys_, attr_model);
+
 static struct configfs_attribute *nvmet_subsys_attrs[] = {
 	&nvmet_subsys_attr_attr_allow_any_host,
 	&nvmet_subsys_attr_attr_version,
 	&nvmet_subsys_attr_attr_serial,
+	&nvmet_subsys_attr_attr_model,
 	NULL,
 };
 
@@ -901,6 +943,7 @@ static struct config_group *nvmet_subsys_make(struct config_group *group,
 	}
 
 	subsys = nvmet_subsys_alloc(name, NVME_NQN_NVME);
+	subsys->model = kstrdup(NVMET_DEFAULT_CTRL_MODEL, GFP_KERNEL);
 	if (IS_ERR(subsys))
 		return ERR_CAST(subsys);
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 3a67e24..5dc90ba 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1418,6 +1418,12 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 		kfree(subsys);
 		return ERR_PTR(-ENOMEM);
 	}
+	subsys->model = kstrdup(NVMET_DEFAULT_CTRL_MODEL, GFP_KERNEL);
+	if (!subsys->model) {
+		kfree(subsys->subsysnqn);
+		kfree(subsys);
+		return ERR_PTR(-ENOMEM);
+	}
 
 	kref_init(&subsys->ref);
 
@@ -1437,6 +1443,7 @@ static void nvmet_subsys_free(struct kref *ref)
 	WARN_ON_ONCE(!list_empty(&subsys->namespaces));
 
 	kfree(subsys->subsysnqn);
+	kfree(subsys->model);
 	kfree(subsys);
 }
 
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index c51f8dd..fd0d89a 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -23,6 +23,7 @@
 #define NVMET_ASYNC_EVENTS		4
 #define NVMET_ERROR_LOG_SLOTS		128
 #define NVMET_NO_ERROR_LOC		((u16)-1)
+#define NVMET_DEFAULT_CTRL_MODEL       "Linux"
 
 /*
  * Supported optional AENs:
@@ -222,6 +223,7 @@ struct nvmet_subsys {
 	u64			ver;
 	u64			serial;
 	char			*subsysnqn;
+	char			*model;
 
 	struct config_group	group;
 

[-- Attachment #3: Type: text/plain, Size: 158 bytes --]

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

  parent reply	other threads:[~2019-11-11 11:09 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-01  8:08 [PATCH] nvmet: make ctrl model configurable Chaitanya Kulkarni
2019-11-11 10:34 ` Christoph Hellwig
2019-11-11 13:31   ` Mark Ruijter
2019-11-12  7:07     ` Chaitanya Kulkarni
2019-11-12  7:06   ` Chaitanya Kulkarni
2019-11-11 11:09 ` Mark Ruijter [this message]
  -- strict thread matches above, loose matches on Subject: below --
2019-10-30 19:48 Chaitanya Kulkarni
2019-10-30 20:08 ` Mark Ruijter
2019-10-30 20:09   ` Chaitanya Kulkarni
2019-10-30 21:43   ` Chaitanya Kulkarni

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6A0D52BF-0ADC-4C82-9AF1-A04067E32663@onestopsystems.com \
    --to=mruijter@onestopsystems.com \
    --cc=chaitanya.kulkarni@wdc.com \
    --cc=hch@lst.de \
    --cc=linux-nvme@lists.infradead.org \
    --cc=sagi@grimberg.me \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).