Re: [PATCH 1/1] nvmet: allow setting model_number once

[Christoph Hellwig <hch@lst.de>]

So looking at this and Chaitanya patch I think this version simplifies
things quite nicely, and it also happens to get rid of the RCU annotation
sparse warnings.

Let me know what you think of the incremental cleanup and micro-opimization
below, though:


diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 3da285e22e9209..2ca13dd7e7b88a 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -313,34 +313,40 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 	nvmet_req_complete(req, NVME_SC_INVALID_FIELD | NVME_SC_DNR);
 }
 
-static int nvmet_id_set_model_number(struct nvme_id_ctrl *id,
-				      struct nvmet_subsys *subsys)
+static u16 nvmet_set_model_number(struct nvme_id_ctrl *id,
+		struct nvmet_subsys *subsys)
 {
-	int ret = 0;
+	u16 status = 0;
 
 	mutex_lock(&subsys->lock);
 	if (!subsys->model_number) {
-		subsys->model_number = kstrdup(NVMET_DEFAULT_CTRL_MODEL,
-					       GFP_KERNEL);
-		if (!subsys->model_number) {
-			ret = -ENOMEM;
-			goto out_unlock;
-		}
+		subsys->model_number =
+			kstrdup(NVMET_DEFAULT_CTRL_MODEL, GFP_KERNEL);
+		if (!subsys->model_number)
+			status = NVME_SC_INTERNAL;
 	}
-	memcpy_and_pad(id->mn, sizeof(id->mn), subsys->model_number,
-		       strlen(subsys->model_number), ' ');
-out_unlock:
 	mutex_unlock(&subsys->lock);
-	return ret;
+
+	return status;
 }
 
 static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
+	struct nvmet_subsys *subsys = ctrl->subsys;
 	struct nvme_id_ctrl *id;
 	u32 cmd_capsule_size;
 	u16 status = 0;
-	int ret;
+
+	/*
+	 * If there is no model number yet, so it now.  It will then remain
+	 * stable for the life time of the subsystem.
+	 */
+	if (!subsys->model_number) {
+		status = nvmet_set_model_number(id, subsys);
+		if (status)
+			goto out;
+	}
 
 	id = kzalloc(sizeof(*id), GFP_KERNEL);
 	if (!id) {
@@ -355,11 +361,8 @@ 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));
-	ret = nvmet_id_set_model_number(id, ctrl->subsys);
-	if (ret) {
-		status = NVME_SC_INTERNAL;
-		goto out_free;
-	}
+	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), ' ');
 
@@ -467,7 +470,6 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 
 	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
 
-out_free:
 	kfree(id);
 out:
 	nvmet_req_complete(req, status);
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 71a1fe2f6330a9..e5dbd1923b7b75 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1118,13 +1118,11 @@ static ssize_t nvmet_subsys_attr_model_show(struct config_item *item,
 					    char *page)
 {
 	struct nvmet_subsys *subsys = to_subsys(item);
-	char *model = "";
 	int ret;
 
 	mutex_lock(&subsys->lock);
-	if (subsys->model_number)
-		model = subsys->model_number;
-	ret = snprintf(page, PAGE_SIZE, "%s\n", model);
+	ret = snprintf(page, PAGE_SIZE, "%s\n", subsys->model_number ?
+			subsys->model_number : NVMET_DEFAULT_CTRL_MODEL);
 	mutex_unlock(&subsys->lock);
 
 	return ret;
@@ -1136,54 +1134,45 @@ static bool nvmet_is_ascii(const char c)
 	return c >= 0x20 && c <= 0x7e;
 }
 
-static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
-					     const char *page, size_t count)
+static ssize_t nvmet_subsys_attr_model_store_locked(struct nvmet_subsys *subsys,
+		const char *page, size_t count)
 {
-	struct nvmet_subsys *subsys = to_subsys(item);
-	char *new_model_number;
 	int pos = 0, len;
 
-	down_write(&nvmet_config_sem);
-	mutex_lock(&subsys->lock);
 	if (subsys->model_number) {
 		pr_err("Can't set model number. %s is already assigned\n",
 		       subsys->model_number);
-		count = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	len = strcspn(page, "\n");
-	if (!len) {
-		count = -EINVAL;
-		goto out_unlock;
-	}
+	if (!len)
+		return -EINVAL;
 
 	for (pos = 0; pos < len; pos++) {
-		if (!nvmet_is_ascii(page[pos])) {
-			count = -EINVAL;
-			goto out_unlock;
-		}
+		if (!nvmet_is_ascii(page[pos]))
+			return -EINVAL;
 	}
 
-	new_model_number = kmemdup_nul(page, len, GFP_KERNEL);
-	if (!new_model_number) {
-		count = -ENOMEM;
-		goto out_unlock;
-	}
+	subsys->model_number = kmemdup_nul(page, len, GFP_KERNEL);
+	if (!subsys->model_number)
+		return -ENOMEM;
+	return count;
+}
 
-	subsys->model_number = kstrdup(new_model_number, GFP_KERNEL);
-	if (!subsys->model_number) {
-		count = -ENOMEM;
-		kfree(new_model_number);
-		goto out_unlock;
-	}
+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);
+	ssize_t ret;
 
-	kfree(new_model_number);
-out_unlock:
+	down_write(&nvmet_config_sem);
+	mutex_lock(&subsys->lock);
+	ret = nvmet_subsys_attr_model_store_locked(subsys, page, count);
 	mutex_unlock(&subsys->lock);
 	up_write(&nvmet_config_sem);
 
-	return count;
+	return ret;
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_model);
 

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