[PATCH 0/4] scsi_debug: add write scattered support

[PATCH 0/4] scsi_debug: add write scattered support

[Douglas Gilbert]

While testing the WRITE SCATTERED command support in a new sg3_utils
utility (sg_write_x) it was helpful to have a target that supported
this command. This command might be attractive to other kernel
subsystems. Even if end devices don't support this command yet, it
would most likely be a performance win if SCSI LLDs supported it (by
breaking it down to a series of WRITE commands), as that would cut
down the overhead of the block layer, the ULD (e.g. sd) and the SCSI
midlevel.

Douglas Gilbert (4):
  tab, kstrto changes, requested by checkpatch.pl
  fix group_number mask 0x1f->0x3f
  expand do_device_access to take sg offset
  add resp_write_scat function

 drivers/scsi/scsi_debug.c | 504 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 382 insertions(+), 122 deletions(-)

-- 
2.14.1

[PATCH 1/4] scsi_debug: tab, kstrto changes, requested by checkpatch.pl

[Douglas Gilbert]

Some of my development tools tend to add spaces (my preference) rather
than tabs (kernel convention). Running unexpand to clean these spaces
up found more of them than checkpatch.pl did. Then checkpatch.pl
complained about other style violations in those newly tabbed lines.

Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/scsi_debug.c | 174 +++++++++++++++++++++++-----------------------
 1 file changed, 87 insertions(+), 87 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 6fcc094a2a77..68f4ab8ffbaf 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -305,8 +305,8 @@ struct opcode_info_t {
 	u32 flags;		/* OR-ed set of SDEB_F_* */
 	int (*pfp)(struct scsi_cmnd *, struct sdebug_dev_info *);
 	const struct opcode_info_t *arrp;  /* num_attached elements or NULL */
-	u8 len_mask[16];	/* len=len_mask[0], then mask for cdb[1]... */
-				/* ignore cdb bytes after position 15 */
+	u8 len_mask[16];	/* len_mask[0]-->cdb_len, then mask for cdb */
+				/* 1 to min(cdb_len, 15); ignore cdb[15...] */
 };
 
 /* SCSI opcodes (first byte of cdb) of interest mapped onto these indexes */
@@ -341,7 +341,7 @@ enum sdeb_opcode_index {
 	SDEB_I_WRITE_SAME = 27,		/* 10, 16 */
 	SDEB_I_SYNC_CACHE = 28,		/* 10 only */
 	SDEB_I_COMP_WRITE = 29,
-	SDEB_I_LAST_ELEMENT = 30,	/* keep this last */
+	SDEB_I_LAST_ELEMENT = 30,	/* keep this last (previous + 1) */
 };
 
 
@@ -1959,7 +1959,7 @@ static unsigned char ctrl_m_pg[] = {0xa, 10, 2, 0, 0, 0, 0, 0,
 static int resp_ctrl_m_pg(unsigned char * p, int pcontrol, int target)
 { 	/* Control mode page for mode_sense */
 	unsigned char ch_ctrl_m_pg[] = {/* 0xa, 10, */ 0x6, 0, 0, 0, 0, 0,
-				        0, 0, 0, 0};
+					0, 0, 0, 0};
 	unsigned char d_ctrl_m_pg[] = {0xa, 10, 2, 0, 0, 0, 0, 0,
 				     0, 0, 0x2, 0x4b};
 
@@ -2136,13 +2136,13 @@ static int resp_mode_sense(struct scsi_cmnd *scp,
 		len = resp_disconnect_pg(ap, pcontrol, target);
 		offset += len;
 		break;
-        case 0x3:       /* Format device page, direct access */
+	case 0x3:       /* Format device page, direct access */
 		if (is_disk) {
 			len = resp_format_pg(ap, pcontrol, target);
 			offset += len;
 		} else
 			bad_pcode = true;
-                break;
+		break;
 	case 0x8:	/* Caching page, direct access */
 		if (is_disk) {
 			len = resp_caching_pg(ap, pcontrol, target);
@@ -2158,7 +2158,7 @@ static int resp_mode_sense(struct scsi_cmnd *scp,
 		if ((subpcode > 0x2) && (subpcode < 0xff)) {
 			mk_sense_invalid_fld(scp, SDEB_IN_CDB, 3, -1);
 			return check_condition_result;
-	        }
+		}
 		len = 0;
 		if ((0x0 == subpcode) || (0xff == subpcode))
 			len += resp_sas_sf_m_pg(ap + len, pcontrol, target);
@@ -2195,7 +2195,7 @@ static int resp_mode_sense(struct scsi_cmnd *scp,
 		} else {
 			mk_sense_invalid_fld(scp, SDEB_IN_CDB, 3, -1);
 			return check_condition_result;
-                }
+		}
 		break;
 	default:
 		bad_pcode = true;
@@ -2231,8 +2231,8 @@ static int resp_mode_select(struct scsi_cmnd *scp,
 		mk_sense_invalid_fld(scp, SDEB_IN_CDB, mselect6 ? 4 : 7, -1);
 		return check_condition_result;
 	}
-        res = fetch_to_dev_buffer(scp, arr, param_len);
-        if (-1 == res)
+	res = fetch_to_dev_buffer(scp, arr, param_len);
+	if (-1 == res)
 		return DID_ERROR << 16;
 	else if (sdebug_verbose && (res < param_len))
 		sdev_printk(KERN_INFO, scp->device,
@@ -2298,8 +2298,8 @@ static int resp_temp_l_pg(unsigned char * arr)
 				     0x0, 0x1, 0x3, 0x2, 0x0, 65,
 		};
 
-        memcpy(arr, temp_l_pg, sizeof(temp_l_pg));
-        return sizeof(temp_l_pg);
+	memcpy(arr, temp_l_pg, sizeof(temp_l_pg));
+	return sizeof(temp_l_pg);
 }
 
 static int resp_ie_l_pg(unsigned char * arr)
@@ -2307,18 +2307,18 @@ static int resp_ie_l_pg(unsigned char * arr)
 	unsigned char ie_l_pg[] = {0x0, 0x0, 0x3, 0x3, 0x0, 0x0, 38,
 		};
 
-        memcpy(arr, ie_l_pg, sizeof(ie_l_pg));
+	memcpy(arr, ie_l_pg, sizeof(ie_l_pg));
 	if (iec_m_pg[2] & 0x4) {	/* TEST bit set */
 		arr[4] = THRESHOLD_EXCEEDED;
 		arr[5] = 0xff;
 	}
-        return sizeof(ie_l_pg);
+	return sizeof(ie_l_pg);
 }
 
 #define SDEBUG_MAX_LSENSE_SZ 512
 
-static int resp_log_sense(struct scsi_cmnd * scp,
-                          struct sdebug_dev_info * devip)
+static int resp_log_sense(struct scsi_cmnd *scp,
+			  struct sdebug_dev_info *devip)
 {
 	int ppc, sp, pcode, subpcode, alloc_len, len, n;
 	unsigned char arr[SDEBUG_MAX_LSENSE_SZ];
@@ -3662,12 +3662,12 @@ static struct sdebug_dev_info *find_build_dev_info(struct scsi_device *sdev)
 	if (!sdbg_host) {
 		pr_err("Host info NULL\n");
 		return NULL;
-        }
+	}
 	list_for_each_entry(devip, &sdbg_host->dev_info_list, dev_list) {
 		if ((devip->used) && (devip->channel == sdev->channel) &&
-                    (devip->target == sdev->id) &&
-                    (devip->lun == sdev->lun))
-                        return devip;
+		    (devip->target == sdev->id) &&
+		    (devip->lun == sdev->lun))
+			return devip;
 		else {
 			if ((!devip->used) && (!open_devip))
 				open_devip = devip;
@@ -3908,8 +3908,8 @@ static int scsi_debug_bus_reset(struct scsi_cmnd * SCpnt)
 {
 	struct sdebug_host_info *sdbg_host;
 	struct sdebug_dev_info *devip;
-        struct scsi_device * sdp;
-        struct Scsi_Host * hp;
+	struct scsi_device *sdp;
+	struct Scsi_Host *hp;
 	int k = 0;
 
 	++num_bus_resets;
@@ -3923,7 +3923,7 @@ static int scsi_debug_bus_reset(struct scsi_cmnd * SCpnt)
 		sdbg_host = *(struct sdebug_host_info **)shost_priv(hp);
 		if (sdbg_host) {
 			list_for_each_entry(devip,
-                                            &sdbg_host->dev_info_list,
+					    &sdbg_host->dev_info_list,
 					    dev_list) {
 				set_bit(SDEBUG_UA_BUS_RESET, devip->uas_bm);
 				++k;
@@ -3946,15 +3946,15 @@ static int scsi_debug_host_reset(struct scsi_cmnd * SCpnt)
 	++num_host_resets;
 	if ((SCpnt->device) && (SDEBUG_OPT_ALL_NOISE & sdebug_opts))
 		sdev_printk(KERN_INFO, SCpnt->device, "%s\n", __func__);
-        spin_lock(&sdebug_host_list_lock);
-        list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
+	spin_lock(&sdebug_host_list_lock);
+	list_for_each_entry(sdbg_host, &sdebug_host_list, host_list) {
 		list_for_each_entry(devip, &sdbg_host->dev_info_list,
 				    dev_list) {
 			set_bit(SDEBUG_UA_BUS_RESET, devip->uas_bm);
 			++k;
 		}
-        }
-        spin_unlock(&sdebug_host_list_lock);
+	}
+	spin_unlock(&sdebug_host_list_lock);
 	stop_all_queued();
 	if (SDEBUG_OPT_RESET_NOISE & sdebug_opts)
 		sdev_printk(KERN_INFO, SCpnt->device,
@@ -3981,7 +3981,7 @@ static void __init sdebug_build_parts(unsigned char *ramp,
 	sectors_per_part = (num_sectors - sdebug_sectors_per)
 			   / sdebug_num_parts;
 	heads_by_sects = sdebug_heads * sdebug_sectors_per;
-        starts[0] = sdebug_sectors_per;
+	starts[0] = sdebug_sectors_per;
 	for (k = 1; k < sdebug_num_parts; ++k)
 		starts[k] = ((k * sectors_per_part) / heads_by_sects)
 			    * heads_by_sects;
@@ -4489,15 +4489,15 @@ static ssize_t opts_show(struct device_driver *ddp, char *buf)
 static ssize_t opts_store(struct device_driver *ddp, const char *buf,
 			  size_t count)
 {
-        int opts;
+	int opts;
 	char work[20];
 
-        if (1 == sscanf(buf, "%10s", work)) {
-		if (0 == strncasecmp(work,"0x", 2)) {
-			if (1 == sscanf(&work[2], "%x", &opts))
+	if (sscanf(buf, "%10s", work) == 1) {
+		if (strncasecmp(work, "0x", 2) == 0) {
+			if (kstrtoint(work + 2, 16, &opts) == 0)
 				goto opts_done;
 		} else {
-			if (1 == sscanf(work, "%d", &opts))
+			if (kstrtoint(work, 10, &opts) == 0)
 				goto opts_done;
 		}
 	}
@@ -4518,7 +4518,7 @@ static ssize_t ptype_show(struct device_driver *ddp, char *buf)
 static ssize_t ptype_store(struct device_driver *ddp, const char *buf,
 			   size_t count)
 {
-        int n;
+	int n;
 
 	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
 		sdebug_ptype = n;
@@ -4535,7 +4535,7 @@ static ssize_t dsense_show(struct device_driver *ddp, char *buf)
 static ssize_t dsense_store(struct device_driver *ddp, const char *buf,
 			    size_t count)
 {
-        int n;
+	int n;
 
 	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
 		sdebug_dsense = n;
@@ -4552,7 +4552,7 @@ static ssize_t fake_rw_show(struct device_driver *ddp, char *buf)
 static ssize_t fake_rw_store(struct device_driver *ddp, const char *buf,
 			     size_t count)
 {
-        int n;
+	int n;
 
 	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
 		n = (n > 0);
@@ -4585,7 +4585,7 @@ static ssize_t no_lun_0_show(struct device_driver *ddp, char *buf)
 static ssize_t no_lun_0_store(struct device_driver *ddp, const char *buf,
 			      size_t count)
 {
-        int n;
+	int n;
 
 	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
 		sdebug_no_lun_0 = n;
@@ -4602,7 +4602,7 @@ static ssize_t num_tgts_show(struct device_driver *ddp, char *buf)
 static ssize_t num_tgts_store(struct device_driver *ddp, const char *buf,
 			      size_t count)
 {
-        int n;
+	int n;
 
 	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
 		sdebug_num_tgts = n;
@@ -4632,7 +4632,7 @@ static ssize_t every_nth_show(struct device_driver *ddp, char *buf)
 static ssize_t every_nth_store(struct device_driver *ddp, const char *buf,
 			       size_t count)
 {
-        int nth;
+	int nth;
 
 	if ((count > 0) && (1 == sscanf(buf, "%d", &nth))) {
 		sdebug_every_nth = nth;
@@ -4654,7 +4654,7 @@ static ssize_t max_luns_show(struct device_driver *ddp, char *buf)
 static ssize_t max_luns_store(struct device_driver *ddp, const char *buf,
 			      size_t count)
 {
-        int n;
+	int n;
 	bool changed;
 
 	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
@@ -4741,7 +4741,7 @@ static ssize_t virtual_gb_show(struct device_driver *ddp, char *buf)
 static ssize_t virtual_gb_store(struct device_driver *ddp, const char *buf,
 				size_t count)
 {
-        int n;
+	int n;
 	bool changed;
 
 	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {
@@ -5195,12 +5195,12 @@ static int __init scsi_debug_init(void)
 	host_to_add = sdebug_add_host;
 	sdebug_add_host = 0;
 
-        for (k = 0; k < host_to_add; k++) {
-                if (sdebug_add_adapter()) {
+	for (k = 0; k < host_to_add; k++) {
+		if (sdebug_add_adapter()) {
 			pr_err("sdebug_add_adapter failed k=%d\n", k);
-                        break;
-                }
-        }
+			break;
+		}
+	}
 
 	if (sdebug_verbose)
 		pr_info("built %d host(s)\n", sdebug_add_host);
@@ -5243,53 +5243,53 @@ module_exit(scsi_debug_exit);
 
 static void sdebug_release_adapter(struct device * dev)
 {
-        struct sdebug_host_info *sdbg_host;
+	struct sdebug_host_info *sdbg_host;
 
 	sdbg_host = to_sdebug_host(dev);
-        kfree(sdbg_host);
+	kfree(sdbg_host);
 }
 
 static int sdebug_add_adapter(void)
 {
 	int k, devs_per_host;
-        int error = 0;
-        struct sdebug_host_info *sdbg_host;
+	int error = 0;
+	struct sdebug_host_info *sdbg_host;
 	struct sdebug_dev_info *sdbg_devinfo, *tmp;
 
-        sdbg_host = kzalloc(sizeof(*sdbg_host),GFP_KERNEL);
-        if (NULL == sdbg_host) {
+	sdbg_host = kzalloc(sizeof(*sdbg_host), GFP_KERNEL);
+	if (sdbg_host == NULL) {
 		pr_err("out of memory at line %d\n", __LINE__);
-                return -ENOMEM;
-        }
+		return -ENOMEM;
+	}
 
-        INIT_LIST_HEAD(&sdbg_host->dev_info_list);
+	INIT_LIST_HEAD(&sdbg_host->dev_info_list);
 
 	devs_per_host = sdebug_num_tgts * sdebug_max_luns;
-        for (k = 0; k < devs_per_host; k++) {
+	for (k = 0; k < devs_per_host; k++) {
 		sdbg_devinfo = sdebug_device_create(sdbg_host, GFP_KERNEL);
 		if (!sdbg_devinfo) {
 			pr_err("out of memory at line %d\n", __LINE__);
-                        error = -ENOMEM;
+			error = -ENOMEM;
 			goto clean;
-                }
-        }
+		}
+	}
 
-        spin_lock(&sdebug_host_list_lock);
-        list_add_tail(&sdbg_host->host_list, &sdebug_host_list);
-        spin_unlock(&sdebug_host_list_lock);
+	spin_lock(&sdebug_host_list_lock);
+	list_add_tail(&sdbg_host->host_list, &sdebug_host_list);
+	spin_unlock(&sdebug_host_list_lock);
 
-        sdbg_host->dev.bus = &pseudo_lld_bus;
-        sdbg_host->dev.parent = pseudo_primary;
-        sdbg_host->dev.release = &sdebug_release_adapter;
+	sdbg_host->dev.bus = &pseudo_lld_bus;
+	sdbg_host->dev.parent = pseudo_primary;
+	sdbg_host->dev.release = &sdebug_release_adapter;
 	dev_set_name(&sdbg_host->dev, "adapter%d", sdebug_add_host);
 
-        error = device_register(&sdbg_host->dev);
+	error = device_register(&sdbg_host->dev);
 
-        if (error)
+	if (error)
 		goto clean;
 
 	++sdebug_add_host;
-        return error;
+	return error;
 
 clean:
 	list_for_each_entry_safe(sdbg_devinfo, tmp, &sdbg_host->dev_info_list,
@@ -5299,20 +5299,20 @@ static int sdebug_add_adapter(void)
 	}
 
 	kfree(sdbg_host);
-        return error;
+	return error;
 }
 
 static void sdebug_remove_adapter(void)
 {
-        struct sdebug_host_info * sdbg_host = NULL;
+	struct sdebug_host_info *sdbg_host = NULL;
 
-        spin_lock(&sdebug_host_list_lock);
-        if (!list_empty(&sdebug_host_list)) {
-                sdbg_host = list_entry(sdebug_host_list.prev,
-                                       struct sdebug_host_info, host_list);
+	spin_lock(&sdebug_host_list_lock);
+	if (!list_empty(&sdebug_host_list)) {
+		sdbg_host = list_entry(sdebug_host_list.prev,
+				       struct sdebug_host_info, host_list);
 		list_del(&sdbg_host->host_list);
 	}
-        spin_unlock(&sdebug_host_list_lock);
+	spin_unlock(&sdebug_host_list_lock);
 
 	if (!sdbg_host)
 		return;
@@ -5566,7 +5566,7 @@ static int sdebug_driver_probe(struct device * dev)
 	if (sdebug_mq_active)
 		hpnt->nr_hw_queues = submit_queues;
 
-        sdbg_host->shost = hpnt;
+	sdbg_host->shost = hpnt;
 	*((struct sdebug_host_info **)hpnt->hostdata) = sdbg_host;
 	if ((hpnt->this_id >= 0) && (sdebug_num_tgts > hpnt->this_id))
 		hpnt->max_id = sdebug_num_tgts + 1;
@@ -5624,12 +5624,12 @@ static int sdebug_driver_probe(struct device * dev)
 	sdebug_any_injecting_opt = !!(SDEBUG_OPT_ALL_INJECTING & sdebug_opts);
 	if (sdebug_every_nth)	/* need stats counters for every_nth */
 		sdebug_statistics = true;
-        error = scsi_add_host(hpnt, &sdbg_host->dev);
-        if (error) {
+	error = scsi_add_host(hpnt, &sdbg_host->dev);
+	if (error) {
 		pr_err("scsi_add_host failed\n");
-                error = -ENODEV;
+		error = -ENODEV;
 		scsi_host_put(hpnt);
-        } else
+	} else
 		scsi_scan_host(hpnt);
 
 	return error;
@@ -5637,7 +5637,7 @@ static int sdebug_driver_probe(struct device * dev)
 
 static int sdebug_driver_remove(struct device * dev)
 {
-        struct sdebug_host_info *sdbg_host;
+	struct sdebug_host_info *sdbg_host;
 	struct sdebug_dev_info *sdbg_devinfo, *tmp;
 
 	sdbg_host = to_sdebug_host(dev);
@@ -5647,16 +5647,16 @@ static int sdebug_driver_remove(struct device * dev)
 		return -ENODEV;
 	}
 
-        scsi_remove_host(sdbg_host->shost);
+	scsi_remove_host(sdbg_host->shost);
 
 	list_for_each_entry_safe(sdbg_devinfo, tmp, &sdbg_host->dev_info_list,
 				 dev_list) {
-                list_del(&sdbg_devinfo->dev_list);
-                kfree(sdbg_devinfo);
-        }
+		list_del(&sdbg_devinfo->dev_list);
+		kfree(sdbg_devinfo);
+	}
 
-        scsi_host_put(sdbg_host->shost);
-        return 0;
+	scsi_host_put(sdbg_host->shost);
+	return 0;
 }
 
 static int pseudo_lld_bus_match(struct device *dev,
-- 
2.14.1

[PATCH 2/4] scsi_debug: fix group_number mask 0x1f->0x3f

[Douglas Gilbert]

Various cdb masks incorrectly assumed the GROUP NUMBER field was
5 bits long. It is actually 6 bits long. Correct.

Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/scsi_debug.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 68f4ab8ffbaf..05661387ccaf 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -422,7 +422,7 @@ static const struct opcode_info_t mselect_iarr[1] = {
 
 static const struct opcode_info_t read_iarr[3] = {
 	{0, 0x28, 0, F_D_IN | FF_DIRECT_IO, resp_read_dt0, NULL,/* READ(10) */
-	    {10,  0xff, 0xff, 0xff, 0xff, 0xff, 0x1f, 0xff, 0xff, 0xc7, 0, 0,
+	    {10,  0xff, 0xff, 0xff, 0xff, 0xff, 0x3f, 0xff, 0xff, 0xc7, 0, 0,
 	     0, 0, 0, 0} },
 	{0, 0x8, 0, F_D_IN | FF_DIRECT_IO, resp_read_dt0, NULL, /* READ(6) */
 	    {6,  0xff, 0xff, 0xff, 0xff, 0xc7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
@@ -433,7 +433,7 @@ static const struct opcode_info_t read_iarr[3] = {
 
 static const struct opcode_info_t write_iarr[3] = {
 	{0, 0x2a, 0, F_D_OUT | FF_DIRECT_IO, resp_write_dt0, NULL,   /* 10 */
-	    {10,  0xfb, 0xff, 0xff, 0xff, 0xff, 0x1f, 0xff, 0xff, 0xc7, 0, 0,
+	    {10,  0xfb, 0xff, 0xff, 0xff, 0xff, 0x3f, 0xff, 0xff, 0xc7, 0, 0,
 	     0, 0, 0, 0} },
 	{0, 0xa, 0, F_D_OUT | FF_DIRECT_IO, resp_write_dt0, NULL,    /* 6 */
 	    {6,  0xff, 0xff, 0xff, 0xff, 0xc7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
@@ -466,7 +466,7 @@ static const struct opcode_info_t maint_in_iarr[2] = {
 static const struct opcode_info_t write_same_iarr[1] = {
 	{0, 0x93, 0, F_D_OUT_MAYBE | FF_DIRECT_IO, resp_write_same_16, NULL,
 	    {16,  0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
-	     0xff, 0xff, 0xff, 0x1f, 0xc7} },
+	     0xff, 0xff, 0xff, 0x3f, 0xc7} },
 };
 
 static const struct opcode_info_t reserve_iarr[1] = {
@@ -548,22 +548,22 @@ static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
 	{0, 0x1d, F_D_OUT, 0, NULL, NULL,	/* SEND DIAGNOSTIC */
 	    {6,  0xf7, 0, 0xff, 0xff, 0xc7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
 	{0, 0x42, 0, F_D_OUT | FF_DIRECT_IO, resp_unmap, NULL, /* UNMAP */
-	    {10,  0x1, 0, 0, 0, 0, 0x1f, 0xff, 0xff, 0xc7, 0, 0, 0, 0, 0, 0} },
+	    {10,  0x1, 0, 0, 0, 0, 0x3f, 0xff, 0xff, 0xc7, 0, 0, 0, 0, 0, 0} },
 	{0, 0x53, 0, F_D_IN | F_D_OUT | FF_DIRECT_IO, resp_xdwriteread_10,
-	    NULL, {10,  0xff, 0xff, 0xff, 0xff, 0xff, 0x1f, 0xff, 0xff, 0xc7,
+	    NULL, {10,  0xff, 0xff, 0xff, 0xff, 0xff, 0x3f, 0xff, 0xff, 0xc7,
 		   0, 0, 0, 0, 0, 0} },
 	{0, 0x3b, 0, F_D_OUT_MAYBE, resp_write_buffer, NULL,
 	    {10,  0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc7, 0, 0,
 	     0, 0, 0, 0} },			/* WRITE_BUFFER */
 	{1, 0x41, 0, F_D_OUT_MAYBE | FF_DIRECT_IO, resp_write_same_10,
-	    write_same_iarr, {10,  0xff, 0xff, 0xff, 0xff, 0xff, 0x1f, 0xff,
+	    write_same_iarr, {10,  0xff, 0xff, 0xff, 0xff, 0xff, 0x3f, 0xff,
 			      0xff, 0xc7, 0, 0, 0, 0, 0, 0} },
 	{0, 0x35, 0, F_DELAY_OVERR | FF_DIRECT_IO, NULL, NULL, /* SYNC_CACHE */
-	    {10,  0x7, 0xff, 0xff, 0xff, 0xff, 0x1f, 0xff, 0xff, 0xc7, 0, 0,
+	    {10,  0x7, 0xff, 0xff, 0xff, 0xff, 0x3f, 0xff, 0xff, 0xc7, 0, 0,
 	     0, 0, 0, 0} },
 	{0, 0x89, 0, F_D_OUT | FF_DIRECT_IO, resp_comp_write, NULL,
 	    {16,  0xf8, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0, 0,
-	     0, 0xff, 0x1f, 0xc7} },		/* COMPARE AND WRITE */
+	     0, 0xff, 0x3f, 0xc7} },		/* COMPARE AND WRITE */
 
 /* 30 */
 	{0xff, 0, 0, 0, NULL, NULL,		/* terminating element */
-- 
2.14.1

[PATCH 3/4] scsi_debug: expand do_device_access to take sg offset

[Douglas Gilbert]

WRITE SCATTERED needs to take several "bites" out of the data-out buffer.
Expand the do_device_access() function to take a sg_skip argument.

Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/scsi_debug.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 05661387ccaf..707c24155fd0 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -2412,8 +2412,8 @@ static int check_device_access_params(struct scsi_cmnd *scp,
 }
 
 /* Returns number of bytes copied or -1 if error. */
-static int do_device_access(struct scsi_cmnd *scmd, u64 lba, u32 num,
-			    bool do_write)
+static int do_device_access(struct scsi_cmnd *scmd, u32 sg_skip, u64 lba,
+			    u32 num, bool do_write)
 {
 	int ret;
 	u64 block, rest = 0;
@@ -2439,14 +2439,15 @@ static int do_device_access(struct scsi_cmnd *scmd, u64 lba, u32 num,
 
 	ret = sg_copy_buffer(sdb->table.sgl, sdb->table.nents,
 		   fake_storep + (block * sdebug_sector_size),
-		   (num - rest) * sdebug_sector_size, 0, do_write);
+		   (num - rest) * sdebug_sector_size, sg_skip, do_write);
 	if (ret != (num - rest) * sdebug_sector_size)
 		return ret;
 
 	if (rest) {
 		ret += sg_copy_buffer(sdb->table.sgl, sdb->table.nents,
 			    fake_storep, rest * sdebug_sector_size,
-			    (num - rest) * sdebug_sector_size, do_write);
+			    sg_skip + ((num - rest) * sdebug_sector_size),
+			    do_write);
 	}
 
 	return ret;
@@ -2707,7 +2708,7 @@ static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 		}
 	}
 
-	ret = do_device_access(scp, lba, num, false);
+	ret = do_device_access(scp, 0, lba, num, false);
 	read_unlock_irqrestore(&atomic_rw, iflags);
 	if (unlikely(ret == -1))
 		return DID_ERROR << 16;
@@ -2995,7 +2996,7 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 		}
 	}
 
-	ret = do_device_access(scp, lba, num, true);
+	ret = do_device_access(scp, 0, lba, num, true);
 	if (unlikely(scsi_debug_lbp()))
 		map_region(lba, num);
 	write_unlock_irqrestore(&atomic_rw, iflags);
@@ -3236,7 +3237,7 @@ static int resp_comp_write(struct scsi_cmnd *scp,
 	 * from data-in into arr. Safe (atomic) since write_lock held. */
 	fake_storep_hold = fake_storep;
 	fake_storep = arr;
-	ret = do_device_access(scp, 0, dnum, true);
+	ret = do_device_access(scp, 0, 0, dnum, true);
 	fake_storep = fake_storep_hold;
 	if (ret == -1) {
 		retval = DID_ERROR << 16;
-- 
2.14.1

[PATCH 4/4] scsi_debug: add resp_write_scat function

[Douglas Gilbert]

Add resp_write_scat() function to support decoding WRITE SCATTERED
(16 and 32). Also weave resp_write_scat() into the cdb decoding
logic. Split SDEB_I_SERV_ACT_IN into 12 and 16 byte variants
(similarly for SERV_ACT_OUT). As yet the driver doesn't need
either of the 12 byte variants.

Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/scsi_debug.c | 207 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 192 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 707c24155fd0..a0f61e7df4b4 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -93,6 +93,7 @@ static const char *sdebug_version_date = "20171202";
 #define MISCOMPARE_VERIFY_ASC 0x1d
 #define MICROCODE_CHANGED_ASCQ 0x1	/* with TARGET_CHANGED_ASC */
 #define MICROCODE_CHANGED_WO_RESET_ASCQ 0x16
+#define WRITE_ERROR_ASC 0xc
 
 /* Additional Sense Code Qualifier (ASCQ) */
 #define ACK_NAK_TO 0x3
@@ -323,12 +324,12 @@ enum sdeb_opcode_index {
 	SDEB_I_READ = 9,		/* 6, 10, 12, 16 */
 	SDEB_I_WRITE = 10,		/* 6, 10, 12, 16 */
 	SDEB_I_START_STOP = 11,
-	SDEB_I_SERV_ACT_IN = 12,	/* 12, 16 */
-	SDEB_I_SERV_ACT_OUT = 13,	/* 12, 16 */
+	SDEB_I_SERV_ACT_IN_16 = 12,	/* add ...SERV_ACT_IN_12 if needed */
+	SDEB_I_SERV_ACT_OUT_16 = 13,	/* add ...SERV_ACT_OUT_12 if needed */
 	SDEB_I_MAINT_IN = 14,
 	SDEB_I_MAINT_OUT = 15,
 	SDEB_I_VERIFY = 16,		/* 10 only */
-	SDEB_I_VARIABLE_LEN = 17,
+	SDEB_I_VARIABLE_LEN = 17,	/* READ(32), WRITE(32), WR_SCAT(32) */
 	SDEB_I_RESERVE = 18,		/* 6, 10 */
 	SDEB_I_RELEASE = 19,		/* 6, 10 */
 	SDEB_I_ALLOW_REMOVAL = 20,	/* PREVENT ALLOW MEDIUM REMOVAL */
@@ -373,12 +374,12 @@ static const unsigned char opcode_ind_arr[256] = {
 	0, 0, 0, 0, 0, SDEB_I_ATA_PT, 0, 0,
 	SDEB_I_READ, SDEB_I_COMP_WRITE, SDEB_I_WRITE, 0, 0, 0, 0, 0,
 	0, 0, 0, SDEB_I_WRITE_SAME, 0, 0, 0, 0,
-	0, 0, 0, 0, 0, 0, SDEB_I_SERV_ACT_IN, SDEB_I_SERV_ACT_OUT,
+	0, 0, 0, 0, 0, 0, SDEB_I_SERV_ACT_IN_16, SDEB_I_SERV_ACT_OUT_16,
 /* 0xa0; 0xa0->0xbf: 12 byte cdbs */
 	SDEB_I_REPORT_LUNS, SDEB_I_ATA_PT, 0, SDEB_I_MAINT_IN,
 	     SDEB_I_MAINT_OUT, 0, 0, 0,
-	SDEB_I_READ, SDEB_I_SERV_ACT_OUT, SDEB_I_WRITE, SDEB_I_SERV_ACT_IN,
-	     0, 0, 0, 0,
+	SDEB_I_READ, 0 /* SDEB_I_SERV_ACT_OUT_12 */, SDEB_I_WRITE,
+	     0 /* SDEB_I_SERV_ACT_IN_12 */, 0, 0, 0, 0,
 	0, 0, 0, 0, 0, 0, 0, 0,
 	0, 0, 0, 0, 0, 0, 0, 0,
 /* 0xc0; 0xc0->0xff: vendor specific */
@@ -397,6 +398,7 @@ static int resp_log_sense(struct scsi_cmnd *, struct sdebug_dev_info *);
 static int resp_readcap(struct scsi_cmnd *, struct sdebug_dev_info *);
 static int resp_read_dt0(struct scsi_cmnd *, struct sdebug_dev_info *);
 static int resp_write_dt0(struct scsi_cmnd *, struct sdebug_dev_info *);
+static int resp_write_scat(struct scsi_cmnd *, struct sdebug_dev_info *);
 static int resp_start_stop(struct scsi_cmnd *, struct sdebug_dev_info *);
 static int resp_readcap16(struct scsi_cmnd *, struct sdebug_dev_info *);
 static int resp_get_lba_status(struct scsi_cmnd *, struct sdebug_dev_info *);
@@ -448,10 +450,13 @@ static const struct opcode_info_t sa_in_iarr[1] = {
 	     0xff, 0xff, 0xff, 0, 0xc7} },
 };
 
-static const struct opcode_info_t vl_iarr[1] = {	/* VARIABLE LENGTH */
+static const struct opcode_info_t vl_iarr[2] = {	/* VARIABLE LENGTH */
 	{0, 0x7f, 0xb, F_SA_HIGH | F_D_OUT | FF_DIRECT_IO, resp_write_dt0,
-	    NULL, {32,  0xc7, 0, 0, 0, 0, 0x1f, 0x18, 0x0, 0xb, 0xfa,
-		   0, 0xff, 0xff, 0xff, 0xff} },	/* WRITE(32) */
+	    NULL, {32,  0xc7, 0, 0, 0, 0, 0x3f, 0x18, 0x0, 0xb, 0xfa,
+		0, 0xff, 0xff, 0xff, 0xff} },	/* WRITE(32) */
+	{0, 0x7f, 0x11, F_SA_HIGH | F_D_OUT | FF_DIRECT_IO, resp_write_scat,
+	    NULL, {32,  0xc7, 0, 0, 0, 0, 0x3f, 0x18, 0x0, 0x11, 0xf8,
+		0, 0xff, 0xff, 0x0, 0x0} },   /* WRITE SCATTERED(32) */
 };
 
 static const struct opcode_info_t maint_in_iarr[2] = {
@@ -518,9 +523,10 @@ static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
 	    {6,  0x1, 0, 0xf, 0xf7, 0xc7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
 	{1, 0x9e, 0x10, F_SA_LOW | F_D_IN, resp_readcap16, sa_in_iarr,
 	    {16,  0x10, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
-	     0xff, 0xff, 0xff, 0x1, 0xc7} },	/* READ CAPACITY(16) */
-	{0, 0, 0, F_INV_OP | FF_RESPOND, NULL, NULL, /* SA OUT */
-	    {0,  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
+	     0xff, 0xff, 0xff, 0x1, 0xc7} },/* SA_IN(16), READ CAPACITY(16) */
+	{0, 0x9f, 0x12, F_SA_LOW | F_D_OUT, resp_write_scat, NULL,
+	    {16,  0x12, 0xf9, 0x0, 0xff, 0xff, 0, 0, 0xff, 0xff, 0xff, 0xff,
+	     0xff, 0xff, 0xff, 0xc7} },  /* SA_OUT(16), WRITE SCATTERED(16) */
 	{2, 0xa3, 0xa, F_SA_LOW | F_D_IN, resp_report_tgtpgs, maint_in_iarr,
 	    {12,  0xea, 0, 0, 0, 0, 0xff, 0xff, 0xff, 0xff, 0, 0xc7, 0, 0, 0,
 	     0} },
@@ -529,9 +535,9 @@ static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
 	{0, 0x2f, 0, F_D_OUT_MAYBE | FF_DIRECT_IO, NULL, NULL, /* VERIFY(10) */
 	    {10,  0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc7,
 	     0, 0, 0, 0, 0, 0} },
-	{1, 0x7f, 0x9, F_SA_HIGH | F_D_IN | FF_DIRECT_IO, resp_read_dt0,
-	    vl_iarr, {32,  0xc7, 0, 0, 0, 0, 0x1f, 0x18, 0x0, 0x9, 0xfe, 0,
-		      0xff, 0xff, 0xff, 0xff} },/* VARIABLE LENGTH, READ(32) */
+	{2, 0x7f, 0x9, F_SA_HIGH | F_D_IN | FF_DIRECT_IO, resp_read_dt0,
+	    vl_iarr, {32,  0xc7, 0, 0, 0, 0, 0x3f, 0x18, 0x0, 0x9, 0xfe, 0,
+		0xff, 0xff, 0xff, 0xff} },    /* VARIABLE LENGTH, READ(32) */
 	{1, 0x56, 0, F_D_OUT, NULL, reserve_iarr, /* RESERVE(10) */
 	    {10,  0xff, 0xff, 0xff, 0, 0, 0, 0xff, 0xff, 0xc7, 0, 0, 0, 0, 0,
 	     0} },
@@ -3030,6 +3036,177 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	return 0;
 }
 
+/*
+ * T10 has only specified WRITE SCATTERED(16) and WRITE SCATTERED(32).
+ * No READ GATHERED yet (requires bidi or long cdb holding gather list).
+ */
+static int resp_write_scat(struct scsi_cmnd *scp,
+			   struct sdebug_dev_info *devip)
+{
+	u8 *cmd = scp->cmnd;
+	u8 *lrdp = NULL;
+	u8 *up;
+	u8 wrprotect;
+	u16 lbdof, num_lrd, k;
+	u32 num, num_by, bt_len, lbdof_blen, sg_off, cum_lb;
+	u32 lb_size = sdebug_sector_size;
+	u32 ei_lba;
+	u64 lba;
+	unsigned long iflags;
+	int ret, res;
+	bool check_prot, is_16;
+	static const u32 lrd_size = 32; /* + parameter list header size */
+
+	if (cmd[0] == VARIABLE_LENGTH_CMD) {
+		is_16 = false;
+		wrprotect = (cmd[10] >> 5) & 0x7;
+		lbdof = get_unaligned_be16(cmd + 12);
+		num_lrd = get_unaligned_be16(cmd + 16);
+		bt_len = get_unaligned_be32(cmd + 28);
+		check_prot = false;
+	} else {        /* that leaves WRITE SCATTERED(16) */
+		is_16 = true;
+		wrprotect = (cmd[2] >> 5) & 0x7;
+		lbdof = get_unaligned_be16(cmd + 4);
+		num_lrd = get_unaligned_be16(cmd + 8);
+		bt_len = get_unaligned_be32(cmd + 10);
+		check_prot = true;
+	}
+	if (unlikely(have_dif_prot && check_prot)) {
+		if (sdebug_dif == T10_PI_TYPE2_PROTECTION && wrprotect) {
+			mk_sense_invalid_opcode(scp);
+			return illegal_condition_result;
+		}
+		if ((sdebug_dif == T10_PI_TYPE1_PROTECTION ||
+		     sdebug_dif == T10_PI_TYPE3_PROTECTION) &&
+		     wrprotect == 0)
+			sdev_printk(KERN_ERR, scp->device,
+				    "Unprotected WR to DIF device\n");
+	}
+	if ((num_lrd == 0) || (bt_len == 0))
+		return 0;       /* T10 says these do-nothings are not errors */
+	if ((lbdof == 0) || (lbdof >= bt_len)) {
+		if (sdebug_verbose)
+			sdev_printk(KERN_INFO, scp->device,
+				"%s: %s: LB Data Offset field bad\n",
+				my_name, __func__);
+		mk_sense_buffer(scp, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB, 0);
+		return illegal_condition_result;
+	}
+	lbdof_blen = lbdof * lb_size;
+	if ((lrd_size + (num_lrd * lrd_size)) > lbdof_blen) {
+		if (sdebug_verbose)
+			sdev_printk(KERN_INFO, scp->device,
+				"%s: %s: LBA range descriptors don't fit\n",
+				my_name, __func__);
+		mk_sense_buffer(scp, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB, 0);
+		return illegal_condition_result;
+	}
+	lrdp = kzalloc(lbdof_blen, GFP_ATOMIC);
+	if (lrdp == NULL) {
+		mk_sense_buffer(scp, ILLEGAL_REQUEST, INSUFF_RES_ASC,
+				INSUFF_RES_ASCQ);
+		return illegal_condition_result;
+	}
+	if (sdebug_verbose)
+		sdev_printk(KERN_INFO, scp->device,
+			"%s: %s: Fetch header+scatter_list, lbdof_blen=%u\n",
+			my_name, __func__, lbdof_blen);
+	res = fetch_to_dev_buffer(scp, lrdp, lbdof_blen);
+	if (res == -1) {
+		ret = DID_ERROR << 16;
+		goto err_out;
+	}
+
+	write_lock_irqsave(&atomic_rw, iflags);
+	sg_off = lbdof_blen;
+	/* Spec says Buffer xfer Length field in number of LBs in dout */
+	cum_lb = 0;
+	for (k = 0, up = lrdp + lrd_size; k < num_lrd; ++k, up += lrd_size) {
+		lba = get_unaligned_be64(up + 0);
+		num = get_unaligned_be32(up + 8);
+		if (sdebug_verbose)
+			sdev_printk(KERN_INFO, scp->device,
+				"%s: %s: k=%d  LBA=0x%llx num=%u  sg_off=%u\n",
+				my_name, __func__, k, lba, num, sg_off);
+		if (num == 0)
+			continue;
+		ret = check_device_access_params(scp, lba, num);
+		if (ret)
+			goto err_out_unlock;
+		num_by = num * lb_size;
+		ei_lba = is_16 ? 0 : get_unaligned_be32(up + 12);
+
+		if ((cum_lb + num) > bt_len) {
+			if (sdebug_verbose)
+				sdev_printk(KERN_INFO, scp->device,
+				    "%s: %s: sum of blocks > data provided\n",
+				    my_name, __func__);
+			mk_sense_buffer(scp, ILLEGAL_REQUEST, WRITE_ERROR_ASC,
+					0);
+			ret = illegal_condition_result;
+			goto err_out_unlock;
+		}
+
+		/* DIX + T10 DIF */
+		if (unlikely(sdebug_dix && scsi_prot_sg_count(scp))) {
+			int prot_ret = prot_verify_write(scp, lba, num,
+							 ei_lba);
+
+			if (prot_ret) {
+				mk_sense_buffer(scp, ILLEGAL_REQUEST, 0x10,
+						prot_ret);
+				ret = illegal_condition_result;
+				goto err_out_unlock;
+			}
+		}
+
+		ret = do_device_access(scp, sg_off, lba, num, true);
+		if (unlikely(scsi_debug_lbp()))
+			map_region(lba, num);
+		if (unlikely(-1 == ret)) {
+			ret = DID_ERROR << 16;
+			goto err_out_unlock;
+		} else if (unlikely(sdebug_verbose && (ret < num_by)))
+			sdev_printk(KERN_INFO, scp->device,
+			    "%s: write: cdb indicated=%u, IO sent=%d bytes\n",
+			    my_name, num_by, ret);
+
+		if (unlikely(sdebug_any_injecting_opt)) {
+			struct sdebug_queued_cmd *sqcp =
+				(struct sdebug_queued_cmd *)scp->host_scribble;
+
+			if (sqcp) {
+				if (sqcp->inj_recovered) {
+					mk_sense_buffer(scp, RECOVERED_ERROR,
+							THRESHOLD_EXCEEDED, 0);
+					ret = illegal_condition_result;
+					goto err_out_unlock;
+				} else if (sqcp->inj_dif) {
+					/* Logical block guard check failed */
+					mk_sense_buffer(scp, ABORTED_COMMAND,
+							0x10, 1);
+					ret = illegal_condition_result;
+					goto err_out_unlock;
+				} else if (sqcp->inj_dix) {
+					mk_sense_buffer(scp, ILLEGAL_REQUEST,
+							0x10, 1);
+					ret = illegal_condition_result;
+					goto err_out_unlock;
+				}
+			}
+		}
+		sg_off += num_by;
+		cum_lb += num;
+	}
+	ret = 0;
+err_out_unlock:
+	write_unlock_irqrestore(&atomic_rw, iflags);
+err_out:
+	kfree(lrdp);
+	return ret;
+}
+
 static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
 			   u32 ei_lba, bool unmap, bool ndob)
 {
-- 
2.14.1

Re: [PATCH 1/4] scsi_debug: tab, kstrto changes, requested by checkpatch.pl

[Bart Van Assche]

On Mon, 2017-12-11 at 20:10 -0500, Douglas Gilbert wrote:
> Some of my development tools tend to add spaces (my preference) rather
> than tabs (kernel convention). Running unexpand to clean these spaces
> up found more of them than checkpatch.pl did. Then checkpatch.pl
> complained about other style violations in those newly tabbed lines.

Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>

Re: [PATCH 2/4] scsi_debug: fix group_number mask 0x1f->0x3f

[Bart Van Assche]

On Mon, 2017-12-11 at 20:10 -0500, Douglas Gilbert wrote:
> Various cdb masks incorrectly assumed the GROUP NUMBER field was
> 5 bits long. It is actually 6 bits long. Correct.

Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>

Re: [PATCH 3/4] scsi_debug: expand do_device_access to take sg offset

[Bart Van Assche]

On Mon, 2017-12-11 at 20:10 -0500, Douglas Gilbert wrote:
> WRITE SCATTERED needs to take several "bites" out of the data-out buffer.
> Expand the do_device_access() function to take a sg_skip argument.

Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>

Re: [PATCH 4/4] scsi_debug: add resp_write_scat function

[Bart Van Assche]

On Mon, 2017-12-11 at 20:10 -0500, Douglas Gilbert wrote:
> -static const struct opcode_info_t vl_iarr[1] = {	/* VARIABLE LENGTH */
> +static const struct opcode_info_t vl_iarr[2] = {	/* VARIABLE LENGTH */

Please leave out the array size and let the compiler determine the array size.

>  	{0, 0x7f, 0xb, F_SA_HIGH | F_D_OUT | FF_DIRECT_IO, resp_write_dt0,
> -	    NULL, {32,  0xc7, 0, 0, 0, 0, 0x1f, 0x18, 0x0, 0xb, 0xfa,
> -		   0, 0xff, 0xff, 0xff, 0xff} },	/* WRITE(32) */
> +	    NULL, {32,  0xc7, 0, 0, 0, 0, 0x3f, 0x18, 0x0, 0xb, 0xfa,
> +		0, 0xff, 0xff, 0xff, 0xff} },	/* WRITE(32) */

Shouldn't this change have been included in the patch that fixes the group
number mask?
 
>  static const struct opcode_info_t maint_in_iarr[2] = {
> @@ -518,9 +523,10 @@ static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
>  	    {6,  0x1, 0, 0xf, 0xf7, 0xc7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
>  	{1, 0x9e, 0x10, F_SA_LOW | F_D_IN, resp_readcap16, sa_in_iarr,
>  	    {16,  0x10, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> -	     0xff, 0xff, 0xff, 0x1, 0xc7} },	/* READ CAPACITY(16) */
> -	{0, 0, 0, F_INV_OP | FF_RESPOND, NULL, NULL, /* SA OUT */
> -	    {0,  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
> +	     0xff, 0xff, 0xff, 0x1, 0xc7} },/* SA_IN(16), READ CAPACITY(16) */

Shouldn't the above change be folded into one of the other patches?

> @@ -529,9 +535,9 @@ static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
>  	{0, 0x2f, 0, F_D_OUT_MAYBE | FF_DIRECT_IO, NULL, NULL, /* VERIFY(10) */
>  	    {10,  0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc7,
>  	     0, 0, 0, 0, 0, 0} },
> -	{1, 0x7f, 0x9, F_SA_HIGH | F_D_IN | FF_DIRECT_IO, resp_read_dt0,
> -	    vl_iarr, {32,  0xc7, 0, 0, 0, 0, 0x1f, 0x18, 0x0, 0x9, 0xfe, 0,
> -		      0xff, 0xff, 0xff, 0xff} },/* VARIABLE LENGTH, READ(32) */
> +	{2, 0x7f, 0x9, F_SA_HIGH | F_D_IN | FF_DIRECT_IO, resp_read_dt0,
> +	    vl_iarr, {32,  0xc7, 0, 0, 0, 0, 0x3f, 0x18, 0x0, 0x9, 0xfe, 0,
> +		0xff, 0xff, 0xff, 0xff} },    /* VARIABLE LENGTH, READ(32) */

Have you considered to use ARRAY_SIZE(vl_iarr) instead of hard-coding the array
size?

> +	if (cmd[0] == VARIABLE_LENGTH_CMD) {
> +		is_16 = false;
> +		wrprotect = (cmd[10] >> 5) & 0x7;
> +		lbdof = get_unaligned_be16(cmd + 12);
> +		num_lrd = get_unaligned_be16(cmd + 16);
> +		bt_len = get_unaligned_be32(cmd + 28);
> +		check_prot = false;
> +	} else {        /* that leaves WRITE SCATTERED(16) */
> +		is_16 = true;
> +		wrprotect = (cmd[2] >> 5) & 0x7;
> +		lbdof = get_unaligned_be16(cmd + 4);
> +		num_lrd = get_unaligned_be16(cmd + 8);
> +		bt_len = get_unaligned_be32(cmd + 10);
> +		check_prot = true;
> +	}

It's not clear to me why check_prot is set to false for WRITE SCATTERED(32)
and set to true for WRITE SCATTERED(16)?

Bart.

Re: [PATCH 4/4] scsi_debug: add resp_write_scat function

[Douglas Gilbert]

On 2017-12-12 02:40 PM, Bart Van Assche wrote:
> On Mon, 2017-12-11 at 20:10 -0500, Douglas Gilbert wrote:
>> -static const struct opcode_info_t vl_iarr[1] = {	/* VARIABLE LENGTH */
>> +static const struct opcode_info_t vl_iarr[2] = {	/* VARIABLE LENGTH */
> 
> Please leave out the array size and let the compiler determine the array size.

I like the "belts and braces" approach. Especially if offsetting an
array element by one position would silently destroy the parser
(which is not the case with vl_iarr but is the case with a few
other arrays).

> 
>>   	{0, 0x7f, 0xb, F_SA_HIGH | F_D_OUT | FF_DIRECT_IO, resp_write_dt0,
>> -	    NULL, {32,  0xc7, 0, 0, 0, 0, 0x1f, 0x18, 0x0, 0xb, 0xfa,
>> -		   0, 0xff, 0xff, 0xff, 0xff} },	/* WRITE(32) */
>> +	    NULL, {32,  0xc7, 0, 0, 0, 0, 0x3f, 0x18, 0x0, 0xb, 0xfa,
>> +		0, 0xff, 0xff, 0xff, 0xff} },	/* WRITE(32) */
> 
> Shouldn't this change have been included in the patch that fixes the group
> number mask?

git add --patch

was used to break up a monolithic patch into 4 parts. However in
the above case it refused to "split" the group_number part (shown)
from the renaming of service actions (not shown above). At that
point I swear at git and move on.

If folks think that hours are spent testing a kernel with 1/4 and
2/4 applied while 3/4 and 4/4 have not been applied then I think
they are dreaming. IMO The split up is eye candy for reviewers.

>>   static const struct opcode_info_t maint_in_iarr[2] = {
>> @@ -518,9 +523,10 @@ static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
>>   	    {6,  0x1, 0, 0xf, 0xf7, 0xc7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
>>   	{1, 0x9e, 0x10, F_SA_LOW | F_D_IN, resp_readcap16, sa_in_iarr,
>>   	    {16,  0x10, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
>> -	     0xff, 0xff, 0xff, 0x1, 0xc7} },	/* READ CAPACITY(16) */
>> -	{0, 0, 0, F_INV_OP | FF_RESPOND, NULL, NULL, /* SA OUT */
>> -	    {0,  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
>> +	     0xff, 0xff, 0xff, 0x1, 0xc7} },/* SA_IN(16), READ CAPACITY(16) */
> 
> Shouldn't the above change be folded into one of the other patches?

I considered the patch adding resp_write_scat to the parsing table
and the splitting SDEB_I_SERV_ACT_IN/OUT to its 12 and 16 byte cdb
components as very closely related, so I put them together.

>> @@ -529,9 +535,9 @@ static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
>>   	{0, 0x2f, 0, F_D_OUT_MAYBE | FF_DIRECT_IO, NULL, NULL, /* VERIFY(10) */
>>   	    {10,  0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc7,
>>   	     0, 0, 0, 0, 0, 0} },
>> -	{1, 0x7f, 0x9, F_SA_HIGH | F_D_IN | FF_DIRECT_IO, resp_read_dt0,
>> -	    vl_iarr, {32,  0xc7, 0, 0, 0, 0, 0x1f, 0x18, 0x0, 0x9, 0xfe, 0,
>> -		      0xff, 0xff, 0xff, 0xff} },/* VARIABLE LENGTH, READ(32) */
>> +	{2, 0x7f, 0x9, F_SA_HIGH | F_D_IN | FF_DIRECT_IO, resp_read_dt0,
>> +	    vl_iarr, {32,  0xc7, 0, 0, 0, 0, 0x3f, 0x18, 0x0, 0x9, 0xfe, 0,
>> +		0xff, 0xff, 0xff, 0xff} },    /* VARIABLE LENGTH, READ(32) */
> 
> Have you considered to use ARRAY_SIZE(vl_iarr) instead of hard-coding the array
> size?

No. It could be done with the advantage of making the code
a bit safer for someone who changes it, but it obfuscates
the number elements in the bucket list (array) making it
harder for the reader of the code (maybe).

>> +	if (cmd[0] == VARIABLE_LENGTH_CMD) {
>> +		is_16 = false;
>> +		wrprotect = (cmd[10] >> 5) & 0x7;
>> +		lbdof = get_unaligned_be16(cmd + 12);
>> +		num_lrd = get_unaligned_be16(cmd + 16);
>> +		bt_len = get_unaligned_be32(cmd + 28);
>> +		check_prot = false;
>> +	} else {        /* that leaves WRITE SCATTERED(16) */
>> +		is_16 = true;
>> +		wrprotect = (cmd[2] >> 5) & 0x7;
>> +		lbdof = get_unaligned_be16(cmd + 4);
>> +		num_lrd = get_unaligned_be16(cmd + 8);
>> +		bt_len = get_unaligned_be32(cmd + 10);
>> +		check_prot = true;
>> +	}
> 
> It's not clear to me why check_prot is set to false for WRITE SCATTERED(32)
> and set to true for WRITE SCATTERED(16)?

Me neither. I was just following what the code for WRITE(16 and 32)
does. My guess is that the code in question is written by Martin P.
who is effectively co-maintainer of this driver having written all
the DIF and DIX stuff. Martin ... ?

Doug Gilbert

Re: [PATCH 4/4] scsi_debug: add resp_write_scat function

[Douglas Gilbert]

v2 coming, addressing some of the points below. Also there is an
issue when fake_rw=1 .

Doug Gilbert

On 2017-12-12 03:47 PM, Douglas Gilbert wrote:
> On 2017-12-12 02:40 PM, Bart Van Assche wrote:
>> On Mon, 2017-12-11 at 20:10 -0500, Douglas Gilbert wrote:
>>> -static const struct opcode_info_t vl_iarr[1] = {    /* VARIABLE LENGTH */
>>> +static const struct opcode_info_t vl_iarr[2] = {    /* VARIABLE LENGTH */
>>
>> Please leave out the array size and let the compiler determine the array size.
> 
> I like the "belts and braces" approach. Especially if offsetting an
> array element by one position would silently destroy the parser
> (which is not the case with vl_iarr but is the case with a few
> other arrays).
> 
>>
>>>       {0, 0x7f, 0xb, F_SA_HIGH | F_D_OUT | FF_DIRECT_IO, resp_write_dt0,
>>> -        NULL, {32,  0xc7, 0, 0, 0, 0, 0x1f, 0x18, 0x0, 0xb, 0xfa,
>>> -           0, 0xff, 0xff, 0xff, 0xff} },    /* WRITE(32) */
>>> +        NULL, {32,  0xc7, 0, 0, 0, 0, 0x3f, 0x18, 0x0, 0xb, 0xfa,
>>> +        0, 0xff, 0xff, 0xff, 0xff} },    /* WRITE(32) */
>>
>> Shouldn't this change have been included in the patch that fixes the group
>> number mask?
> 
> git add --patch
> 
> was used to break up a monolithic patch into 4 parts. However in
> the above case it refused to "split" the group_number part (shown)
> from the renaming of service actions (not shown above). At that
> point I swear at git and move on.
> 
> If folks think that hours are spent testing a kernel with 1/4 and
> 2/4 applied while 3/4 and 4/4 have not been applied then I think
> they are dreaming. IMO The split up is eye candy for reviewers.
> 
>>>   static const struct opcode_info_t maint_in_iarr[2] = {
>>> @@ -518,9 +523,10 @@ static const struct opcode_info_t 
>>> opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
>>>           {6,  0x1, 0, 0xf, 0xf7, 0xc7, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
>>>       {1, 0x9e, 0x10, F_SA_LOW | F_D_IN, resp_readcap16, sa_in_iarr,
>>>           {16,  0x10, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
>>> -         0xff, 0xff, 0xff, 0x1, 0xc7} },    /* READ CAPACITY(16) */
>>> -    {0, 0, 0, F_INV_OP | FF_RESPOND, NULL, NULL, /* SA OUT */
>>> -        {0,  0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} },
>>> +         0xff, 0xff, 0xff, 0x1, 0xc7} },/* SA_IN(16), READ CAPACITY(16) */
>>
>> Shouldn't the above change be folded into one of the other patches?
> 
> I considered the patch adding resp_write_scat to the parsing table
> and the splitting SDEB_I_SERV_ACT_IN/OUT to its 12 and 16 byte cdb
> components as very closely related, so I put them together.
> 
>>> @@ -529,9 +535,9 @@ static const struct opcode_info_t 
>>> opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = {
>>>       {0, 0x2f, 0, F_D_OUT_MAYBE | FF_DIRECT_IO, NULL, NULL, /* VERIFY(10) */
>>>           {10,  0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc7,
>>>            0, 0, 0, 0, 0, 0} },
>>> -    {1, 0x7f, 0x9, F_SA_HIGH | F_D_IN | FF_DIRECT_IO, resp_read_dt0,
>>> -        vl_iarr, {32,  0xc7, 0, 0, 0, 0, 0x1f, 0x18, 0x0, 0x9, 0xfe, 0,
>>> -              0xff, 0xff, 0xff, 0xff} },/* VARIABLE LENGTH, READ(32) */
>>> +    {2, 0x7f, 0x9, F_SA_HIGH | F_D_IN | FF_DIRECT_IO, resp_read_dt0,
>>> +        vl_iarr, {32,  0xc7, 0, 0, 0, 0, 0x3f, 0x18, 0x0, 0x9, 0xfe, 0,
>>> +        0xff, 0xff, 0xff, 0xff} },    /* VARIABLE LENGTH, READ(32) */
>>
>> Have you considered to use ARRAY_SIZE(vl_iarr) instead of hard-coding the array
>> size?
> 
> No. It could be done with the advantage of making the code
> a bit safer for someone who changes it, but it obfuscates
> the number elements in the bucket list (array) making it
> harder for the reader of the code (maybe).
> 
>>> +    if (cmd[0] == VARIABLE_LENGTH_CMD) {
>>> +        is_16 = false;
>>> +        wrprotect = (cmd[10] >> 5) & 0x7;
>>> +        lbdof = get_unaligned_be16(cmd + 12);
>>> +        num_lrd = get_unaligned_be16(cmd + 16);
>>> +        bt_len = get_unaligned_be32(cmd + 28);
>>> +        check_prot = false;
>>> +    } else {        /* that leaves WRITE SCATTERED(16) */
>>> +        is_16 = true;
>>> +        wrprotect = (cmd[2] >> 5) & 0x7;
>>> +        lbdof = get_unaligned_be16(cmd + 4);
>>> +        num_lrd = get_unaligned_be16(cmd + 8);
>>> +        bt_len = get_unaligned_be32(cmd + 10);
>>> +        check_prot = true;
>>> +    }
>>
>> It's not clear to me why check_prot is set to false for WRITE SCATTERED(32)
>> and set to true for WRITE SCATTERED(16)?
> 
> Me neither. I was just following what the code for WRITE(16 and 32)
> does. My guess is that the code in question is written by Martin P.
> who is effectively co-maintainer of this driver having written all
> the DIF and DIX stuff. Martin ... ?
> 
> Doug Gilbert
> 
>