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@xxxxxxxxxxxx>
Cc: John Garry <john.g.garry@xxxxxxxxxx>
Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
---
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
