| Message ID | 20220214184104.1710107-1-dlatypov@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | thunderbolt: test: get running under UML, add kunitconfig | expand |
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?
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
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! :)
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
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);
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 base-commit: 754e0b0e35608ed5206d6a67a791563c631cec07