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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Mar 23, 2023 at 03:30:34PM +0800, David Gow wrote:
> On Wed, 22 Mar 2023 at 17:06, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >
> > The creation of a dummy device in order to test managed interfaces is a
> > generally useful test feature. The drm test helpers
> > drm_kunit_helper_alloc_device() and drm_kunit_helper_free_device()
> > are doing exactly this. It makes no sense that each and every component
> > which intends to be testing managed interfaces will create similar
> > helpers so stole these for more generic use.
> >
> > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> >
> > ---
> > Changes:
> > v4 => v5:
> > - Add accidentally dropped header and email recipients
> > - do not rename + move helpers from DRM but add temporary dublicates to
> >   simplify merging. (This patch does not touch DRM and can be merged
> >   separately. DRM patch and IIO test patch still depend on this one).
> >
> > Please note that there's something similar ongoing in the CCF:
> > https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@kernel.org/
> >
> > I do like the simplicity of these DRM-originated helpers so I think
> > they're worth. I do equally like the Stephen's idea of having the
> > "dummy platform device" related helpers under drivers/base and the
> > header being in include/kunit/platform_device.h which is similar to real
> > platform device under include/linux/platform_device.h
> > ---
> 
> Thanks for sending this my way.
> 
> It's clear we need some way of creating "fake" devices for KUnit
> tests. Given that there are now (at least) three different drivers
> looking to use this, we'll ultimately need something which works for
> everyone.
> 
> 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.

Any struct device would work, so please do NOT use a platform device.

Use aux devices, or better yet, just a normal virtual device by calling
device_create().

> - What helpers do we need to make creating, using, and cleaning up
> these devices as simple as possible.

Becides what the driver core already provides you?  What exactly are you
wanting to do here?

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

Why abuse root_device_*() for something that really is not a root device
in the system?  Why not use a virtual device?  Or better yet, a kunit
bus with devices on it that are just for testing?  That way you can
properly test the bus and driver and device code fully, which is what
you are implying is needed here.

> 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);".)
> 
> 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.

Again, make a kunit bus and devices, that might be "simplest" overall.

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

Yes.

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

Have your own bus.  No need to mess with USB or any real bus UNLESS you
want to actually test those busses, and if so, just create fake USB or
clk or whatever devices so that you can test them.

Or just rely on the testing that some busses have for their devices (USB
has this today and syzbot knows how to test it), as busses for hardware
can be complex and usually require a "real" driver to be written for
them to test things.

thanks,

gre gk-h