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

[Stephen Boyd <sboyd@kernel.org>]

Quoting David Gow (2023-03-02 23:15:31)
> On Thu, 2 Mar 2023 at 09:38, Stephen Boyd <sboyd@kernel.org> 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@linux.dev>
> > Cc: David Gow <davidgow@google.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> > ---
> >
> > 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.

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