[PATCH v2] scsi_debug: Make CRC_T10DIF support optional

[PATCH v2] scsi_debug: Make CRC_T10DIF support optional

[Bart Van Assche]

Not all scsi_debug users need data integrity support. Hence modify the
scsi_debug driver such that it becomes possible to build this driver
without data integrity support. The changes in this patch are as
follows:
- Split the scsi_debug source code into two files without modifying any
  functionality.
- Instead of selecting CRC_T10DIF no matter how the scsi_debug driver is
  built, only select CRC_T10DIF if the scsi_debug driver is built-in to
  the kernel.

Cc: Douglas Gilbert <dgilbert@interlog.com>
Cc: John Garry <john.g.garry@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---

Changes compared with v1: made the patch description more detailed.

 drivers/scsi/Kconfig                          |   2 +-
 drivers/scsi/Makefile                         |   2 +
 drivers/scsi/scsi_debug-dif.h                 |  65 +++++
 drivers/scsi/scsi_debug_dif.c                 | 224 +++++++++++++++
 .../scsi/{scsi_debug.c => scsi_debug_main.c}  | 257 ++----------------
 5 files changed, 308 insertions(+), 242 deletions(-)
 create mode 100644 drivers/scsi/scsi_debug-dif.h
 create mode 100644 drivers/scsi/scsi_debug_dif.c
 rename drivers/scsi/{scsi_debug.c => scsi_debug_main.c} (97%)

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 3328052c8715..b7c92d7af73d 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -1227,7 +1227,7 @@ config SCSI_WD719X
 config SCSI_DEBUG
 	tristate "SCSI debugging host and device simulator"
 	depends on SCSI
-	select CRC_T10DIF
+	select CRC_T10DIF if SCSI_DEBUG = y
 	help
 	  This pseudo driver simulates one or more hosts (SCSI initiators),
 	  each with one or more targets, each with one or more logical units.
diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 1313ddf2fd1a..6287d9d65f04 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -156,6 +156,8 @@ obj-$(CONFIG_SCSI_HISI_SAS) += hisi_sas/
 
 # This goes last, so that "real" scsi devices probe earlier
 obj-$(CONFIG_SCSI_DEBUG)	+= scsi_debug.o
+scsi_debug-y			+= scsi_debug_main.o
+scsi_debug-$(CONFIG_CRC_T10DIF) += scsi_debug_dif.o
 scsi_mod-y			+= scsi.o hosts.o scsi_ioctl.o \
 				   scsicam.o scsi_error.o scsi_lib.o
 scsi_mod-$(CONFIG_SCSI_CONSTANTS) += constants.o
diff --git a/drivers/scsi/scsi_debug-dif.h b/drivers/scsi/scsi_debug-dif.h
new file mode 100644
index 000000000000..d1d9e57b528b
--- /dev/null
+++ b/drivers/scsi/scsi_debug-dif.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _SCSI_DEBUG_DIF_H
+#define _SCSI_DEBUG_DIF_H
+
+#include <linux/kconfig.h>
+#include <linux/types.h>
+#include <linux/spinlock_types.h>
+
+struct scsi_cmnd;
+struct sdebug_dev_info;
+struct t10_pi_tuple;
+
+extern int dix_writes;
+extern int dix_reads;
+extern int dif_errors;
+extern struct xarray *const per_store_ap;
+extern int sdebug_dif;
+extern int sdebug_dix;
+extern unsigned int sdebug_guard;
+extern int sdebug_sector_size;
+extern unsigned int sdebug_store_sectors;
+
+/* There is an xarray of pointers to this struct's objects, one per host */
+struct sdeb_store_info {
+	rwlock_t macc_lck;	/* for atomic media access on this store */
+	u8 *storep;		/* user data storage (ram) */
+	struct t10_pi_tuple *dif_storep; /* protection info */
+	void *map_storep;	/* provisioning map */
+};
+
+struct sdeb_store_info *devip2sip(struct sdebug_dev_info *devip,
+				  bool bug_if_fake_rw);
+
+#if IS_ENABLED(CONFIG_CRC_T10DIF)
+
+int dif_verify(struct t10_pi_tuple *sdt, const void *data, sector_t sector,
+	       u32 ei_lba);
+int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec,
+		     unsigned int sectors, u32 ei_lba);
+int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
+		      unsigned int sectors, u32 ei_lba);
+
+#else /* CONFIG_CRC_T10DIF */
+
+static inline int dif_verify(struct t10_pi_tuple *sdt, const void *data,
+			     sector_t sector, u32 ei_lba)
+{
+	return 0x01; /*GUARD check failed*/
+}
+
+static inline int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec,
+				   unsigned int sectors, u32 ei_lba)
+{
+	return 0x01; /*GUARD check failed*/
+}
+
+static inline int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
+				    unsigned int sectors, u32 ei_lba)
+{
+	return 0x01; /*GUARD check failed*/
+}
+
+#endif /* CONFIG_CRC_T10DIF */
+
+#endif /* _SCSI_DEBUG_DIF_H */
diff --git a/drivers/scsi/scsi_debug_dif.c b/drivers/scsi/scsi_debug_dif.c
new file mode 100644
index 000000000000..a6599d787248
--- /dev/null
+++ b/drivers/scsi/scsi_debug_dif.c
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include "scsi_debug-dif.h"
+#include <linux/crc-t10dif.h>
+#include <linux/t10-pi.h>
+#include <net/checksum.h>
+#include <scsi/scsi_cmnd.h>
+
+static __be16 dif_compute_csum(const void *buf, int len)
+{
+	__be16 csum;
+
+	if (sdebug_guard)
+		csum = (__force __be16)ip_compute_csum(buf, len);
+	else
+		csum = cpu_to_be16(crc_t10dif(buf, len));
+
+	return csum;
+}
+
+int dif_verify(struct t10_pi_tuple *sdt, const void *data, sector_t sector,
+	       u32 ei_lba)
+{
+	__be16 csum = dif_compute_csum(data, sdebug_sector_size);
+
+	if (sdt->guard_tag != csum) {
+		pr_err("GUARD check failed on sector %lu rcvd 0x%04x, data 0x%04x\n",
+			(unsigned long)sector,
+			be16_to_cpu(sdt->guard_tag),
+			be16_to_cpu(csum));
+		return 0x01;
+	}
+	if (sdebug_dif == T10_PI_TYPE1_PROTECTION &&
+	    be32_to_cpu(sdt->ref_tag) != (sector & 0xffffffff)) {
+		pr_err("REF check failed on sector %lu\n",
+			(unsigned long)sector);
+		return 0x03;
+	}
+	if (sdebug_dif == T10_PI_TYPE2_PROTECTION &&
+	    be32_to_cpu(sdt->ref_tag) != ei_lba) {
+		pr_err("REF check failed on sector %lu\n",
+			(unsigned long)sector);
+		return 0x03;
+	}
+	return 0;
+}
+
+static struct t10_pi_tuple *dif_store(struct sdeb_store_info *sip,
+				      sector_t sector)
+{
+	sector = sector_div(sector, sdebug_store_sectors);
+
+	return sip->dif_storep + sector;
+}
+
+static void dif_copy_prot(struct scsi_cmnd *scp, sector_t sector,
+			  unsigned int sectors, bool read)
+{
+	size_t resid;
+	void *paddr;
+	struct sdeb_store_info *sip = devip2sip((struct sdebug_dev_info *)
+						scp->device->hostdata, true);
+	struct t10_pi_tuple *dif_storep = sip->dif_storep;
+	const void *dif_store_end = dif_storep + sdebug_store_sectors;
+	struct sg_mapping_iter miter;
+
+	/* Bytes of protection data to copy into sgl */
+	resid = sectors * sizeof(*dif_storep);
+
+	sg_miter_start(&miter, scsi_prot_sglist(scp),
+		       scsi_prot_sg_count(scp), SG_MITER_ATOMIC |
+		       (read ? SG_MITER_TO_SG : SG_MITER_FROM_SG));
+
+	while (sg_miter_next(&miter) && resid > 0) {
+		size_t len = min_t(size_t, miter.length, resid);
+		void *start = dif_store(sip, sector);
+		size_t rest = 0;
+
+		if (dif_store_end < start + len)
+			rest = start + len - dif_store_end;
+
+		paddr = miter.addr;
+
+		if (read)
+			memcpy(paddr, start, len - rest);
+		else
+			memcpy(start, paddr, len - rest);
+
+		if (rest) {
+			if (read)
+				memcpy(paddr + len - rest, dif_storep, rest);
+			else
+				memcpy(dif_storep, paddr + len - rest, rest);
+		}
+
+		sector += len / sizeof(*dif_storep);
+		resid -= len;
+	}
+	sg_miter_stop(&miter);
+}
+
+static void *lba2fake_store(struct sdeb_store_info *sip,
+			    unsigned long long lba)
+{
+	struct sdeb_store_info *lsip = sip;
+
+	lba = do_div(lba, sdebug_store_sectors);
+	if (!sip || !sip->storep) {
+		WARN_ON_ONCE(true);
+		lsip = xa_load(per_store_ap, 0);  /* should never be NULL */
+	}
+	return lsip->storep + lba * sdebug_sector_size;
+}
+
+int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec,
+		     unsigned int sectors, u32 ei_lba)
+{
+	int ret = 0;
+	unsigned int i;
+	sector_t sector;
+	struct sdeb_store_info *sip = devip2sip((struct sdebug_dev_info *)
+						scp->device->hostdata, true);
+	struct t10_pi_tuple *sdt;
+
+	for (i = 0; i < sectors; i++, ei_lba++) {
+		sector = start_sec + i;
+		sdt = dif_store(sip, sector);
+
+		if (sdt->app_tag == cpu_to_be16(0xffff))
+			continue;
+
+		/*
+		 * Because scsi_debug acts as both initiator and
+		 * target we proceed to verify the PI even if
+		 * RDPROTECT=3. This is done so the "initiator" knows
+		 * which type of error to return. Otherwise we would
+		 * have to iterate over the PI twice.
+		 */
+		if (scp->cmnd[1] >> 5) { /* RDPROTECT */
+			ret = dif_verify(sdt, lba2fake_store(sip, sector),
+					 sector, ei_lba);
+			if (ret) {
+				dif_errors++;
+				break;
+			}
+		}
+	}
+
+	dif_copy_prot(scp, start_sec, sectors, true);
+	dix_reads++;
+
+	return ret;
+}
+
+int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
+		      unsigned int sectors, u32 ei_lba)
+{
+	int ret;
+	struct t10_pi_tuple *sdt;
+	void *daddr;
+	sector_t sector = start_sec;
+	int ppage_offset;
+	int dpage_offset;
+	struct sg_mapping_iter diter;
+	struct sg_mapping_iter piter;
+
+	BUG_ON(scsi_sg_count(SCpnt) == 0);
+	BUG_ON(scsi_prot_sg_count(SCpnt) == 0);
+
+	sg_miter_start(&piter, scsi_prot_sglist(SCpnt),
+			scsi_prot_sg_count(SCpnt),
+			SG_MITER_ATOMIC | SG_MITER_FROM_SG);
+	sg_miter_start(&diter, scsi_sglist(SCpnt), scsi_sg_count(SCpnt),
+			SG_MITER_ATOMIC | SG_MITER_FROM_SG);
+
+	/* For each protection page */
+	while (sg_miter_next(&piter)) {
+		dpage_offset = 0;
+		if (WARN_ON(!sg_miter_next(&diter))) {
+			ret = 0x01;
+			goto out;
+		}
+
+		for (ppage_offset = 0; ppage_offset < piter.length;
+		     ppage_offset += sizeof(struct t10_pi_tuple)) {
+			/* If we're at the end of the current
+			 * data page advance to the next one
+			 */
+			if (dpage_offset >= diter.length) {
+				if (WARN_ON(!sg_miter_next(&diter))) {
+					ret = 0x01;
+					goto out;
+				}
+				dpage_offset = 0;
+			}
+
+			sdt = piter.addr + ppage_offset;
+			daddr = diter.addr + dpage_offset;
+
+			if (SCpnt->cmnd[1] >> 5 != 3) { /* WRPROTECT */
+				ret = dif_verify(sdt, daddr, sector, ei_lba);
+				if (ret)
+					goto out;
+			}
+
+			sector++;
+			ei_lba++;
+			dpage_offset += sdebug_sector_size;
+		}
+		diter.consumed = dpage_offset;
+		sg_miter_stop(&diter);
+	}
+	sg_miter_stop(&piter);
+
+	dif_copy_prot(SCpnt, start_sec, sectors, false);
+	dix_writes++;
+
+	return 0;
+
+out:
+	dif_errors++;
+	sg_miter_stop(&diter);
+	sg_miter_stop(&piter);
+	return ret;
+}
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug_main.c
similarity index 97%
rename from drivers/scsi/scsi_debug.c
rename to drivers/scsi/scsi_debug_main.c
index acf0592d63da..34a7028b2392 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug_main.c
@@ -30,13 +30,11 @@
 #include <linux/moduleparam.h>
 #include <linux/scatterlist.h>
 #include <linux/blkdev.h>
-#include <linux/crc-t10dif.h>
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
 #include <linux/atomic.h>
 #include <linux/hrtimer.h>
 #include <linux/uuid.h>
-#include <linux/t10-pi.h>
 #include <linux/msdos_partition.h>
 #include <linux/random.h>
 #include <linux/xarray.h>
@@ -45,8 +43,6 @@
 #include <linux/async.h>
 #include <linux/cleanup.h>
 
-#include <net/checksum.h>
-
 #include <asm/unaligned.h>
 
 #include <scsi/scsi.h>
@@ -59,6 +55,7 @@
 #include <scsi/scsi_dbg.h>
 
 #include "sd.h"
+#include "scsi_debug-dif.h"
 #include "scsi_logging.h"
 
 /* make sure inq_product_rev string corresponds to this version */
@@ -372,14 +369,6 @@ struct sdebug_host_info {
 	struct list_head dev_info_list;
 };
 
-/* There is an xarray of pointers to this struct's objects, one per host */
-struct sdeb_store_info {
-	rwlock_t macc_lck;	/* for atomic media access on this store */
-	u8 *storep;		/* user data storage (ram) */
-	struct t10_pi_tuple *dif_storep; /* protection info */
-	void *map_storep;	/* provisioning map */
-};
-
 #define dev_to_sdebug_host(d)	\
 	container_of(d, struct sdebug_host_info, dev)
 
@@ -799,12 +788,12 @@ static int sdebug_ato = DEF_ATO;
 static int sdebug_cdb_len = DEF_CDB_LEN;
 static int sdebug_jdelay = DEF_JDELAY;	/* if > 0 then unit is jiffies */
 static int sdebug_dev_size_mb = DEF_DEV_SIZE_PRE_INIT;
-static int sdebug_dif = DEF_DIF;
-static int sdebug_dix = DEF_DIX;
+int sdebug_dif = DEF_DIF;
+int sdebug_dix = DEF_DIX;
 static int sdebug_dsense = DEF_D_SENSE;
 static int sdebug_every_nth = DEF_EVERY_NTH;
 static int sdebug_fake_rw = DEF_FAKE_RW;
-static unsigned int sdebug_guard = DEF_GUARD;
+unsigned int sdebug_guard = DEF_GUARD;
 static int sdebug_host_max_queue;	/* per host */
 static int sdebug_lowest_aligned = DEF_LOWEST_ALIGNED;
 static int sdebug_max_luns = DEF_MAX_LUNS;
@@ -822,7 +811,7 @@ static int sdebug_physblk_exp = DEF_PHYSBLK_EXP;
 static int sdebug_opt_xferlen_exp = DEF_OPT_XFERLEN_EXP;
 static int sdebug_ptype = DEF_PTYPE; /* SCSI peripheral device type */
 static int sdebug_scsi_level = DEF_SCSI_LEVEL;
-static int sdebug_sector_size = DEF_SECTOR_SIZE;
+int sdebug_sector_size = DEF_SECTOR_SIZE;
 static int sdeb_tur_ms_to_ready = DEF_TUR_MS_TO_READY;
 static int sdebug_virtual_gb = DEF_VIRTUAL_GB;
 static int sdebug_vpd_use_hostno = DEF_VPD_USE_HOSTNO;
@@ -864,7 +853,7 @@ enum sam_lun_addr_method {SAM_LUN_AM_PERIPHERAL = 0x0,
 static enum sam_lun_addr_method sdebug_lun_am = SAM_LUN_AM_PERIPHERAL;
 static int sdebug_lun_am_i = (int)SAM_LUN_AM_PERIPHERAL;
 
-static unsigned int sdebug_store_sectors;
+unsigned int sdebug_store_sectors;
 static sector_t sdebug_capacity;	/* in sectors */
 
 /* old BIOS stuff, kernel may get rid of them but some mode sense pages
@@ -877,7 +866,7 @@ static LIST_HEAD(sdebug_host_list);
 static DEFINE_MUTEX(sdebug_host_list_mutex);
 
 static struct xarray per_store_arr;
-static struct xarray *per_store_ap = &per_store_arr;
+struct xarray *const per_store_ap = &per_store_arr;
 static int sdeb_first_idx = -1;		/* invalid index ==> none created */
 static int sdeb_most_recent_idx = -1;
 static DEFINE_RWLOCK(sdeb_fake_rw_lck);	/* need a RW lock when fake_rw=1 */
@@ -888,9 +877,9 @@ static int num_dev_resets;
 static int num_target_resets;
 static int num_bus_resets;
 static int num_host_resets;
-static int dix_writes;
-static int dix_reads;
-static int dif_errors;
+int dix_writes;
+int dix_reads;
+int dif_errors;
 
 /* ZBC global data */
 static bool sdeb_zbc_in_use;	/* true for host-aware and host-managed disks */
@@ -1188,27 +1177,6 @@ static inline bool scsi_debug_lbp(void)
 		(sdebug_lbpu || sdebug_lbpws || sdebug_lbpws10);
 }
 
-static void *lba2fake_store(struct sdeb_store_info *sip,
-			    unsigned long long lba)
-{
-	struct sdeb_store_info *lsip = sip;
-
-	lba = do_div(lba, sdebug_store_sectors);
-	if (!sip || !sip->storep) {
-		WARN_ON_ONCE(true);
-		lsip = xa_load(per_store_ap, 0);  /* should never be NULL */
-	}
-	return lsip->storep + lba * sdebug_sector_size;
-}
-
-static struct t10_pi_tuple *dif_store(struct sdeb_store_info *sip,
-				      sector_t sector)
-{
-	sector = sector_div(sector, sdebug_store_sectors);
-
-	return sip->dif_storep + sector;
-}
-
 static void sdebug_max_tgts_luns(void)
 {
 	struct sdebug_host_info *sdbg_host;
@@ -3367,8 +3335,8 @@ static inline int check_device_access_params
  * that access any of the "stores" in struct sdeb_store_info should call this
  * function with bug_if_fake_rw set to true.
  */
-static inline struct sdeb_store_info *devip2sip(struct sdebug_dev_info *devip,
-						bool bug_if_fake_rw)
+struct sdeb_store_info *devip2sip(struct sdebug_dev_info *devip,
+				  bool bug_if_fake_rw)
 {
 	if (sdebug_fake_rw) {
 		BUG_ON(bug_if_fake_rw);	/* See note above */
@@ -3471,131 +3439,6 @@ static bool comp_write_worker(struct sdeb_store_info *sip, u64 lba, u32 num,
 	return res;
 }
 
-static __be16 dif_compute_csum(const void *buf, int len)
-{
-	__be16 csum;
-
-	if (sdebug_guard)
-		csum = (__force __be16)ip_compute_csum(buf, len);
-	else
-		csum = cpu_to_be16(crc_t10dif(buf, len));
-
-	return csum;
-}
-
-static int dif_verify(struct t10_pi_tuple *sdt, const void *data,
-		      sector_t sector, u32 ei_lba)
-{
-	__be16 csum = dif_compute_csum(data, sdebug_sector_size);
-
-	if (sdt->guard_tag != csum) {
-		pr_err("GUARD check failed on sector %lu rcvd 0x%04x, data 0x%04x\n",
-			(unsigned long)sector,
-			be16_to_cpu(sdt->guard_tag),
-			be16_to_cpu(csum));
-		return 0x01;
-	}
-	if (sdebug_dif == T10_PI_TYPE1_PROTECTION &&
-	    be32_to_cpu(sdt->ref_tag) != (sector & 0xffffffff)) {
-		pr_err("REF check failed on sector %lu\n",
-			(unsigned long)sector);
-		return 0x03;
-	}
-	if (sdebug_dif == T10_PI_TYPE2_PROTECTION &&
-	    be32_to_cpu(sdt->ref_tag) != ei_lba) {
-		pr_err("REF check failed on sector %lu\n",
-			(unsigned long)sector);
-		return 0x03;
-	}
-	return 0;
-}
-
-static void dif_copy_prot(struct scsi_cmnd *scp, sector_t sector,
-			  unsigned int sectors, bool read)
-{
-	size_t resid;
-	void *paddr;
-	struct sdeb_store_info *sip = devip2sip((struct sdebug_dev_info *)
-						scp->device->hostdata, true);
-	struct t10_pi_tuple *dif_storep = sip->dif_storep;
-	const void *dif_store_end = dif_storep + sdebug_store_sectors;
-	struct sg_mapping_iter miter;
-
-	/* Bytes of protection data to copy into sgl */
-	resid = sectors * sizeof(*dif_storep);
-
-	sg_miter_start(&miter, scsi_prot_sglist(scp),
-		       scsi_prot_sg_count(scp), SG_MITER_ATOMIC |
-		       (read ? SG_MITER_TO_SG : SG_MITER_FROM_SG));
-
-	while (sg_miter_next(&miter) && resid > 0) {
-		size_t len = min_t(size_t, miter.length, resid);
-		void *start = dif_store(sip, sector);
-		size_t rest = 0;
-
-		if (dif_store_end < start + len)
-			rest = start + len - dif_store_end;
-
-		paddr = miter.addr;
-
-		if (read)
-			memcpy(paddr, start, len - rest);
-		else
-			memcpy(start, paddr, len - rest);
-
-		if (rest) {
-			if (read)
-				memcpy(paddr + len - rest, dif_storep, rest);
-			else
-				memcpy(dif_storep, paddr + len - rest, rest);
-		}
-
-		sector += len / sizeof(*dif_storep);
-		resid -= len;
-	}
-	sg_miter_stop(&miter);
-}
-
-static int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec,
-			    unsigned int sectors, u32 ei_lba)
-{
-	int ret = 0;
-	unsigned int i;
-	sector_t sector;
-	struct sdeb_store_info *sip = devip2sip((struct sdebug_dev_info *)
-						scp->device->hostdata, true);
-	struct t10_pi_tuple *sdt;
-
-	for (i = 0; i < sectors; i++, ei_lba++) {
-		sector = start_sec + i;
-		sdt = dif_store(sip, sector);
-
-		if (sdt->app_tag == cpu_to_be16(0xffff))
-			continue;
-
-		/*
-		 * Because scsi_debug acts as both initiator and
-		 * target we proceed to verify the PI even if
-		 * RDPROTECT=3. This is done so the "initiator" knows
-		 * which type of error to return. Otherwise we would
-		 * have to iterate over the PI twice.
-		 */
-		if (scp->cmnd[1] >> 5) { /* RDPROTECT */
-			ret = dif_verify(sdt, lba2fake_store(sip, sector),
-					 sector, ei_lba);
-			if (ret) {
-				dif_errors++;
-				break;
-			}
-		}
-	}
-
-	dif_copy_prot(scp, start_sec, sectors, true);
-	dix_reads++;
-
-	return ret;
-}
-
 static inline void
 sdeb_read_lock(struct sdeb_store_info *sip)
 {
@@ -3803,78 +3646,6 @@ static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 	return 0;
 }
 
-static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
-			     unsigned int sectors, u32 ei_lba)
-{
-	int ret;
-	struct t10_pi_tuple *sdt;
-	void *daddr;
-	sector_t sector = start_sec;
-	int ppage_offset;
-	int dpage_offset;
-	struct sg_mapping_iter diter;
-	struct sg_mapping_iter piter;
-
-	BUG_ON(scsi_sg_count(SCpnt) == 0);
-	BUG_ON(scsi_prot_sg_count(SCpnt) == 0);
-
-	sg_miter_start(&piter, scsi_prot_sglist(SCpnt),
-			scsi_prot_sg_count(SCpnt),
-			SG_MITER_ATOMIC | SG_MITER_FROM_SG);
-	sg_miter_start(&diter, scsi_sglist(SCpnt), scsi_sg_count(SCpnt),
-			SG_MITER_ATOMIC | SG_MITER_FROM_SG);
-
-	/* For each protection page */
-	while (sg_miter_next(&piter)) {
-		dpage_offset = 0;
-		if (WARN_ON(!sg_miter_next(&diter))) {
-			ret = 0x01;
-			goto out;
-		}
-
-		for (ppage_offset = 0; ppage_offset < piter.length;
-		     ppage_offset += sizeof(struct t10_pi_tuple)) {
-			/* If we're at the end of the current
-			 * data page advance to the next one
-			 */
-			if (dpage_offset >= diter.length) {
-				if (WARN_ON(!sg_miter_next(&diter))) {
-					ret = 0x01;
-					goto out;
-				}
-				dpage_offset = 0;
-			}
-
-			sdt = piter.addr + ppage_offset;
-			daddr = diter.addr + dpage_offset;
-
-			if (SCpnt->cmnd[1] >> 5 != 3) { /* WRPROTECT */
-				ret = dif_verify(sdt, daddr, sector, ei_lba);
-				if (ret)
-					goto out;
-			}
-
-			sector++;
-			ei_lba++;
-			dpage_offset += sdebug_sector_size;
-		}
-		diter.consumed = dpage_offset;
-		sg_miter_stop(&diter);
-	}
-	sg_miter_stop(&piter);
-
-	dif_copy_prot(SCpnt, start_sec, sectors, false);
-	dix_writes++;
-
-	return 0;
-
-out:
-	dif_errors++;
-	sg_miter_stop(&diter);
-	sg_miter_stop(&piter);
-	return ret;
-}
-
 static unsigned long lba_to_map_index(sector_t lba)
 {
 	if (sdebug_unmap_alignment)
@@ -6266,8 +6037,10 @@ module_param_named(cdb_len, sdebug_cdb_len, int, 0644);
 module_param_named(clustering, sdebug_clustering, bool, S_IRUGO | S_IWUSR);
 module_param_named(delay, sdebug_jdelay, int, S_IRUGO | S_IWUSR);
 module_param_named(dev_size_mb, sdebug_dev_size_mb, int, S_IRUGO);
+#ifdef CONFIG_CRC_T10DIF
 module_param_named(dif, sdebug_dif, int, S_IRUGO);
 module_param_named(dix, sdebug_dix, int, S_IRUGO);
+#endif
 module_param_named(dsense, sdebug_dsense, int, S_IRUGO | S_IWUSR);
 module_param_named(every_nth, sdebug_every_nth, int, S_IRUGO | S_IWUSR);
 module_param_named(fake_rw, sdebug_fake_rw, int, S_IRUGO | S_IWUSR);
@@ -6343,8 +6116,10 @@ MODULE_PARM_DESC(cdb_len, "suggest CDB lengths to drivers (def=10)");
 MODULE_PARM_DESC(clustering, "when set enables larger transfers (def=0)");
 MODULE_PARM_DESC(delay, "response delay (def=1 jiffy); 0:imm, -1,-2:tiny");
 MODULE_PARM_DESC(dev_size_mb, "size in MiB of ram shared by devs(def=8)");
+#ifdef CONFIG_CRC_T10DIF
 MODULE_PARM_DESC(dif, "data integrity field type: 0-3 (def=0)");
 MODULE_PARM_DESC(dix, "data integrity extensions mask (def=0)");
+#endif
 MODULE_PARM_DESC(dsense, "use descriptor sense format(def=0 -> fixed)");
 MODULE_PARM_DESC(every_nth, "timeout every nth command(def=0)");
 MODULE_PARM_DESC(fake_rw, "fake reads/writes instead of copying (def=0)");

Re: [PATCH v2] scsi_debug: Make CRC_T10DIF support optional

[John Garry]

On 05/03/2024 22:25, Bart Van Assche wrote:
> Not all scsi_debug users need data integrity support. Hence modify the
> scsi_debug driver such that it becomes possible to build this driver
> without data integrity support. The changes in this patch are as
> follows:
> - Split the scsi_debug source code into two files without modifying any
>    functionality.
> - Instead of selecting CRC_T10DIF no matter how the scsi_debug driver is
>    built, only select CRC_T10DIF if the scsi_debug driver is built-in to
>    the kernel.
> 
> Cc: Douglas Gilbert <dgilbert@interlog.com>
> Cc: John Garry <john.g.garry@oracle.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

> ---
> 
> Changes compared with v1: made the patch description more detailed.
> 
>   drivers/scsi/Kconfig                          |   2 +-
>   drivers/scsi/Makefile                         |   2 +
>   drivers/scsi/scsi_debug-dif.h                 |  65 +++++
>   drivers/scsi/scsi_debug_dif.c                 | 224 +++++++++++++++

inconsistent filename format: scsi_debug-dif.c vs scsi_debug_dif.h - is 
that intentional?

>   .../scsi/{scsi_debug.c => scsi_debug_main.c}  | 257 ++----------------
>   5 files changed, 308 insertions(+), 242 deletions(-)
>   create mode 100644 drivers/scsi/scsi_debug-dif.h
>   create mode 100644 drivers/scsi/scsi_debug_dif.c
>   rename drivers/scsi/{scsi_debug.c => scsi_debug_main.c} (97%)
> 
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index 3328052c8715..b7c92d7af73d 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -1227,7 +1227,7 @@ config SCSI_WD719X
>   config SCSI_DEBUG
>   	tristate "SCSI debugging host and device simulator"
>   	depends on SCSI
> -	select CRC_T10DIF
> +	select CRC_T10DIF if SCSI_DEBUG = y

Do we really need to select at all now? What does this buy us? 
Preference is generally not to use select.

>   	help
>   	  This pseudo driver simulates one or more hosts (SCSI initiators),
>   	  each with one or more targets, each with one or more logical units.
> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
> index 1313ddf2fd1a..6287d9d65f04 100644
> --- a/drivers/scsi/Makefile
> +++ b/drivers/scsi/Makefile
> @@ -156,6 +156,8 @@ obj-$(CONFIG_SCSI_HISI_SAS) += hisi_sas/
>   
>   # This goes last, so that "real" scsi devices probe earlier
>   obj-$(CONFIG_SCSI_DEBUG)	+= scsi_debug.o
> +scsi_debug-y			+= scsi_debug_main.o
> +scsi_debug-$(CONFIG_CRC_T10DIF) += scsi_debug_dif.o
>   scsi_mod-y			+= scsi.o hosts.o scsi_ioctl.o \
>   				   scsicam.o scsi_error.o scsi_lib.o
>   scsi_mod-$(CONFIG_SCSI_CONSTANTS) += constants.o
> diff --git a/drivers/scsi/scsi_debug-dif.h b/drivers/scsi/scsi_debug-dif.h
> new file mode 100644
> index 000000000000..d1d9e57b528b
> --- /dev/null
> +++ b/drivers/scsi/scsi_debug-dif.h
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _SCSI_DEBUG_DIF_H
> +#define _SCSI_DEBUG_DIF_H
> +
> +#include <linux/kconfig.h>
> +#include <linux/types.h>
> +#include <linux/spinlock_types.h>
> +
> +struct scsi_cmnd;

Do you really need to have a prototype for this? I'm a bit in shock 
seeing this in a scsi low-level driver.

> +struct sdebug_dev_info;

How is this specific to dif? This should be defined in a common header 
file if used by both scsi_debug_main.c and scsi_debug_dif.c

> +struct t10_pi_tuple;
> +
> +extern int dix_writes;

For whos benefit is this in a dif header file?

dix_writes is defined in main.c, so surely this extern needs to be in 
scsi_debug_dif.c or a common header

For me, I would actually just declare this in scsi_debug_dif.c and have 
scsi_debug_dif_get_dix_writes() or similar to return this value. This 
function would be stubbed for CONFIG_CRC_T10DIF=n

> +extern int dix_reads;
> +extern int dif_errors;
> +extern struct xarray *const per_store_ap;
> +extern int sdebug_dif;
> +extern int sdebug_dix;
> +extern unsigned int sdebug_guard;
> +extern int sdebug_sector_size;
> +extern unsigned int sdebug_store_sectors;

I doubt why all these are here

> +
> +/* There is an xarray of pointers to this struct's objects, one per host */
> +struct sdeb_store_info {

this is not specific to dif, yet in a dif header

> +	rwlock_t macc_lck;	/* for atomic media access on this store */
> +	u8 *storep;		/* user data storage (ram) */
> +	struct t10_pi_tuple *dif_storep; /* protection info */
> +	void *map_storep;	/* provisioning map */
> +};
> +
> +struct sdeb_store_info *devip2sip(struct sdebug_dev_info *devip,
> +				  bool bug_if_fake_rw);
> +
> +#if IS_ENABLED(CONFIG_CRC_T10DIF)
> +
> +int dif_verify(struct t10_pi_tuple *sdt, const void *data, sector_t sector,
> +	       u32 ei_lba);

Is this actually used in main.c?

I do also notice that we have chunks of code in main.c that does PI 
checking, like in resp_write_scat() - surely dif stuff should be in 
dif.c now

> +int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec,
> +		     unsigned int sectors, u32 ei_lba);
> +int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
> +		      unsigned int sectors, u32 ei_lba);
> +
> +#else /* CONFIG_CRC_T10DIF */
> +
> +static inline int dif_verify(struct t10_pi_tuple *sdt, const void *data,
> +			     sector_t sector, u32 ei_lba)
> +{
> +	return 0x01; /*GUARD check failed*/

leave space before and after /* and */

> +}
> +
> +static inline int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec,
> +				   unsigned int sectors, u32 ei_lba)
> +{
> +	return 0x01; /*GUARD check failed*/
> +}
> +
> +static inline int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
> +				    unsigned int sectors, u32 ei_lba)
> +{
> +	return 0x01; /*GUARD check failed*/
> +}
> +
> +#endif /* CONFIG_CRC_T10DIF */
> +
> +#endif /* _SCSI_DEBUG_DIF_H */
> diff --git a/drivers/scsi/scsi_debug_dif.c b/drivers/scsi/scsi_debug_dif.c
> new file mode 100644
> index 000000000000..a6599d787248
> --- /dev/null
> +++ b/drivers/scsi/scsi_debug_dif.c
> @@ -0,0 +1,224 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include "scsi_debug-dif.h"

I always find it is strange to include driver proprietary header files 
before common header files - I guess that is why the scsi_cmnd prototype 
is declared, above

> +#include <linux/crc-t10dif.h>
> +#include <linux/t10-pi.h>
> +#include <net/checksum.h>
> +#include <scsi/scsi_cmnd.h>
> +

Thanks,
John

Re: [PATCH v2] scsi_debug: Make CRC_T10DIF support optional

[Bart Van Assche]

On 3/6/24 05:19, John Garry wrote:
> On 05/03/2024 22:25, Bart Van Assche wrote:
>>   drivers/scsi/Kconfig                          |   2 +-
>>   drivers/scsi/Makefile                         |   2 +
>>   drivers/scsi/scsi_debug-dif.h                 |  65 +++++
>>   drivers/scsi/scsi_debug_dif.c                 | 224 +++++++++++++++
> 
> inconsistent filename format: scsi_debug-dif.c vs scsi_debug_dif.h - is 
> that intentional?

That should be fixed. Do you perhaps have a preference?

>>   .../scsi/{scsi_debug.c => scsi_debug_main.c}  | 257 ++----------------
>>   5 files changed, 308 insertions(+), 242 deletions(-)
>>   create mode 100644 drivers/scsi/scsi_debug-dif.h
>>   create mode 100644 drivers/scsi/scsi_debug_dif.c
>>   rename drivers/scsi/{scsi_debug.c => scsi_debug_main.c} (97%)
>>
>> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
>> index 3328052c8715..b7c92d7af73d 100644
>> --- a/drivers/scsi/Kconfig
>> +++ b/drivers/scsi/Kconfig
>> @@ -1227,7 +1227,7 @@ config SCSI_WD719X
>>   config SCSI_DEBUG
>>       tristate "SCSI debugging host and device simulator"
>>       depends on SCSI
>> -    select CRC_T10DIF
>> +    select CRC_T10DIF if SCSI_DEBUG = y
> 
> Do we really need to select at all now? What does this buy us? 
> Preference is generally not to use select.

I want to exclude the CRC_T10DIF = m and SCSI_DEBUG = y combination
because it causes the build to fail. I will leave out the select
statement and will change "depends on SCSI" into the following:

	depends on SCSI && (m || CRC_T10DIF = y)

>> --- /dev/null
>> +++ b/drivers/scsi/scsi_debug-dif.h
>> @@ -0,0 +1,65 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _SCSI_DEBUG_DIF_H
>> +#define _SCSI_DEBUG_DIF_H
>> +
>> +#include <linux/kconfig.h>
>> +#include <linux/types.h>
>> +#include <linux/spinlock_types.h>
>> +
>> +struct scsi_cmnd;
> 
> Do you really need to have a prototype for this? I'm a bit in shock 
> seeing this in a scsi low-level driver.

I will leave the above out and add "#include <scsi/scsi_cmnd.h>"
instead.

> dix_writes is defined in main.c, so surely this extern needs to be in 
> scsi_debug_dif.c or a common header

The scsi_debug-dif.h header file is included from two .c files. Without
this header file, the compiler wouldn't be able to check the consistency
of the declarations in scsi_debug-dif.h with the definitions in the two
scsi_debug .c files.

> For me, I would actually just declare this in scsi_debug_dif.c and have 
> scsi_debug_dif_get_dix_writes() or similar to return this value. This 
> function would be stubbed for CONFIG_CRC_T10DIF=n

My goal is to minimize changes while splitting the scsi_debug source
code. Hence the "extern" declaration.

>> +extern int dix_reads;
>> +extern int dif_errors;
>> +extern struct xarray *const per_store_ap;
>> +extern int sdebug_dif;
>> +extern int sdebug_dix;
>> +extern unsigned int sdebug_guard;
>> +extern int sdebug_sector_size;
>> +extern unsigned int sdebug_store_sectors;
> 
> I doubt why all these are here

All the variables declared above are used in both scsi_debug_dif.c and
scsi_debug_main.c.

>> +int dif_verify(struct t10_pi_tuple *sdt, const void *data, sector_t 
>> sector,
>> +           u32 ei_lba);
> 
> Is this actually used in main.c?

It is not. I will remove it.

> I do also notice that we have chunks of code in main.c that does PI 
> checking, like in resp_write_scat() - surely dif stuff should be in 
> dif.c now

I'm concerned that moving that code would make resp_write_scat() much
less readable. Please note that the code in resp_write_scat() that does
PI checking is guarded by an 'if (have_dif_prot)' check and that
have_dif_prot = false if CONFIG_CRC_T10DIF=n.

>> --- /dev/null
>> +++ b/drivers/scsi/scsi_debug_dif.c
>> @@ -0,0 +1,224 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +#include "scsi_debug-dif.h"
> 
> I always find it is strange to include driver proprietary header files 
> before common header files - I guess that is why the scsi_cmnd prototype 
> is declared, above

Including driver-private header files first is required by some coding
style guides because it causes the build to fail if the driver-private
header file does not include all include files it should include. See
also
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
(I am aware that this style guide does not apply to Linux kernel code).

Thanks,

Bart.

Re: [PATCH v2] scsi_debug: Make CRC_T10DIF support optional

[John Garry]

On 06/03/2024 18:27, Bart Van Assche wrote:

As an alternative, what about something like this:

----->8----
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 3328052c8715..3120e951f5d2 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -1227,7 +1227,6 @@ config SCSI_WD719X
  config SCSI_DEBUG
  	tristate "SCSI debugging host and device simulator"
  	depends on SCSI
-	select CRC_T10DIF
  	help
  	  This pseudo driver simulates one or more hosts (SCSI initiators),
  	  each with one or more targets, each with one or more logical units.
diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index acf0592d63da..5bac3b5aa5fa 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3471,6 +3471,7 @@ static bool comp_write_worker(struct 
sdeb_store_info *sip, u64 lba, u32 num,
  	return res;
  }

+#if (IS_ENABLED(CONFIG_CRC_T10DIF))
  static __be16 dif_compute_csum(const void *buf, int len)
  {
  	__be16 csum;
@@ -3509,6 +3510,13 @@ static int dif_verify(struct t10_pi_tuple *sdt, 
const void *data,
  	}
  	return 0;
  }
+#else
+static int dif_verify(struct t10_pi_tuple *sdt, const void *data,
+		      sector_t sector, u32 ei_lba)
+{
+	return -EOPNOTSUPP;
+}
+#endif

  static void dif_copy_prot(struct scsi_cmnd *scp, sector_t sector,
  			  unsigned int sectors, bool read)
@@ -7421,7 +7429,12 @@ static int __init scsi_debug_init(void)
  	case T10_PI_TYPE1_PROTECTION:
  	case T10_PI_TYPE2_PROTECTION:
  	case T10_PI_TYPE3_PROTECTION:
+		#if (IS_ENABLED(CONFIG_CRC_T10DIF))
  		have_dif_prot = true;
+		#else
+		pr_err("CRC_T10DIF not enabled\n");
+		return -EINVAL;
+		#endif
  		break;

  	default:
----8<-----

I know that IS_ENABLED(CONFIG_XXX)) ain't too nice to use, but this is a 
lot less change and we also don't need multiple files for the driver. As 
below, it's not easy to separate the CRC_T10DIF-related stuff out.

Thanks,
John

EOM

> On 3/6/24 05:19, John Garry wrote:
>> On 05/03/2024 22:25, Bart Van Assche wrote:
>>>   drivers/scsi/Kconfig                          |   2 +-
>>>   drivers/scsi/Makefile                         |   2 +
>>>   drivers/scsi/scsi_debug-dif.h                 |  65 +++++
>>>   drivers/scsi/scsi_debug_dif.c                 | 224 +++++++++++++++
>>
>> inconsistent filename format: scsi_debug-dif.c vs scsi_debug_dif.h - 
>> is that intentional?
> 
> That should be fixed. Do you perhaps have a preference?
> 
>>>   .../scsi/{scsi_debug.c => scsi_debug_main.c}  | 257 ++----------------
>>>   5 files changed, 308 insertions(+), 242 deletions(-)
>>>   create mode 100644 drivers/scsi/scsi_debug-dif.h
>>>   create mode 100644 drivers/scsi/scsi_debug_dif.c
>>>   rename drivers/scsi/{scsi_debug.c => scsi_debug_main.c} (97%)
>>>
>>> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
>>> index 3328052c8715..b7c92d7af73d 100644
>>> --- a/drivers/scsi/Kconfig
>>> +++ b/drivers/scsi/Kconfig
>>> @@ -1227,7 +1227,7 @@ config SCSI_WD719X
>>>   config SCSI_DEBUG
>>>       tristate "SCSI debugging host and device simulator"
>>>       depends on SCSI
>>> -    select CRC_T10DIF
>>> +    select CRC_T10DIF if SCSI_DEBUG = y
>>
>> Do we really need to select at all now? What does this buy us? 
>> Preference is generally not to use select.
> 
> I want to exclude the CRC_T10DIF = m and SCSI_DEBUG = y combination
> because it causes the build to fail. I will leave out the select
> statement and will change "depends on SCSI" into the following:
> 
>      depends on SCSI && (m || CRC_T10DIF = y)
> 
>>> --- /dev/null
>>> +++ b/drivers/scsi/scsi_debug-dif.h
>>> @@ -0,0 +1,65 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef _SCSI_DEBUG_DIF_H
>>> +#define _SCSI_DEBUG_DIF_H
>>> +
>>> +#include <linux/kconfig.h>
>>> +#include <linux/types.h>
>>> +#include <linux/spinlock_types.h>
>>> +
>>> +struct scsi_cmnd;
>>
>> Do you really need to have a prototype for this? I'm a bit in shock 
>> seeing this in a scsi low-level driver.
> 
> I will leave the above out and add "#include <scsi/scsi_cmnd.h>"
> instead.
> 
>> dix_writes is defined in main.c, so surely this extern needs to be in 
>> scsi_debug_dif.c or a common header
> 
> The scsi_debug-dif.h header file is included from two .c files. Without
> this header file, the compiler wouldn't be able to check the consistency
> of the declarations in scsi_debug-dif.h with the definitions in the two
> scsi_debug .c files.
> 
>> For me, I would actually just declare this in scsi_debug_dif.c and 
>> have scsi_debug_dif_get_dix_writes() or similar to return this value. 
>> This function would be stubbed for CONFIG_CRC_T10DIF=n
> 
> My goal is to minimize changes while splitting the scsi_debug source
> code. Hence the "extern" declaration.
> 
>>> +extern int dix_reads;
>>> +extern int dif_errors;
>>> +extern struct xarray *const per_store_ap;
>>> +extern int sdebug_dif;
>>> +extern int sdebug_dix;
>>> +extern unsigned int sdebug_guard;
>>> +extern int sdebug_sector_size;
>>> +extern unsigned int sdebug_store_sectors;
>>
>> I doubt why all these are here
> 
> All the variables declared above are used in both scsi_debug_dif.c and
> scsi_debug_main.c.
> 
>>> +int dif_verify(struct t10_pi_tuple *sdt, const void *data, sector_t 
>>> sector,
>>> +           u32 ei_lba);
>>
>> Is this actually used in main.c?
> 
> It is not. I will remove it.
> 
>> I do also notice that we have chunks of code in main.c that does PI 
>> checking, like in resp_write_scat() - surely dif stuff should be in 
>> dif.c now
> 
> I'm concerned that moving that code would make resp_write_scat() much
> less readable. Please note that the code in resp_write_scat() that does
> PI checking is guarded by an 'if (have_dif_prot)' check and that
> have_dif_prot = false if CONFIG_CRC_T10DIF=n.
> 
>>> --- /dev/null
>>> +++ b/drivers/scsi/scsi_debug_dif.c
>>> @@ -0,0 +1,224 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +#include "scsi_debug-dif.h"
>>
>> I always find it is strange to include driver proprietary header files 
>> before common header files - I guess that is why the scsi_cmnd 
>> prototype is declared, above
> 
> Including driver-private header files first is required by some coding
> style guides because it causes the build to fail if the driver-private
> header file does not include all include files it should include. See
> also
> https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
> (I am aware that this style guide does not apply to Linux kernel code).
> 
> Thanks,
> 
> Bart.
> 

Re: [PATCH v2] scsi_debug: Make CRC_T10DIF support optional

[Bart Van Assche]

On 3/7/24 01:37, John Garry wrote:
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index acf0592d63da..5bac3b5aa5fa 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -3471,6 +3471,7 @@ static bool comp_write_worker(struct 
> sdeb_store_info *sip, u64 lba, u32 num,
>       return res;
>   }
> 
> +#if (IS_ENABLED(CONFIG_CRC_T10DIF))
>   static __be16 dif_compute_csum(const void *buf, int len)
>   {
>       __be16 csum;
> @@ -3509,6 +3510,13 @@ static int dif_verify(struct t10_pi_tuple *sdt, 
> const void *data,
>       }
>       return 0;
>   }
> +#else
> +static int dif_verify(struct t10_pi_tuple *sdt, const void *data,
> +              sector_t sector, u32 ei_lba)
> +{
> +    return -EOPNOTSUPP;
> +}
> +#endif
> 
>   static void dif_copy_prot(struct scsi_cmnd *scp, sector_t sector,
>                 unsigned int sectors, bool read)
> @@ -7421,7 +7429,12 @@ static int __init scsi_debug_init(void)
>       case T10_PI_TYPE1_PROTECTION:
>       case T10_PI_TYPE2_PROTECTION:
>       case T10_PI_TYPE3_PROTECTION:
> +        #if (IS_ENABLED(CONFIG_CRC_T10DIF))
>           have_dif_prot = true;
> +        #else
> +        pr_err("CRC_T10DIF not enabled\n");
> +        return -EINVAL;
> +        #endif
>           break;
> 
>       default:
> ----8<-----
> 
> I know that IS_ENABLED(CONFIG_XXX)) ain't too nice to use, but this is a 
> lot less change and we also don't need multiple files for the driver. As 
> below, it's not easy to separate the CRC_T10DIF-related stuff out.

The above suggestion violates the following rule from the Linux kernel 
coding style: "As a general rule, #ifdef use should be confined to
header files whenever possible." See also
Documentation/process/4.Coding.rst.

The approach to use multiple files in order to avoid #ifdefs in .c files
is strongly preferred in Linux kernel code.

Thanks,

Bart.

Re: [PATCH v2] scsi_debug: Make CRC_T10DIF support optional

[John Garry]

On 07/03/2024 17:59, Bart Van Assche wrote:
>> +#else
>> +static int dif_verify(struct t10_pi_tuple *sdt, const void *data,
>> +              sector_t sector, u32 ei_lba)
>> +{
>> +    return -EOPNOTSUPP;
>> +}
>> +#endif
>>
>>   static void dif_copy_prot(struct scsi_cmnd *scp, sector_t sector,
>>                 unsigned int sectors, bool read)
>> @@ -7421,7 +7429,12 @@ static int __init scsi_debug_init(void)
>>       case T10_PI_TYPE1_PROTECTION:
>>       case T10_PI_TYPE2_PROTECTION:
>>       case T10_PI_TYPE3_PROTECTION:
>> +        #if (IS_ENABLED(CONFIG_CRC_T10DIF))
>>           have_dif_prot = true;
>> +        #else
>> +        pr_err("CRC_T10DIF not enabled\n");
>> +        return -EINVAL;
>> +        #endif
>>           break;
>>
>>       default:
>> ----8<-----
>>
>> I know that IS_ENABLED(CONFIG_XXX)) ain't too nice to use, but this is 
>> a lot less change and we also don't need multiple files for the 
>> driver. As below, it's not easy to separate the CRC_T10DIF-related 
>> stuff out.
> 
> The above suggestion violates the following rule from the Linux kernel 
> coding style: "As a general rule, #ifdef use should be confined to
> header files whenever possible." See also
> Documentation/process/4.Coding.rst.
> 
> The approach to use multiple files in order to avoid #ifdefs in .c files
> is strongly preferred in Linux kernel code.

Then what about this change in this patch:

 > +#ifdef CONFIG_CRC_T10DIF
 > MODULE_PARM_DESC(dif, "data integrity field type: 0-3 (def=0)");
 > MODULE_PARM_DESC(dix, "data integrity extensions mask (def=0)");
 > +#endif

The idea of putting #ifdef in headers is that we can stub APIs. But that 
does not work for CRC_T10DIF, as the APIs don't return error codes - 
that's why there are no stubs today.

And, as noted, this is a general rule.

BTW, I don't think that modparams should be compiled out like this. 
Better would be to leave as is, and error when the user sets them.

scsi_debug is complex, to put it gently. I don't see any value in 
splitting it into further files, spreading the complexity. More 
especially when there seems to be little gain. scsi_debug requires 
CRC_T10DIF, so let it have it.

Thanks,
John

Re: [PATCH v2] scsi_debug: Make CRC_T10DIF support optional

[Martin K. Petersen]

John,

> scsi_debug is complex, to put it gently. I don't see any value in
> splitting it into further files, spreading the complexity. More
> especially when there seems to be little gain. scsi_debug requires
> CRC_T10DIF, so let it have it.

Yeah, I'm not sure this is worth the effort. It's a debug driver.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2] scsi_debug: Make CRC_T10DIF support optional

[Bart Van Assche]

On 3/8/24 01:45, John Garry wrote:
> On 07/03/2024 17:59, Bart Van Assche wrote:
>> The approach to use multiple files in order to avoid #ifdefs in .c files
>> is strongly preferred in Linux kernel code.
> 
> Then what about this change in this patch:
> 
>  > +#ifdef CONFIG_CRC_T10DIF
>  > MODULE_PARM_DESC(dif, "data integrity field type: 0-3 (def=0)");
>  > MODULE_PARM_DESC(dix, "data integrity extensions mask (def=0)");
>  > +#endif

This ifdef has been removed in v3.

> BTW, I don't think that modparams should be compiled out like this. 
> Better would be to leave as is, and error when the user sets them.

Hmm ... aren't unrecognized kernel module parameters ignored silently?
See also unknown_module_param_cb() in the kernel code.

> scsi_debug is complex, to put it gently. I don't see any value in 
> splitting it into further files, spreading the complexity. More 
> especially when there seems to be little gain. scsi_debug requires 
> CRC_T10DIF, so let it have it.

We are using the scsi_debug driver in Android for kernel regression
testing but there are no plans to support T10-PI in Android. Any
proposals for alternative solutions for removing the dependency of the
scsi_debug driver on the CRC_T10DIF code are welcome.

Thanks,

Bart.