scsi_debug: add write scattered support

PATCH Superseded Douglas Gilbert 2 participants 11 messages Dec 12, 2017 – Dec 13, 2017 SCSI

Share & tools

Douglas Gilbert

author

2017-12-12 01:10 UTC #0 lore raw

cover scsi_debug: add write scattered support

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(-)

Signature · 1 line

2.14.1

Douglas Gilbert

author

2017-12-12 01:10 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/scsi/scsi_debug.c | 174 +++++++++++++++++++++++-----------------------
 1 file changed, 87 insertions(+), 87 deletions(-)

drivers/scsi/scsi_debug.c +87 ~~-87~~

index 6fcc094a2a77..68f4ab8ffbaf 100644
@@ -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,

Signature · 1 line

2.14.1

Bart Van Assche

Reviewed-by on v1 1/4

2017-12-12 18:32 UTC #2 lore raw

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

Quoted 4 lines · 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.

Reviewed-by Bart Van Assche <[email protected]>

Douglas Gilbert

author

2017-12-12 01:10 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/scsi/scsi_debug.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

drivers/scsi/scsi_debug.c +8 -8

index 68f4ab8ffbaf..05661387ccaf 100644
@@ -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 */

Signature · 1 line

2.14.1

Bart Van Assche

Reviewed-by on v1 2/4

2017-12-12 18:34 UTC #4 lore raw

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

Quoted 2 lines · Douglas Gilbert

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 <[email protected]>

Douglas Gilbert

author

2017-12-12 01:10 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/scsi/scsi_debug.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

drivers/scsi/scsi_debug.c +8 -7

index 05661387ccaf..707c24155fd0 100644
@@ -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;

Signature · 1 line

2.14.1

Bart Van Assche

Reviewed-by on v1 3/4

2017-12-12 18:35 UTC #6 lore raw

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

Quoted 2 lines · 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.

Reviewed-by Bart Van Assche <[email protected]>

Douglas Gilbert

author

2017-12-12 01:10 UTC #7 lore raw

PATCH 4/4 scsi_debug: add resp_write_scat function

In reply to 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 <[email protected]>

---
 drivers/scsi/scsi_debug.c | 207 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 192 insertions(+), 15 deletions(-)

drivers/scsi/scsi_debug.c +192 ~~-15~~

index 707c24155fd0..a0f61e7df4b4 100644
@@ -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)
 {

Signature · 1 line

2.14.1

Bart Van Assche

2017-12-12 19:40 UTC #8 lore raw

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

drivers/scsi/scsi_debug.c static const struct opcode_info_t sa_in_iarr[1] = { · line 453

Quoted 2 lines · Douglas Gilbert

drivers/scsi/scsi_debug.c +1 -1

-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.

drivers/scsi/scsi_debug.c static const struct opcode_info_t sa_in_iarr[1] = { · line 456

Quoted 5 lines

drivers/scsi/scsi_debug.c +2 -2

 	{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?

drivers/scsi/scsi_debug.c static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = { · line 526

Quoted 9 lines

static const struct opcode_info_t maint_in_iarr[2] = {

drivers/scsi/scsi_debug.c +1 -3

@@ -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?

drivers/scsi/scsi_debug.c static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = { · line 540

Quoted 10 lines

drivers/scsi/scsi_debug.c +3 -3

@@ -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?

Quoted 5 lines

drivers/scsi/scsi_debug.c +5 -0

+	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);

drivers/scsi/scsi_debug.c static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip) · line 3074

Quoted 10 lines

drivers/scsi/scsi_debug.c +10 -0

+		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.

Douglas Gilbert

author

2017-12-12 20:47 UTC #9 lore raw

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

Quoted 5 lines · Bart Van Assche

drivers/scsi/scsi_debug.c static const struct opcode_info_t sa_in_iarr[1] = { · line 453
Quoted 2 lines · Douglas Gilbert
drivers/scsi/scsi_debug.c +1 -1
-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).

Quoted 9 lines

drivers/scsi/scsi_debug.c static const struct opcode_info_t sa_in_iarr[1] = { · line 456
Quoted 6 lines
{0, 0x7f, 0xb, F_SA_HIGH | F_D_OUT | FF_DIRECT_IO, resp_write_dt0,
drivers/scsi/scsi_debug.c +2 -2
-	    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.

Quoted 11 lines

drivers/scsi/scsi_debug.c static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = { · line 526

Quoted 9 lines

  static const struct opcode_info_t maint_in_iarr[2] = {

drivers/scsi/scsi_debug.c +1 -3

@@ -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.

Quoted 5 lines

drivers/scsi/scsi_debug.c +0 -1

@@ -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,

Quoted 8 lines

drivers/scsi/scsi_debug.c static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = { · line 540
Quoted 5 lines
drivers/scsi/scsi_debug.c +3 -2
-	    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).

Quoted 10 lines

drivers/scsi/scsi_debug.c +10 -0

+	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;

Quoted 8 lines

drivers/scsi/scsi_debug.c static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip) · line 3074
Quoted 5 lines
drivers/scsi/scsi_debug.c +5 -0
+		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

Douglas Gilbert

author

2017-12-13 19:35 UTC #10 lore raw

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

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

Doug Gilbert

Quoted 97 lines · Douglas Gilbert

Quoted 5 lines · Bart Van Assche
drivers/scsi/scsi_debug.c static const struct opcode_info_t sa_in_iarr[1] = { · line 453
Quoted 2 lines · Douglas Gilbert
drivers/scsi/scsi_debug.c +1 -1
-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).
Quoted 9 lines
Quoted 6 lines
{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.
Quoted 4 lines
Quoted 4 lines
static const struct opcode_info_t maint_in_iarr[2] = {
(quoted diff) +0 -0
@@ -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} },
Quoted 8 lines
drivers/scsi/scsi_debug.c static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEMENT + 1] = { · line 526
Quoted 6 lines
drivers/scsi/scsi_debug.c +1 -3
       {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.
Quoted 6 lines
Quoted 6 lines
(quoted diff) +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,
Quoted 8 lines
Quoted 5 lines
-        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).
Quoted 16 lines
Quoted 7 lines
+    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;
Quoted 8 lines
+    } 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;
+    }
Quoted 2 lines
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