Re: [PATCH v5 1/8] drivers: kunit: Generic helpers for test device creation

From: Maxime Ripard <maxime@cerno.tech>
To: David Gow <davidgow@google.com>
Cc: Matti Vaittinen <mazziesaccount@gmail.com>,
	Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Brendan Higgins <brendan.higgins@linux.dev>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
	kunit-dev@googlegroups.com, Stephen Boyd <sboyd@kernel.org>,
	Jonathan Cameron <jic23@kernel.org>,
	linux-iio@vger.kernel.org
Subject: Re: [PATCH v5 1/8] drivers: kunit: Generic helpers for test device creation
Date: Thu, 23 Mar 2023 11:07:44 +0100	[thread overview]
Message-ID: <20230323100744.qirdtwsd327apau7@houat> (raw)
In-Reply-To: <CABVgOS=KUg+18wJe=O29tgOB0tQghAk030kONEm5fOzJHgKLgw@mail.gmail.com>

Hi David,

On Thu, Mar 23, 2023 at 03:30:34PM +0800, David Gow wrote:
> I think the questions we therefore need to answer are:
> - Do we specifically need a platform_device (or, any other specific
> device struct), or would any old struct device work? I can see why we
> would need a platform device for cases where we're testing things like
> device-tree properties (or, in the future, having e.g. USB-specific
> helpers which create usb_device). Most tests just use
> root_device_register() thus far, though.
> - What helpers do we need to make creating, using, and cleaning up
> these devices as simple as possible.
> 
> I think that in this particular case, we don't actually need a struct
> platform_device. Replacing these helpers with simple calls to
> root_device_register() and root_device_unregister() seems to work fine
> with this patch series for me. (It does break the
> drm_test_managed_run_action test, though.) So I don't think having
> these helpers actually help this series at the moment.

I'm not sure that a root_device is a good option, see below.

> That being said, if they used the KUnit resource system to
> automatically clean up the device when the test is finished (or
> otherwise exits), that would seem like a real advantage. The clk and
> drm examples both do this, and I'm hoping to add an API to make it
> even simpler going forward. (With the work-in-progress API described
> here[1], the whole thing should boil down to "struct device *dev =
> root_device_register(name); kunit_defer(root_device_unregister,
> dev);".)

Oh, yes, please make it happen :)

The current API is a bit of a pain compared to other managed APIs like
devm or drmm.

> So, I guess we have three cases we need to look at:
> - A test just needs any old struct device. Tests thus far have
> hardcoded, or had their own wrappers around root_device_register() for
> this.
> - A test needs a device attached to a bus (but doesn't care which
> bus). Thus far, people have used struct platform_device for this (see
> the DRM helpers, which use a platform device for managed resource
> tests[2]). Maybe the right solution here is something like a struct
> kunit_device?
> - A test needs a device attached to a specific bus. We'll probably
> need some more detailed faking of that bus. This covers cases like
> having fake USB devices, devicetree integration, etc.

Yeah, from the current discussion on the IIO and clk patchsets, and what
we've done in DRM, I guess there's two major use cases:

  * You test an (isolated) function that takes a device as an argument.
    Here you probably don't really care about what the device is as long
    as you can provide one. This is what is being done for the DRM
    helpers at the moment, even though it's not really isolated. The
    device is essentially mocked. This could be your points 1 and 2.

  * You want to probe the driver with a fake context. The device and
    drivers are very much real, but the device tree (or whatever) is
    mocked. This is what the clocks patches from Stephen are doing. This
    could be covered by your point 3.

> I'd suggest that, for the majority of cases which only care about the
> first case, let's just use root_device_register() directly, or have a
> thin wrapper like the old root_device-based version of the DRM
> helpers[3]. This will probable serve us well enough while we work out
> how to handle the other two cases properly (which is already being
> looked at for the CLK/DeviceTree patches and the DRM stuff).

I disagree, and I think your cases 1 and 2 should be merged. We were
initially using a root_device in DRM. We had to switch to
platform_device (but actually any device attached to a bus) because a
root device isn't attached to a bus and thus the devm resources are
never freed.

When a function takes a device as an argument, there's a good chance
that it will use devm nowadays, so we ended up exhausting resources
pools (like IDs iirc) when running a lot of tests in sequence.

So yeah, I think you can't just assume that a root device will do even
for a true unit test that would test an isolated function. It either
needs to be tied to a bus, or we need to force the devm resource release
when the root device is removed somehow.

Maxime