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

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Matti Vaittinen <mazziesaccount@gmail.com>
Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Brendan Higgins <brendan.higgins@linux.dev>,
	David Gow <davidgow@google.com>,
	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>,
	Maxime Ripard <maxime@cerno.tech>,
	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: Wed, 22 Mar 2023 19:57:10 +0100	[thread overview]
Message-ID: <ZBtPhoelZo4U5jwC@kroah.com> (raw)
In-Reply-To: <25f9758f-0010-0181-742a-b18a344110cf@gmail.com>

On Wed, Mar 22, 2023 at 03:48:00PM +0200, Matti Vaittinen wrote:
> Hi Greg,
> 
> Thanks for looking at this.
> 
> On 3/22/23 14:07, Greg Kroah-Hartman wrote:
> > On Wed, Mar 22, 2023 at 11:05:55AM +0200, Matti Vaittinen wrote:
> > > --- /dev/null
> > > +++ b/drivers/base/test/test_kunit_device.c
> > > @@ -0,0 +1,83 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * These helpers have been extracted from drm test code at
> > > + * drm_kunit_helpers.c which was authored by
> > > + * Maxime Ripard <maxime@cerno.tech>
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#include <kunit/platform_device.h>
> > > +
> > > +#define KUNIT_DEVICE_NAME	"test-kunit-mock-device"
> > > +
> > > +static int fake_probe(struct platform_device *pdev)
> > 
> > Please do not abuse platform devices and drivers for things that are not
> > actually platform devices and drivers.
> > 
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static int fake_remove(struct platform_device *pdev)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +static struct platform_driver fake_platform_driver = {
> > > +	.probe	= fake_probe,
> > > +	.remove	= fake_remove,
> > > +	.driver = {
> > > +		.name	= KUNIT_DEVICE_NAME,
> > > +	},
> > > +};
> > 
> > Why do you need this fake platform driver at all?
> > 
> > Why not just use a virtual device?
> 
> I can only answer on my behalf. In my case the answer to why I used
> platform_devices is practicality. I wanted to test devm_ APIs using KUnit
> tests and I was pointed to an existing implementation in DRM (seen in these
> patches). It didn't seem to make any sense to re-invent the wheel by writing
> another implementation for the existing in-tree functionality.

That's fine, but please, let's do this right if it's going to be in the
driver core, that way we can actually test the driver core code as well.

> > > +/**
> > > + * test_kunit_helper_alloc_device - Allocate a mock device for a KUnit test
> > > + * @test: The test context object
> > > + *
> > > + * This allocates a fake struct &device to create a mock for a KUnit
> > > + * test. The device will also be bound to a fake driver. It will thus be
> > > + * able to leverage the usual infrastructure and most notably the
> > > + * device-managed resources just like a "real" device.
> > 
> > What specific "usual infrastructure" are you wanting to access here?
> > 
> > And again, if you want a fake device, make a virtual one, by just
> > calling device_create().
> > 
> > Or are you wanting to do "more" with that device pointer than
> > device_create() can give you?
> 
> Personally, I was (am) only interested in devm_ unwinding. I guess the
> device_create(), device_add(), device_remove()... (didn't study this
> sequence in details so sorry if there is errors) could've been sufficient
> for me. I haven't looked how much of the code that there is for 'platform
> devices' should be duplicated to support that sequence for testability
> purposes.

Any device can access devm_ code, there's no need for it to be a
platform device at all.

> The biggest thing for me is that I don't like the idea of creating own 'test
> device' in <add subsystem here> while we already have some in DRM (or
> others). Thus, I do see value in adding generic helpers for supporting
> running KUnit tests on devm_* APIs. Hence it'd be good to have _some_
> support for it.

I agree, let's use a virtual device and a virtual bus (you can use the
auxbus code for this as that's all there for this type of thing) and
then you can test the devm_* code, _AND_ you can test the driver core at
the same time.

> And having them in drivers/base/test seemed like a correct
> place to me. What I really don't know is if there are legitimate use-cases
> for using platform_devices in DRM tests. Perhaps Maxime can shed light on
> that.

I agree that this could be in drivers/base/test/ but then let's test the
driver core, not just provide a dummy platform device.

If you want to test the platform driver/device api, that would be great
too, that can be plaform device/driver specific, but don't use one for
some other random driver core functionality please.

thanks,

greg k-h