DIF/DIX updates for 2.6.32

DIF/DIX updates for 2.6.32

[Martin K. Petersen]

Updates to DIF/DIX for 2.6.32.  Only changes since last post are:

 - Add a flag to control deallocation of 32-byte CDBs

 - Removed stray BUG_ON() in sd.c

[PATCH 1/5] SCSI: Add support for 32-byte CDBs

[Martin K. Petersen]

DIF Type 2 drives require 32-byte commands.  Add a suitable flag to
struct scsi_device that will cause scsi_setup_fs_cmnd() to allocate a
bigger command buffer.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/scsi_lib.c    |   38 +++++++++++++++++++++++++++++++++++++-
 include/scsi/scsi.h        |    1 +
 include/scsi/scsi_cmnd.h   |    6 ++++++
 include/scsi/scsi_device.h |    1 +
 4 files changed, 45 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 662024d..643769c 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -67,6 +67,9 @@ static struct scsi_host_sg_pool scsi_sg_pools[] = {
 
 struct kmem_cache *scsi_sdb_cache;
 
+struct kmem_cache *cdb_cache;
+mempool_t *cdb_pool;
+
 static void scsi_run_queue(struct request_queue *q);
 
 /*
@@ -642,6 +645,11 @@ static void __scsi_release_buffers(struct scsi_cmnd *cmd, int do_bidi_check)
 
 	if (scsi_prot_sg_count(cmd))
 		scsi_free_sgtable(cmd->prot_sdb);
+
+	if (cmd->flags & SCSI_CMND_EXT_CDB) {
+		mempool_free(cmd->cmnd, cdb_pool);
+		cmd->flags &= ~SCSI_CMND_EXT_CDB;
+	}
 }
 
 /*
@@ -1109,7 +1117,19 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
 	if (unlikely(!cmd))
 		return BLKPREP_DEFER;
 
-	memset(cmd->cmnd, 0, BLK_MAX_CDB);
+	if (sdev->use_32_for_rw) {
+		req->cmd = mempool_alloc(cdb_pool, GFP_ATOMIC);
+
+		if (unlikely(!req->cmd))
+			return BLKPREP_DEFER;
+
+		cmd->cmnd = req->cmd;
+		cmd->cmd_len = req->cmd_len = SCSI_EXT_CDB_SIZE;
+		cmd->flags |= SCSI_CMND_EXT_CDB;
+		memset(cmd->cmnd, 0, cmd->cmd_len);
+	} else
+		memset(cmd->cmnd, 0, BLK_MAX_CDB);
+
 	return scsi_init_io(cmd, GFP_ATOMIC);
 }
 EXPORT_SYMBOL(scsi_setup_fs_cmnd);
@@ -1745,6 +1765,19 @@ int __init scsi_init_queue(void)
 		}
 	}
 
+	cdb_cache = kmem_cache_create("scsi_cdb_32", SCSI_EXT_CDB_SIZE,
+				      0, 0, NULL);
+	if (!cdb_cache) {
+		printk(KERN_ERR "SCSI: can't init scsi cdb cache\n");
+		goto cleanup_sdb;
+	}
+
+	cdb_pool = mempool_create_slab_pool(SG_MEMPOOL_SIZE, cdb_cache);
+	if (!cdb_pool) {
+		printk(KERN_ERR "SCSI: can't init scsi cdb pool\n");
+		goto cleanup_sdb;
+	}
+
 	return 0;
 
 cleanup_sdb:
@@ -1771,6 +1804,9 @@ void scsi_exit_queue(void)
 		mempool_destroy(sgp->pool);
 		kmem_cache_destroy(sgp->slab);
 	}
+
+	mempool_destroy(cdb_pool);
+	kmem_cache_destroy(cdb_cache);
 }
 
 /**
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 084478e..a7ae10a 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -138,6 +138,7 @@ struct scsi_cmnd;
  *	SCSI command lengths
  */
 
+#define SCSI_EXT_CDB_SIZE 32
 #define SCSI_MAX_VARLEN_CDB_SIZE 260
 
 /* defined in T10 SCSI Primary Commands-2 (SPC2) */
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 3878d1d..d4a6a80 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -50,6 +50,10 @@ struct scsi_pointer {
 	volatile int phase;
 };
 
+enum scsi_cmnd_flags {
+	SCSI_CMND_EXT_CDB = 1 << 0,
+};
+
 struct scsi_cmnd {
 	struct scsi_device *device;
 	struct list_head list;  /* scsi_cmnd participates in queue lists */
@@ -129,6 +133,8 @@ struct scsi_cmnd {
 	int result;		/* Status code from lower level driver */
 
 	unsigned char tag;	/* SCSI-II queued command tag */
+
+	enum scsi_cmnd_flags flags;
 };
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 3f566af..de2966b 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -129,6 +129,7 @@ struct scsi_device {
 				 * this device */
 	unsigned expecting_cc_ua:1; /* Expecting a CHECK_CONDITION/UNIT_ATTN
 				     * because we did a bus reset. */
+	unsigned use_32_for_rw:1; /* use 32-byte read / write (DIF Type 2) */
 	unsigned use_10_for_rw:1; /* first try 10-byte read / write */
 	unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
 	unsigned skip_ms_page_8:1;	/* do not use MODE SENSE page 0x08 */
-- 
1.6.0.6

[PATCH 2/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 d4a6a80..609875c 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -235,10 +235,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 3/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 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 support for
Type 2 drives in the protection type detection logic.

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

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 36e2333..bf7ead6 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -413,6 +413,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 +546,40 @@ 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;
-
-	if (block > 0xffffffff) {
+		protect = 0;
+
+	if (host_dif == SD_DIF_TYPE2_PROTECTION) {
+		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 +600,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;
@@ -1271,22 +1299,20 @@ 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;
 		return;
 	}
 
-	if (scsi_host_dif_capable(sdp->host, type))
+	if (scsi_host_dif_capable(sdp->host, type)) {
 		sd_printk(KERN_NOTICE, sdkp,
 			  "Enabling DIF Type %u protection\n", type);
-	else
+
+		if (type == SD_DIF_TYPE2_PROTECTION)
+			sdp->use_32_for_rw = 1;
+	} else
 		sd_printk(KERN_NOTICE, sdkp,
 			  "Disabling DIF Type %u protection\n", type);
 }
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index a7ae10a..847de86 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 1/5] SCSI: Add support for 32-byte CDBs

[Boaz Harrosh]

On 09/04/2009 11:36 AM, Martin K. Petersen wrote:
> DIF Type 2 drives require 32-byte commands.  Add a suitable flag to
> struct scsi_device that will cause scsi_setup_fs_cmnd() to allocate a
> bigger command buffer.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/scsi_lib.c    |   38 +++++++++++++++++++++++++++++++++++++-
>  include/scsi/scsi.h        |    1 +
>  include/scsi/scsi_cmnd.h   |    6 ++++++
>  include/scsi/scsi_device.h |    1 +
>  4 files changed, 45 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 662024d..643769c 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -67,6 +67,9 @@ static struct scsi_host_sg_pool scsi_sg_pools[] = {
>  
>  struct kmem_cache *scsi_sdb_cache;
>  
> +struct kmem_cache *cdb_cache;
> +mempool_t *cdb_pool;
> +
>  static void scsi_run_queue(struct request_queue *q);
>  
>  /*
> @@ -642,6 +645,11 @@ static void __scsi_release_buffers(struct scsi_cmnd *cmd, int do_bidi_check)
>  
>  	if (scsi_prot_sg_count(cmd))
>  		scsi_free_sgtable(cmd->prot_sdb);
> +
> +	if (cmd->flags & SCSI_CMND_EXT_CDB) {
> +		mempool_free(cmd->cmnd, cdb_pool);
> +		cmd->flags &= ~SCSI_CMND_EXT_CDB;
> +	}
>  }
>  
>  /*
> @@ -1109,7 +1117,19 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
>  	if (unlikely(!cmd))
>  		return BLKPREP_DEFER;
>  
> -	memset(cmd->cmnd, 0, BLK_MAX_CDB);
> +	if (sdev->use_32_for_rw) {
> +		req->cmd = mempool_alloc(cdb_pool, GFP_ATOMIC);
> +

Again, This might have a potential live-lock problem as we've seen
a few times in the passed. Two threads are fighting between the allocation
of a command+sense and here, each one is able to allocate one part but not the
second. This is why command+sense is needed to be allocated atomically, and fail
as a whole.

I think you need to move this check and allocation to scsi_host_alloc_command().

BTW: I think I found a bug in __scsi_get_command() in regard to:
	"scsi_host_get_prot(shost) >= SHOST_DIX_TYPE0_PROTECTION"

in the use of pre-allocated from "shost->free_list" case it does:

		if (cmd) {
			buf = cmd->sense_buffer;
			memset(cmd, 0, sizeof(*cmd));
			cmd->sense_buffer = buf;
		}

this here will reset the cmd->prot_sdb pointer, which will leak
eventually. And will be missing for this operation.

> +		if (unlikely(!req->cmd))
> +			return BLKPREP_DEFER;
> +
> +		cmd->cmnd = req->cmd;
> +		cmd->cmd_len = req->cmd_len = SCSI_EXT_CDB_SIZE;
> +		cmd->flags |= SCSI_CMND_EXT_CDB;
> +		memset(cmd->cmnd, 0, cmd->cmd_len);
> +	} else
> +		memset(cmd->cmnd, 0, BLK_MAX_CDB);
> +
>  	return scsi_init_io(cmd, GFP_ATOMIC);
>  }
>  EXPORT_SYMBOL(scsi_setup_fs_cmnd);
> @@ -1745,6 +1765,19 @@ int __init scsi_init_queue(void)
>  		}
>  	}
>  
> +	cdb_cache = kmem_cache_create("scsi_cdb_32", SCSI_EXT_CDB_SIZE,
> +				      0, 0, NULL);
> +	if (!cdb_cache) {
> +		printk(KERN_ERR "SCSI: can't init scsi cdb cache\n");
> +		goto cleanup_sdb;
> +	}
> +
> +	cdb_pool = mempool_create_slab_pool(SG_MEMPOOL_SIZE, cdb_cache);
> +	if (!cdb_pool) {
> +		printk(KERN_ERR "SCSI: can't init scsi cdb pool\n");
> +		goto cleanup_sdb;
> +	}
> +
>  	return 0;
>  
>  cleanup_sdb:
> @@ -1771,6 +1804,9 @@ void scsi_exit_queue(void)
>  		mempool_destroy(sgp->pool);
>  		kmem_cache_destroy(sgp->slab);
>  	}
> +
> +	mempool_destroy(cdb_pool);
> +	kmem_cache_destroy(cdb_cache);
>  }
>  
>  /**
> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
> index 084478e..a7ae10a 100644
> --- a/include/scsi/scsi.h
> +++ b/include/scsi/scsi.h
> @@ -138,6 +138,7 @@ struct scsi_cmnd;
>   *	SCSI command lengths
>   */
>  
> +#define SCSI_EXT_CDB_SIZE 32
>  #define SCSI_MAX_VARLEN_CDB_SIZE 260
>  
>  /* defined in T10 SCSI Primary Commands-2 (SPC2) */
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 3878d1d..d4a6a80 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -50,6 +50,10 @@ struct scsi_pointer {
>  	volatile int phase;
>  };
>  
> +enum scsi_cmnd_flags {
> +	SCSI_CMND_EXT_CDB = 1 << 0,
> +};
> +
>  struct scsi_cmnd {
>  	struct scsi_device *device;
>  	struct list_head list;  /* scsi_cmnd participates in queue lists */
> @@ -129,6 +133,8 @@ struct scsi_cmnd {
>  	int result;		/* Status code from lower level driver */
>  
>  	unsigned char tag;	/* SCSI-II queued command tag */
> +
> +	enum scsi_cmnd_flags flags;
>  };
>  
>  extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 3f566af..de2966b 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -129,6 +129,7 @@ struct scsi_device {
>  				 * this device */
>  	unsigned expecting_cc_ua:1; /* Expecting a CHECK_CONDITION/UNIT_ATTN
>  				     * because we did a bus reset. */
> +	unsigned use_32_for_rw:1; /* use 32-byte read / write (DIF Type 2) */
>  	unsigned use_10_for_rw:1; /* first try 10-byte read / write */
>  	unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
>  	unsigned skip_ms_page_8:1;	/* do not use MODE SENSE page 0x08 */

Boaz

Re: [PATCH 1/5] SCSI: Add support for 32-byte CDBs

[Martin K. Petersen]

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

Boaz> Again, This might have a potential live-lock problem as we've seen
Boaz> a few times in the passed. Two threads are fighting between the
Boaz> allocation of a command+sense and here, each one is able to
Boaz> allocate one part but not the second. This is why command+sense is
Boaz> needed to be allocated atomically, and fail as a whole.

Boaz> I think you need to move this check and allocation to
Boaz> scsi_host_alloc_command().

It's not that simple.

Originally I wanted to do the allocation at the block layer to avoid
wasting the existing 16 byte command array.  I did this by moving
__cmd[] to the end of struct request.  I had a queue flag (set by sd.c)
that would toggle between blk_request_16 and blk_request_32 pools.

However, there are several places where we allocate a struct request
statically or on the stack, and those assume that it has a fixed size.
The patch that attempted to clean up this stuff grew bigger and bigger.
Next hurdle was finding corner cases that zeroed out struct request,
zapping the 16/32 flag in the process and subsequently leading to
de-allocation failures.  Eventually Jens recommended that I abandon that
idea and just do it in SCSI for now.  It seemed that the block approach
was more effort than it was worth.

Doing the allocation in SCSI opens up another set of problems, however.
Namely the host free_list.  Things are going to blow up if we mix 16 and
32-byte CDBs.  So that means switching all DIF Type 2-capable scsi_hosts
to 32-byte CDBs wholesale.  Or we can postpone such a switch until a
Type 2 target is discovered and then empty free_list and make all
subsequent commands bigger.  Or we could have a separate list +
scsi_host_cmd_pool that we allocate from if it's a Type 2 dev.  That
requires passing the scsi_device info through to the allocation
routines.  Right now they are scsi_host centric.

I tried several approaches, none of which ended up being very small, nor
pretty.

There's less than a handful of Type 2 devices out there.  No array
vendor I know of plan to support Type 2 on the host side, only on the
back end.  DIF disk drives will ship formatted with Type 1.  The main
intent with my patch was to ensure that a Type 2 drive would work if you
pulled it from an array and hooked it up directly.

Short of bumping BLK_MAX_CDB to 32 the patch I posted a few days was by
far the least intrusive solution.  It only causes an allocation for FS
requests, and only for Type 2 drives, and only if the HBA supports DIF
Type 2.

I've had a several requests for Type 2 support in Linux (mainly for
target device testing) so I wanted to get some code out there.  But I'm
happy to spend more time on the various approaches if people feel that's
justified.  I'd just like to know which one before I burn more cycles.
So far it's been a huge time sink.

James: What's your take on this?


Boaz> BTW: I think I found a bug in __scsi_get_command() in regard to:
Boaz> 	"scsi_host_get_prot(shost) >= SHOST_DIX_TYPE0_PROTECTION"

Boaz> this here will reset the cmd->prot_sdb pointer, which will leak
Boaz> eventually. And will be missing for this operation.

Good spotting!  Will fix.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/5] SCSI: Add support for 32-byte CDBs

[Boaz Harrosh]

On 09/09/2009 07:31 AM, Martin K. Petersen wrote:
>>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
> 
> Boaz> Again, This might have a potential live-lock problem as we've seen
> Boaz> a few times in the passed. Two threads are fighting between the
> Boaz> allocation of a command+sense and here, each one is able to
> Boaz> allocate one part but not the second. This is why command+sense is
> Boaz> needed to be allocated atomically, and fail as a whole.
> 
> Boaz> I think you need to move this check and allocation to
> Boaz> scsi_host_alloc_command().
> 
> It's not that simple.
> 
> Originally I wanted to do the allocation at the block layer to avoid
> wasting the existing 16 byte command array.  I did this by moving
> __cmd[] to the end of struct request.  I had a queue flag (set by sd.c)
> that would toggle between blk_request_16 and blk_request_32 pools.
> 
> However, there are several places where we allocate a struct request
> statically or on the stack, and those assume that it has a fixed size.
> The patch that attempted to clean up this stuff grew bigger and bigger.
> Next hurdle was finding corner cases that zeroed out struct request,
> zapping the 16/32 flag in the process and subsequently leading to
> de-allocation failures.  Eventually Jens recommended that I abandon that
> idea and just do it in SCSI for now.  It seemed that the block approach
> was more effort than it was worth.
> 
> Doing the allocation in SCSI opens up another set of problems, however.
> Namely the host free_list.  Things are going to blow up if we mix 16 and
> 32-byte CDBs.  So that means switching all DIF Type 2-capable scsi_hosts
> to 32-byte CDBs wholesale.  Or we can postpone such a switch until a
> Type 2 target is discovered and then empty free_list and make all
> subsequent commands bigger.  Or we could have a separate list +
> scsi_host_cmd_pool that we allocate from if it's a Type 2 dev.  That
> requires passing the scsi_device info through to the allocation
> routines.  Right now they are scsi_host centric.
> 
> I tried several approaches, none of which ended up being very small, nor
> pretty.
> 
> There's less than a handful of Type 2 devices out there.  No array
> vendor I know of plan to support Type 2 on the host side, only on the
> back end.  DIF disk drives will ship formatted with Type 1.  The main
> intent with my patch was to ensure that a Type 2 drive would work if you
> pulled it from an array and hooked it up directly.
> 
> Short of bumping BLK_MAX_CDB to 32 the patch I posted a few days was by
> far the least intrusive solution.  It only causes an allocation for FS
> requests, and only for Type 2 drives, and only if the HBA supports DIF
> Type 2.
> 
> I've had a several requests for Type 2 support in Linux (mainly for
> target device testing) so I wanted to get some code out there.  But I'm
> happy to spend more time on the various approaches if people feel that's
> justified.  I'd just like to know which one before I burn more cycles.
> So far it's been a huge time sink.
> 
> James: What's your take on this?
> 
> 

OK I'm convinced. I only have some concerns at allocation see new reply
to patch (first mail) near the code in question.

BTW have you thought of doing all this inside sd.c? it might be very simple.
Since it is sd that probes for type 2 disks, and it is sd that finally decides
what commands to send, when. It would be easy to just allocate the extra cmnd
space there, and be done with it. It sounds like all this can be sd private stuff.

Boaz
> Boaz> BTW: I think I found a bug in __scsi_get_command() in regard to:
> Boaz> 	"scsi_host_get_prot(shost) >= SHOST_DIX_TYPE0_PROTECTION"
> 
> Boaz> this here will reset the cmd->prot_sdb pointer, which will leak
> Boaz> eventually. And will be missing for this operation.
> 
> Good spotting!  Will fix.
> 

Re: [PATCH 1/5] SCSI: Add support for 32-byte CDBs

[Boaz Harrosh]

On 09/04/2009 11:36 AM, Martin K. Petersen wrote:
> DIF Type 2 drives require 32-byte commands.  Add a suitable flag to
> struct scsi_device that will cause scsi_setup_fs_cmnd() to allocate a
> bigger command buffer.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/scsi_lib.c    |   38 +++++++++++++++++++++++++++++++++++++-
>  include/scsi/scsi.h        |    1 +
>  include/scsi/scsi_cmnd.h   |    6 ++++++
>  include/scsi/scsi_device.h |    1 +
>  4 files changed, 45 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 662024d..643769c 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -67,6 +67,9 @@ static struct scsi_host_sg_pool scsi_sg_pools[] = {
>  
>  struct kmem_cache *scsi_sdb_cache;
>  
> +struct kmem_cache *cdb_cache;
> +mempool_t *cdb_pool;
> +
>  static void scsi_run_queue(struct request_queue *q);
>  
>  /*
> @@ -642,6 +645,11 @@ static void __scsi_release_buffers(struct scsi_cmnd *cmd, int do_bidi_check)
>  
>  	if (scsi_prot_sg_count(cmd))
>  		scsi_free_sgtable(cmd->prot_sdb);
> +
> +	if (cmd->flags & SCSI_CMND_EXT_CDB) {
> +		mempool_free(cmd->cmnd, cdb_pool);
> +		cmd->flags &= ~SCSI_CMND_EXT_CDB;
> +	}
>  }
>  
>  /*
> @@ -1109,7 +1117,19 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
>  	if (unlikely(!cmd))
>  		return BLKPREP_DEFER;
>  
> -	memset(cmd->cmnd, 0, BLK_MAX_CDB);
> +	if (sdev->use_32_for_rw) {
> +		req->cmd = mempool_alloc(cdb_pool, GFP_ATOMIC);
> +
> +		if (unlikely(!req->cmd))

Here we should not hang onto the cmd, because it might be the free-list
command and we'll dead-lock. Better do in this case:

			scsi_put_command(cmd);
			req->special = NULL;

You might want to add a flag to scsi_get_cmd_from_req() and do the allocation
and failure there, where it is more clear what you are doing and why. Also this
way in the future blk_pc_cmnds could use it too, if needed.

> +			return BLKPREP_DEFER;
> +
> +		cmd->cmnd = req->cmd;
> +		cmd->cmd_len = req->cmd_len = SCSI_EXT_CDB_SIZE;
> +		cmd->flags |= SCSI_CMND_EXT_CDB;
> +		memset(cmd->cmnd, 0, cmd->cmd_len);
> +	} else
> +		memset(cmd->cmnd, 0, BLK_MAX_CDB);
> +
>  	return scsi_init_io(cmd, GFP_ATOMIC);
>  }
>  EXPORT_SYMBOL(scsi_setup_fs_cmnd);
> @@ -1745,6 +1765,19 @@ int __init scsi_init_queue(void)
>  		}
>  	}
>  
> +	cdb_cache = kmem_cache_create("scsi_cdb_32", SCSI_EXT_CDB_SIZE,
> +				      0, 0, NULL);
> +	if (!cdb_cache) {
> +		printk(KERN_ERR "SCSI: can't init scsi cdb cache\n");
> +		goto cleanup_sdb;
> +	}
> +
> +	cdb_pool = mempool_create_slab_pool(SG_MEMPOOL_SIZE, cdb_cache);
> +	if (!cdb_pool) {
> +		printk(KERN_ERR "SCSI: can't init scsi cdb pool\n");
> +		goto cleanup_sdb;
> +	}
> +
>  	return 0;
>  
>  cleanup_sdb:
> @@ -1771,6 +1804,9 @@ void scsi_exit_queue(void)
>  		mempool_destroy(sgp->pool);
>  		kmem_cache_destroy(sgp->slab);
>  	}
> +
> +	mempool_destroy(cdb_pool);
> +	kmem_cache_destroy(cdb_cache);
>  }
>  
>  /**
> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
> index 084478e..a7ae10a 100644
> --- a/include/scsi/scsi.h
> +++ b/include/scsi/scsi.h
> @@ -138,6 +138,7 @@ struct scsi_cmnd;
>   *	SCSI command lengths
>   */
>  
> +#define SCSI_EXT_CDB_SIZE 32
>  #define SCSI_MAX_VARLEN_CDB_SIZE 260
>  
>  /* defined in T10 SCSI Primary Commands-2 (SPC2) */
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 3878d1d..d4a6a80 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -50,6 +50,10 @@ struct scsi_pointer {
>  	volatile int phase;
>  };
>  
> +enum scsi_cmnd_flags {
> +	SCSI_CMND_EXT_CDB = 1 << 0,
> +};
> +
>  struct scsi_cmnd {
>  	struct scsi_device *device;
>  	struct list_head list;  /* scsi_cmnd participates in queue lists */
> @@ -129,6 +133,8 @@ struct scsi_cmnd {
>  	int result;		/* Status code from lower level driver */
>  
>  	unsigned char tag;	/* SCSI-II queued command tag */
> +
> +	enum scsi_cmnd_flags flags;
>  };
>  
>  extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 3f566af..de2966b 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -129,6 +129,7 @@ struct scsi_device {
>  				 * this device */
>  	unsigned expecting_cc_ua:1; /* Expecting a CHECK_CONDITION/UNIT_ATTN
>  				     * because we did a bus reset. */
> +	unsigned use_32_for_rw:1; /* use 32-byte read / write (DIF Type 2) */
>  	unsigned use_10_for_rw:1; /* first try 10-byte read / write */
>  	unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
>  	unsigned skip_ms_page_8:1;	/* do not use MODE SENSE page 0x08 */

Re: [PATCH 1/5] SCSI: Add support for 32-byte CDBs

[Martin K. Petersen]

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

Boaz> BTW have you thought of doing all this inside sd.c? it might be
Boaz> very simple.  Since it is sd that probes for type 2 disks, and it
Boaz> is sd that finally decides what commands to send, when. It would
Boaz> be easy to just allocate the extra cmnd space there, and be done
Boaz> with it. It sounds like all this can be sd private stuff.

How about this?


sd: Support disks formatted with DIF Type 2

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>

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: Add support for 32-byte CDBs

[Boaz Harrosh]

On 09/11/2009 09:33 AM, Martin K. Petersen wrote:
>>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
> 
> Boaz> BTW have you thought of doing all this inside sd.c? it might be
> Boaz> very simple.  Since it is sd that probes for type 2 disks, and it
> Boaz> is sd that finally decides what commands to send, when. It would
> Boaz> be easy to just allocate the extra cmnd space there, and be done
> Boaz> with it. It sounds like all this can be sd private stuff.
> 
> How about this?
> 

This patch replaces both:
	[PATCH 1/5] SCSI: Add support for 32-byte CDBs
	[PATCH 4/5] sd: Support disks formatted with DIF Type 2

right?

looks very good. Well I like it better. Don't you?
(support of the rule, keep it simple.)

One question below

> 
> sd: Support disks formatted with DIF Type 2
> 
> 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>
> 
> 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);
> +

The cmd_len gets updated later on, in scsi_lib.c right?

> +		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 */

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

Re: [PATCH 1/5] SCSI: Add support for 32-byte CDBs

[Martin K. Petersen]

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

Boaz> This patch replaces both:
Boaz> 	[PATCH 1/5] SCSI: Add support for 32-byte CDBs
Boaz>   [PATCH 4/5] sd: Support disks formatted with DIF Type 2

Boaz> right?

Yep.


Boaz> One question below

>> + SCpnt->cmnd = mempool_alloc(sd_cdb_pool, GFP_ATOMIC);
>> +

Boaz> The cmd_len gets updated later on, in scsi_lib.c right?

Nope.  But you are right that I should set cmd_len correctly.  Otherwise
LLDs won't transfer sufficient CDB data (that's obviously not a problem
for scsi_debug which is what I've been using for testing).

-- 
Martin K. Petersen	Oracle Linux Engineering


sd: Support disks formatted with DIF Type 2

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>

---

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 36e2333..fa44936 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,49 @@ 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) {
+		SCpnt->cmd_len = SD_EXT_CDB_SIZE;
+		memset(SCpnt->cmnd, 0, SCpnt->cmd_len);
+		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 +612,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 +1087,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 +1149,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 +1316,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 +2361,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 +2398,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: Add support for 32-byte CDBs

[Boaz Harrosh]

On 09/15/2009 10:29 AM, Martin K. Petersen wrote:
>>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
> 
> Boaz> This patch replaces both:
> Boaz> 	[PATCH 1/5] SCSI: Add support for 32-byte CDBs
> Boaz>   [PATCH 4/5] sd: Support disks formatted with DIF Type 2
> 
> Boaz> right?
> 
> Yep.
> 
> 
> Boaz> One question below
> 
>>> + SCpnt->cmnd = mempool_alloc(sd_cdb_pool, GFP_ATOMIC);
>>> +
> 
> Boaz> The cmd_len gets updated later on, in scsi_lib.c right?
> 
> Nope.  But you are right that I should set cmd_len correctly.  Otherwise
> LLDs won't transfer sufficient CDB data (that's obviously not a problem
> for scsi_debug which is what I've been using for testing).
> 

It does in scsi_init_cmd_errh() which is called from scsi_request_fn().
It does:
	if (cmd->cmd_len == 0)
		cmd->cmd_len = scsi_command_size(cmd->cmnd);

Now scsi_command_size() perfectly understand VARIABLE_LENGTH_CMD, but it might
be clearer to set it here next to the command setup, in sd.c.

Boaz

Re: [PATCH 1/5] SCSI: Add support for 32-byte CDBs

[James Bottomley]

On Fri, 2009-09-04 at 04:36 -0400, Martin K. Petersen wrote:
> DIF Type 2 drives require 32-byte commands.  Add a suitable flag to
> struct scsi_device that will cause scsi_setup_fs_cmnd() to allocate a
> bigger command buffer.

This does look better than the previous version.

However, I'm afraid the lifetime rules don't look to be correct.

The fundamental problem you have is that the request lives longer than
the cmnd.  You populate req->cmnd but never reset it to the original
when you free the 32 byte command.

Fundamentally, since you know you're always going to be using a 32 byte
command, it looks like the consistent place to do the allocation is in
the block layer.  If we go that route, I'd recommend using a variable
for the length so ATA can use it also for transmitting task files. I've
cc'd Jens to see what he thinks of this.

If we just want to hack it up in the SCSI layer, it seems to me you
don't need to muck with req->cmd.  Just populate cmd->cmnd from the pool
and use the signature (req->cmd != cmd->cmnd) to know you did (thus
dispensing with the flag and keeping req->cmd consistent).

James

Re: [PATCH 1/5] SCSI: Add support for 32-byte CDBs

[Martin K. Petersen]

>>>>> "James" == James Bottomley <James.Bottomley@suse.de> writes:

James> This does look better than the previous version.

However, you're a couple of weeks behind :)


James> Fundamentally, since you know you're always going to be using a
James> 32 byte command, it looks like the consistent place to do the
James> allocation is in the block layer.  If we go that route, I'd
James> recommend using a variable for the length so ATA can use it also
James> for transmitting task files. I've cc'd Jens to see what he thinks
James> of this.

If you check the archives you'll see that that was my initial approach.
I moved the command array to the end of struct request so it could be an
arbitrary size depending on queue.  However, there are many places where
it is assumed that struct request is a fixed size, we allocate it on the
stack, etc.  My patch grew bigger and bigger and got quite hairy.
Eventually Jens recommended that I abandoned this for now because it was
more trouble than it was worth.  It's still something I'd like to do
longer term, but I was interested in doing something that would allow
Type 2 devices to work in 2.6.32.


James> If we just want to hack it up in the SCSI layer, it seems to me
James> you don't need to muck with req->cmd.  Just populate cmd->cmnd
James> from the pool and use the signature (req->cmd != cmd->cmnd) to
James> know you did (thus dispensing with the flag and keeping req->cmd
James> consistent).

Here's what I posted a few days ago:

sd: Support disks formatted with DIF Type 2

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>

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 36e2333..fa44936 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,49 @@ 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) {
+		SCpnt->cmd_len = SD_EXT_CDB_SIZE;
+		memset(SCpnt->cmnd, 0, SCpnt->cmd_len);
+		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 +612,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 +1087,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 +1149,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 +1316,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 +2361,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 +2398,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: Add support for 32-byte CDBs

[Martin K. Petersen]

>> + if (cmd->cmd_len == SCSI_EXT_CDB_SIZE)
>> + 	mempool_free(cmd->cmnd, cdb_pool);
>> +

Boaz> This will not work. The cmd->cmnd might happen to be form an upper
Boaz> layer through bsg or blk_execute_xxx and might just be
Boaz> SCSI_EXT_CDB_SIZE.

This patch has been kicking around in my tree for a while and it was a
huge, gnarly mess.  I trimmed it down a lot before sending it out
yesterday and forgot why I originally had an allocation flag in
scsi_cmnd.  I think I'll reinstate that just to play it safe.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/5] SCSI: Add support for 32-byte CDBs

[Boaz Harrosh]

On 08/26/2009 09:17 AM, Martin K. Petersen wrote:
> DIF Type 2 drives require 32-byte commands.  Add a suitable flag to
> struct scsi_device that will cause scsi_setup_fs_cmnd() to allocate a
> bigger command buffer.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/scsi_lib.c    |   36 +++++++++++++++++++++++++++++++++++-
>  include/scsi/scsi.h        |    1 +
>  include/scsi/scsi_device.h |    1 +
>  3 files changed, 37 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 662024d..fc752b5 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -67,6 +67,9 @@ static struct scsi_host_sg_pool scsi_sg_pools[] = {
>  
>  struct kmem_cache *scsi_sdb_cache;
>  
> +struct kmem_cache *cdb_cache;
> +mempool_t *cdb_pool;
> +
>  static void scsi_run_queue(struct request_queue *q);
>  
>  /*
> @@ -642,6 +645,10 @@ static void __scsi_release_buffers(struct scsi_cmnd *cmd, int do_bidi_check)
>  
>  	if (scsi_prot_sg_count(cmd))
>  		scsi_free_sgtable(cmd->prot_sdb);
> +
> +	if (cmd->cmd_len == SCSI_EXT_CDB_SIZE)
> +		mempool_free(cmd->cmnd, cdb_pool);
> +

This will not work. The cmd->cmnd might happen to be form an upper
layer through bsg or blk_execute_xxx and might just be SCSI_EXT_CDB_SIZE.

We could say to maybe:
	if (cmd->cmnd != cmd->req->cmd)

which means scsi allocated the buffer and not use the block supplied one
but that might be dangerous and will need a fat comment that states that
scsi drivers must use the scsi_cmnd pointer and not the request's.
Alternatively you could:

	if ((cmd->cmd_len == SCSI_EXT_CDB_SIZE) && blk_fs_request(cmnd->req))

since variable length can only come BLOCK_PC currently. But this will need to change
again later when ULDs might want to issue other commands that are SCSI_EXT_CDB_SIZE
in the future, which again might be dangerous.

or use an extra flag. You could use the first "fat comment" option above, all the drivers
I know do only use the scsi_cmnd->cmnd pointer.

>  }
>  
>  /*
> @@ -1109,7 +1116,18 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
>  	if (unlikely(!cmd))
>  		return BLKPREP_DEFER;
>  
> -	memset(cmd->cmnd, 0, BLK_MAX_CDB);
> +	if (sdev->use_32_for_rw) {
> +		req->cmd = mempool_alloc(cdb_pool, GFP_ATOMIC);
> +

James could we get that live-lock problem we had when not
atomically allocating the scsi_cmnd, (when sense was separated
out)

> +		if (unlikely(!req->cmd))
> +			return BLKPREP_DEFER;
> +
> +		cmd->cmnd = req->cmd;
> +		cmd->cmd_len = req->cmd_len = SCSI_EXT_CDB_SIZE;
> +		memset(cmd->cmnd, 0, cmd->cmd_len);
> +	} else
> +		memset(cmd->cmnd, 0, BLK_MAX_CDB);
> +
>  	return scsi_init_io(cmd, GFP_ATOMIC);
>  }
>  EXPORT_SYMBOL(scsi_setup_fs_cmnd);
> @@ -1745,6 +1763,19 @@ int __init scsi_init_queue(void)
>  		}
>  	}
>  
> +	cdb_cache = kmem_cache_create("scsi_cdb_32", SCSI_EXT_CDB_SIZE,
> +				      0, 0, NULL);
> +	if (!cdb_cache) {
> +		printk(KERN_ERR "SCSI: can't init scsi cdb cache\n");
> +		goto cleanup_sdb;
> +	}
> +
> +	cdb_pool = mempool_create_slab_pool(SG_MEMPOOL_SIZE, cdb_cache);
> +	if (!cdb_pool) {
> +		printk(KERN_ERR "SCSI: can't init scsi cdb pool\n");
> +		goto cleanup_sdb;
> +	}
> +
>  	return 0;
>  
>  cleanup_sdb:
> @@ -1771,6 +1802,9 @@ void scsi_exit_queue(void)
>  		mempool_destroy(sgp->pool);
>  		kmem_cache_destroy(sgp->slab);
>  	}
> +
> +	mempool_destroy(cdb_pool);
> +	kmem_cache_destroy(cdb_cache);
>  }
>  
>  /**
> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
> index 084478e..a7ae10a 100644
> --- a/include/scsi/scsi.h
> +++ b/include/scsi/scsi.h
> @@ -138,6 +138,7 @@ struct scsi_cmnd;
>   *	SCSI command lengths
>   */
>  
> +#define SCSI_EXT_CDB_SIZE 32
>  #define SCSI_MAX_VARLEN_CDB_SIZE 260
>  
>  /* defined in T10 SCSI Primary Commands-2 (SPC2) */
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 3f566af..de2966b 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -129,6 +129,7 @@ struct scsi_device {
>  				 * this device */
>  	unsigned expecting_cc_ua:1; /* Expecting a CHECK_CONDITION/UNIT_ATTN
>  				     * because we did a bus reset. */
> +	unsigned use_32_for_rw:1; /* use 32-byte read / write (DIF Type 2) */
>  	unsigned use_10_for_rw:1; /* first try 10-byte read / write */
>  	unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
>  	unsigned skip_ms_page_8:1;	/* do not use MODE SENSE page 0x08 */


Boaz

[PATCH 1/5] SCSI: Add support for 32-byte CDBs

[Martin K. Petersen]

DIF Type 2 drives require 32-byte commands.  Add a suitable flag to
struct scsi_device that will cause scsi_setup_fs_cmnd() to allocate a
bigger command buffer.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/scsi_lib.c    |   36 +++++++++++++++++++++++++++++++++++-
 include/scsi/scsi.h        |    1 +
 include/scsi/scsi_device.h |    1 +
 3 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 662024d..fc752b5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -67,6 +67,9 @@ static struct scsi_host_sg_pool scsi_sg_pools[] = {
 
 struct kmem_cache *scsi_sdb_cache;
 
+struct kmem_cache *cdb_cache;
+mempool_t *cdb_pool;
+
 static void scsi_run_queue(struct request_queue *q);
 
 /*
@@ -642,6 +645,10 @@ static void __scsi_release_buffers(struct scsi_cmnd *cmd, int do_bidi_check)
 
 	if (scsi_prot_sg_count(cmd))
 		scsi_free_sgtable(cmd->prot_sdb);
+
+	if (cmd->cmd_len == SCSI_EXT_CDB_SIZE)
+		mempool_free(cmd->cmnd, cdb_pool);
+
 }
 
 /*
@@ -1109,7 +1116,18 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
 	if (unlikely(!cmd))
 		return BLKPREP_DEFER;
 
-	memset(cmd->cmnd, 0, BLK_MAX_CDB);
+	if (sdev->use_32_for_rw) {
+		req->cmd = mempool_alloc(cdb_pool, GFP_ATOMIC);
+
+		if (unlikely(!req->cmd))
+			return BLKPREP_DEFER;
+
+		cmd->cmnd = req->cmd;
+		cmd->cmd_len = req->cmd_len = SCSI_EXT_CDB_SIZE;
+		memset(cmd->cmnd, 0, cmd->cmd_len);
+	} else
+		memset(cmd->cmnd, 0, BLK_MAX_CDB);
+
 	return scsi_init_io(cmd, GFP_ATOMIC);
 }
 EXPORT_SYMBOL(scsi_setup_fs_cmnd);
@@ -1745,6 +1763,19 @@ int __init scsi_init_queue(void)
 		}
 	}
 
+	cdb_cache = kmem_cache_create("scsi_cdb_32", SCSI_EXT_CDB_SIZE,
+				      0, 0, NULL);
+	if (!cdb_cache) {
+		printk(KERN_ERR "SCSI: can't init scsi cdb cache\n");
+		goto cleanup_sdb;
+	}
+
+	cdb_pool = mempool_create_slab_pool(SG_MEMPOOL_SIZE, cdb_cache);
+	if (!cdb_pool) {
+		printk(KERN_ERR "SCSI: can't init scsi cdb pool\n");
+		goto cleanup_sdb;
+	}
+
 	return 0;
 
 cleanup_sdb:
@@ -1771,6 +1802,9 @@ void scsi_exit_queue(void)
 		mempool_destroy(sgp->pool);
 		kmem_cache_destroy(sgp->slab);
 	}
+
+	mempool_destroy(cdb_pool);
+	kmem_cache_destroy(cdb_cache);
 }
 
 /**
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 084478e..a7ae10a 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -138,6 +138,7 @@ struct scsi_cmnd;
  *	SCSI command lengths
  */
 
+#define SCSI_EXT_CDB_SIZE 32
 #define SCSI_MAX_VARLEN_CDB_SIZE 260
 
 /* defined in T10 SCSI Primary Commands-2 (SPC2) */
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 3f566af..de2966b 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -129,6 +129,7 @@ struct scsi_device {
 				 * this device */
 	unsigned expecting_cc_ua:1; /* Expecting a CHECK_CONDITION/UNIT_ATTN
 				     * because we did a bus reset. */
+	unsigned use_32_for_rw:1; /* use 32-byte read / write (DIF Type 2) */
 	unsigned use_10_for_rw:1; /* first try 10-byte read / write */
 	unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
 	unsigned skip_ms_page_8:1;	/* do not use MODE SENSE page 0x08 */
-- 
1.6.0.6