Skip to contentSkip to conversation

NVMe: write_long SCSI to NVMe translation implementation

PATCH Superseded Sunad Bhandary S 3 participants 7 messages Mar 23, 2015 – Apr 16, 2015

Subsystems (2)
Block layer NVMe
lore
Share & tools
lore

Sunad Bhandary S

author
#0 lore raw

PATCH v3 1/1 NVMe: write_long SCSI to NVMe translation implementation

From: Sunad Bhandary S <[email protected]>

This patch implements the SCSI to NVMe translation for write_long. write_long is translated to the NVMe command write_uncorrectable as defined by the translation specification version 1.4.

This patch also indicates the device support for write_uncorrectable method in the response of extended inquiry as defined in the translation spec.

This patch is based on for-linus branch.

Changes:
v1 -> v2 : __le32 nlb field changed to __le16 nlb followed by __u16 rsvd.
v2 -> v3 : Check for SAO16 value for write_long_16.

Signed-off-by Sunad Bhandary S <sunad.s at samsung.com>
---
 drivers/block/nvme-scsi.c | 78 +++++++++++++++++++++++++++++++++++++++++++++--
 include/uapi/linux/nvme.h | 13 ++++++++
 2 files changed, 89 insertions(+), 2 deletions(-)
drivers/block/nvme-scsi.c +76 -2
index e10196e..3b778e4 100644
@@ -245,6 +245,17 @@ static int sg_version_num = 30534;	/* 2 digits for each component */
 /* Report LUNs defines */
 #define REPORT_LUNS_FIRST_LUN_OFFSET			8
 
+/*Write Long defines */
+#define WRITE_LONG_CDB_COR_DIS_OFFSET			1
+#define WRITE_LONG_CDB_COR_DIS_MASK			0x80
+#define WRITE_LONG_CDB_WR_UNCOR_OFFSET			1
+#define WRITE_LONG_CDB_WR_UNCOR_MASK			0x40
+#define WRITE_LONG_CDB_PBLOCK_OFFSET			1
+#define WRITE_LONG_CDB_PBLOCK_MASK			0x20
+#define WRITE_LONG_CDB_LBA_OFFSET			2
+#define WRITE_LONG_SAO_MASK				0x1F
+#define SAO_WRITE_LONG_16				0x11
+
 /* SCSI ADDITIONAL SENSE Codes */
 
 #define SCSI_ASC_NO_SENSE				0x00

@@ -331,6 +342,10 @@ INQUIRY_EVPD_BIT_MASK) ? 1 : 0)
 #define IS_READ_CAP_16(cdb)					\
 ((cdb[0] == SERVICE_ACTION_IN_16 && cdb[1] == SAI_READ_CAPACITY_16) ? 1 : 0)
 
+#define IS_WRITE_LONG_16(cdb)					\
+((cdb[0] == SERVICE_ACTION_OUT_16 &&				\
+(cdb[1] & WRITE_LONG_SAO_MASK) == SAO_WRITE_LONG_16) ? 1 : 0)
+
 /* Request Sense Helper Macros */
 #define GET_REQUEST_SENSE_ALLOC_LENGTH(cdb)			\
 (GET_U8_FROM_CDB(cdb, REQUEST_SENSE_CDB_ALLOC_LENGTH_OFFSET))

@@ -871,7 +886,7 @@ static int nvme_trans_ext_inq_page(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	u8 spt_lut[8] = {0, 0, 2, 1, 4, 6, 5, 7};
 	u8 grd_chk, app_chk, ref_chk, protect;
 	u8 uask_sup = 0x20;
-	u8 v_sup;
+	u8 v_sup, wrt_uncor, wu_sup, cd_sup;
 	u8 luiclr = 0x01;
 
 	inq_response = kmalloc(EXTENDED_INQUIRY_DATA_PAGE_LENGTH, GFP_KERNEL);

@@ -914,6 +929,10 @@ static int nvme_trans_ext_inq_page(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	}
 	id_ctrl = mem;
 	v_sup = id_ctrl->vwc;
+	wrt_uncor = (id_ctrl->oncs &
+				NVME_CTRL_ONCS_WRITE_UNCORRECTABLE) ? 1 : 0;
+	wu_sup = wrt_uncor << 3;
+	cd_sup = wrt_uncor << 2;
 
 	memset(inq_response, 0, EXTENDED_INQUIRY_DATA_PAGE_LENGTH);
 	inq_response[1] = INQ_EXTENDED_INQUIRY_DATA_PAGE;    /* Page Code */

@@ -921,7 +940,7 @@ static int nvme_trans_ext_inq_page(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	inq_response[3] = 0x3C;    /* Page Length LSB */
 	inq_response[4] = microcode | spt | grd_chk | app_chk | ref_chk;
 	inq_response[5] = uask_sup;
-	inq_response[6] = v_sup;
+	inq_response[6] = wu_sup | cd_sup | v_sup;
 	inq_response[7] = luiclr;
 	inq_response[8] = 0;
 	inq_response[9] = 0;

@@ -2916,6 +2935,52 @@ static int nvme_trans_unmap(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	return res;
 }
 
+static int nvme_trans_write_long(struct nvme_ns *ns, struct sg_io_hdr *hdr,
+				u8 *cmd)
+{
+	int res = SNTI_TRANSLATION_SUCCESS;
+	int nvme_sc;
+	struct nvme_command c;
+	u64 slba;
+	u8 cor_dis, wr_uncor, pblock;
+
+	cor_dis = GET_U8_FROM_CDB(cmd, WRITE_LONG_CDB_COR_DIS_OFFSET) &
+					WRITE_LONG_CDB_COR_DIS_MASK;
+	wr_uncor = GET_U8_FROM_CDB(cmd, WRITE_LONG_CDB_WR_UNCOR_OFFSET) &
+					WRITE_LONG_CDB_WR_UNCOR_MASK;
+	pblock = GET_U8_FROM_CDB(cmd, WRITE_LONG_CDB_PBLOCK_OFFSET) &
+					WRITE_LONG_CDB_PBLOCK_MASK;
+
+	if (!cor_dis || !wr_uncor || pblock) {
+		res = nvme_trans_completion(hdr, SAM_STAT_CHECK_CONDITION,
+					ILLEGAL_REQUEST,
+					SCSI_ASC_INVALID_CDB,
+					SCSI_ASCQ_CAUSE_NOT_REPORTABLE);
+		goto out;
+	}
+
+	if (cmd[0] == WRITE_LONG)
+		slba = GET_U32_FROM_CDB(cmd, WRITE_LONG_CDB_LBA_OFFSET);
+	else
+		slba = GET_U64_FROM_CDB(cmd, WRITE_LONG_CDB_LBA_OFFSET);
+
+	memset(&c, 0, sizeof(c));
+	c.wu.opcode = nvme_cmd_write_uncor;
+	c.wu.nsid = cpu_to_le32(ns->ns_id);
+	c.wu.slba = cpu_to_le64(slba);
+	c.wu.nlb = 0;
+
+	nvme_sc = nvme_submit_io_cmd(ns->dev, ns, &c, NULL);
+	res = nvme_trans_status_code(hdr, nvme_sc);
+	if (res)
+		goto out;
+	if (nvme_sc)
+		res = nvme_sc;
+
+out:
+	return res;
+}
+
 static int nvme_scsi_translate(struct nvme_ns *ns, struct sg_io_hdr *hdr)
 {
 	u8 cmd[BLK_MAX_CDB];

@@ -3001,6 +3066,15 @@ static int nvme_scsi_translate(struct nvme_ns *ns, struct sg_io_hdr *hdr)
 	case UNMAP:
 		retcode = nvme_trans_unmap(ns, hdr, cmd);
 		break;
+	case WRITE_LONG:
+		retcode = nvme_trans_write_long(ns, hdr, cmd);
+		break;
+	case SERVICE_ACTION_OUT_16:
+		if (IS_WRITE_LONG_16(cmd))
+			retcode = nvme_trans_write_long(ns, hdr, cmd);
+		else
+			goto out;
+		break;
 	default:
  out:
 		retcode = nvme_trans_completion(hdr, SAM_STAT_CHECK_CONDITION,
include/uapi/linux/nvme.h +13 -0
index aef9a81..068db6d 100644
@@ -310,6 +310,18 @@ struct nvme_dsm_range {
 	__le64			slba;
 };
 
+struct nvme_write_uncor_cmd {
+	__u8			opcode;
+	__u8			flags;
+	__u16			command_id;
+	__le32			nsid;
+	__u32			rsvd2[8];
+	__le64			slba;
+	__le16			nlb;
+	__u16			rsvd;
+	__u32			rsvd13[3];
+};
+
 /* Admin commands */
 
 enum nvme_admin_opcode {

@@ -469,6 +481,7 @@ struct nvme_command {
 		struct nvme_download_firmware dlfw;
 		struct nvme_format_cmd format;
 		struct nvme_dsm_cmd dsm;
+		struct nvme_write_uncor_cmd wu;
 		struct nvme_abort_cmd abort;
 	};
 };
Signature · 1 line
1.8.3.2
author
#1 lore raw

PATCH v3 1/1 NVMe: write_long SCSI to NVMe translation implementation

Hi Keith,

I think this patch suffices for the shortcomings of the previous versions. Does anything else seem amiss ?

If I am to submit a version of the patch for the legacy-nvme , the SCSI macro SERVICE_ACTION _OUT_16 is not present in the kernel. Is it alright to define a Macro SAO_16(0x9f) exclusive to nvme-scsi to support WRITE_LONG_16?

Regards,
Sunad

-----Original Message-----
From: Linux-nvme [mailto:[email protected]] On Behalf Of Sunad Bhandary S
Sent: Monday, March 23, 2015 7:18 PM
To: linux-nvme at lists.infradead.org; keith.busch at intel.com
Cc: Sunad Bhandary S
Subject: [PATCHv3] NVMe: write_long SCSI to NVMe translation implementation

From: Sunad Bhandary S <[email protected]>

This patch implements the SCSI to NVMe translation for write_long. write_long is translated to the NVMe command write_uncorrectable as defined by the translation specification version 1.4.

This patch also indicates the device support for write_uncorrectable method in the response of extended inquiry as defined in the translation spec.

This patch is based on for-linus branch.

Changes:
v1 -> v2 : __le32 nlb field changed to __le16 nlb followed by __u16 rsvd. v2 -> v3 : Check for SAO16 value for write_long_16.

Signed-off-by Sunad Bhandary S <sunad.s at samsung.com>
---
 drivers/block/nvme-scsi.c | 78
+++++++++++++++++++++++++++++++++++++++++++++--
 include/uapi/linux/nvme.h | 13 ++++++++
 2 files changed, 89 insertions(+), 2 deletions(-)
drivers/block/nvme-scsi.c index renamed from drivers/block/nvme-scsi.c +0 -0

e10196e..3b778e4 100644

drivers/block/nvme-scsi.c +0 -0
@@ -245,6 +245,17 @@ static int sg_version_num = 30534;	/* 2 digits for each

component */
/* Report LUNs defines */
#define REPORT_LUNS_FIRST_LUN_OFFSET 8

+/*Write Long defines */
+#define WRITE_LONG_CDB_COR_DIS_OFFSET 1
+#define WRITE_LONG_CDB_COR_DIS_MASK 0x80
+#define WRITE_LONG_CDB_WR_UNCOR_OFFSET 1
+#define WRITE_LONG_CDB_WR_UNCOR_MASK 0x40
+#define WRITE_LONG_CDB_PBLOCK_OFFSET 1
+#define WRITE_LONG_CDB_PBLOCK_MASK 0x20
+#define WRITE_LONG_CDB_LBA_OFFSET 2
+#define WRITE_LONG_SAO_MASK 0x1F
+#define SAO_WRITE_LONG_16 0x11
+
/* SCSI ADDITIONAL SENSE Codes */

#define SCSI_ASC_NO_SENSE 0x00
@@ -331,6 +342,10 @@ INQUIRY_EVPD_BIT_MASK) ? 1 : 0)
#define IS_READ_CAP_16(cdb) \
((cdb[0] == SERVICE_ACTION_IN_16 && cdb[1] == SAI_READ_CAPACITY_16) ? 1 :
0)

+#define IS_WRITE_LONG_16(cdb) \
+((cdb[0] == SERVICE_ACTION_OUT_16 && \
+(cdb[1] & WRITE_LONG_SAO_MASK) == SAO_WRITE_LONG_16) ? 1 : 0) +
/* Request Sense Helper Macros */
#define GET_REQUEST_SENSE_ALLOC_LENGTH(cdb) \
(GET_U8_FROM_CDB(cdb, REQUEST_SENSE_CDB_ALLOC_LENGTH_OFFSET))
@@ -871,7 +886,7 @@ static int nvme_trans_ext_inq_page(struct nvme_ns *ns, struct sg_io_hdr *hdr,
u8 spt_lut[8] = {0, 0, 2, 1, 4, 6, 5, 7};
u8 grd_chk, app_chk, ref_chk, protect;
u8 uask_sup = 0x20;
- u8 v_sup;
+ u8 v_sup, wrt_uncor, wu_sup, cd_sup;
u8 luiclr = 0x01;

inq_response = kmalloc(EXTENDED_INQUIRY_DATA_PAGE_LENGTH,
GFP_KERNEL); @@ -914,6 +929,10 @@ static int nvme_trans_ext_inq_page(struct nvme_ns *ns, struct sg_io_hdr *hdr,
}
id_ctrl = mem;
v_sup = id_ctrl->vwc;
+ wrt_uncor = (id_ctrl->oncs &
+ NVME_CTRL_ONCS_WRITE_UNCORRECTABLE) ? 1 : 0;
+ wu_sup = wrt_uncor << 3;
+ cd_sup = wrt_uncor << 2;

memset(inq_response, 0, EXTENDED_INQUIRY_DATA_PAGE_LENGTH);
inq_response[1] = INQ_EXTENDED_INQUIRY_DATA_PAGE; /* Page Code */
@@ -921,7 +940,7 @@ static int nvme_trans_ext_inq_page(struct nvme_ns *ns, struct sg_io_hdr *hdr,
inq_response[3] = 0x3C; /* Page Length LSB */
inq_response[4] = microcode | spt | grd_chk | app_chk | ref_chk; inq_response[5] = uask_sup;
- inq_response[6] = v_sup;
+ inq_response[6] = wu_sup | cd_sup | v_sup;
inq_response[7] = luiclr;
inq_response[8] = 0;
inq_response[9] = 0;
@@ -2916,6 +2935,52 @@ static int nvme_trans_unmap(struct nvme_ns *ns, struct sg_io_hdr *hdr,
return res;
}

+static int nvme_trans_write_long(struct nvme_ns *ns, struct sg_io_hdr *hdr,
+ u8 *cmd)
+{
+ int res = SNTI_TRANSLATION_SUCCESS;
+ int nvme_sc;
+ struct nvme_command c;
+ u64 slba;
+ u8 cor_dis, wr_uncor, pblock;
+
+ cor_dis = GET_U8_FROM_CDB(cmd, WRITE_LONG_CDB_COR_DIS_OFFSET) &
+ WRITE_LONG_CDB_COR_DIS_MASK;
+ wr_uncor = GET_U8_FROM_CDB(cmd, WRITE_LONG_CDB_WR_UNCOR_OFFSET) &
+ WRITE_LONG_CDB_WR_UNCOR_MASK;
+ pblock = GET_U8_FROM_CDB(cmd, WRITE_LONG_CDB_PBLOCK_OFFSET) &
+ WRITE_LONG_CDB_PBLOCK_MASK;
+
+ if (!cor_dis || !wr_uncor || pblock) {
+ res = nvme_trans_completion(hdr, SAM_STAT_CHECK_CONDITION,
+ ILLEGAL_REQUEST,
+ SCSI_ASC_INVALID_CDB,
+ SCSI_ASCQ_CAUSE_NOT_REPORTABLE);
+ goto out;
+ }
+
+ if (cmd[0] == WRITE_LONG)
+ slba = GET_U32_FROM_CDB(cmd, WRITE_LONG_CDB_LBA_OFFSET);
+ else
+ slba = GET_U64_FROM_CDB(cmd, WRITE_LONG_CDB_LBA_OFFSET);
+
+ memset(&c, 0, sizeof(c));
+ c.wu.opcode = nvme_cmd_write_uncor;
+ c.wu.nsid = cpu_to_le32(ns->ns_id);
+ c.wu.slba = cpu_to_le64(slba);
+ c.wu.nlb = 0;
+
+ nvme_sc = nvme_submit_io_cmd(ns->dev, ns, &c, NULL);
+ res = nvme_trans_status_code(hdr, nvme_sc);
+ if (res)
+ goto out;
+ if (nvme_sc)
+ res = nvme_sc;
+
+out:
+ return res;
+}
+
static int nvme_scsi_translate(struct nvme_ns *ns, struct sg_io_hdr *hdr)
{
u8 cmd[BLK_MAX_CDB];
@@ -3001,6 +3066,15 @@ static int nvme_scsi_translate(struct nvme_ns *ns, struct sg_io_hdr *hdr)
case UNMAP:
retcode = nvme_trans_unmap(ns, hdr, cmd);
break;
+ case WRITE_LONG:
+ retcode = nvme_trans_write_long(ns, hdr, cmd);
+ break;
+ case SERVICE_ACTION_OUT_16:
+ if (IS_WRITE_LONG_16(cmd))
+ retcode = nvme_trans_write_long(ns, hdr, cmd);
+ else
+ goto out;
+ break;
default:
out:
retcode = nvme_trans_completion(hdr,
SAM_STAT_CHECK_CONDITION, diff --git a/include/uapi/linux/nvme.h b/include/uapi/linux/nvme.h index aef9a81..068db6d 100644

include/uapi/linux/nvme.h +13 -0
@@ -310,6 +310,18 @@ struct nvme_dsm_range {
 	__le64			slba;
 };
 
+struct nvme_write_uncor_cmd {
+	__u8			opcode;
+	__u8			flags;
+	__u16			command_id;
+	__le32			nsid;
+	__u32			rsvd2[8];
+	__le64			slba;
+	__le16			nlb;
+	__u16			rsvd;
+	__u32			rsvd13[3];
+};
+
 /* Admin commands */
 
 enum nvme_admin_opcode {

@@ -469,6 +481,7 @@ struct nvme_command {
 		struct nvme_download_firmware dlfw;
 		struct nvme_format_cmd format;
 		struct nvme_dsm_cmd dsm;
+		struct nvme_write_uncor_cmd wu;
 		struct nvme_abort_cmd abort;
 	};
 };

--
1.8.3.2

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

#2 lore raw

PATCH v3 1/1 NVMe: write_long SCSI to NVMe translation implementation

Hi Sunad,

Quoted 9 lines · Sunad Bhandary

Hi Keith,

I think this patch suffices for the shortcomings of the previous versions. Does anything else seem amiss ?

If I am to submit a version of the patch for the legacy-nvme , the SCSI macro SERVICE_ACTION _OUT_16 is not present in the kernel. Is it alright to define a Macro SAO_16(0x9f) exclusive to nvme-scsi to support WRITE_LONG_16?

Sounds good. Patch v3 looks good to me as well.

Thanks,
Keith

Quoted 8 lines

From: Sunad Bhandary S <sunad.s at samsung.com>

This patch implements the SCSI to NVMe translation for write_long. write_long is translated to the NVMe command write_uncorrectable as defined by the translation specification version 1.4.

This patch also indicates the device support for write_uncorrectable method in the response of extended inquiry as defined in the translation spec.

Acked-by Keith Busch <keith.busch at intel.com>
Quoted 1 line
Signed-off-by Sunad Bhandary S <sunad.s at samsung.com>
author
#3 lore raw

PATCH v3 1/1 NVMe: write_long SCSI to NVMe translation implementation

Hi Keith,

I see that this has not been committed yet. Are any changes needed to the patch from my side ?

Thanks and regards,
Sunad

-----Original Message-----
From: Linux-nvme [mailto:[email protected]] On Behalf Of Keith Busch
Sent: Tuesday, March 24, 2015 7:25 PM
To: Sunad Bhandary
Cc: keith.busch at intel.com; linux-nvme at lists.infradead.org
Subject: RE: [PATCHv3] NVMe: write_long SCSI to NVMe translation implementation

Hi Sunad,

Quoted 9 lines · Sunad Bhandary

Hi Keith,

I think this patch suffices for the shortcomings of the previous versions. Does anything else seem amiss ?

If I am to submit a version of the patch for the legacy-nvme , the SCSI macro SERVICE_ACTION _OUT_16 is not present in the kernel. Is it alright to define a Macro SAO_16(0x9f) exclusive to nvme-scsi to support WRITE_LONG_16?

Sounds good. Patch v3 looks good to me as well.

Thanks,
Keith

Quoted 8 lines

From: Sunad Bhandary S <sunad.s at samsung.com>

This patch implements the SCSI to NVMe translation for write_long. write_long is translated to the NVMe command write_uncorrectable as defined by the translation specification version 1.4.

This patch also indicates the device support for write_uncorrectable method in the response of extended inquiry as defined in the translation

spec.

Acked-by Keith Busch <keith.busch at intel.com>
Quoted 1 line
Signed-off-by Sunad Bhandary S <sunad.s at samsung.com>

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

#4 lore raw

PATCH v3 1/1 NVMe: write_long SCSI to NVMe translation implementation

Quoted 4 lines · Sunad Bhandary

Hi Keith,

I see that this has not been committed yet. Are any changes needed to the patch from my side ?

I don't know, I'm not a maintainer. I can only provide acks and reviews.

Honestly I don't see scarring a single block at a time through SG_IO as a particularly useful feature when NVMe can do entire ranges in one command with a native passthrough, but it doesn't appear to break anything so I won't stand in the way.

#5 lore raw

PATCH v3 1/1 NVMe: write_long SCSI to NVMe translation implementation

In reply to Sunad Bhandary S

drivers/block/nvme-scsi.c static int nvme_scsi_translate(struct nvme_ns *ns, struct sg_io_hdr *hdr) · line 3079

Quoted 11 lines · Sunad Bhandary S
drivers/block/nvme-scsi.c +9 -0
+	case WRITE_LONG:
+		retcode = nvme_trans_write_long(ns, hdr, cmd);
+		break;
+	case SERVICE_ACTION_OUT_16:
+		if (IS_WRITE_LONG_16(cmd))
+			retcode = nvme_trans_write_long(ns, hdr, cmd);
+		else
+			goto out;
+		break;
 	default:
  out:

Umm, why not simply:

	case SERVICE_ACTION_OUT_16:
		if (IS_WRITE_LONG_16(cmd))
			retcode = nvme_trans_write_long(ns, hdr, cmd);
		break;
#6 lore raw

PATCH v3 1/1 NVMe: write_long SCSI to NVMe translation implementation

Quoted 15 lines · Matthew Wilcox

drivers/block/nvme-scsi.c static int nvme_scsi_translate(struct nvme_ns *ns, struct sg_io_hdr *hdr) · line 3079

Quoted 11 lines · Sunad Bhandary S
drivers/block/nvme-scsi.c +9 -0
+	case WRITE_LONG:
+		retcode = nvme_trans_write_long(ns, hdr, cmd);
+		break;
+	case SERVICE_ACTION_OUT_16:
+		if (IS_WRITE_LONG_16(cmd))
+			retcode = nvme_trans_write_long(ns, hdr, cmd);
+		else
+			goto out;
+		break;
 	default:
  out:

Umm, why not simply:

Quoted 4 lines
	case SERVICE_ACTION_OUT_16:
		if (IS_WRITE_LONG_16(cmd))
			retcode = nvme_trans_write_long(ns, hdr, cmd);
		break;

We need to jump to a case that sets the sense data for the IllReq Check Condition.

Actually I'm not sure why there are a bunch of:

case foobar:
retcode = nvme_trans_foobar();
break;

Rather than just:

case foobar:
return nvme_trans_foobar();

It make the whole thing cleaner and allow breaking out to an error handler at the end of the function.

Anyhow, it's on my list to clean up this up since we are blk-mq now and can leverage REQ_BLOCK_TYPE_PC to do all the boiler plate user space copying, among other things. We can reduce the nvme-scsi.c file size by half.