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.
