Re: [PATCH 3/8] kunit: Add test managed platform_device/driver APIs

[David Gow]

On Fri, 10 Mar 2023 at 07:25, Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
>
> Quoting David Gow (2023-03-02 23:15:31)
> > On Thu, 2 Mar 2023 at 09:38, Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
> > >
> > > Introduce KUnit resource wrappers around platform_driver_register(),
> > > platform_device_alloc(), and platform_device_add() so that test authors
> > > can register platform drivers/devices from their tests and have the
> > > drivers/devices automatically be unregistered when the test is done.
> > >
> > > This makes test setup code simpler when a platform driver or platform
> > > device is needed. Add a few test cases at the same time to make sure the
> > > APIs work as intended.
> > >
> > > Cc: Brendan Higgins <brendan.higgins@xxxxxxxxx>
> > > Cc: David Gow <davidgow@xxxxxxxxxx>
> > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> > > Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxx>
> > > ---
> > >
> > > Should this be moved to drivers/base/ and called platform_kunit.c?
> > > The include/kunit/platform_driver.h could also be
> > > kunit/platform_device.h to match linux/platform_device.h if that is more
> > > familiar.
> >
> > DRM has a similar thing already (albeit with a root_device, which is
> > more common with KUnit tests generally):
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/drm_kunit_helpers.h
> >
> > But that's reasonably drm-specific, so it makes sense that it lives
> > with DRM stuff. platform_device is a bit more generic.
> >
> > I'd probably personally err on the side of having these in
> > drivers/base/, as I think we'll ultimately need similar things for a
> > lot of different devices, and I'd rather not end up with things like
> > USB device helpers living in the lib/kunit directory alongside the
> > "core" KUnit code. But I could be persuaded otherwise.
>
> Ok no problem. I'll move it.
>
> >
> > >
> > > And I'm not super certain about allocating a driver structure and
> > > embedding it in a wrapper struct. Maybe the code should just use
> > > kunit_get_current_test() instead?
> >
> > I think there are enough cases througout the kernel where
> > device/driver structs are needed that having this makes sense.
> > Combined with the fact that, while kunit_get_current_test() can be
> > used even when KUnit is not loaded, actually doing anything with the
> > resulting struct kunit pointer will probably require (at least for the
> > moment) KUnit functions to be reachable, so would break if
> > CONFIG_KUNIT=m.
>
> Wouldn't it still work in that case? The unit tests would be modular as
> well because they depend on CONFIG_KUNIT.
>

Yeah, the only case where this starts to get hairy is if the tests end
up in the same module as the thing being tested (which sometimes
happens to avoid having to export a bunch of symbols: see, e.g.
thunderbolt and amdgpu), and then someone wants to build production
kernels with CONFIG_KUNIT=m (alas, Red Hat and Android).

So that's the only real place where you might need to avoid the
non-'hook' KUnit functions, but those drivers are pretty few and far
between, and most of the really useful functionality should be moving
to 'hooks' which will be patched out cleanly at runtime.

> >
> > So, unless you actually find kunit_get_current_test() and friends to
> > be easier to work with, I'd probably stick with this.
> >
>
> Alright thanks.
>
> > > diff --git a/lib/kunit/platform_driver.c b/lib/kunit/platform_driver.c
> > > new file mode 100644
> > > index 000000000000..11d155114936
> > > --- /dev/null
> > > +++ b/lib/kunit/platform_driver.c
> > > @@ -0,0 +1,207 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Test managed platform driver
> > > + */
> > > +
> > > +#include <linux/device/driver.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#include <kunit/resource.h>
> > > +
> > > +struct kunit_platform_device_alloc_params {
> > > +       const char *name;
> > > +       int id;
> > > +};
> >
> > FYI: It's my plan to eventually get rid of (or at least de-emphasize)
> > the whole 'init' function aspect of KUnit resources so we don't need
> > all of these extra structs and the like. It probably won't make it in
> > for 6.4, but we'll see...
>
> Will we be able to get the error values out of the init function? It's
> annoying that the error values can't be returned as error pointers to
> kunit_alloc_resource(). I end up skipping init, and doing it directly
> before or after calling the kunit_alloc_resource() function. I'll try to
> avoid init functions in the allocations.

Yeah, that's largely why the plan is to get rid of them: it just made
passing things around an enormous pain.
Just doing your own initialisation before adding it as a resource is
usually the right thing to do.

There's also going to be a simpler kunit_defer() wrapper around it,
which would just allow you to schedule a cleanup function to be called
(without the need to keep kunit_resource pointers around, etc), for
the cases where you don't need to look up resources elsewhere.

But just doing your own thing and calling kunit_alloc_resource() is
probably best for now, and should map well onto whatever this ends up
evolving into.

Cheers,
-- David