Quoting David Gow (2024-05-04 01:30:34)
On Fri, 3 May 2024 at 09:04, Stephen Boyd sboyd@kernel.org wrote:
Quoting David Gow (2024-05-01 00:55:46)
On Tue, 23 Apr 2024 at 07:24, Stephen Boyd sboyd@kernel.org wrote:
diff --git a/Documentation/dev-tools/kunit/api/platformdevice.rst b/Documentation/dev-tools/kunit/api/platformdevice.rst new file mode 100644 index 000000000000..b228fb6558c2 --- /dev/null +++ b/Documentation/dev-tools/kunit/api/platformdevice.rst @@ -0,0 +1,10 @@ +.. SPDX-License-Identifier: GPL-2.0
+=================== +Platform Device API +===================
+The KUnit platform device API is used to test platform devices.
+.. kernel-doc:: drivers/base/test/platform_kunit.c
- :export:
diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile index e321dfc7e922..740aef267fbe 100644 --- a/drivers/base/test/Makefile +++ b/drivers/base/test/Makefile @@ -1,8 +1,11 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE) += test_async_driver_probe.o
+obj-$(CONFIG_KUNIT) += platform_kunit.o
Do we want this to be part of the kunit.ko module (and hence, probably, under lib/kunit), or to keep this as a separate module. I'm tempted, personally, to treat this as a part of KUnit, and have it be part of the same module. There are a couple of reasons for this:
- It's nice to have CONFIG_KUNIT produce only one module. If we want
this to be separate, I'd be tempted to put it behind its own kconfig entry.
- The name platform_kunit.ko suggests (to me, at least) that this is
the test for platform devices, not the implementation of the helper.
I was following *_kunit as "helpers" and *_test as the test. Only loosely based on the documentation that mentions to use _test or _kunit for test files. Maybe it should have _kunit_helpers postfix?
Yeah, the style guide currently suggests that *_test is the default for tests, but that _kunit may also be used for tests if _test is already used for non-KUnit tests: https://docs.kernel.org/dev-tools/kunit/style.html#test-file-and-module-name...
DRM has drm_kunit_helpers, so _kunit_helpers seems like a good suffix to settle on.
Alright, I'll rename the files.
Following the single module design should I merge the tests for this code into kunit-test.c? And do the same sort of thing for clk helpers? That sounds like it won't scale very well if everything is in one module.
I don't think it's as important that the tests live in the same module. It's nice from an ergonomic point-of-view to only have to modprobe the one thing, but we've already let that ship sail somewhat with string-stream-test.
Either way, splitting up kunit-test.c is something we'll almost certainly want to do at some point, and we can always put them into the same module even if they're different source files if we have to.
Alright.
Shouldn't the wrapper code for subsystems live in those subsystems like drm_kunit_helpers.c does? Maybe the struct device kunit wrappers should be moved out to drivers/base/? lib/kunit can stay focused on providing pure kunit code then.
I tend to agree that wrapper code for subsystems should live in those subsystems, especially if the subsystems are relatively self-contained (i.e., the helpers are used to test that subsystem itself, rather than exported for other parts of the kernel to use to test interactions with said subsystem). For 'core' parts of the kernel, I think it makes it easier to make these obviously part of KUnit (e.g. kunit_kzalloc() is easier to have within KUnit, rather than as a part of the allocators).
The struct device wrappers have the problem that they rely on the kunit_bus being registered, which is currently done when the kunit module is loaded. So it hooks more deeply into KUnit than is comfortable to do from drivers/base. So we've treated it as a 'core' part of the kernel.
Ok, thanks. The kzalloc wrappers look like the best example here. They are so essential that they are in lib/kunit. The platform bus is built into the kernel all the time, similar to mm, so I can see it being essential and desired to have the wrappers in lib/kunit.
Ultimately, it's a grey area, so I can live with this going either way, depending on the actual helpers, so long as we don't end up with lots of half-in/half-out helpers, which behave a bit like both. (For example, at the moment, helpers which live outside lib/kunit are documented and have headers in the respective subsystems' directories.)
FWIW, my gut feeling for what's "most consistent" with what we've done so far is:
- platform_device helpers should live alongside the current managed
device stuff, which is currently in lib/kunit 2. clk helpers should probably live in clk 3. of/of_overlay sits a bit in the middle, but having thought more about it, it'd probably lean towards having it be part of 'of', not 'kunit.
Sounds good. I'll follow this route.
But all of this is, to some extent, just bikeshedding, so as long as we pick somewhere to put them, and don't mix things up too much, I don't think it matters exactly what side of this fuzzy line they end up on.
Yeah. My final hesitation is that it will be "too easy" to make devices that live on the platform_bus when they should really be on the kunit_bus. I guess we'll have to watch out for folks making platform devices that don't use any other platform device APIs like IO resources, etc.