DIF/DIX updates for 2.6.32

DIF/DIX updates for 2.6.32

[Martin K. Petersen]

Updates since last post:

 - The CDB allocation has been moved to sd.c and the Type 2 support collapsed
   into a single patch.

The first three patches constitute fixes.  The last two contain new features.

--  
Martin K. Petersen	Oracle Linux Engineering

[PATCH 1/5] scsi: Deprecate SCSI_PROT_*_CONVERT operations

[Martin K. Petersen]

The checksum format is orthogonal to whether the protection information
is being passed on beyond the HBA or not.  It is perfectly valid to use
a non-T10 CRC with WRITE_STRIP and READ_INSERT.

Consequently it no longer makes sense to explicitly refer to the
conversion in the protection operation.  Update sd_dif and lpfc
accordingly.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/lpfc/lpfc_scsi.c |   15 +++------------
 drivers/scsi/sd_dif.c         |   20 ++++----------------
 include/scsi/scsi_cmnd.h      |    4 ----
 3 files changed, 7 insertions(+), 32 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
index da59c4f..ad75e19 100644
--- a/drivers/scsi/lpfc/lpfc_scsi.c
+++ b/drivers/scsi/lpfc/lpfc_scsi.c
@@ -56,8 +56,6 @@ static char *dif_op_str[] = {
 	"SCSI_PROT_WRITE_INSERT",
 	"SCSI_PROT_READ_PASS",
 	"SCSI_PROT_WRITE_PASS",
-	"SCSI_PROT_READ_CONVERT",
-	"SCSI_PROT_WRITE_CONVERT"
 };
 static void
 lpfc_release_scsi_buf_s4(struct lpfc_hba *phba, struct lpfc_scsi_buf *psb);
@@ -1131,13 +1129,11 @@ lpfc_sc_to_sli_prof(struct scsi_cmnd *sc)
 			ret_prof = LPFC_PROF_A1;
 			break;
 
-		case SCSI_PROT_READ_CONVERT:
-		case SCSI_PROT_WRITE_CONVERT:
+		case SCSI_PROT_READ_PASS:
+		case SCSI_PROT_WRITE_PASS:
 			ret_prof = LPFC_PROF_AST1;
 			break;
 
-		case SCSI_PROT_READ_PASS:
-		case SCSI_PROT_WRITE_PASS:
 		case SCSI_PROT_NORMAL:
 		default:
 			printk(KERN_ERR "Bad op/guard:%d/%d combination\n",
@@ -1157,8 +1153,6 @@ lpfc_sc_to_sli_prof(struct scsi_cmnd *sc)
 			ret_prof = LPFC_PROF_C1;
 			break;
 
-		case SCSI_PROT_READ_CONVERT:
-		case SCSI_PROT_WRITE_CONVERT:
 		case SCSI_PROT_READ_INSERT:
 		case SCSI_PROT_WRITE_STRIP:
 		case SCSI_PROT_NORMAL:
@@ -1209,8 +1203,7 @@ lpfc_get_cmd_dif_parms(struct scsi_cmnd *sc, uint16_t *apptagmask,
 	static int cnt;
 
 	if (protcnt && (op == SCSI_PROT_WRITE_STRIP ||
-				op == SCSI_PROT_WRITE_PASS ||
-				op == SCSI_PROT_WRITE_CONVERT)) {
+				op == SCSI_PROT_WRITE_PASS)) {
 
 		cnt++;
 		spt = page_address(sg_page(scsi_prot_sglist(sc))) +
@@ -1501,8 +1494,6 @@ lpfc_prot_group_type(struct lpfc_hba *phba, struct scsi_cmnd *sc)
 	case SCSI_PROT_WRITE_STRIP:
 	case SCSI_PROT_READ_PASS:
 	case SCSI_PROT_WRITE_PASS:
-	case SCSI_PROT_WRITE_CONVERT:
-	case SCSI_PROT_READ_CONVERT:
 		ret = LPFC_PG_TYPE_DIF_BUF;
 		break;
 	default:
diff --git a/drivers/scsi/sd_dif.c b/drivers/scsi/sd_dif.c
index 82f14a9..84224dd 100644
--- a/drivers/scsi/sd_dif.c
+++ b/drivers/scsi/sd_dif.c
@@ -364,15 +364,9 @@ void sd_dif_config_host(struct scsi_disk *sdkp)
  */
 void sd_dif_op(struct scsi_cmnd *scmd, unsigned int dif, unsigned int dix, unsigned int type)
 {
-	int csum_convert, prot_op;
+	int prot_op;
 
-	prot_op = 0;
-
-	/* Convert checksum? */
-	if (scsi_host_get_guard(scmd->device->host) != SHOST_DIX_GUARD_CRC)
-		csum_convert = 1;
-	else
-		csum_convert = 0;
+	prot_op = SCSI_PROT_NORMAL;
 
 	BUG_ON(dif && (scmd->cmnd[0] == READ_6 || scmd->cmnd[0] == WRITE_6));
 
@@ -382,10 +376,7 @@ void sd_dif_op(struct scsi_cmnd *scmd, unsigned int dif, unsigned int dix, unsig
 	case READ_12:
 	case READ_16:
 		if (dif && dix)
-			if (csum_convert)
-				prot_op = SCSI_PROT_READ_CONVERT;
-			else
-				prot_op = SCSI_PROT_READ_PASS;
+			prot_op = SCSI_PROT_READ_PASS;
 		else if (dif && !dix)
 			prot_op = SCSI_PROT_READ_STRIP;
 		else if (!dif && dix)
@@ -398,10 +389,7 @@ void sd_dif_op(struct scsi_cmnd *scmd, unsigned int dif, unsigned int dix, unsig
 	case WRITE_12:
 	case WRITE_16:
 		if (dif && dix)
-			if (csum_convert)
-				prot_op = SCSI_PROT_WRITE_CONVERT;
-			else
-				prot_op = SCSI_PROT_WRITE_PASS;
+			prot_op = SCSI_PROT_WRITE_PASS;
 		else if (dif && !dix)
 			prot_op = SCSI_PROT_WRITE_INSERT;
 		else if (!dif && dix)
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 3878d1d..a5e885a 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -229,10 +229,6 @@ enum scsi_prot_operations {
 	/* OS-HBA: Protected, HBA-Target: Protected */
 	SCSI_PROT_READ_PASS,
 	SCSI_PROT_WRITE_PASS,
-
-	/* OS-HBA: Protected, HBA-Target: Protected, checksum conversion */
-	SCSI_PROT_READ_CONVERT,
-	SCSI_PROT_WRITE_CONVERT,
 };
 
 static inline void scsi_set_prot_op(struct scsi_cmnd *scmd, unsigned char op)
-- 
1.6.0.6

[PATCH 2/5] sd: Detach DIF from block integrity infrastructure

[Martin K. Petersen]

So far we have only issued DIF commands if CONFIG_BLK_DEV_INTEGRITY is
enabled.  However, communication between initiator and target should be
independent of protection information DMA.  There are DIF-only host
adapters coming out that will be able to take advantage of this.

Move the relevant DIF bits to sd.c.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/sd.c        |   61 +++++++++++++++++++++++++++++++--------------
 drivers/scsi/sd.h        |    4 ---
 drivers/scsi/sd_dif.c    |   53 ----------------------------------------
 include/scsi/scsi_host.h |   15 ++++++++---
 4 files changed, 53 insertions(+), 80 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index b7b9fec..36e2333 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -370,6 +370,31 @@ static void scsi_disk_put(struct scsi_disk *sdkp)
 	mutex_unlock(&sd_ref_mutex);
 }
 
+static void sd_prot_op(struct scsi_cmnd *scmd, unsigned int dif)
+{
+	unsigned int prot_op = SCSI_PROT_NORMAL;
+	unsigned int dix = scsi_prot_sg_count(scmd);
+
+	if (scmd->sc_data_direction == DMA_FROM_DEVICE) {
+		if (dif && dix)
+			prot_op = SCSI_PROT_READ_PASS;
+		else if (dif && !dix)
+			prot_op = SCSI_PROT_READ_STRIP;
+		else if (!dif && dix)
+			prot_op = SCSI_PROT_READ_INSERT;
+	} else {
+		if (dif && dix)
+			prot_op = SCSI_PROT_WRITE_PASS;
+		else if (dif && !dix)
+			prot_op = SCSI_PROT_WRITE_INSERT;
+		else if (!dif && dix)
+			prot_op = SCSI_PROT_WRITE_STRIP;
+	}
+
+	scsi_set_prot_op(scmd, prot_op);
+	scsi_set_prot_type(scmd, dif);
+}
+
 /**
  *	sd_init_command - build a scsi (read or write) command from
  *	information in the request structure.
@@ -578,8 +603,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 
 	/* If DIF or DIX is enabled, tell HBA how to handle request */
 	if (host_dif || scsi_prot_sg_count(SCpnt))
-		sd_dif_op(SCpnt, host_dif, scsi_prot_sg_count(SCpnt),
-			  sdkp->protection_type);
+		sd_prot_op(SCpnt, host_dif);
 
 	/*
 	 * We shouldn't disconnect in the middle of a sector, so with a dumb
@@ -1238,34 +1262,33 @@ void sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
 	u8 type;
 
 	if (scsi_device_protection(sdp) == 0 || (buffer[12] & 1) == 0)
-		type = 0;
-	else
-		type = ((buffer[12] >> 1) & 7) + 1; /* P_TYPE 0 = Type 1 */
+		return;
+
+	type = ((buffer[12] >> 1) & 7) + 1; /* P_TYPE 0 = Type 1 */
+
+	if (type == sdkp->protection_type || !sdkp->first_scan)
+		return;
 
 	sdkp->protection_type = type;
 
 	switch (type) {
-	case SD_DIF_TYPE0_PROTECTION:
 	case SD_DIF_TYPE1_PROTECTION:
 	case SD_DIF_TYPE3_PROTECTION:
 		break;
 
-	case SD_DIF_TYPE2_PROTECTION:
-		sd_printk(KERN_ERR, sdkp, "formatted with DIF Type 2 "	\
-			  "protection which is currently unsupported. "	\
-			  "Disabling disk!\n");
-		goto disable;
-
 	default:
-		sd_printk(KERN_ERR, sdkp, "formatted with unknown "	\
-			  "protection type %d. Disabling disk!\n", type);
-		goto disable;
+		sd_printk(KERN_ERR, sdkp, "formatted with unsupported "	\
+			  "protection type %u. Disabling disk!\n", type);
+		sdkp->capacity = 0;
+		return;
 	}
 
-	return;
-
-disable:
-	sdkp->capacity = 0;
+	if (scsi_host_dif_capable(sdp->host, type))
+		sd_printk(KERN_NOTICE, sdkp,
+			  "Enabling DIF Type %u protection\n", type);
+	else
+		sd_printk(KERN_NOTICE, sdkp,
+			  "Disabling DIF Type %u protection\n", type);
 }
 
 static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device *sdp,
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 8474b5b..ce1f5f8 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -101,16 +101,12 @@ struct sd_dif_tuple {
 
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 
-extern void sd_dif_op(struct scsi_cmnd *, unsigned int, unsigned int, unsigned int);
 extern void sd_dif_config_host(struct scsi_disk *);
 extern int sd_dif_prepare(struct request *rq, sector_t, unsigned int);
 extern void sd_dif_complete(struct scsi_cmnd *, unsigned int);
 
 #else /* CONFIG_BLK_DEV_INTEGRITY */
 
-static inline void sd_dif_op(struct scsi_cmnd *cmd, unsigned int a, unsigned int b, unsigned int c)
-{
-}
 static inline void sd_dif_config_host(struct scsi_disk *disk)
 {
 }
diff --git a/drivers/scsi/sd_dif.c b/drivers/scsi/sd_dif.c
index 84224dd..88da977 100644
--- a/drivers/scsi/sd_dif.c
+++ b/drivers/scsi/sd_dif.c
@@ -320,15 +320,6 @@ void sd_dif_config_host(struct scsi_disk *sdkp)
 		dif = 0; dix = 1;
 	}
 
-	if (type) {
-		if (dif)
-			sd_printk(KERN_NOTICE, sdkp,
-				  "Enabling DIF Type %d protection\n", type);
-		else
-			sd_printk(KERN_NOTICE, sdkp,
-				  "Disabling DIF Type %d protection\n", type);
-	}
-
 	if (!dix)
 		return;
 
@@ -360,50 +351,6 @@ void sd_dif_config_host(struct scsi_disk *sdkp)
 }
 
 /*
- * DIF DMA operation magic decoder ring.
- */
-void sd_dif_op(struct scsi_cmnd *scmd, unsigned int dif, unsigned int dix, unsigned int type)
-{
-	int prot_op;
-
-	prot_op = SCSI_PROT_NORMAL;
-
-	BUG_ON(dif && (scmd->cmnd[0] == READ_6 || scmd->cmnd[0] == WRITE_6));
-
-	switch (scmd->cmnd[0]) {
-	case READ_6:
-	case READ_10:
-	case READ_12:
-	case READ_16:
-		if (dif && dix)
-			prot_op = SCSI_PROT_READ_PASS;
-		else if (dif && !dix)
-			prot_op = SCSI_PROT_READ_STRIP;
-		else if (!dif && dix)
-			prot_op = SCSI_PROT_READ_INSERT;
-
-		break;
-
-	case WRITE_6:
-	case WRITE_10:
-	case WRITE_12:
-	case WRITE_16:
-		if (dif && dix)
-			prot_op = SCSI_PROT_WRITE_PASS;
-		else if (dif && !dix)
-			prot_op = SCSI_PROT_WRITE_INSERT;
-		else if (!dif && dix)
-			prot_op = SCSI_PROT_WRITE_STRIP;
-
-		break;
-	}
-
-	scsi_set_prot_op(scmd, prot_op);
-	if (dif)
-		scsi_set_prot_type(scmd, type);
-}
-
-/*
  * The virtual start sector is the one that was originally submitted
  * by the block layer.	Due to partitioning, MD/DM cloning, etc. the
  * actual physical start sector is likely to be different.  Remap
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index b62a097..6e728b1 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -798,9 +798,15 @@ static inline unsigned int scsi_host_get_prot(struct Scsi_Host *shost)
 static inline unsigned int scsi_host_dif_capable(struct Scsi_Host *shost, unsigned int target_type)
 {
 	switch (target_type) {
-	case 1: return shost->prot_capabilities & SHOST_DIF_TYPE1_PROTECTION;
-	case 2: return shost->prot_capabilities & SHOST_DIF_TYPE2_PROTECTION;
-	case 3: return shost->prot_capabilities & SHOST_DIF_TYPE3_PROTECTION;
+	case 1:
+		if (shost->prot_capabilities & SHOST_DIF_TYPE1_PROTECTION)
+			return target_type;
+	case 2:
+		if (shost->prot_capabilities & SHOST_DIF_TYPE2_PROTECTION)
+			return target_type;
+	case 3:
+		if (shost->prot_capabilities & SHOST_DIF_TYPE3_PROTECTION)
+			return target_type;
 	}
 
 	return 0;
@@ -808,13 +814,14 @@ static inline unsigned int scsi_host_dif_capable(struct Scsi_Host *shost, unsign
 
 static inline unsigned int scsi_host_dix_capable(struct Scsi_Host *shost, unsigned int target_type)
 {
+#if defined(CONFIG_BLK_DEV_INTEGRITY)
 	switch (target_type) {
 	case 0: return shost->prot_capabilities & SHOST_DIX_TYPE0_PROTECTION;
 	case 1: return shost->prot_capabilities & SHOST_DIX_TYPE1_PROTECTION;
 	case 2: return shost->prot_capabilities & SHOST_DIX_TYPE2_PROTECTION;
 	case 3: return shost->prot_capabilities & SHOST_DIX_TYPE3_PROTECTION;
 	}
-
+#endif
 	return 0;
 }
 
-- 
1.6.0.6

[PATCH 3/5] scsi: Fix protection scsi_data_buffer leak

[Martin K. Petersen]

We would leak a scsi_data_buffer if the free_list command was of the
protected variety.

Reported-by: Boaz Harrosh <bharrosh@panasas.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/scsi.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 2de5f3a..69397bb 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -241,10 +241,7 @@ scsi_host_alloc_command(struct Scsi_Host *shost, gfp_t gfp_mask)
  */
 struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
 {
-	struct scsi_cmnd *cmd;
-	unsigned char *buf;
-
-	cmd = scsi_host_alloc_command(shost, gfp_mask);
+	struct scsi_cmnd *cmd = scsi_host_alloc_command(shost, gfp_mask);
 
 	if (unlikely(!cmd)) {
 		unsigned long flags;
@@ -258,9 +255,15 @@ struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
 		spin_unlock_irqrestore(&shost->free_list_lock, flags);
 
 		if (cmd) {
+			void *buf, *prot;
+
 			buf = cmd->sense_buffer;
+			prot = cmd->prot_sdb;
+
 			memset(cmd, 0, sizeof(*cmd));
+
 			cmd->sense_buffer = buf;
+			cmd->prot_sdb = prot;
 		}
 	}
 
-- 
1.6.0.6

[PATCH 4/5] sd: Support disks formatted with DIF Type 2

[Martin K. Petersen]

Disks formatted with DIF Type 2 reject READ/WRITE 6/10/12/16 commands
when protection is enabled.  Only the 32-byte variants are supported.

Implement support for issusing 32-byte READ/WRITE and enable Type 2
drives in the protection type detection logic.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/sd.c   |   80 ++++++++++++++++++++++++++++++++++++++++++++-------
 drivers/scsi/sd.h   |    5 +++
 include/scsi/scsi.h |    3 ++
 3 files changed, 77 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 36e2333..d138e6b 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -116,6 +116,9 @@ static DEFINE_IDA(sd_index_ida);
  * object after last put) */
 static DEFINE_MUTEX(sd_ref_mutex);
 
+struct kmem_cache *sd_cdb_cache;
+mempool_t *sd_cdb_pool;
+
 static const char *sd_cache_types[] = {
 	"write through", "none", "write back",
 	"write back, no read (daft)"
@@ -413,6 +416,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	sector_t threshold;
 	unsigned int this_count = blk_rq_sectors(rq);
 	int ret, host_dif;
+	unsigned char protect;
 
 	if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
 		ret = scsi_setup_blk_pc_cmnd(sdp, rq);
@@ -545,13 +549,48 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	/* Set RDPROTECT/WRPROTECT if disk is formatted with DIF */
 	host_dif = scsi_host_dif_capable(sdp->host, sdkp->protection_type);
 	if (host_dif)
-		SCpnt->cmnd[1] = 1 << 5;
+		protect = 1 << 5;
 	else
-		SCpnt->cmnd[1] = 0;
+		protect = 0;
+
+	if (host_dif == SD_DIF_TYPE2_PROTECTION) {
+		SCpnt->cmnd = mempool_alloc(sd_cdb_pool, GFP_ATOMIC);
+
+		if (unlikely(SCpnt->cmnd == NULL)) {
+			ret = BLKPREP_DEFER;
+			goto out;
+		}
 
-	if (block > 0xffffffff) {
+		memset(SCpnt->cmnd, 0, SD_EXT_CDB_SIZE);
+		SCpnt->cmnd[0] = VARIABLE_LENGTH_CMD;
+		SCpnt->cmnd[7] = 0x18;
+		SCpnt->cmnd[9] = (rq_data_dir(rq) == READ) ? READ_32 : WRITE_32;
+		SCpnt->cmnd[10] = protect | (blk_fua_rq(rq) ? 0x8 : 0);
+
+		/* LBA */
+		SCpnt->cmnd[12] = sizeof(block) > 4 ? (unsigned char) (block >> 56) & 0xff : 0;
+		SCpnt->cmnd[13] = sizeof(block) > 4 ? (unsigned char) (block >> 48) & 0xff : 0;
+		SCpnt->cmnd[14] = sizeof(block) > 4 ? (unsigned char) (block >> 40) & 0xff : 0;
+		SCpnt->cmnd[15] = sizeof(block) > 4 ? (unsigned char) (block >> 32) & 0xff : 0;
+		SCpnt->cmnd[16] = (unsigned char) (block >> 24) & 0xff;
+		SCpnt->cmnd[17] = (unsigned char) (block >> 16) & 0xff;
+		SCpnt->cmnd[18] = (unsigned char) (block >> 8) & 0xff;
+		SCpnt->cmnd[19] = (unsigned char) block & 0xff;
+
+		/* Expected Indirect LBA */
+		SCpnt->cmnd[20] = (unsigned char) (block >> 24) & 0xff;
+		SCpnt->cmnd[21] = (unsigned char) (block >> 16) & 0xff;
+		SCpnt->cmnd[22] = (unsigned char) (block >> 8) & 0xff;
+		SCpnt->cmnd[23] = (unsigned char) block & 0xff;
+
+		/* Transfer length */
+		SCpnt->cmnd[28] = (unsigned char) (this_count >> 24) & 0xff;
+		SCpnt->cmnd[29] = (unsigned char) (this_count >> 16) & 0xff;
+		SCpnt->cmnd[30] = (unsigned char) (this_count >> 8) & 0xff;
+		SCpnt->cmnd[31] = (unsigned char) this_count & 0xff;
+	} else if (block > 0xffffffff) {
 		SCpnt->cmnd[0] += READ_16 - READ_6;
-		SCpnt->cmnd[1] |= blk_fua_rq(rq) ? 0x8 : 0;
+		SCpnt->cmnd[1] = protect | (blk_fua_rq(rq) ? 0x8 : 0);
 		SCpnt->cmnd[2] = sizeof(block) > 4 ? (unsigned char) (block >> 56) & 0xff : 0;
 		SCpnt->cmnd[3] = sizeof(block) > 4 ? (unsigned char) (block >> 48) & 0xff : 0;
 		SCpnt->cmnd[4] = sizeof(block) > 4 ? (unsigned char) (block >> 40) & 0xff : 0;
@@ -572,7 +611,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 			this_count = 0xffff;
 
 		SCpnt->cmnd[0] += READ_10 - READ_6;
-		SCpnt->cmnd[1] |= blk_fua_rq(rq) ? 0x8 : 0;
+		SCpnt->cmnd[1] = protect | (blk_fua_rq(rq) ? 0x8 : 0);
 		SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
 		SCpnt->cmnd[3] = (unsigned char) (block >> 16) & 0xff;
 		SCpnt->cmnd[4] = (unsigned char) (block >> 8) & 0xff;
@@ -1047,6 +1086,7 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 	int result = SCpnt->result;
 	unsigned int good_bytes = result ? 0 : scsi_bufflen(SCpnt);
 	struct scsi_sense_hdr sshdr;
+	struct scsi_disk *sdkp = scsi_disk(SCpnt->request->rq_disk);
 	int sense_valid = 0;
 	int sense_deferred = 0;
 
@@ -1108,6 +1148,10 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 	if (rq_data_dir(SCpnt->request) == READ && scsi_prot_sg_count(SCpnt))
 		sd_dif_complete(SCpnt, good_bytes);
 
+	if (scsi_host_dif_capable(sdkp->device->host, sdkp->protection_type)
+	    == SD_DIF_TYPE2_PROTECTION)
+		mempool_free(SCpnt->cmnd, sd_cdb_pool);
+
 	return good_bytes;
 }
 
@@ -1271,12 +1315,7 @@ void sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
 
 	sdkp->protection_type = type;
 
-	switch (type) {
-	case SD_DIF_TYPE1_PROTECTION:
-	case SD_DIF_TYPE3_PROTECTION:
-		break;
-
-	default:
+	if (type > SD_DIF_TYPE3_PROTECTION) {
 		sd_printk(KERN_ERR, sdkp, "formatted with unsupported "	\
 			  "protection type %u. Disabling disk!\n", type);
 		sdkp->capacity = 0;
@@ -2321,8 +2360,24 @@ static int __init init_sd(void)
 	if (err)
 		goto err_out_class;
 
+	sd_cdb_cache = kmem_cache_create("sd_ext_cdb", SD_EXT_CDB_SIZE,
+					 0, 0, NULL);
+	if (!sd_cdb_cache) {
+		printk(KERN_ERR "sd: can't init extended cdb cache\n");
+		goto err_out_class;
+	}
+
+	sd_cdb_pool = mempool_create_slab_pool(SD_MEMPOOL_SIZE, sd_cdb_cache);
+	if (!sd_cdb_pool) {
+		printk(KERN_ERR "sd: can't init extended cdb pool\n");
+		goto err_out_cache;
+	}
+
 	return 0;
 
+err_out_cache:
+	kmem_cache_destroy(sd_cdb_cache);
+
 err_out_class:
 	class_unregister(&sd_disk_class);
 err_out:
@@ -2342,6 +2397,9 @@ static void __exit exit_sd(void)
 
 	SCSI_LOG_HLQUEUE(3, printk("exit_sd: exiting sd driver\n"));
 
+	mempool_destroy(sd_cdb_pool);
+	kmem_cache_destroy(sd_cdb_cache);
+
 	scsi_unregister_driver(&sd_template.gendrv);
 	class_unregister(&sd_disk_class);
 
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index ce1f5f8..e374804 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -37,6 +37,11 @@
  */
 #define SD_LAST_BUGGY_SECTORS	8
 
+enum {
+	SD_EXT_CDB_SIZE = 32,	/* Extended CDB size */
+	SD_MEMPOOL_SIZE = 2,	/* CDB pool size */
+};
+
 struct scsi_disk {
 	struct scsi_driver *driver;	/* always &sd_template */
 	struct scsi_device *device;
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 084478e..34c46ab 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -129,6 +129,9 @@ struct scsi_cmnd;
 #define MI_REPORT_TARGET_PGS  0x0a
 /* values for maintenance out */
 #define MO_SET_TARGET_PGS     0x0a
+/* values for variable length command */
+#define READ_32		      0x09
+#define WRITE_32	      0x0b
 
 /* Values for T10/04-262r7 */
 #define	ATA_16		      0x85	/* 16-byte pass-thru */
-- 
1.6.0.6

[PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Martin K. Petersen]

Add support for 32-byte READ/WRITE as well as DIF Type 2 protection.

Reject protected 10/12/16 byte READ/WRITE commands when Type 2 is
enabled.

Verify Type 2 reference tag according to Expected Initial LBA in 32-byte
CDB.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/scsi_debug.c |  135 +++++++++++++++++++++++++++++++++++++--------
 1 files changed, 112 insertions(+), 23 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index fb9af20..f551ec3 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -50,6 +50,7 @@
 #include <scsi/scsi_host.h>
 #include <scsi/scsicam.h>
 #include <scsi/scsi_eh.h>
+#include <scsi/scsi_dbg.h>
 
 #include "sd.h"
 #include "scsi_logging.h"
@@ -64,6 +65,7 @@ static const char * scsi_debug_version_date = "20070104";
 #define PARAMETER_LIST_LENGTH_ERR 0x1a
 #define INVALID_OPCODE 0x20
 #define ADDR_OUT_OF_RANGE 0x21
+#define INVALID_COMMAND_OPCODE 0x20
 #define INVALID_FIELD_IN_CDB 0x24
 #define INVALID_FIELD_IN_PARAM_LIST 0x26
 #define POWERON_RESET 0x29
@@ -180,7 +182,7 @@ static int sdebug_sectors_per;		/* sectors per cylinder */
 #define SDEBUG_SENSE_LEN 32
 
 #define SCSI_DEBUG_CANQUEUE  255
-#define SCSI_DEBUG_MAX_CMD_LEN 16
+#define SCSI_DEBUG_MAX_CMD_LEN 32
 
 struct sdebug_dev_info {
 	struct list_head dev_list;
@@ -296,9 +298,25 @@ static void mk_sense_buffer(struct sdebug_dev_info *devip, int key,
 }
 
 static void get_data_transfer_info(unsigned char *cmd,
-				   unsigned long long *lba, unsigned int *num)
+				   unsigned long long *lba, unsigned int *num,
+				   u32 *ei_lba)
 {
+	*ei_lba = 0;
+
 	switch (*cmd) {
+	case VARIABLE_LENGTH_CMD:
+		*lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
+			(u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
+			(u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
+			(u64)cmd[13] << 48 | (u64)cmd[12] << 56;
+
+		*ei_lba = (u32)cmd[23] | (u32)cmd[22] << 8 |
+			(u32)cmd[21] << 16 | (u32)cmd[20] << 24;
+
+		*num = (u32)cmd[31] | (u32)cmd[30] << 8 | (u32)cmd[29] << 16 |
+			(u32)cmd[28] << 24;
+		break;
+
 	case WRITE_16:
 	case READ_16:
 		*lba = (u64)cmd[9] | (u64)cmd[8] << 8 |
@@ -1589,7 +1607,7 @@ static int do_device_access(struct scsi_cmnd *scmd,
 }
 
 static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
-			    unsigned int sectors)
+			    unsigned int sectors, u32 ei_lba)
 {
 	unsigned int i, resid;
 	struct scatterlist *psgl;
@@ -1636,13 +1654,23 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 			return 0x01;
 		}
 
-		if (scsi_debug_dif != SD_DIF_TYPE3_PROTECTION &&
+		if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
 		    be32_to_cpu(sdt[i].ref_tag) != (sector & 0xffffffff)) {
 			printk(KERN_ERR "%s: REF check failed on sector %lu\n",
 			       __func__, (unsigned long)sector);
 			dif_errors++;
 			return 0x03;
 		}
+
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+		    be32_to_cpu(sdt[i].ref_tag) != ei_lba) {
+			printk(KERN_ERR "%s: REF check failed on sector %lu\n",
+			       __func__, (unsigned long)sector);
+			dif_errors++;
+			return 0x03;
+		}
+
+		ei_lba++;
 	}
 
 	resid = sectors * 8; /* Bytes of protection data to copy into sgl */
@@ -1670,7 +1698,8 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 }
 
 static int resp_read(struct scsi_cmnd *SCpnt, unsigned long long lba,
-		     unsigned int num, struct sdebug_dev_info *devip)
+		     unsigned int num, struct sdebug_dev_info *devip,
+		     u32 ei_lba)
 {
 	unsigned long iflags;
 	int ret;
@@ -1699,7 +1728,7 @@ static int resp_read(struct scsi_cmnd *SCpnt, unsigned long long lba,
 
 	/* DIX + T10 DIF */
 	if (scsi_debug_dix && scsi_prot_sg_count(SCpnt)) {
-		int prot_ret = prot_verify_read(SCpnt, lba, num);
+		int prot_ret = prot_verify_read(SCpnt, lba, num, ei_lba);
 
 		if (prot_ret) {
 			mk_sense_buffer(devip, ABORTED_COMMAND, 0x10, prot_ret);
@@ -1735,7 +1764,7 @@ void dump_sector(unsigned char *buf, int len)
 }
 
 static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
-			     unsigned int sectors)
+			     unsigned int sectors, u32 ei_lba)
 {
 	int i, j, ret;
 	struct sd_dif_tuple *sdt;
@@ -1749,11 +1778,6 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 
 	sector = do_div(tmp_sec, sdebug_store_sectors);
 
-	if (((SCpnt->cmnd[1] >> 5) & 7) != 1) {
-		printk(KERN_WARNING "scsi_debug: WRPROTECT != 1\n");
-		return 0;
-	}
-
 	BUG_ON(scsi_sg_count(SCpnt) == 0);
 	BUG_ON(scsi_prot_sg_count(SCpnt) == 0);
 
@@ -1808,7 +1832,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				goto out;
 			}
 
-			if (scsi_debug_dif != SD_DIF_TYPE3_PROTECTION &&
+			if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
 			    be32_to_cpu(sdt->ref_tag)
 			    != (start_sec & 0xffffffff)) {
 				printk(KERN_ERR
@@ -1819,6 +1843,16 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				goto out;
 			}
 
+			if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+			    be32_to_cpu(sdt->ref_tag) != ei_lba) {
+				printk(KERN_ERR
+				       "%s: REF check failed on sector %lu\n",
+				       __func__, (unsigned long)sector);
+				ret = 0x03;
+				dump_sector(daddr, scsi_debug_sector_size);
+				goto out;
+			}
+
 			/* Would be great to copy this in bigger
 			 * chunks.  However, for the sake of
 			 * correctness we need to verify each sector
@@ -1832,6 +1866,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				sector = 0;	/* Force wrap */
 
 			start_sec++;
+			ei_lba++;
 			daddr += scsi_debug_sector_size;
 			ppage_offset += sizeof(struct sd_dif_tuple);
 		}
@@ -1853,7 +1888,8 @@ out:
 }
 
 static int resp_write(struct scsi_cmnd *SCpnt, unsigned long long lba,
-		      unsigned int num, struct sdebug_dev_info *devip)
+		      unsigned int num, struct sdebug_dev_info *devip,
+		      u32 ei_lba)
 {
 	unsigned long iflags;
 	int ret;
@@ -1864,7 +1900,7 @@ static int resp_write(struct scsi_cmnd *SCpnt, unsigned long long lba,
 
 	/* DIX + T10 DIF */
 	if (scsi_debug_dix && scsi_prot_sg_count(SCpnt)) {
-		int prot_ret = prot_verify_write(SCpnt, lba, num);
+		int prot_ret = prot_verify_write(SCpnt, lba, num, ei_lba);
 
 		if (prot_ret) {
 			mk_sense_buffer(devip, ILLEGAL_REQUEST, 0x10, prot_ret);
@@ -2872,11 +2908,12 @@ static int __init scsi_debug_init(void)
 
 	case SD_DIF_TYPE0_PROTECTION:
 	case SD_DIF_TYPE1_PROTECTION:
+	case SD_DIF_TYPE2_PROTECTION:
 	case SD_DIF_TYPE3_PROTECTION:
 		break;
 
 	default:
-		printk(KERN_ERR "scsi_debug_init: dif must be 0, 1 or 3\n");
+		printk(KERN_ERR "scsi_debug_init: dif must be 0, 1, 2 or 3\n");
 		return -EINVAL;
 	}
 
@@ -3121,6 +3158,7 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 	int len, k;
 	unsigned int num;
 	unsigned long long lba;
+	u32 ei_lba;
 	int errsts = 0;
 	int target = SCpnt->device->id;
 	struct sdebug_dev_info *devip = NULL;
@@ -3254,14 +3292,30 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 	case READ_16:
 	case READ_12:
 	case READ_10:
+		/* READ{10,12,16} and DIF Type 2 are natural enemies */
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+		    cmd[1] & 0xe0) {
+			mk_sense_buffer(devip, ILLEGAL_REQUEST,
+					INVALID_COMMAND_OPCODE, 0);
+			errsts = check_condition_result;
+			break;
+		}
+
+		if ((scsi_debug_dif == SD_DIF_TYPE1_PROTECTION ||
+		     scsi_debug_dif == SD_DIF_TYPE3_PROTECTION) &&
+		    (cmd[1] & 0xe0) == 0)
+			printk(KERN_ERR "Unprotected RD/WR to DIF device\n");
+
+		/* fall through */
 	case READ_6:
+read:
 		errsts = check_readiness(SCpnt, 0, devip);
 		if (errsts)
 			break;
 		if (scsi_debug_fake_rw)
 			break;
-		get_data_transfer_info(cmd, &lba, &num);
-		errsts = resp_read(SCpnt, lba, num, devip);
+		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
+		errsts = resp_read(SCpnt, lba, num, devip, ei_lba);
 		if (inj_recovered && (0 == errsts)) {
 			mk_sense_buffer(devip, RECOVERED_ERROR,
 					THRESHOLD_EXCEEDED, 0);
@@ -3288,14 +3342,30 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 	case WRITE_16:
 	case WRITE_12:
 	case WRITE_10:
+		/* WRITE{10,12,16} and DIF Type 2 are natural enemies */
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+		    cmd[1] & 0xe0) {
+			mk_sense_buffer(devip, ILLEGAL_REQUEST,
+					INVALID_COMMAND_OPCODE, 0);
+			errsts = check_condition_result;
+			break;
+		}
+
+		if ((scsi_debug_dif == SD_DIF_TYPE1_PROTECTION ||
+		     scsi_debug_dif == SD_DIF_TYPE3_PROTECTION) &&
+		    (cmd[1] & 0xe0) == 0)
+			printk(KERN_ERR "Unprotected RD/WR to DIF device\n");
+
+		/* fall through */
 	case WRITE_6:
+write:
 		errsts = check_readiness(SCpnt, 0, devip);
 		if (errsts)
 			break;
 		if (scsi_debug_fake_rw)
 			break;
-		get_data_transfer_info(cmd, &lba, &num);
-		errsts = resp_write(SCpnt, lba, num, devip);
+		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
+		errsts = resp_write(SCpnt, lba, num, devip, ei_lba);
 		if (inj_recovered && (0 == errsts)) {
 			mk_sense_buffer(devip, RECOVERED_ERROR,
 					THRESHOLD_EXCEEDED, 0);
@@ -3341,15 +3411,34 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 			break;
 		if (scsi_debug_fake_rw)
 			break;
-		get_data_transfer_info(cmd, &lba, &num);
-		errsts = resp_read(SCpnt, lba, num, devip);
+		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
+		errsts = resp_read(SCpnt, lba, num, devip, ei_lba);
 		if (errsts)
 			break;
-		errsts = resp_write(SCpnt, lba, num, devip);
+		errsts = resp_write(SCpnt, lba, num, devip, ei_lba);
 		if (errsts)
 			break;
 		errsts = resp_xdwriteread(SCpnt, lba, num, devip);
 		break;
+	case VARIABLE_LENGTH_CMD:
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION) {
+
+			if ((cmd[10] & 0xe0) == 0)
+				printk(KERN_ERR
+				       "Unprotected RD/WR to DIF device\n");
+
+			if (cmd[9] == READ_32)
+				goto read;
+
+			if (cmd[9] == WRITE_32)
+				goto write;
+		}
+
+		mk_sense_buffer(devip, ILLEGAL_REQUEST,
+				INVALID_FIELD_IN_CDB, 0);
+		errsts = check_condition_result;
+		break;
+
 	default:
 		if (SCSI_DEBUG_OPT_NOISE & scsi_debug_opts)
 			printk(KERN_INFO "scsi_debug: Opcode: 0x%x not "
-- 
1.6.0.6

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Douglas Gilbert]

Martin K. Petersen wrote:
> Add support for 32-byte READ/WRITE as well as DIF Type 2 protection.
> 
> Reject protected 10/12/16 byte READ/WRITE commands when Type 2 is
> enabled.
> 
> Verify Type 2 reference tag according to Expected Initial LBA in 32-byte
> CDB.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>

Re: [PATCH 3/5] scsi: Fix protection scsi_data_buffer leak

[Boaz Harrosh]

On 09/11/2009 10:20 PM, Martin K. Petersen wrote:
> We would leak a scsi_data_buffer if the free_list command was of the
> protected variety.
> 
> Reported-by: Boaz Harrosh <bharrosh@panasas.com>

Reviewed-by: Boaz Harrosh <bharrosh@panasas.com>

I like the locality of the temp variables, thanks.
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/scsi.c |   11 +++++++----
>  1 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 2de5f3a..69397bb 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -241,10 +241,7 @@ scsi_host_alloc_command(struct Scsi_Host *shost, gfp_t gfp_mask)
>   */
>  struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
>  {
> -	struct scsi_cmnd *cmd;
> -	unsigned char *buf;
> -
> -	cmd = scsi_host_alloc_command(shost, gfp_mask);
> +	struct scsi_cmnd *cmd = scsi_host_alloc_command(shost, gfp_mask);
>  
>  	if (unlikely(!cmd)) {
>  		unsigned long flags;
> @@ -258,9 +255,15 @@ struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
>  		spin_unlock_irqrestore(&shost->free_list_lock, flags);
>  
>  		if (cmd) {
> +			void *buf, *prot;
> +
>  			buf = cmd->sense_buffer;
> +			prot = cmd->prot_sdb;
> +
>  			memset(cmd, 0, sizeof(*cmd));
> +
>  			cmd->sense_buffer = buf;
> +			cmd->prot_sdb = prot;
>  		}
>  	}
>  

Re: [PATCH 4/5] sd: Support disks formatted with DIF Type 2

[Boaz Harrosh]

On 09/11/2009 10:20 PM, Martin K. Petersen wrote:
> Disks formatted with DIF Type 2 reject READ/WRITE 6/10/12/16 commands
> when protection is enabled.  Only the 32-byte variants are supported.
> 
> Implement support for issusing 32-byte READ/WRITE and enable Type 2
> drives in the protection type detection logic.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

Reviewed-by: Boaz Harrosh <bharrosh@panasas.com>

> ---
>  drivers/scsi/sd.c   |   80 ++++++++++++++++++++++++++++++++++++++++++++-------
>  drivers/scsi/sd.h   |    5 +++
>  include/scsi/scsi.h |    3 ++
>  3 files changed, 77 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 36e2333..d138e6b 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -116,6 +116,9 @@ static DEFINE_IDA(sd_index_ida);
>   * object after last put) */
>  static DEFINE_MUTEX(sd_ref_mutex);
>  
> +struct kmem_cache *sd_cdb_cache;
> +mempool_t *sd_cdb_pool;
> +
>  static const char *sd_cache_types[] = {
>  	"write through", "none", "write back",
>  	"write back, no read (daft)"
> @@ -413,6 +416,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>  	sector_t threshold;
>  	unsigned int this_count = blk_rq_sectors(rq);
>  	int ret, host_dif;
> +	unsigned char protect;
>  
>  	if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
>  		ret = scsi_setup_blk_pc_cmnd(sdp, rq);
> @@ -545,13 +549,48 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>  	/* Set RDPROTECT/WRPROTECT if disk is formatted with DIF */
>  	host_dif = scsi_host_dif_capable(sdp->host, sdkp->protection_type);
>  	if (host_dif)
> -		SCpnt->cmnd[1] = 1 << 5;
> +		protect = 1 << 5;
>  	else
> -		SCpnt->cmnd[1] = 0;
> +		protect = 0;
> +
> +	if (host_dif == SD_DIF_TYPE2_PROTECTION) {
> +		SCpnt->cmnd = mempool_alloc(sd_cdb_pool, GFP_ATOMIC);
> +
> +		if (unlikely(SCpnt->cmnd == NULL)) {
> +			ret = BLKPREP_DEFER;
> +			goto out;
> +		}
>  
> -	if (block > 0xffffffff) {
> +		memset(SCpnt->cmnd, 0, SD_EXT_CDB_SIZE);
> +		SCpnt->cmnd[0] = VARIABLE_LENGTH_CMD;
> +		SCpnt->cmnd[7] = 0x18;
> +		SCpnt->cmnd[9] = (rq_data_dir(rq) == READ) ? READ_32 : WRITE_32;
> +		SCpnt->cmnd[10] = protect | (blk_fua_rq(rq) ? 0x8 : 0);
> +
> +		/* LBA */
> +		SCpnt->cmnd[12] = sizeof(block) > 4 ? (unsigned char) (block >> 56) & 0xff : 0;
> +		SCpnt->cmnd[13] = sizeof(block) > 4 ? (unsigned char) (block >> 48) & 0xff : 0;
> +		SCpnt->cmnd[14] = sizeof(block) > 4 ? (unsigned char) (block >> 40) & 0xff : 0;
> +		SCpnt->cmnd[15] = sizeof(block) > 4 ? (unsigned char) (block >> 32) & 0xff : 0;
> +		SCpnt->cmnd[16] = (unsigned char) (block >> 24) & 0xff;
> +		SCpnt->cmnd[17] = (unsigned char) (block >> 16) & 0xff;
> +		SCpnt->cmnd[18] = (unsigned char) (block >> 8) & 0xff;
> +		SCpnt->cmnd[19] = (unsigned char) block & 0xff;
> +
> +		/* Expected Indirect LBA */
> +		SCpnt->cmnd[20] = (unsigned char) (block >> 24) & 0xff;
> +		SCpnt->cmnd[21] = (unsigned char) (block >> 16) & 0xff;
> +		SCpnt->cmnd[22] = (unsigned char) (block >> 8) & 0xff;
> +		SCpnt->cmnd[23] = (unsigned char) block & 0xff;
> +
> +		/* Transfer length */
> +		SCpnt->cmnd[28] = (unsigned char) (this_count >> 24) & 0xff;
> +		SCpnt->cmnd[29] = (unsigned char) (this_count >> 16) & 0xff;
> +		SCpnt->cmnd[30] = (unsigned char) (this_count >> 8) & 0xff;
> +		SCpnt->cmnd[31] = (unsigned char) this_count & 0xff;
> +	} else if (block > 0xffffffff) {
>  		SCpnt->cmnd[0] += READ_16 - READ_6;
> -		SCpnt->cmnd[1] |= blk_fua_rq(rq) ? 0x8 : 0;
> +		SCpnt->cmnd[1] = protect | (blk_fua_rq(rq) ? 0x8 : 0);
>  		SCpnt->cmnd[2] = sizeof(block) > 4 ? (unsigned char) (block >> 56) & 0xff : 0;
>  		SCpnt->cmnd[3] = sizeof(block) > 4 ? (unsigned char) (block >> 48) & 0xff : 0;
>  		SCpnt->cmnd[4] = sizeof(block) > 4 ? (unsigned char) (block >> 40) & 0xff : 0;
> @@ -572,7 +611,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>  			this_count = 0xffff;
>  
>  		SCpnt->cmnd[0] += READ_10 - READ_6;
> -		SCpnt->cmnd[1] |= blk_fua_rq(rq) ? 0x8 : 0;
> +		SCpnt->cmnd[1] = protect | (blk_fua_rq(rq) ? 0x8 : 0);
>  		SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
>  		SCpnt->cmnd[3] = (unsigned char) (block >> 16) & 0xff;
>  		SCpnt->cmnd[4] = (unsigned char) (block >> 8) & 0xff;
> @@ -1047,6 +1086,7 @@ static int sd_done(struct scsi_cmnd *SCpnt)
>  	int result = SCpnt->result;
>  	unsigned int good_bytes = result ? 0 : scsi_bufflen(SCpnt);
>  	struct scsi_sense_hdr sshdr;
> +	struct scsi_disk *sdkp = scsi_disk(SCpnt->request->rq_disk);
>  	int sense_valid = 0;
>  	int sense_deferred = 0;
>  
> @@ -1108,6 +1148,10 @@ static int sd_done(struct scsi_cmnd *SCpnt)
>  	if (rq_data_dir(SCpnt->request) == READ && scsi_prot_sg_count(SCpnt))
>  		sd_dif_complete(SCpnt, good_bytes);
>  
> +	if (scsi_host_dif_capable(sdkp->device->host, sdkp->protection_type)
> +	    == SD_DIF_TYPE2_PROTECTION)
> +		mempool_free(SCpnt->cmnd, sd_cdb_pool);
> +
>  	return good_bytes;
>  }
>  
> @@ -1271,12 +1315,7 @@ void sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
>  
>  	sdkp->protection_type = type;
>  
> -	switch (type) {
> -	case SD_DIF_TYPE1_PROTECTION:
> -	case SD_DIF_TYPE3_PROTECTION:
> -		break;
> -
> -	default:
> +	if (type > SD_DIF_TYPE3_PROTECTION) {
>  		sd_printk(KERN_ERR, sdkp, "formatted with unsupported "	\
>  			  "protection type %u. Disabling disk!\n", type);
>  		sdkp->capacity = 0;
> @@ -2321,8 +2360,24 @@ static int __init init_sd(void)
>  	if (err)
>  		goto err_out_class;
>  
> +	sd_cdb_cache = kmem_cache_create("sd_ext_cdb", SD_EXT_CDB_SIZE,
> +					 0, 0, NULL);
> +	if (!sd_cdb_cache) {
> +		printk(KERN_ERR "sd: can't init extended cdb cache\n");
> +		goto err_out_class;
> +	}
> +
> +	sd_cdb_pool = mempool_create_slab_pool(SD_MEMPOOL_SIZE, sd_cdb_cache);
> +	if (!sd_cdb_pool) {
> +		printk(KERN_ERR "sd: can't init extended cdb pool\n");
> +		goto err_out_cache;
> +	}
> +
>  	return 0;
>  
> +err_out_cache:
> +	kmem_cache_destroy(sd_cdb_cache);
> +
>  err_out_class:
>  	class_unregister(&sd_disk_class);
>  err_out:
> @@ -2342,6 +2397,9 @@ static void __exit exit_sd(void)
>  
>  	SCSI_LOG_HLQUEUE(3, printk("exit_sd: exiting sd driver\n"));
>  
> +	mempool_destroy(sd_cdb_pool);
> +	kmem_cache_destroy(sd_cdb_cache);
> +
>  	scsi_unregister_driver(&sd_template.gendrv);
>  	class_unregister(&sd_disk_class);
>  
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index ce1f5f8..e374804 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -37,6 +37,11 @@
>   */
>  #define SD_LAST_BUGGY_SECTORS	8
>  
> +enum {
> +	SD_EXT_CDB_SIZE = 32,	/* Extended CDB size */
> +	SD_MEMPOOL_SIZE = 2,	/* CDB pool size */
> +};
> +
>  struct scsi_disk {
>  	struct scsi_driver *driver;	/* always &sd_template */
>  	struct scsi_device *device;
> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
> index 084478e..34c46ab 100644
> --- a/include/scsi/scsi.h
> +++ b/include/scsi/scsi.h
> @@ -129,6 +129,9 @@ struct scsi_cmnd;
>  #define MI_REPORT_TARGET_PGS  0x0a
>  /* values for maintenance out */
>  #define MO_SET_TARGET_PGS     0x0a
> +/* values for variable length command */
> +#define READ_32		      0x09
> +#define WRITE_32	      0x0b
>  
>  /* Values for T10/04-262r7 */
>  #define	ATA_16		      0x85	/* 16-byte pass-thru */

Re: [PATCH 1/5] scsi: Deprecate SCSI_PROT_*_CONVERT operations

[James Smart]

lpfc mods look fine.

Acked-by: James Smart <james.smart@emulex.com>
Acked-by: Ihab Hamadi<Ihab.Hamadi@Emulex.Com>

-- james s

Martin K. Petersen wrote:
> The checksum format is orthogonal to whether the protection information
> is being passed on beyond the HBA or not.  It is perfectly valid to use
> a non-T10 CRC with WRITE_STRIP and READ_INSERT.
>
> Consequently it no longer makes sense to explicitly refer to the
> conversion in the protection operation.  Update sd_dif and lpfc
> accordingly.
>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/lpfc/lpfc_scsi.c |   15 +++------------
>  drivers/scsi/sd_dif.c         |   20 ++++----------------
>  include/scsi/scsi_cmnd.h      |    4 ----
>  3 files changed, 7 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
> index da59c4f..ad75e19 100644
> --- a/drivers/scsi/lpfc/lpfc_scsi.c
> +++ b/drivers/scsi/lpfc/lpfc_scsi.c
> @@ -56,8 +56,6 @@ static char *dif_op_str[] = {
>  	"SCSI_PROT_WRITE_INSERT",
>  	"SCSI_PROT_READ_PASS",
>  	"SCSI_PROT_WRITE_PASS",
> -	"SCSI_PROT_READ_CONVERT",
> -	"SCSI_PROT_WRITE_CONVERT"
>  };
>  static void
>  lpfc_release_scsi_buf_s4(struct lpfc_hba *phba, struct lpfc_scsi_buf *psb);
> @@ -1131,13 +1129,11 @@ lpfc_sc_to_sli_prof(struct scsi_cmnd *sc)
>  			ret_prof = LPFC_PROF_A1;
>  			break;
>  
> -		case SCSI_PROT_READ_CONVERT:
> -		case SCSI_PROT_WRITE_CONVERT:
> +		case SCSI_PROT_READ_PASS:
> +		case SCSI_PROT_WRITE_PASS:
>  			ret_prof = LPFC_PROF_AST1;
>  			break;
>  
> -		case SCSI_PROT_READ_PASS:
> -		case SCSI_PROT_WRITE_PASS:
>  		case SCSI_PROT_NORMAL:
>  		default:
>  			printk(KERN_ERR "Bad op/guard:%d/%d combination\n",
> @@ -1157,8 +1153,6 @@ lpfc_sc_to_sli_prof(struct scsi_cmnd *sc)
>  			ret_prof = LPFC_PROF_C1;
>  			break;
>  
> -		case SCSI_PROT_READ_CONVERT:
> -		case SCSI_PROT_WRITE_CONVERT:
>  		case SCSI_PROT_READ_INSERT:
>  		case SCSI_PROT_WRITE_STRIP:
>  		case SCSI_PROT_NORMAL:
> @@ -1209,8 +1203,7 @@ lpfc_get_cmd_dif_parms(struct scsi_cmnd *sc, uint16_t *apptagmask,
>  	static int cnt;
>  
>  	if (protcnt && (op == SCSI_PROT_WRITE_STRIP ||
> -				op == SCSI_PROT_WRITE_PASS ||
> -				op == SCSI_PROT_WRITE_CONVERT)) {
> +				op == SCSI_PROT_WRITE_PASS)) {
>  
>  		cnt++;
>  		spt = page_address(sg_page(scsi_prot_sglist(sc))) +
> @@ -1501,8 +1494,6 @@ lpfc_prot_group_type(struct lpfc_hba *phba, struct scsi_cmnd *sc)
>  	case SCSI_PROT_WRITE_STRIP:
>  	case SCSI_PROT_READ_PASS:
>  	case SCSI_PROT_WRITE_PASS:
> -	case SCSI_PROT_WRITE_CONVERT:
> -	case SCSI_PROT_READ_CONVERT:
>  		ret = LPFC_PG_TYPE_DIF_BUF;
>  		break;
>  	default:
> diff --git a/drivers/scsi/sd_dif.c b/drivers/scsi/sd_dif.c
> index 82f14a9..84224dd 100644
> --- a/drivers/scsi/sd_dif.c
> +++ b/drivers/scsi/sd_dif.c
> @@ -364,15 +364,9 @@ void sd_dif_config_host(struct scsi_disk *sdkp)
>   */
>  void sd_dif_op(struct scsi_cmnd *scmd, unsigned int dif, unsigned int dix, unsigned int type)
>  {
> -	int csum_convert, prot_op;
> +	int prot_op;
>  
> -	prot_op = 0;
> -
> -	/* Convert checksum? */
> -	if (scsi_host_get_guard(scmd->device->host) != SHOST_DIX_GUARD_CRC)
> -		csum_convert = 1;
> -	else
> -		csum_convert = 0;
> +	prot_op = SCSI_PROT_NORMAL;
>  
>  	BUG_ON(dif && (scmd->cmnd[0] == READ_6 || scmd->cmnd[0] == WRITE_6));
>  
> @@ -382,10 +376,7 @@ void sd_dif_op(struct scsi_cmnd *scmd, unsigned int dif, unsigned int dix, unsig
>  	case READ_12:
>  	case READ_16:
>  		if (dif && dix)
> -			if (csum_convert)
> -				prot_op = SCSI_PROT_READ_CONVERT;
> -			else
> -				prot_op = SCSI_PROT_READ_PASS;
> +			prot_op = SCSI_PROT_READ_PASS;
>  		else if (dif && !dix)
>  			prot_op = SCSI_PROT_READ_STRIP;
>  		else if (!dif && dix)
> @@ -398,10 +389,7 @@ void sd_dif_op(struct scsi_cmnd *scmd, unsigned int dif, unsigned int dix, unsig
>  	case WRITE_12:
>  	case WRITE_16:
>  		if (dif && dix)
> -			if (csum_convert)
> -				prot_op = SCSI_PROT_WRITE_CONVERT;
> -			else
> -				prot_op = SCSI_PROT_WRITE_PASS;
> +			prot_op = SCSI_PROT_WRITE_PASS;
>  		else if (dif && !dix)
>  			prot_op = SCSI_PROT_WRITE_INSERT;
>  		else if (!dif && dix)
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 3878d1d..a5e885a 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -229,10 +229,6 @@ enum scsi_prot_operations {
>  	/* OS-HBA: Protected, HBA-Target: Protected */
>  	SCSI_PROT_READ_PASS,
>  	SCSI_PROT_WRITE_PASS,
> -
> -	/* OS-HBA: Protected, HBA-Target: Protected, checksum conversion */
> -	SCSI_PROT_READ_CONVERT,
> -	SCSI_PROT_WRITE_CONVERT,
>  };
>  
>  static inline void scsi_set_prot_op(struct scsi_cmnd *scmd, unsigned char op)
>   

[PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Martin K. Petersen]

Add support for 32-byte READ/WRITE as well as DIF Type 2 protection.

Reject protected 10/12/16 byte READ/WRITE commands when Type 2 is
enabled.

Verify Type 2 reference tag according to Expected Initial LBA in 32-byte
CDB.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/scsi_debug.c |  139 +++++++++++++++++++++++++++++++++++++--------
 1 files changed, 116 insertions(+), 23 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index fb9af20..c4103be 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -50,6 +50,7 @@
 #include <scsi/scsi_host.h>
 #include <scsi/scsicam.h>
 #include <scsi/scsi_eh.h>
+#include <scsi/scsi_dbg.h>
 
 #include "sd.h"
 #include "scsi_logging.h"
@@ -64,6 +65,7 @@ static const char * scsi_debug_version_date = "20070104";
 #define PARAMETER_LIST_LENGTH_ERR 0x1a
 #define INVALID_OPCODE 0x20
 #define ADDR_OUT_OF_RANGE 0x21
+#define INVALID_COMMAND_OPCODE 0x20
 #define INVALID_FIELD_IN_CDB 0x24
 #define INVALID_FIELD_IN_PARAM_LIST 0x26
 #define POWERON_RESET 0x29
@@ -180,7 +182,7 @@ static int sdebug_sectors_per;		/* sectors per cylinder */
 #define SDEBUG_SENSE_LEN 32
 
 #define SCSI_DEBUG_CANQUEUE  255
-#define SCSI_DEBUG_MAX_CMD_LEN 16
+#define SCSI_DEBUG_MAX_CMD_LEN 32
 
 struct sdebug_dev_info {
 	struct list_head dev_list;
@@ -296,9 +298,25 @@ static void mk_sense_buffer(struct sdebug_dev_info *devip, int key,
 }
 
 static void get_data_transfer_info(unsigned char *cmd,
-				   unsigned long long *lba, unsigned int *num)
+				   unsigned long long *lba, unsigned int *num,
+				   u32 *ei_lba)
 {
+	*ei_lba = 0;
+
 	switch (*cmd) {
+	case VARIABLE_LENGTH_CMD:
+		*lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
+			(u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
+			(u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
+			(u64)cmd[13] << 48 | (u64)cmd[12] << 56;
+
+		*ei_lba = (u32)cmd[23] | (u32)cmd[22] << 8 |
+			(u32)cmd[21] << 16 | (u32)cmd[20] << 24;
+
+		*num = (u32)cmd[31] | (u32)cmd[30] << 8 | (u32)cmd[29] << 16 |
+			(u32)cmd[28] << 24;
+		break;
+
 	case WRITE_16:
 	case READ_16:
 		*lba = (u64)cmd[9] | (u64)cmd[8] << 8 |
@@ -1589,7 +1607,7 @@ static int do_device_access(struct scsi_cmnd *scmd,
 }
 
 static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
-			    unsigned int sectors)
+			    unsigned int sectors, u32 ei_lba)
 {
 	unsigned int i, resid;
 	struct scatterlist *psgl;
@@ -1636,13 +1654,23 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 			return 0x01;
 		}
 
-		if (scsi_debug_dif != SD_DIF_TYPE3_PROTECTION &&
+		if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
 		    be32_to_cpu(sdt[i].ref_tag) != (sector & 0xffffffff)) {
 			printk(KERN_ERR "%s: REF check failed on sector %lu\n",
 			       __func__, (unsigned long)sector);
 			dif_errors++;
 			return 0x03;
 		}
+
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+		    be32_to_cpu(sdt[i].ref_tag) != ei_lba) {
+			printk(KERN_ERR "%s: REF check failed on sector %lu\n",
+			       __func__, (unsigned long)sector);
+			dif_errors++;
+			return 0x03;
+		}
+
+		ei_lba++;
 	}
 
 	resid = sectors * 8; /* Bytes of protection data to copy into sgl */
@@ -1670,7 +1698,8 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 }
 
 static int resp_read(struct scsi_cmnd *SCpnt, unsigned long long lba,
-		     unsigned int num, struct sdebug_dev_info *devip)
+		     unsigned int num, struct sdebug_dev_info *devip,
+		     u32 ei_lba)
 {
 	unsigned long iflags;
 	int ret;
@@ -1699,7 +1728,7 @@ static int resp_read(struct scsi_cmnd *SCpnt, unsigned long long lba,
 
 	/* DIX + T10 DIF */
 	if (scsi_debug_dix && scsi_prot_sg_count(SCpnt)) {
-		int prot_ret = prot_verify_read(SCpnt, lba, num);
+		int prot_ret = prot_verify_read(SCpnt, lba, num, ei_lba);
 
 		if (prot_ret) {
 			mk_sense_buffer(devip, ABORTED_COMMAND, 0x10, prot_ret);
@@ -1735,7 +1764,7 @@ void dump_sector(unsigned char *buf, int len)
 }
 
 static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
-			     unsigned int sectors)
+			     unsigned int sectors, u32 ei_lba)
 {
 	int i, j, ret;
 	struct sd_dif_tuple *sdt;
@@ -1749,11 +1778,6 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 
 	sector = do_div(tmp_sec, sdebug_store_sectors);
 
-	if (((SCpnt->cmnd[1] >> 5) & 7) != 1) {
-		printk(KERN_WARNING "scsi_debug: WRPROTECT != 1\n");
-		return 0;
-	}
-
 	BUG_ON(scsi_sg_count(SCpnt) == 0);
 	BUG_ON(scsi_prot_sg_count(SCpnt) == 0);
 
@@ -1808,7 +1832,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				goto out;
 			}
 
-			if (scsi_debug_dif != SD_DIF_TYPE3_PROTECTION &&
+			if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
 			    be32_to_cpu(sdt->ref_tag)
 			    != (start_sec & 0xffffffff)) {
 				printk(KERN_ERR
@@ -1819,6 +1843,16 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				goto out;
 			}
 
+			if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+			    be32_to_cpu(sdt->ref_tag) != ei_lba) {
+				printk(KERN_ERR
+				       "%s: REF check failed on sector %lu\n",
+				       __func__, (unsigned long)sector);
+				ret = 0x03;
+				dump_sector(daddr, scsi_debug_sector_size);
+				goto out;
+			}
+
 			/* Would be great to copy this in bigger
 			 * chunks.  However, for the sake of
 			 * correctness we need to verify each sector
@@ -1832,6 +1866,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				sector = 0;	/* Force wrap */
 
 			start_sec++;
+			ei_lba++;
 			daddr += scsi_debug_sector_size;
 			ppage_offset += sizeof(struct sd_dif_tuple);
 		}
@@ -1853,7 +1888,8 @@ out:
 }
 
 static int resp_write(struct scsi_cmnd *SCpnt, unsigned long long lba,
-		      unsigned int num, struct sdebug_dev_info *devip)
+		      unsigned int num, struct sdebug_dev_info *devip,
+		      u32 ei_lba)
 {
 	unsigned long iflags;
 	int ret;
@@ -1864,7 +1900,7 @@ static int resp_write(struct scsi_cmnd *SCpnt, unsigned long long lba,
 
 	/* DIX + T10 DIF */
 	if (scsi_debug_dix && scsi_prot_sg_count(SCpnt)) {
-		int prot_ret = prot_verify_write(SCpnt, lba, num);
+		int prot_ret = prot_verify_write(SCpnt, lba, num, ei_lba);
 
 		if (prot_ret) {
 			mk_sense_buffer(devip, ILLEGAL_REQUEST, 0x10, prot_ret);
@@ -2872,11 +2908,12 @@ static int __init scsi_debug_init(void)
 
 	case SD_DIF_TYPE0_PROTECTION:
 	case SD_DIF_TYPE1_PROTECTION:
+	case SD_DIF_TYPE2_PROTECTION:
 	case SD_DIF_TYPE3_PROTECTION:
 		break;
 
 	default:
-		printk(KERN_ERR "scsi_debug_init: dif must be 0, 1 or 3\n");
+		printk(KERN_ERR "scsi_debug_init: dif must be 0, 1, 2 or 3\n");
 		return -EINVAL;
 	}
 
@@ -3121,6 +3158,7 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 	int len, k;
 	unsigned int num;
 	unsigned long long lba;
+	u32 ei_lba;
 	int errsts = 0;
 	int target = SCpnt->device->id;
 	struct sdebug_dev_info *devip = NULL;
@@ -3254,14 +3292,30 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 	case READ_16:
 	case READ_12:
 	case READ_10:
+		/* READ{10,12,16} and DIF Type 2 are natural enemies */
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+		    cmd[1] & 0xe0) {
+			mk_sense_buffer(devip, ILLEGAL_REQUEST,
+					INVALID_COMMAND_OPCODE, 0);
+			errsts = check_condition_result;
+			break;
+		}
+
+		if ((scsi_debug_dif == SD_DIF_TYPE1_PROTECTION ||
+		     scsi_debug_dif == SD_DIF_TYPE3_PROTECTION) &&
+		    (cmd[1] & 0xe0) == 0)
+			printk(KERN_ERR "Unprotected RD/WR to DIF device\n");
+
+		/* fall through */
 	case READ_6:
+read:
 		errsts = check_readiness(SCpnt, 0, devip);
 		if (errsts)
 			break;
 		if (scsi_debug_fake_rw)
 			break;
-		get_data_transfer_info(cmd, &lba, &num);
-		errsts = resp_read(SCpnt, lba, num, devip);
+		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
+		errsts = resp_read(SCpnt, lba, num, devip, ei_lba);
 		if (inj_recovered && (0 == errsts)) {
 			mk_sense_buffer(devip, RECOVERED_ERROR,
 					THRESHOLD_EXCEEDED, 0);
@@ -3288,14 +3342,30 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 	case WRITE_16:
 	case WRITE_12:
 	case WRITE_10:
+		/* WRITE{10,12,16} and DIF Type 2 are natural enemies */
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+		    cmd[1] & 0xe0) {
+			mk_sense_buffer(devip, ILLEGAL_REQUEST,
+					INVALID_COMMAND_OPCODE, 0);
+			errsts = check_condition_result;
+			break;
+		}
+
+		if ((scsi_debug_dif == SD_DIF_TYPE1_PROTECTION ||
+		     scsi_debug_dif == SD_DIF_TYPE3_PROTECTION) &&
+		    (cmd[1] & 0xe0) == 0)
+			printk(KERN_ERR "Unprotected RD/WR to DIF device\n");
+
+		/* fall through */
 	case WRITE_6:
+write:
 		errsts = check_readiness(SCpnt, 0, devip);
 		if (errsts)
 			break;
 		if (scsi_debug_fake_rw)
 			break;
-		get_data_transfer_info(cmd, &lba, &num);
-		errsts = resp_write(SCpnt, lba, num, devip);
+		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
+		errsts = resp_write(SCpnt, lba, num, devip, ei_lba);
 		if (inj_recovered && (0 == errsts)) {
 			mk_sense_buffer(devip, RECOVERED_ERROR,
 					THRESHOLD_EXCEEDED, 0);
@@ -3341,15 +3411,38 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 			break;
 		if (scsi_debug_fake_rw)
 			break;
-		get_data_transfer_info(cmd, &lba, &num);
-		errsts = resp_read(SCpnt, lba, num, devip);
+		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
+		errsts = resp_read(SCpnt, lba, num, devip, ei_lba);
 		if (errsts)
 			break;
-		errsts = resp_write(SCpnt, lba, num, devip);
+		errsts = resp_write(SCpnt, lba, num, devip, ei_lba);
 		if (errsts)
 			break;
 		errsts = resp_xdwriteread(SCpnt, lba, num, devip);
 		break;
+	case VARIABLE_LENGTH_CMD:
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION) {
+
+			if ((cmd[10] & 0xe0) == 0)
+				printk(KERN_ERR
+				       "Unprotected RD/WR to DIF device\n");
+
+			if (cmd[9] == READ_32) {
+				BUG_ON(SCpnt->cmd_len < 32);
+				goto read;
+			}
+
+			if (cmd[9] == WRITE_32) {
+				BUG_ON(SCpnt->cmd_len < 32);
+				goto write;
+			}
+		}
+
+		mk_sense_buffer(devip, ILLEGAL_REQUEST,
+				INVALID_FIELD_IN_CDB, 0);
+		errsts = check_condition_result;
+		break;
+
 	default:
 		if (SCSI_DEBUG_OPT_NOISE & scsi_debug_opts)
 			printk(KERN_INFO "scsi_debug: Opcode: 0x%x not "
-- 
1.6.0.6

[PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Martin K. Petersen]

Add support for 32-byte READ/WRITE as well as DIF Type 2 protection.

Reject protected 10/12/16 byte READ/WRITE commands when Type 2 is
enabled.

Verify Type 2 reference tag according to Expected Initial LBA in 32-byte
CDB.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/scsi_debug.c |  135 +++++++++++++++++++++++++++++++++++++--------
 1 files changed, 112 insertions(+), 23 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index fb9af20..f551ec3 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -50,6 +50,7 @@
 #include <scsi/scsi_host.h>
 #include <scsi/scsicam.h>
 #include <scsi/scsi_eh.h>
+#include <scsi/scsi_dbg.h>
 
 #include "sd.h"
 #include "scsi_logging.h"
@@ -64,6 +65,7 @@ static const char * scsi_debug_version_date = "20070104";
 #define PARAMETER_LIST_LENGTH_ERR 0x1a
 #define INVALID_OPCODE 0x20
 #define ADDR_OUT_OF_RANGE 0x21
+#define INVALID_COMMAND_OPCODE 0x20
 #define INVALID_FIELD_IN_CDB 0x24
 #define INVALID_FIELD_IN_PARAM_LIST 0x26
 #define POWERON_RESET 0x29
@@ -180,7 +182,7 @@ static int sdebug_sectors_per;		/* sectors per cylinder */
 #define SDEBUG_SENSE_LEN 32
 
 #define SCSI_DEBUG_CANQUEUE  255
-#define SCSI_DEBUG_MAX_CMD_LEN 16
+#define SCSI_DEBUG_MAX_CMD_LEN 32
 
 struct sdebug_dev_info {
 	struct list_head dev_list;
@@ -296,9 +298,25 @@ static void mk_sense_buffer(struct sdebug_dev_info *devip, int key,
 }
 
 static void get_data_transfer_info(unsigned char *cmd,
-				   unsigned long long *lba, unsigned int *num)
+				   unsigned long long *lba, unsigned int *num,
+				   u32 *ei_lba)
 {
+	*ei_lba = 0;
+
 	switch (*cmd) {
+	case VARIABLE_LENGTH_CMD:
+		*lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
+			(u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
+			(u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
+			(u64)cmd[13] << 48 | (u64)cmd[12] << 56;
+
+		*ei_lba = (u32)cmd[23] | (u32)cmd[22] << 8 |
+			(u32)cmd[21] << 16 | (u32)cmd[20] << 24;
+
+		*num = (u32)cmd[31] | (u32)cmd[30] << 8 | (u32)cmd[29] << 16 |
+			(u32)cmd[28] << 24;
+		break;
+
 	case WRITE_16:
 	case READ_16:
 		*lba = (u64)cmd[9] | (u64)cmd[8] << 8 |
@@ -1589,7 +1607,7 @@ static int do_device_access(struct scsi_cmnd *scmd,
 }
 
 static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
-			    unsigned int sectors)
+			    unsigned int sectors, u32 ei_lba)
 {
 	unsigned int i, resid;
 	struct scatterlist *psgl;
@@ -1636,13 +1654,23 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 			return 0x01;
 		}
 
-		if (scsi_debug_dif != SD_DIF_TYPE3_PROTECTION &&
+		if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
 		    be32_to_cpu(sdt[i].ref_tag) != (sector & 0xffffffff)) {
 			printk(KERN_ERR "%s: REF check failed on sector %lu\n",
 			       __func__, (unsigned long)sector);
 			dif_errors++;
 			return 0x03;
 		}
+
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+		    be32_to_cpu(sdt[i].ref_tag) != ei_lba) {
+			printk(KERN_ERR "%s: REF check failed on sector %lu\n",
+			       __func__, (unsigned long)sector);
+			dif_errors++;
+			return 0x03;
+		}
+
+		ei_lba++;
 	}
 
 	resid = sectors * 8; /* Bytes of protection data to copy into sgl */
@@ -1670,7 +1698,8 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 }
 
 static int resp_read(struct scsi_cmnd *SCpnt, unsigned long long lba,
-		     unsigned int num, struct sdebug_dev_info *devip)
+		     unsigned int num, struct sdebug_dev_info *devip,
+		     u32 ei_lba)
 {
 	unsigned long iflags;
 	int ret;
@@ -1699,7 +1728,7 @@ static int resp_read(struct scsi_cmnd *SCpnt, unsigned long long lba,
 
 	/* DIX + T10 DIF */
 	if (scsi_debug_dix && scsi_prot_sg_count(SCpnt)) {
-		int prot_ret = prot_verify_read(SCpnt, lba, num);
+		int prot_ret = prot_verify_read(SCpnt, lba, num, ei_lba);
 
 		if (prot_ret) {
 			mk_sense_buffer(devip, ABORTED_COMMAND, 0x10, prot_ret);
@@ -1735,7 +1764,7 @@ void dump_sector(unsigned char *buf, int len)
 }
 
 static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
-			     unsigned int sectors)
+			     unsigned int sectors, u32 ei_lba)
 {
 	int i, j, ret;
 	struct sd_dif_tuple *sdt;
@@ -1749,11 +1778,6 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 
 	sector = do_div(tmp_sec, sdebug_store_sectors);
 
-	if (((SCpnt->cmnd[1] >> 5) & 7) != 1) {
-		printk(KERN_WARNING "scsi_debug: WRPROTECT != 1\n");
-		return 0;
-	}
-
 	BUG_ON(scsi_sg_count(SCpnt) == 0);
 	BUG_ON(scsi_prot_sg_count(SCpnt) == 0);
 
@@ -1808,7 +1832,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				goto out;
 			}
 
-			if (scsi_debug_dif != SD_DIF_TYPE3_PROTECTION &&
+			if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
 			    be32_to_cpu(sdt->ref_tag)
 			    != (start_sec & 0xffffffff)) {
 				printk(KERN_ERR
@@ -1819,6 +1843,16 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				goto out;
 			}
 
+			if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+			    be32_to_cpu(sdt->ref_tag) != ei_lba) {
+				printk(KERN_ERR
+				       "%s: REF check failed on sector %lu\n",
+				       __func__, (unsigned long)sector);
+				ret = 0x03;
+				dump_sector(daddr, scsi_debug_sector_size);
+				goto out;
+			}
+
 			/* Would be great to copy this in bigger
 			 * chunks.  However, for the sake of
 			 * correctness we need to verify each sector
@@ -1832,6 +1866,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				sector = 0;	/* Force wrap */
 
 			start_sec++;
+			ei_lba++;
 			daddr += scsi_debug_sector_size;
 			ppage_offset += sizeof(struct sd_dif_tuple);
 		}
@@ -1853,7 +1888,8 @@ out:
 }
 
 static int resp_write(struct scsi_cmnd *SCpnt, unsigned long long lba,
-		      unsigned int num, struct sdebug_dev_info *devip)
+		      unsigned int num, struct sdebug_dev_info *devip,
+		      u32 ei_lba)
 {
 	unsigned long iflags;
 	int ret;
@@ -1864,7 +1900,7 @@ static int resp_write(struct scsi_cmnd *SCpnt, unsigned long long lba,
 
 	/* DIX + T10 DIF */
 	if (scsi_debug_dix && scsi_prot_sg_count(SCpnt)) {
-		int prot_ret = prot_verify_write(SCpnt, lba, num);
+		int prot_ret = prot_verify_write(SCpnt, lba, num, ei_lba);
 
 		if (prot_ret) {
 			mk_sense_buffer(devip, ILLEGAL_REQUEST, 0x10, prot_ret);
@@ -2872,11 +2908,12 @@ static int __init scsi_debug_init(void)
 
 	case SD_DIF_TYPE0_PROTECTION:
 	case SD_DIF_TYPE1_PROTECTION:
+	case SD_DIF_TYPE2_PROTECTION:
 	case SD_DIF_TYPE3_PROTECTION:
 		break;
 
 	default:
-		printk(KERN_ERR "scsi_debug_init: dif must be 0, 1 or 3\n");
+		printk(KERN_ERR "scsi_debug_init: dif must be 0, 1, 2 or 3\n");
 		return -EINVAL;
 	}
 
@@ -3121,6 +3158,7 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 	int len, k;
 	unsigned int num;
 	unsigned long long lba;
+	u32 ei_lba;
 	int errsts = 0;
 	int target = SCpnt->device->id;
 	struct sdebug_dev_info *devip = NULL;
@@ -3254,14 +3292,30 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 	case READ_16:
 	case READ_12:
 	case READ_10:
+		/* READ{10,12,16} and DIF Type 2 are natural enemies */
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+		    cmd[1] & 0xe0) {
+			mk_sense_buffer(devip, ILLEGAL_REQUEST,
+					INVALID_COMMAND_OPCODE, 0);
+			errsts = check_condition_result;
+			break;
+		}
+
+		if ((scsi_debug_dif == SD_DIF_TYPE1_PROTECTION ||
+		     scsi_debug_dif == SD_DIF_TYPE3_PROTECTION) &&
+		    (cmd[1] & 0xe0) == 0)
+			printk(KERN_ERR "Unprotected RD/WR to DIF device\n");
+
+		/* fall through */
 	case READ_6:
+read:
 		errsts = check_readiness(SCpnt, 0, devip);
 		if (errsts)
 			break;
 		if (scsi_debug_fake_rw)
 			break;
-		get_data_transfer_info(cmd, &lba, &num);
-		errsts = resp_read(SCpnt, lba, num, devip);
+		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
+		errsts = resp_read(SCpnt, lba, num, devip, ei_lba);
 		if (inj_recovered && (0 == errsts)) {
 			mk_sense_buffer(devip, RECOVERED_ERROR,
 					THRESHOLD_EXCEEDED, 0);
@@ -3288,14 +3342,30 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 	case WRITE_16:
 	case WRITE_12:
 	case WRITE_10:
+		/* WRITE{10,12,16} and DIF Type 2 are natural enemies */
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+		    cmd[1] & 0xe0) {
+			mk_sense_buffer(devip, ILLEGAL_REQUEST,
+					INVALID_COMMAND_OPCODE, 0);
+			errsts = check_condition_result;
+			break;
+		}
+
+		if ((scsi_debug_dif == SD_DIF_TYPE1_PROTECTION ||
+		     scsi_debug_dif == SD_DIF_TYPE3_PROTECTION) &&
+		    (cmd[1] & 0xe0) == 0)
+			printk(KERN_ERR "Unprotected RD/WR to DIF device\n");
+
+		/* fall through */
 	case WRITE_6:
+write:
 		errsts = check_readiness(SCpnt, 0, devip);
 		if (errsts)
 			break;
 		if (scsi_debug_fake_rw)
 			break;
-		get_data_transfer_info(cmd, &lba, &num);
-		errsts = resp_write(SCpnt, lba, num, devip);
+		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
+		errsts = resp_write(SCpnt, lba, num, devip, ei_lba);
 		if (inj_recovered && (0 == errsts)) {
 			mk_sense_buffer(devip, RECOVERED_ERROR,
 					THRESHOLD_EXCEEDED, 0);
@@ -3341,15 +3411,34 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 			break;
 		if (scsi_debug_fake_rw)
 			break;
-		get_data_transfer_info(cmd, &lba, &num);
-		errsts = resp_read(SCpnt, lba, num, devip);
+		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
+		errsts = resp_read(SCpnt, lba, num, devip, ei_lba);
 		if (errsts)
 			break;
-		errsts = resp_write(SCpnt, lba, num, devip);
+		errsts = resp_write(SCpnt, lba, num, devip, ei_lba);
 		if (errsts)
 			break;
 		errsts = resp_xdwriteread(SCpnt, lba, num, devip);
 		break;
+	case VARIABLE_LENGTH_CMD:
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION) {
+
+			if ((cmd[10] & 0xe0) == 0)
+				printk(KERN_ERR
+				       "Unprotected RD/WR to DIF device\n");
+
+			if (cmd[9] == READ_32)
+				goto read;
+
+			if (cmd[9] == WRITE_32)
+				goto write;
+		}
+
+		mk_sense_buffer(devip, ILLEGAL_REQUEST,
+				INVALID_FIELD_IN_CDB, 0);
+		errsts = check_condition_result;
+		break;
+
 	default:
 		if (SCSI_DEBUG_OPT_NOISE & scsi_debug_opts)
 			printk(KERN_INFO "scsi_debug: Opcode: 0x%x not "
-- 
1.6.0.6

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Boaz Harrosh]

On 08/27/2009 06:17 PM, Douglas Gilbert wrote:
> James Bottomley wrote:
>> On Thu, 2009-08-27 at 12:35 +0300, Boaz Harrosh wrote:
>>> On 08/27/2009 09:58 AM, Martin K. Petersen wrote:
>>>>>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
>>>>>> + *lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
>>>>>> + (u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
>>>>>> + (u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
>>>>>> + (u64)cmd[13] << 48 | (u64)cmd[12] << 56;
>>>> Boaz> get_unaligned_be64()
>>>>
>>>> As you noticed further down in that patch I do generally use the
>>>> get_unaligned_* macros in "my own" code.
>>>>
>>>> However, when I update somebody else's code I try to match the existing
>>>> style.  And in this case rest of get_data_transfer_info() is using
>>>> explicit shifts and to me it looks absolutely horrendous to mix the two.
>>>>
>>>> I generally avoid mixing cleanups and new functionality.  I don't have a
>>>> problem with switching over to the macros, but in that case I think the
>>>> whole function should be updated.  And that should be an orthogonal
>>>> patch.
>>>>
>>> I don't know. For me it is like checkpatch. I do not submit code over 80
>>> chars even if surrounding code does. "The new code rule".
>>
>> I'd take that as a guideline rather than a rule ... Especially when
>> lindent will generate 80 column long function templates over tens of
>> lines squashing about five letters per line ...
>>
>>> I generally agree with what you say but I think there is a balance.
>>> Personally, I think this is over the balance point, but it's your call.
>>
>> The general rule is not to confuse coding styles, so the correct way to
>> add stuff is to use the existing conventions in the file.  You can
>> optionally convert the entire style if necessary.  However, for these
>> get_cpu_be macros, there's no real benefit other than saving typing, so
>> a global conversion effort simply isn't worth it.
> 
> There is another aspect that I look at: portability to
> the user space of Linux and other operating systems.
> 
> Are there C99 or POSIX equivalents of get_unaligned_be64()?
> 
> Recently sg_read_block_limits was contributed to sg3_utils.
> Its author found a novel solution to this "problem" with
> network stack calls: ntohl() and friends. So I ran with it
> until I found it broke my builds in the MinGW environment
> for Windows. Rather than put in conditional compiles and
> wonder if my MinGW executables would get bloated, I opted
> to go back to what has worked for the last 25 years.
> 
> Doug Gilbert

Hi

As I said. I don't have a strong heart the way you guys have ;-)
even if I had to write it from scratch for every body of code I do
I would.

That said, the aligned accessors are exported from
linux to user space, the none-aligned I just kindly borrow into my project
as a couple of GPLed headers and that is that. Open source for you.

Boaz

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Douglas Gilbert]

James Bottomley wrote:
> On Thu, 2009-08-27 at 12:35 +0300, Boaz Harrosh wrote:
>> On 08/27/2009 09:58 AM, Martin K. Petersen wrote:
>>>>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
>>>>> + *lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
>>>>> + (u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
>>>>> + (u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
>>>>> + (u64)cmd[13] << 48 | (u64)cmd[12] << 56;
>>> Boaz> get_unaligned_be64()
>>>
>>> As you noticed further down in that patch I do generally use the
>>> get_unaligned_* macros in "my own" code.
>>>
>>> However, when I update somebody else's code I try to match the existing
>>> style.  And in this case rest of get_data_transfer_info() is using
>>> explicit shifts and to me it looks absolutely horrendous to mix the two.
>>>
>>> I generally avoid mixing cleanups and new functionality.  I don't have a
>>> problem with switching over to the macros, but in that case I think the
>>> whole function should be updated.  And that should be an orthogonal
>>> patch.
>>>
>> I don't know. For me it is like checkpatch. I do not submit code over 80
>> chars even if surrounding code does. "The new code rule".
> 
> I'd take that as a guideline rather than a rule ... Especially when
> lindent will generate 80 column long function templates over tens of
> lines squashing about five letters per line ...
> 
>> I generally agree with what you say but I think there is a balance.
>> Personally, I think this is over the balance point, but it's your call.
> 
> The general rule is not to confuse coding styles, so the correct way to
> add stuff is to use the existing conventions in the file.  You can
> optionally convert the entire style if necessary.  However, for these
> get_cpu_be macros, there's no real benefit other than saving typing, so
> a global conversion effort simply isn't worth it.

There is another aspect that I look at: portability to
the user space of Linux and other operating systems.

Are there C99 or POSIX equivalents of get_unaligned_be64()?

Recently sg_read_block_limits was contributed to sg3_utils.
Its author found a novel solution to this "problem" with
network stack calls: ntohl() and friends. So I ran with it
until I found it broke my builds in the MinGW environment
for Windows. Rather than put in conditional compiles and
wonder if my MinGW executables would get bloated, I opted
to go back to what has worked for the last 25 years.

Doug Gilbert

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[James Bottomley]

On Thu, 2009-08-27 at 17:47 +0300, Boaz Harrosh wrote:
> On 08/27/2009 05:30 PM, James Bottomley wrote:
> > On Thu, 2009-08-27 at 17:20 +0300, Boaz Harrosh wrote:
> >> On 08/27/2009 04:41 PM, James Bottomley wrote:
> >>>
> >>> The general rule is not to confuse coding styles, so the correct way to
> >>> add stuff is to use the existing conventions in the file.  You can
> >>> optionally convert the entire style if necessary.  However, for these
> >>> get_cpu_be macros, there's no real benefit other than saving typing, so
> >>> a global conversion effort simply isn't worth it.
> >>>
> >>
> >> This is not right. The get_cpu_be macros are ten fold faster and smaller
> >> on all the platforms we ever use. I'm talking about 16-96 to 1 for a 64 bit
> >> operation.
> > 
> > Assembly comparisons didn't bear this out the last time I looked; what
> > changed?
> > 
> 
> I'm not sure what test you made. But for instance on x86_64 which has unaligned
> cpu support, the get_unaligned_be64 is a simple SWAB instructions as opposed to
> 8 "or" + 8 "shift". Not to mention BE systems which do nothing (memcpy)

Well, what I saw on x86 is that gcc optimises the shifts to exactly the
same code as get_unaligned_be.  What does the assembly comparison look
like on x86-64?  However, regardless, this is hardly critical path
affecting stuff.

James

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Boaz Harrosh]

On 08/27/2009 05:30 PM, James Bottomley wrote:
> On Thu, 2009-08-27 at 17:20 +0300, Boaz Harrosh wrote:
>> On 08/27/2009 04:41 PM, James Bottomley wrote:
>>>
>>> The general rule is not to confuse coding styles, so the correct way to
>>> add stuff is to use the existing conventions in the file.  You can
>>> optionally convert the entire style if necessary.  However, for these
>>> get_cpu_be macros, there's no real benefit other than saving typing, so
>>> a global conversion effort simply isn't worth it.
>>>
>>
>> This is not right. The get_cpu_be macros are ten fold faster and smaller
>> on all the platforms we ever use. I'm talking about 16-96 to 1 for a 64 bit
>> operation.
> 
> Assembly comparisons didn't bear this out the last time I looked; what
> changed?
> 

I'm not sure what test you made. But for instance on x86_64 which has unaligned
cpu support, the get_unaligned_be64 is a simple SWAB instructions as opposed to
8 "or" + 8 "shift". Not to mention BE systems which do nothing (memcpy)

>> Not to mention the heart attack it gives me every time. Is that index go down
>> or up? the shifts go bigger or smaller? even just for that I would wrap them,
>> triple check, and never use anything else. But the stronger fact of the matter
>> is that I don't think there is a single used ARCH that does shifts anymore.
> 
> OK, but for those of us who read the standards, they explicitly specify
> byte offset fields for everything, so cmnd[n] does map exactly to that,
> so I find the fully folded out form easier to read and compare with the
> relevant standard text.
> 
> That's why I'm not mandating anything other than keep the styles
> consistent per file.  You're free to use whatever you like in your
> files.
> 
> James
> 
> 

Boaz

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[James Bottomley]

On Thu, 2009-08-27 at 17:20 +0300, Boaz Harrosh wrote:
> On 08/27/2009 04:41 PM, James Bottomley wrote:
> > 
> > The general rule is not to confuse coding styles, so the correct way to
> > add stuff is to use the existing conventions in the file.  You can
> > optionally convert the entire style if necessary.  However, for these
> > get_cpu_be macros, there's no real benefit other than saving typing, so
> > a global conversion effort simply isn't worth it.
> > 
> 
> This is not right. The get_cpu_be macros are ten fold faster and smaller
> on all the platforms we ever use. I'm talking about 16-96 to 1 for a 64 bit
> operation.

Assembly comparisons didn't bear this out the last time I looked; what
changed?

> Not to mention the heart attack it gives me every time. Is that index go down
> or up? the shifts go bigger or smaller? even just for that I would wrap them,
> triple check, and never use anything else. But the stronger fact of the matter
> is that I don't think there is a single used ARCH that does shifts anymore.

OK, but for those of us who read the standards, they explicitly specify
byte offset fields for everything, so cmnd[n] does map exactly to that,
so I find the fully folded out form easier to read and compare with the
relevant standard text.

That's why I'm not mandating anything other than keep the styles
consistent per file.  You're free to use whatever you like in your
files.

James

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Boaz Harrosh]

On 08/27/2009 04:41 PM, James Bottomley wrote:
> 
> The general rule is not to confuse coding styles, so the correct way to
> add stuff is to use the existing conventions in the file.  You can
> optionally convert the entire style if necessary.  However, for these
> get_cpu_be macros, there's no real benefit other than saving typing, so
> a global conversion effort simply isn't worth it.
> 

This is not right. The get_cpu_be macros are ten fold faster and smaller
on all the platforms we ever use. I'm talking about 16-96 to 1 for a 64 bit
operation.

Not to mention the heart attack it gives me every time. Is that index go down
or up? the shifts go bigger or smaller? even just for that I would wrap them,
triple check, and never use anything else. But the stronger fact of the matter
is that I don't think there is a single used ARCH that does shifts anymore.

> James
> 
> 

Boaz

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[James Bottomley]

On Thu, 2009-08-27 at 12:35 +0300, Boaz Harrosh wrote:
> On 08/27/2009 09:58 AM, Martin K. Petersen wrote:
> >>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
> > 
> >>> + *lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
> >>> + (u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
> >>> + (u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
> >>> + (u64)cmd[13] << 48 | (u64)cmd[12] << 56;
> > 
> > Boaz> get_unaligned_be64()
> > 
> > As you noticed further down in that patch I do generally use the
> > get_unaligned_* macros in "my own" code.
> > 
> > However, when I update somebody else's code I try to match the existing
> > style.  And in this case rest of get_data_transfer_info() is using
> > explicit shifts and to me it looks absolutely horrendous to mix the two.
> > 
> > I generally avoid mixing cleanups and new functionality.  I don't have a
> > problem with switching over to the macros, but in that case I think the
> > whole function should be updated.  And that should be an orthogonal
> > patch.
> > 
> 
> I don't know. For me it is like checkpatch. I do not submit code over 80
> chars even if surrounding code does. "The new code rule".

I'd take that as a guideline rather than a rule ... Especially when
lindent will generate 80 column long function templates over tens of
lines squashing about five letters per line ...

> I generally agree with what you say but I think there is a balance.
> Personally, I think this is over the balance point, but it's your call.

The general rule is not to confuse coding styles, so the correct way to
add stuff is to use the existing conventions in the file.  You can
optionally convert the entire style if necessary.  However, for these
get_cpu_be macros, there's no real benefit other than saving typing, so
a global conversion effort simply isn't worth it.

James

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Boaz Harrosh]

On 08/27/2009 09:58 AM, Martin K. Petersen wrote:
>>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
> 
>>> + *lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
>>> + (u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
>>> + (u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
>>> + (u64)cmd[13] << 48 | (u64)cmd[12] << 56;
> 
> Boaz> get_unaligned_be64()
> 
> As you noticed further down in that patch I do generally use the
> get_unaligned_* macros in "my own" code.
> 
> However, when I update somebody else's code I try to match the existing
> style.  And in this case rest of get_data_transfer_info() is using
> explicit shifts and to me it looks absolutely horrendous to mix the two.
> 
> I generally avoid mixing cleanups and new functionality.  I don't have a
> problem with switching over to the macros, but in that case I think the
> whole function should be updated.  And that should be an orthogonal
> patch.
> 

I don't know. For me it is like checkpatch. I do not submit code over 80
chars even if surrounding code does. "The new code rule".

I generally agree with what you say but I think there is a balance.
Personally, I think this is over the balance point, but it's your call.

Thanks
Boaz

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Martin K. Petersen]

>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:

>> + *lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
>> + (u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
>> + (u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
>> + (u64)cmd[13] << 48 | (u64)cmd[12] << 56;

Boaz> get_unaligned_be64()

As you noticed further down in that patch I do generally use the
get_unaligned_* macros in "my own" code.

However, when I update somebody else's code I try to match the existing
style.  And in this case rest of get_data_transfer_info() is using
explicit shifts and to me it looks absolutely horrendous to mix the two.

I generally avoid mixing cleanups and new functionality.  I don't have a
problem with switching over to the macros, but in that case I think the
whole function should be updated.  And that should be an orthogonal
patch.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Boaz Harrosh]

On 08/26/2009 09:18 AM, Martin K. Petersen wrote:
> Add support for 32-byte READ/WRITE as well as DIF Type 2 protection.
> 
> Reject protected 10/12/16 byte READ/WRITE commands when Type 2 is
> enabled.
> 
> Verify Type 2 reference tag according to Expected Initial LBA in 32-byte
> CDB.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/scsi_debug.c |  135 +++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 112 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index fb9af20..f551ec3 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -50,6 +50,7 @@
>  #include <scsi/scsi_host.h>
>  #include <scsi/scsicam.h>
>  #include <scsi/scsi_eh.h>
> +#include <scsi/scsi_dbg.h>
>  
>  #include "sd.h"
>  #include "scsi_logging.h"
> @@ -64,6 +65,7 @@ static const char * scsi_debug_version_date = "20070104";
>  #define PARAMETER_LIST_LENGTH_ERR 0x1a
>  #define INVALID_OPCODE 0x20
>  #define ADDR_OUT_OF_RANGE 0x21
> +#define INVALID_COMMAND_OPCODE 0x20
>  #define INVALID_FIELD_IN_CDB 0x24
>  #define INVALID_FIELD_IN_PARAM_LIST 0x26
>  #define POWERON_RESET 0x29
> @@ -180,7 +182,7 @@ static int sdebug_sectors_per;		/* sectors per cylinder */
>  #define SDEBUG_SENSE_LEN 32
>  
>  #define SCSI_DEBUG_CANQUEUE  255
> -#define SCSI_DEBUG_MAX_CMD_LEN 16
> +#define SCSI_DEBUG_MAX_CMD_LEN 32
>  
>  struct sdebug_dev_info {
>  	struct list_head dev_list;
> @@ -296,9 +298,25 @@ static void mk_sense_buffer(struct sdebug_dev_info *devip, int key,
>  }
>  
>  static void get_data_transfer_info(unsigned char *cmd,
> -				   unsigned long long *lba, unsigned int *num)
> +				   unsigned long long *lba, unsigned int *num,
> +				   u32 *ei_lba)
>  {
> +	*ei_lba = 0;
> +
>  	switch (*cmd) {
> +	case VARIABLE_LENGTH_CMD:
> +		*lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
> +			(u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
> +			(u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
> +			(u64)cmd[13] << 48 | (u64)cmd[12] << 56;
> +

get_unaligned_be64()

> +		*ei_lba = (u32)cmd[23] | (u32)cmd[22] << 8 |
> +			(u32)cmd[21] << 16 | (u32)cmd[20] << 24;
> +

be32_to_cpup()

> +		*num = (u32)cmd[31] | (u32)cmd[30] << 8 | (u32)cmd[29] << 16 |
> +			(u32)cmd[28] << 24;

be32_to_cpup()

On some arches, above code will produce 96 assembly instructions as opossed to
a single MOV. I understand that the old code is full of this crap, but "new code"?

I know I could never blasphem like that. As if you are not proud of being a programmer

> +		break;
> +
>  	case WRITE_16:
>  	case READ_16:
>  		*lba = (u64)cmd[9] | (u64)cmd[8] << 8 |
> @@ -1589,7 +1607,7 @@ static int do_device_access(struct scsi_cmnd *scmd,
>  }
>  
>  static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
> -			    unsigned int sectors)
> +			    unsigned int sectors, u32 ei_lba)
>  {
>  	unsigned int i, resid;
>  	struct scatterlist *psgl;
> @@ -1636,13 +1654,23 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
>  			return 0x01;
>  		}
>  
> -		if (scsi_debug_dif != SD_DIF_TYPE3_PROTECTION &&
> +		if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
>  		    be32_to_cpu(sdt[i].ref_tag) != (sector & 0xffffffff)) {
>  			printk(KERN_ERR "%s: REF check failed on sector %lu\n",
>  			       __func__, (unsigned long)sector);
>  			dif_errors++;
>  			return 0x03;
>  		}
> +
> +		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
> +		    be32_to_cpu(sdt[i].ref_tag) != ei_lba) {

Here you do it right

> +			printk(KERN_ERR "%s: REF check failed on sector %lu\n",
> +			       __func__, (unsigned long)sector);
> +			dif_errors++;
> +			return 0x03;
> +		}
> +
> +		ei_lba++;
>  	}
>  
>  	resid = sectors * 8; /* Bytes of protection data to copy into sgl */
> @@ -1670,7 +1698,8 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
>  }
>  
>  static int resp_read(struct scsi_cmnd *SCpnt, unsigned long long lba,
> -		     unsigned int num, struct sdebug_dev_info *devip)
> +		     unsigned int num, struct sdebug_dev_info *devip,
> +		     u32 ei_lba)
>  {
>  	unsigned long iflags;
>  	int ret;
> @@ -1699,7 +1728,7 @@ static int resp_read(struct scsi_cmnd *SCpnt, unsigned long long lba,
>  
>  	/* DIX + T10 DIF */
>  	if (scsi_debug_dix && scsi_prot_sg_count(SCpnt)) {
> -		int prot_ret = prot_verify_read(SCpnt, lba, num);
> +		int prot_ret = prot_verify_read(SCpnt, lba, num, ei_lba);
>  
>  		if (prot_ret) {
>  			mk_sense_buffer(devip, ABORTED_COMMAND, 0x10, prot_ret);
> @@ -1735,7 +1764,7 @@ void dump_sector(unsigned char *buf, int len)
>  }
>  
>  static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
> -			     unsigned int sectors)
> +			     unsigned int sectors, u32 ei_lba)
>  {
>  	int i, j, ret;
>  	struct sd_dif_tuple *sdt;
> @@ -1749,11 +1778,6 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
>  
>  	sector = do_div(tmp_sec, sdebug_store_sectors);
>  
> -	if (((SCpnt->cmnd[1] >> 5) & 7) != 1) {
> -		printk(KERN_WARNING "scsi_debug: WRPROTECT != 1\n");
> -		return 0;
> -	}
> -
>  	BUG_ON(scsi_sg_count(SCpnt) == 0);
>  	BUG_ON(scsi_prot_sg_count(SCpnt) == 0);
>  
> @@ -1808,7 +1832,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
>  				goto out;
>  			}
>  
> -			if (scsi_debug_dif != SD_DIF_TYPE3_PROTECTION &&
> +			if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
>  			    be32_to_cpu(sdt->ref_tag)
>  			    != (start_sec & 0xffffffff)) {
>  				printk(KERN_ERR
> @@ -1819,6 +1843,16 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
>  				goto out;
>  			}
>  
> +			if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
> +			    be32_to_cpu(sdt->ref_tag) != ei_lba) {
> +				printk(KERN_ERR
> +				       "%s: REF check failed on sector %lu\n",
> +				       __func__, (unsigned long)sector);
> +				ret = 0x03;
> +				dump_sector(daddr, scsi_debug_sector_size);
> +				goto out;
> +			}
> +
>  			/* Would be great to copy this in bigger
>  			 * chunks.  However, for the sake of
>  			 * correctness we need to verify each sector
> @@ -1832,6 +1866,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
>  				sector = 0;	/* Force wrap */
>  
>  			start_sec++;
> +			ei_lba++;
>  			daddr += scsi_debug_sector_size;
>  			ppage_offset += sizeof(struct sd_dif_tuple);
>  		}
> @@ -1853,7 +1888,8 @@ out:
>  }
>  
>  static int resp_write(struct scsi_cmnd *SCpnt, unsigned long long lba,
> -		      unsigned int num, struct sdebug_dev_info *devip)
> +		      unsigned int num, struct sdebug_dev_info *devip,
> +		      u32 ei_lba)
>  {
>  	unsigned long iflags;
>  	int ret;
> @@ -1864,7 +1900,7 @@ static int resp_write(struct scsi_cmnd *SCpnt, unsigned long long lba,
>  
>  	/* DIX + T10 DIF */
>  	if (scsi_debug_dix && scsi_prot_sg_count(SCpnt)) {
> -		int prot_ret = prot_verify_write(SCpnt, lba, num);
> +		int prot_ret = prot_verify_write(SCpnt, lba, num, ei_lba);
>  
>  		if (prot_ret) {
>  			mk_sense_buffer(devip, ILLEGAL_REQUEST, 0x10, prot_ret);
> @@ -2872,11 +2908,12 @@ static int __init scsi_debug_init(void)
>  
>  	case SD_DIF_TYPE0_PROTECTION:
>  	case SD_DIF_TYPE1_PROTECTION:
> +	case SD_DIF_TYPE2_PROTECTION:
>  	case SD_DIF_TYPE3_PROTECTION:
>  		break;
>  
>  	default:
> -		printk(KERN_ERR "scsi_debug_init: dif must be 0, 1 or 3\n");
> +		printk(KERN_ERR "scsi_debug_init: dif must be 0, 1, 2 or 3\n");
>  		return -EINVAL;
>  	}
>  
> @@ -3121,6 +3158,7 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
>  	int len, k;
>  	unsigned int num;
>  	unsigned long long lba;
> +	u32 ei_lba;
>  	int errsts = 0;
>  	int target = SCpnt->device->id;
>  	struct sdebug_dev_info *devip = NULL;
> @@ -3254,14 +3292,30 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
>  	case READ_16:
>  	case READ_12:
>  	case READ_10:
> +		/* READ{10,12,16} and DIF Type 2 are natural enemies */
> +		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
> +		    cmd[1] & 0xe0) {
> +			mk_sense_buffer(devip, ILLEGAL_REQUEST,
> +					INVALID_COMMAND_OPCODE, 0);
> +			errsts = check_condition_result;
> +			break;
> +		}
> +
> +		if ((scsi_debug_dif == SD_DIF_TYPE1_PROTECTION ||
> +		     scsi_debug_dif == SD_DIF_TYPE3_PROTECTION) &&
> +		    (cmd[1] & 0xe0) == 0)
> +			printk(KERN_ERR "Unprotected RD/WR to DIF device\n");
> +
> +		/* fall through */
>  	case READ_6:
> +read:
>  		errsts = check_readiness(SCpnt, 0, devip);
>  		if (errsts)
>  			break;
>  		if (scsi_debug_fake_rw)
>  			break;
> -		get_data_transfer_info(cmd, &lba, &num);
> -		errsts = resp_read(SCpnt, lba, num, devip);
> +		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
> +		errsts = resp_read(SCpnt, lba, num, devip, ei_lba);
>  		if (inj_recovered && (0 == errsts)) {
>  			mk_sense_buffer(devip, RECOVERED_ERROR,
>  					THRESHOLD_EXCEEDED, 0);
> @@ -3288,14 +3342,30 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
>  	case WRITE_16:
>  	case WRITE_12:
>  	case WRITE_10:
> +		/* WRITE{10,12,16} and DIF Type 2 are natural enemies */
> +		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
> +		    cmd[1] & 0xe0) {
> +			mk_sense_buffer(devip, ILLEGAL_REQUEST,
> +					INVALID_COMMAND_OPCODE, 0);
> +			errsts = check_condition_result;
> +			break;
> +		}
> +
> +		if ((scsi_debug_dif == SD_DIF_TYPE1_PROTECTION ||
> +		     scsi_debug_dif == SD_DIF_TYPE3_PROTECTION) &&
> +		    (cmd[1] & 0xe0) == 0)
> +			printk(KERN_ERR "Unprotected RD/WR to DIF device\n");
> +
> +		/* fall through */
>  	case WRITE_6:
> +write:
>  		errsts = check_readiness(SCpnt, 0, devip);
>  		if (errsts)
>  			break;
>  		if (scsi_debug_fake_rw)
>  			break;
> -		get_data_transfer_info(cmd, &lba, &num);
> -		errsts = resp_write(SCpnt, lba, num, devip);
> +		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
> +		errsts = resp_write(SCpnt, lba, num, devip, ei_lba);
>  		if (inj_recovered && (0 == errsts)) {
>  			mk_sense_buffer(devip, RECOVERED_ERROR,
>  					THRESHOLD_EXCEEDED, 0);
> @@ -3341,15 +3411,34 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
>  			break;
>  		if (scsi_debug_fake_rw)
>  			break;
> -		get_data_transfer_info(cmd, &lba, &num);
> -		errsts = resp_read(SCpnt, lba, num, devip);
> +		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
> +		errsts = resp_read(SCpnt, lba, num, devip, ei_lba);
>  		if (errsts)
>  			break;
> -		errsts = resp_write(SCpnt, lba, num, devip);
> +		errsts = resp_write(SCpnt, lba, num, devip, ei_lba);
>  		if (errsts)
>  			break;
>  		errsts = resp_xdwriteread(SCpnt, lba, num, devip);
>  		break;
> +	case VARIABLE_LENGTH_CMD:
> +		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION) {
> +
> +			if ((cmd[10] & 0xe0) == 0)
> +				printk(KERN_ERR
> +				       "Unprotected RD/WR to DIF device\n");
> +
> +			if (cmd[9] == READ_32)
> +				goto read;
> +
> +			if (cmd[9] == WRITE_32)
> +				goto write;
> +		}
> +
> +		mk_sense_buffer(devip, ILLEGAL_REQUEST,
> +				INVALID_FIELD_IN_CDB, 0);
> +		errsts = check_condition_result;
> +		break;
> +
>  	default:
>  		if (SCSI_DEBUG_OPT_NOISE & scsi_debug_opts)
>  			printk(KERN_INFO "scsi_debug: Opcode: 0x%x not "

Boaz

[PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Martin K. Petersen]

Add support for 32-byte READ/WRITE as well as DIF Type 2 protection.

Reject protected 10/12/16 byte READ/WRITE commands when Type 2 is
enabled.

Verify Type 2 reference tag according to Expected Initial LBA in 32-byte
CDB.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/scsi_debug.c |  135 +++++++++++++++++++++++++++++++++++++--------
 1 files changed, 112 insertions(+), 23 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index fb9af20..f551ec3 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -50,6 +50,7 @@
 #include <scsi/scsi_host.h>
 #include <scsi/scsicam.h>
 #include <scsi/scsi_eh.h>
+#include <scsi/scsi_dbg.h>
 
 #include "sd.h"
 #include "scsi_logging.h"
@@ -64,6 +65,7 @@ static const char * scsi_debug_version_date = "20070104";
 #define PARAMETER_LIST_LENGTH_ERR 0x1a
 #define INVALID_OPCODE 0x20
 #define ADDR_OUT_OF_RANGE 0x21
+#define INVALID_COMMAND_OPCODE 0x20
 #define INVALID_FIELD_IN_CDB 0x24
 #define INVALID_FIELD_IN_PARAM_LIST 0x26
 #define POWERON_RESET 0x29
@@ -180,7 +182,7 @@ static int sdebug_sectors_per;		/* sectors per cylinder */
 #define SDEBUG_SENSE_LEN 32
 
 #define SCSI_DEBUG_CANQUEUE  255
-#define SCSI_DEBUG_MAX_CMD_LEN 16
+#define SCSI_DEBUG_MAX_CMD_LEN 32
 
 struct sdebug_dev_info {
 	struct list_head dev_list;
@@ -296,9 +298,25 @@ static void mk_sense_buffer(struct sdebug_dev_info *devip, int key,
 }
 
 static void get_data_transfer_info(unsigned char *cmd,
-				   unsigned long long *lba, unsigned int *num)
+				   unsigned long long *lba, unsigned int *num,
+				   u32 *ei_lba)
 {
+	*ei_lba = 0;
+
 	switch (*cmd) {
+	case VARIABLE_LENGTH_CMD:
+		*lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
+			(u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
+			(u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
+			(u64)cmd[13] << 48 | (u64)cmd[12] << 56;
+
+		*ei_lba = (u32)cmd[23] | (u32)cmd[22] << 8 |
+			(u32)cmd[21] << 16 | (u32)cmd[20] << 24;
+
+		*num = (u32)cmd[31] | (u32)cmd[30] << 8 | (u32)cmd[29] << 16 |
+			(u32)cmd[28] << 24;
+		break;
+
 	case WRITE_16:
 	case READ_16:
 		*lba = (u64)cmd[9] | (u64)cmd[8] << 8 |
@@ -1589,7 +1607,7 @@ static int do_device_access(struct scsi_cmnd *scmd,
 }
 
 static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
-			    unsigned int sectors)
+			    unsigned int sectors, u32 ei_lba)
 {
 	unsigned int i, resid;
 	struct scatterlist *psgl;
@@ -1636,13 +1654,23 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 			return 0x01;
 		}
 
-		if (scsi_debug_dif != SD_DIF_TYPE3_PROTECTION &&
+		if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
 		    be32_to_cpu(sdt[i].ref_tag) != (sector & 0xffffffff)) {
 			printk(KERN_ERR "%s: REF check failed on sector %lu\n",
 			       __func__, (unsigned long)sector);
 			dif_errors++;
 			return 0x03;
 		}
+
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+		    be32_to_cpu(sdt[i].ref_tag) != ei_lba) {
+			printk(KERN_ERR "%s: REF check failed on sector %lu\n",
+			       __func__, (unsigned long)sector);
+			dif_errors++;
+			return 0x03;
+		}
+
+		ei_lba++;
 	}
 
 	resid = sectors * 8; /* Bytes of protection data to copy into sgl */
@@ -1670,7 +1698,8 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 }
 
 static int resp_read(struct scsi_cmnd *SCpnt, unsigned long long lba,
-		     unsigned int num, struct sdebug_dev_info *devip)
+		     unsigned int num, struct sdebug_dev_info *devip,
+		     u32 ei_lba)
 {
 	unsigned long iflags;
 	int ret;
@@ -1699,7 +1728,7 @@ static int resp_read(struct scsi_cmnd *SCpnt, unsigned long long lba,
 
 	/* DIX + T10 DIF */
 	if (scsi_debug_dix && scsi_prot_sg_count(SCpnt)) {
-		int prot_ret = prot_verify_read(SCpnt, lba, num);
+		int prot_ret = prot_verify_read(SCpnt, lba, num, ei_lba);
 
 		if (prot_ret) {
 			mk_sense_buffer(devip, ABORTED_COMMAND, 0x10, prot_ret);
@@ -1735,7 +1764,7 @@ void dump_sector(unsigned char *buf, int len)
 }
 
 static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
-			     unsigned int sectors)
+			     unsigned int sectors, u32 ei_lba)
 {
 	int i, j, ret;
 	struct sd_dif_tuple *sdt;
@@ -1749,11 +1778,6 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 
 	sector = do_div(tmp_sec, sdebug_store_sectors);
 
-	if (((SCpnt->cmnd[1] >> 5) & 7) != 1) {
-		printk(KERN_WARNING "scsi_debug: WRPROTECT != 1\n");
-		return 0;
-	}
-
 	BUG_ON(scsi_sg_count(SCpnt) == 0);
 	BUG_ON(scsi_prot_sg_count(SCpnt) == 0);
 
@@ -1808,7 +1832,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				goto out;
 			}
 
-			if (scsi_debug_dif != SD_DIF_TYPE3_PROTECTION &&
+			if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
 			    be32_to_cpu(sdt->ref_tag)
 			    != (start_sec & 0xffffffff)) {
 				printk(KERN_ERR
@@ -1819,6 +1843,16 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				goto out;
 			}
 
+			if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+			    be32_to_cpu(sdt->ref_tag) != ei_lba) {
+				printk(KERN_ERR
+				       "%s: REF check failed on sector %lu\n",
+				       __func__, (unsigned long)sector);
+				ret = 0x03;
+				dump_sector(daddr, scsi_debug_sector_size);
+				goto out;
+			}
+
 			/* Would be great to copy this in bigger
 			 * chunks.  However, for the sake of
 			 * correctness we need to verify each sector
@@ -1832,6 +1866,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				sector = 0;	/* Force wrap */
 
 			start_sec++;
+			ei_lba++;
 			daddr += scsi_debug_sector_size;
 			ppage_offset += sizeof(struct sd_dif_tuple);
 		}
@@ -1853,7 +1888,8 @@ out:
 }
 
 static int resp_write(struct scsi_cmnd *SCpnt, unsigned long long lba,
-		      unsigned int num, struct sdebug_dev_info *devip)
+		      unsigned int num, struct sdebug_dev_info *devip,
+		      u32 ei_lba)
 {
 	unsigned long iflags;
 	int ret;
@@ -1864,7 +1900,7 @@ static int resp_write(struct scsi_cmnd *SCpnt, unsigned long long lba,
 
 	/* DIX + T10 DIF */
 	if (scsi_debug_dix && scsi_prot_sg_count(SCpnt)) {
-		int prot_ret = prot_verify_write(SCpnt, lba, num);
+		int prot_ret = prot_verify_write(SCpnt, lba, num, ei_lba);
 
 		if (prot_ret) {
 			mk_sense_buffer(devip, ILLEGAL_REQUEST, 0x10, prot_ret);
@@ -2872,11 +2908,12 @@ static int __init scsi_debug_init(void)
 
 	case SD_DIF_TYPE0_PROTECTION:
 	case SD_DIF_TYPE1_PROTECTION:
+	case SD_DIF_TYPE2_PROTECTION:
 	case SD_DIF_TYPE3_PROTECTION:
 		break;
 
 	default:
-		printk(KERN_ERR "scsi_debug_init: dif must be 0, 1 or 3\n");
+		printk(KERN_ERR "scsi_debug_init: dif must be 0, 1, 2 or 3\n");
 		return -EINVAL;
 	}
 
@@ -3121,6 +3158,7 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 	int len, k;
 	unsigned int num;
 	unsigned long long lba;
+	u32 ei_lba;
 	int errsts = 0;
 	int target = SCpnt->device->id;
 	struct sdebug_dev_info *devip = NULL;
@@ -3254,14 +3292,30 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 	case READ_16:
 	case READ_12:
 	case READ_10:
+		/* READ{10,12,16} and DIF Type 2 are natural enemies */
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+		    cmd[1] & 0xe0) {
+			mk_sense_buffer(devip, ILLEGAL_REQUEST,
+					INVALID_COMMAND_OPCODE, 0);
+			errsts = check_condition_result;
+			break;
+		}
+
+		if ((scsi_debug_dif == SD_DIF_TYPE1_PROTECTION ||
+		     scsi_debug_dif == SD_DIF_TYPE3_PROTECTION) &&
+		    (cmd[1] & 0xe0) == 0)
+			printk(KERN_ERR "Unprotected RD/WR to DIF device\n");
+
+		/* fall through */
 	case READ_6:
+read:
 		errsts = check_readiness(SCpnt, 0, devip);
 		if (errsts)
 			break;
 		if (scsi_debug_fake_rw)
 			break;
-		get_data_transfer_info(cmd, &lba, &num);
-		errsts = resp_read(SCpnt, lba, num, devip);
+		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
+		errsts = resp_read(SCpnt, lba, num, devip, ei_lba);
 		if (inj_recovered && (0 == errsts)) {
 			mk_sense_buffer(devip, RECOVERED_ERROR,
 					THRESHOLD_EXCEEDED, 0);
@@ -3288,14 +3342,30 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 	case WRITE_16:
 	case WRITE_12:
 	case WRITE_10:
+		/* WRITE{10,12,16} and DIF Type 2 are natural enemies */
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+		    cmd[1] & 0xe0) {
+			mk_sense_buffer(devip, ILLEGAL_REQUEST,
+					INVALID_COMMAND_OPCODE, 0);
+			errsts = check_condition_result;
+			break;
+		}
+
+		if ((scsi_debug_dif == SD_DIF_TYPE1_PROTECTION ||
+		     scsi_debug_dif == SD_DIF_TYPE3_PROTECTION) &&
+		    (cmd[1] & 0xe0) == 0)
+			printk(KERN_ERR "Unprotected RD/WR to DIF device\n");
+
+		/* fall through */
 	case WRITE_6:
+write:
 		errsts = check_readiness(SCpnt, 0, devip);
 		if (errsts)
 			break;
 		if (scsi_debug_fake_rw)
 			break;
-		get_data_transfer_info(cmd, &lba, &num);
-		errsts = resp_write(SCpnt, lba, num, devip);
+		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
+		errsts = resp_write(SCpnt, lba, num, devip, ei_lba);
 		if (inj_recovered && (0 == errsts)) {
 			mk_sense_buffer(devip, RECOVERED_ERROR,
 					THRESHOLD_EXCEEDED, 0);
@@ -3341,15 +3411,34 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 			break;
 		if (scsi_debug_fake_rw)
 			break;
-		get_data_transfer_info(cmd, &lba, &num);
-		errsts = resp_read(SCpnt, lba, num, devip);
+		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
+		errsts = resp_read(SCpnt, lba, num, devip, ei_lba);
 		if (errsts)
 			break;
-		errsts = resp_write(SCpnt, lba, num, devip);
+		errsts = resp_write(SCpnt, lba, num, devip, ei_lba);
 		if (errsts)
 			break;
 		errsts = resp_xdwriteread(SCpnt, lba, num, devip);
 		break;
+	case VARIABLE_LENGTH_CMD:
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION) {
+
+			if ((cmd[10] & 0xe0) == 0)
+				printk(KERN_ERR
+				       "Unprotected RD/WR to DIF device\n");
+
+			if (cmd[9] == READ_32)
+				goto read;
+
+			if (cmd[9] == WRITE_32)
+				goto write;
+		}
+
+		mk_sense_buffer(devip, ILLEGAL_REQUEST,
+				INVALID_FIELD_IN_CDB, 0);
+		errsts = check_condition_result;
+		break;
+
 	default:
 		if (SCSI_DEBUG_OPT_NOISE & scsi_debug_opts)
 			printk(KERN_INFO "scsi_debug: Opcode: 0x%x not "
-- 
1.6.0.6