Quoting David Gow (2024-05-04 01:30:34) > On Fri, 3 May 2024 at 09:04, Stephen Boyd <sboyd@xxxxxxxxxx> wrote: > > > > Quoting David Gow (2024-05-01 00:55:46) > > > On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <sboyd@xxxxxxxxxx> 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-names > > 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: > 1. 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.
