Re: [PATCH v4 05/10] platform: Add test managed platform_device/driver APIs

[Stephen Boyd <sboyd@kernel.org>]

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-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.