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
