[PATCH] thunderbolt: test: get running under UML, add kunitconfig

[PATCH] thunderbolt: test: get running under UML, add kunitconfig

[Daniel Latypov]

These tests didn't work under the normal `kunit.py run` command since
they require CONFIG_PCI=y, which could not be set on ARCH=um.

Commit 68f5d3f3b654 ("um: add PCI over virtio emulation driver") lets us
do so. To make it so people don't have to figure out how to do so, we
add a drivers/thunderbolt/.kunitconfig.

Can now run these tests using
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/thunderbolt

Potentially controversial bits:
1. this .kunitconfig is UML-specific, can't do this for example
$ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kunitconfig=drivers/thunderbolt
2. this removes the manual call to __kunit_test_suites_init(), which
   allowed us to control exactly when the tests got run.

The main motivation for both is convenience.

For #1, we could drop the UML specific options, but then users would
always have to set --arch. Since UML is a bit faster and there's less to
type when running it with kunit.py, let's make it work by default.
Users can manually edit the .kunitconfig to run under x86_64.

For #2, running our suite separately prevents kunit.py from picking up
results properly as it only parses one set of KUnit results. I.e.
there's an assumption that __kunit_test_suites_init() only gets called
once. Since the tests seem to run fine when kunit runs them, giving up
this control seems fine.

Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
 drivers/thunderbolt/.kunitconfig |  7 +++++++
 drivers/thunderbolt/domain.c     |  3 ---
 drivers/thunderbolt/tb.h         |  8 --------
 drivers/thunderbolt/test.c       | 12 +-----------
 4 files changed, 8 insertions(+), 22 deletions(-)
 create mode 100644 drivers/thunderbolt/.kunitconfig

diff --git a/drivers/thunderbolt/.kunitconfig b/drivers/thunderbolt/.kunitconfig
new file mode 100644
index 000000000000..c8c9467bc144
--- /dev/null
+++ b/drivers/thunderbolt/.kunitconfig
@@ -0,0 +1,7 @@
+CONFIG_PCI=y
+CONFIG_VIRTIO_UML=y
+CONFIG_UML_PCI_OVER_VIRTIO=y
+
+CONFIG_KUNIT=y
+CONFIG_USB4=y
+CONFIG_USB4_KUNIT_TEST=y
diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
index 7018d959f775..dedc2866f51b 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -878,7 +878,6 @@ int tb_domain_init(void)
 {
 	int ret;
 
-	tb_test_init();
 	tb_debugfs_init();
 	tb_acpi_init();
 
@@ -896,7 +895,6 @@ int tb_domain_init(void)
 err_acpi:
 	tb_acpi_exit();
 	tb_debugfs_exit();
-	tb_test_exit();
 
 	return ret;
 }
@@ -909,5 +907,4 @@ void tb_domain_exit(void)
 	tb_xdomain_exit();
 	tb_acpi_exit();
 	tb_debugfs_exit();
-	tb_test_exit();
 }
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 74d3b14f004e..db54f8a27ba8 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -1261,12 +1261,4 @@ static inline void tb_service_debugfs_init(struct tb_service *svc) { }
 static inline void tb_service_debugfs_remove(struct tb_service *svc) { }
 #endif
 
-#ifdef CONFIG_USB4_KUNIT_TEST
-int tb_test_init(void);
-void tb_test_exit(void);
-#else
-static inline int tb_test_init(void) { return 0; }
-static inline void tb_test_exit(void) { }
-#endif
-
 #endif
diff --git a/drivers/thunderbolt/test.c b/drivers/thunderbolt/test.c
index 1f69bab236ee..601c04aaa7d9 100644
--- a/drivers/thunderbolt/test.c
+++ b/drivers/thunderbolt/test.c
@@ -2725,14 +2725,4 @@ static struct kunit_suite tb_test_suite = {
 	.test_cases = tb_test_cases,
 };
 
-static struct kunit_suite *tb_test_suites[] = { &tb_test_suite, NULL };
-
-int tb_test_init(void)
-{
-	return __kunit_test_suites_init(tb_test_suites);
-}
-
-void tb_test_exit(void)
-{
-	return __kunit_test_suites_exit(tb_test_suites);
-}
+kunit_test_suites(&tb_test_suite);

base-commit: 754e0b0e35608ed5206d6a67a791563c631cec07
-- 
2.35.1.265.g69c8d7142f-goog

Re: [PATCH] thunderbolt: test: get running under UML, add kunitconfig

[Daniel Latypov]

On Mon, Feb 14, 2022 at 10:41 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> These tests didn't work under the normal `kunit.py run` command since
> they require CONFIG_PCI=y, which could not be set on ARCH=um.
>
> Commit 68f5d3f3b654 ("um: add PCI over virtio emulation driver") lets us
> do so. To make it so people don't have to figure out how to do so, we
> add a drivers/thunderbolt/.kunitconfig.
>
> Can now run these tests using
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/thunderbolt
>
> Potentially controversial bits:
> 1. this .kunitconfig is UML-specific, can't do this for example
> $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kunitconfig=drivers/thunderbolt
> 2. this removes the manual call to __kunit_test_suites_init(), which
>    allowed us to control exactly when the tests got run.

kernel-test-robot points out something I had forgotten.
Doing this prevents us from being able to build this test as a module.

kunit_test_suites() defines an init_module() which conflicts with the
existing ones.

There's some relevant discussion about reworking how kunit modules
work here, https://lore.kernel.org/linux-kselftest/e5fa413ed59083ca63f3479d507b972380da0dcf.camel@codeconstruct.com.au/

So I think we have two options for this patch:
a) proceed, but disable building the test as a module for now (tristate => bool)
b) wait on this patch until kunit module support is refactored

Basically the question is: does this slightly easier way of running
the test seem worth losing the ability to test as a module in the
short-term?

Re: [PATCH] thunderbolt: test: get running under UML, add kunitconfig

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 2297 bytes --]

On Tue, Feb 15, 2022 at 10:39 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Mon, Feb 14, 2022 at 10:41 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > These tests didn't work under the normal `kunit.py run` command since
> > they require CONFIG_PCI=y, which could not be set on ARCH=um.
> >
> > Commit 68f5d3f3b654 ("um: add PCI over virtio emulation driver") lets us
> > do so. To make it so people don't have to figure out how to do so, we
> > add a drivers/thunderbolt/.kunitconfig.
> >
> > Can now run these tests using
> > $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/thunderbolt
> >
> > Potentially controversial bits:
> > 1. this .kunitconfig is UML-specific, can't do this for example
> > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kunitconfig=drivers/thunderbolt
> > 2. this removes the manual call to __kunit_test_suites_init(), which
> >    allowed us to control exactly when the tests got run.
>
> kernel-test-robot points out something I had forgotten.
> Doing this prevents us from being able to build this test as a module.
>
> kunit_test_suites() defines an init_module() which conflicts with the
> existing ones.
>
> There's some relevant discussion about reworking how kunit modules
> work here, https://lore.kernel.org/linux-kselftest/e5fa413ed59083ca63f3479d507b972380da0dcf.camel@codeconstruct.com.au/
>
> So I think we have two options for this patch:
> a) proceed, but disable building the test as a module for now (tristate => bool)
> b) wait on this patch until kunit module support is refactored
>
> Basically the question is: does this slightly easier way of running
> the test seem worth losing the ability to test as a module in the
> short-term?

Since this was originally changed to support modules (so clearly,
they're being used), I don't think breaking module support (even
temporarily) is going to be worth it:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2c6ea4e2cefe2e86af782a5b8e1070f4d434f2f2

Obviously, when module support is improved, then this fix will make a
lot of sense.

I do think adding the .kunitconfig file is worth doing (though
obviously there are some small problems with the way results show up
separately from any other tests due to the issue above).

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH] thunderbolt: test: get running under UML, add kunitconfig

[Mika Westerberg]

Hi Daniel,

On Mon, Feb 14, 2022 at 06:39:25PM -0800, Daniel Latypov wrote:
> On Mon, Feb 14, 2022 at 10:41 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > These tests didn't work under the normal `kunit.py run` command since
> > they require CONFIG_PCI=y, which could not be set on ARCH=um.
> >
> > Commit 68f5d3f3b654 ("um: add PCI over virtio emulation driver") lets us
> > do so. To make it so people don't have to figure out how to do so, we
> > add a drivers/thunderbolt/.kunitconfig.
> >
> > Can now run these tests using
> > $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/thunderbolt
> >
> > Potentially controversial bits:
> > 1. this .kunitconfig is UML-specific, can't do this for example
> > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kunitconfig=drivers/thunderbolt
> > 2. this removes the manual call to __kunit_test_suites_init(), which
> >    allowed us to control exactly when the tests got run.
> 
> kernel-test-robot points out something I had forgotten.
> Doing this prevents us from being able to build this test as a module.
> 
> kunit_test_suites() defines an init_module() which conflicts with the
> existing ones.
> 
> There's some relevant discussion about reworking how kunit modules
> work here, https://lore.kernel.org/linux-kselftest/e5fa413ed59083ca63f3479d507b972380da0dcf.camel@codeconstruct.com.au/
> 
> So I think we have two options for this patch:
> a) proceed, but disable building the test as a module for now (tristate => bool)
> b) wait on this patch until kunit module support is refactored
> 
> Basically the question is: does this slightly easier way of running
> the test seem worth losing the ability to test as a module in the
> short-term?

I would like to keep the module option available.

For me, I can just continue running this under QEMU for now so let's
wait until the reworking has been done. Thanks for looking into this,
though! :)

Re: [PATCH] thunderbolt: test: get running under UML, add kunitconfig

[Daniel Latypov]

On Mon, Feb 14, 2022 at 10:41 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi Daniel,
>
> On Mon, Feb 14, 2022 at 06:39:25PM -0800, Daniel Latypov wrote:
> > On Mon, Feb 14, 2022 at 10:41 AM Daniel Latypov <dlatypov@google.com> wrote:
> > >
> > > These tests didn't work under the normal `kunit.py run` command since
> > > they require CONFIG_PCI=y, which could not be set on ARCH=um.
> > >
> > > Commit 68f5d3f3b654 ("um: add PCI over virtio emulation driver") lets us
> > > do so. To make it so people don't have to figure out how to do so, we
> > > add a drivers/thunderbolt/.kunitconfig.
> > >
> > > Can now run these tests using
> > > $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/thunderbolt
> > >
> > > Potentially controversial bits:
> > > 1. this .kunitconfig is UML-specific, can't do this for example
> > > $ ./tools/testing/kunit/kunit.py run --arch=x86_64 --kunitconfig=drivers/thunderbolt
> > > 2. this removes the manual call to __kunit_test_suites_init(), which
> > >    allowed us to control exactly when the tests got run.
> >
> > kernel-test-robot points out something I had forgotten.
> > Doing this prevents us from being able to build this test as a module.
> >
> > kunit_test_suites() defines an init_module() which conflicts with the
> > existing ones.
> >
> > There's some relevant discussion about reworking how kunit modules
> > work here, https://lore.kernel.org/linux-kselftest/e5fa413ed59083ca63f3479d507b972380da0dcf.camel@codeconstruct.com.au/
> >
> > So I think we have two options for this patch:
> > a) proceed, but disable building the test as a module for now (tristate => bool)
> > b) wait on this patch until kunit module support is refactored
> >
> > Basically the question is: does this slightly easier way of running
> > the test seem worth losing the ability to test as a module in the
> > short-term?
>
> I would like to keep the module option available.
>
> For me, I can just continue running this under QEMU for now so let's
> wait until the reworking has been done. Thanks for looking into this,
> though! :)

Sounds good.
We can treat this patch as just an example of what people can manually
do if they want to run tests under UML.

And I'll also look to this when I inevitably forget how to enable
CONFIG_PCI=y on UML again.

Thanks!
Daniel