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.
