[PATCH] scsi: core: Make scsi_lib KUnit tests modular for real

[PATCH] scsi: core: Make scsi_lib KUnit tests modular for real

[Geert Uytterhoeven]

While SCSI_LIB_KUNIT_TEST is a tristate config symbol, configuring a
modular build of this test does not do anything: as the test code is
just included by the mid layer code, it only works in the built-in case.

Fix this by converting the test to a stand-alone module.  This requires
exporting scsi_check_passthrough() and adding a MODULE_LICENSE() tag.

Fixes: 25a1f7a0a1fe6fa6 ("scsi: core: Add kunit tests for scsi_check_passthrough()")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/scsi/Makefile        | 1 +
 drivers/scsi/scsi_lib.c      | 9 +++------
 drivers/scsi/scsi_lib_test.c | 4 ++++
 drivers/scsi/scsi_priv.h     | 2 ++
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index f055bfd54a6832b3..396a24aa43486678 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -149,6 +149,7 @@ obj-$(CONFIG_BLK_DEV_SR)	+= sr_mod.o
 obj-$(CONFIG_CHR_DEV_SG)	+= sg.o
 obj-$(CONFIG_CHR_DEV_SCH)	+= ch.o
 obj-$(CONFIG_SCSI_ENCLOSURE)	+= ses.o
+obj-$(CONFIG_SCSI_LIB_KUNIT_TEST) += scsi_lib_test.o
 
 obj-$(CONFIG_SCSI_HISI_SAS) += hisi_sas/
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 2e28e2360c85740d..23e94e9bf85781a9 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -203,8 +203,8 @@ EXPORT_SYMBOL_GPL(scsi_failures_reset_retries);
  *
  * Returns -EAGAIN if the caller should retry else 0.
  */
-static int scsi_check_passthrough(struct scsi_cmnd *scmd,
-				  struct scsi_failures *failures)
+int scsi_check_passthrough(struct scsi_cmnd *scmd,
+			   struct scsi_failures *failures)
 {
 	struct scsi_failure *failure;
 	struct scsi_sense_hdr sshdr;
@@ -269,6 +269,7 @@ static int scsi_check_passthrough(struct scsi_cmnd *scmd,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(scsi_check_passthrough);
 
 /**
  * scsi_execute_cmd - insert request and wait for the result
@@ -3436,7 +3437,3 @@ void scsi_build_sense(struct scsi_cmnd *scmd, int desc, u8 key, u8 asc, u8 ascq)
 	scmd->result = SAM_STAT_CHECK_CONDITION;
 }
 EXPORT_SYMBOL_GPL(scsi_build_sense);
-
-#ifdef CONFIG_SCSI_LIB_KUNIT_TEST
-#include "scsi_lib_test.c"
-#endif
diff --git a/drivers/scsi/scsi_lib_test.c b/drivers/scsi/scsi_lib_test.c
index 99834426a100a754..13045ac12fa99d24 100644
--- a/drivers/scsi/scsi_lib_test.c
+++ b/drivers/scsi/scsi_lib_test.c
@@ -10,6 +10,8 @@
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_device.h>
 
+#include "scsi_priv.h"
+
 #define SCSI_LIB_TEST_MAX_ALLOWED 3
 #define SCSI_LIB_TEST_TOTAL_MAX_ALLOWED 5
 
@@ -328,3 +330,5 @@ static struct kunit_suite scsi_lib_test_suite = {
 };
 
 kunit_test_suite(scsi_lib_test_suite);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 9fc397a9ce7a4f91..7f7e55341192e50e 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -113,6 +113,8 @@ extern int scsi_mq_setup_tags(struct Scsi_Host *shost);
 extern void scsi_mq_free_tags(struct kref *kref);
 extern void scsi_exit_queue(void);
 extern void scsi_evt_thread(struct work_struct *work);
+extern int scsi_check_passthrough(struct scsi_cmnd *scmd,
+				  struct scsi_failures *failures);
 
 /* scsi_proc.c */
 #ifdef CONFIG_SCSI_PROC_FS
-- 
2.34.1

Re: [PATCH] scsi: core: Make scsi_lib KUnit tests modular for real

[Bart Van Assche]

On 3/19/24 05:02, Geert Uytterhoeven wrote:
> While SCSI_LIB_KUNIT_TEST is a tristate config symbol, configuring a
> modular build of this test does not do anything: as the test code is
> just included by the mid layer code, it only works in the built-in case.
> 
> Fix this by converting the test to a stand-alone module.  This requires
> exporting scsi_check_passthrough() and adding a MODULE_LICENSE() tag.

I don't like it that scsi_check_passthrough() is exported so that counts
as a disadvantage of this patch. Why to convert scsi_lib_test into a
kernel module? What are the advantages compared to the current approach?
That information is missing from the patch description.

Thanks,

Bart.

Re: [PATCH] scsi: core: Make scsi_lib KUnit tests modular for real

[Geert Uytterhoeven]

Hoi Bart,

On Tue, Mar 19, 2024 at 5:03 PM Bart Van Assche <bvanassche@acm.org> wrote:
> On 3/19/24 05:02, Geert Uytterhoeven wrote:
> > While SCSI_LIB_KUNIT_TEST is a tristate config symbol, configuring a
> > modular build of this test does not do anything: as the test code is
> > just included by the mid layer code, it only works in the built-in case.
> >
> > Fix this by converting the test to a stand-alone module.  This requires
> > exporting scsi_check_passthrough() and adding a MODULE_LICENSE() tag.
>
> I don't like it that scsi_check_passthrough() is exported so that counts
> as a disadvantage of this patch. Why to convert scsi_lib_test into a

Perhaps the exported symbol should be __scsi_check_passthrough(),
to make it clearer this is not meant for general use?

> kernel module? What are the advantages compared to the current approach?
> That information is missing from the patch description.

SCSI_LIB_KUNIT_TEST is already tristate, so the original author must
have meant it to be modular.  Or perhaps he just copied it from
(most/all) other tests ;-)

Anyway, I find it very useful to be able to do "modprobe kunit" and
"modprobe <test>" to run a test when I feel the need to do so.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] scsi: core: Make scsi_lib KUnit tests modular for real

[Bart Van Assche]

On 3/19/24 09:10, Geert Uytterhoeven wrote:
> On Tue, Mar 19, 2024 at 5:03 PM Bart Van Assche <bvanassche@acm.org> wrote:
>> On 3/19/24 05:02, Geert Uytterhoeven wrote:
>> kernel module? What are the advantages compared to the current approach?
>> That information is missing from the patch description.
> 
> SCSI_LIB_KUNIT_TEST is already tristate, so the original author must
> have meant it to be modular.  Or perhaps he just copied it from
> (most/all) other tests ;-)
> 
> Anyway, I find it very useful to be able to do "modprobe kunit" and
> "modprobe <test>" to run a test when I feel the need to do so.

Hi Geert,

Why to run hardware-independent kunit tests on the target system instead
of on the host? Isn't it much more convenient when developing embedded
software to run kunit tests on the host using UML? The script I use to
run SCSI kunit tests is available below. And if there is a desire to run
SCSI tests on the target system, how about adding triggers in sysfs for
running kunit tests? The (GPL v2) Samsung smartphone kernel supports
this but I have not yet checked whether their implementation is
appropriate for the upstream kernel.

Thanks,

Bart.


#!/bin/sh

set -e

mkdir -p .kunit
if [ -e .config ]; then
     rm -f .config
     make ARCH=um mrproper
fi
if [ ! -e .kunit/.kunitconfig ] || [ "$0" -nt .kunit/.kunitconfig ]; then
     echo "Regenerating .kunit/.kunitconfig"
     cat <<EOF >.kunit/.kunitconfig
CONFIG_BLK_DEV_SD=y
CONFIG_BLK_DEV_ZONED=y
CONFIG_MQ_IOSCHED_DEADLINE=y
CONFIG_BLOCK=y
CONFIG_EISA=n
CONFIG_KUNIT=y
CONFIG_SCSI_PROCFS=n
#CONFIG_PROVE_LOCKING=y
CONFIG_SCSI=y
#CONFIG_SYSFS=y
CONFIG_UBSAN=y
CONFIG_KASAN=y
CONFIG_RUNTIME_TESTING_MENU=n
CONFIG_WERROR=y
EOF
     syms=(
	CONFIG_SCSI_ERROR_TEST
	CONFIG_SCSI_PROTO_TEST
	CONFIG_SCSI_SD_TEST
     )
     for s in "${syms[@]}"; do
     if git grep -qw "${s#CONFIG_}" block/Kconfig* drivers/scsi/Kconfig; 
then
	echo "$s=y" >> .kunit/.kunitconfig
     fi
     done
     cp .kunit/.kunitconfig .kunit/.config
fi
./tools/testing/kunit/kunit.py run

Re: [PATCH] scsi: core: Make scsi_lib KUnit tests modular for real

[Geert Uytterhoeven]

Hoi Bart,

CC linux-kselftest@vger.kernel.org

On Tue, Mar 19, 2024 at 6:01 PM Bart Van Assche <bvanassche@acm.org> wrote:
> On 3/19/24 09:10, Geert Uytterhoeven wrote:
> > On Tue, Mar 19, 2024 at 5:03 PM Bart Van Assche <bvanassche@acm.org> wrote:
> >> On 3/19/24 05:02, Geert Uytterhoeven wrote:
> >> kernel module? What are the advantages compared to the current approach?
> >> That information is missing from the patch description.
> >
> > SCSI_LIB_KUNIT_TEST is already tristate, so the original author must
> > have meant it to be modular.  Or perhaps he just copied it from
> > (most/all) other tests ;-)
> >
> > Anyway, I find it very useful to be able to do "modprobe kunit" and
> > "modprobe <test>" to run a test when I feel the need to do so.
>
> Why to run hardware-independent kunit tests on the target system instead
> of on the host? Isn't it much more convenient when developing embedded
> software to run kunit tests on the host using UML? The script I use to

Because test results may differ between target and host?
It's not uncommon for supposedly hardware-independent tests to behave
differently on different architectures and platforms, due to subtle
differences in word size, endianness, alignment rules, CPU topology, ...

> run SCSI kunit tests is available below. And if there is a desire to run
> SCSI tests on the target system, how about adding triggers in sysfs for
> running kunit tests? The (GPL v2) Samsung smartphone kernel supports
> this but I have not yet checked whether their implementation is
> appropriate for the upstream kernel.

That would require all tests to be built-in, reducing the amount of memory
(if any remains at all) available to the real application.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] scsi: core: Make scsi_lib KUnit tests modular for real

[Bart Van Assche]

On 3/20/24 01:08, Geert Uytterhoeven wrote:
> On Tue, Mar 19, 2024 at 6:01 PM Bart Van Assche <bvanassche@acm.org> wrote:
>> run SCSI kunit tests is available below. And if there is a desire to run
>> SCSI tests on the target system, how about adding triggers in sysfs for
>> running kunit tests? The (GPL v2) Samsung smartphone kernel supports
>> this but I have not yet checked whether their implementation is
>> appropriate for the upstream kernel.
> 
> That would require all tests to be built-in, reducing the amount of memory
> (if any remains at all) available to the real application.

It would be great if it would be possible to convert scsi_lib_test.c
into a kernel module without exporting the functions that are being
tested. Exporting functions from scsi_lib.c only because these are
called from code in scsi_lib_test.c is not desired. More tests may be
added in the future into scsi_lib_test.c. If that would result in
exporting every static scsi_lib.c function that would make the SCSI core
harder to maintain than necessary.

Thanks,

Bart.