Re: [PATCH 1/4] kunit: Add APIs for managing devices

[David Gow <davidgow@xxxxxxxxxx>]

Hey Greg,

On Wed, 6 Dec 2023 at 01:31, Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Dec 05, 2023 at 03:31:33PM +0800, davidgow@xxxxxxxxxx wrote:
> > Tests for drivers often require a struct device to pass to other
> > functions. While it's possible to create these with
> > root_device_register(), or to use something like a platform device, this
> > is both a misuse of those APIs, and can be difficult to clean up after,
> > for example, a failed assertion.
> >
> > Add some KUnit-specific functions for registering and unregistering a
> > struct device:
> > - kunit_device_register()
> > - kunit_device_register_with_driver()
> > - kunit_device_unregister()
> >
> > These helpers allocate a on a 'kunit' bus which will either probe the
> > driver passed in (kunit_device_register_with_driver), or will create a
> > stub driver (kunit_device_register) which is cleaned up on test shutdown.
> >
> > Devices are automatically unregistered on test shutdown, but can be
> > manually unregistered earlier with kunit_device_unregister() in order
> > to, for example, test device release code.
>
> At first glance, nice work.  But looks like 0-day doesn't like it that
> much, so I'll wait for the next version to review it properly.

Thanks very much for taking a look. I'll send v2 with the 0-day (and
other) issues fixed sometime tomorrow.

In the meantime, I've tried to explain some of the weirder decisions
below -- it mostly boils down to the existing use-cases only wanting
an opaque 'struct device *' they can pass around, and my attempt to
find a minimal (but still sensible) implementation of that. I'm
definitely happy to tweak this to make it a more 'normal' use of the
device model where that makes sense, though, especially if it doesn't
require too much boilerplate on the part of test authors.

> One nit I did notice:
>
> > +// For internal use only -- registers the kunit_bus.
> > +int kunit_bus_init(void);
>
> Put stuff like this in a local .h file, don't pollute the include/linux/
> files for things that you do not want any other part of the kernel to
> call.
>

v2 will have this in lib/kunit/device-impl.h

> > +/**
> > + * kunit_device_register_with_driver() - Create a struct device for use in KUnit tests
> > + * @test: The test context object.
> > + * @name: The name to give the created device.
> > + * @drv: The struct device_driver to associate with the device.
> > + *
> > + * Creates a struct kunit_device (which is a struct device) with the given
> > + * name, and driver. The device will be cleaned up on test exit, or when
> > + * kunit_device_unregister is called. See also kunit_device_register, if you
> > + * wish KUnit to create and manage a driver for you
> > + */
> > +struct device *kunit_device_register_with_driver(struct kunit *test,
> > +                                              const char *name,
> > +                                              struct device_driver *drv);
>
> Shouldn't "struct device_driver *" be a constant pointer?

Done (and for the internal functions) for v2.
>
> But really, why is this a "raw" device_driver pointer and not a pointer
> to the driver type for your bus?

So, this is where the more difficult questions start (and where my
knowledge of the driver model gets a bit shakier).

At the moment, there's no struct kunit_driver; the goal was to have
whatever the minimal amount of infrastructure needed to get a 'struct
device *' that could be plumbed through existing code which accepts
it. (Read: mostly devres resource management stuff, get_device(),
etc.)

So, in this version, there's no:
- struct kunit_driver: we've no extra data to store / function
pointers other than what's in struct device_driver.
- The kunit_bus is as minimal as I could get it: each device matches
exactly one driver pointer (which is passed as struct
kunit_device->driver).
- The 'struct kunit_device' type is kept private, and 'struct device'
is used instead, as this is supposed to only be passed to generic
device functions (KUnit is just managing its lifecycle).

I've no problem adding these extra types to flesh this out into a more
'normal' setup, though I'd rather keep the boilerplate on the user
side minimal if possible. I suspect if we were to return a struct
kunit_device, everyone would be quickly grabbing and passing around a
raw 'struct device *' anyway, which is what the existing tests with
fake devices do (via root_device_register, which returns struct
device, or by returning &platform_device->dev from a helper).

Similarly, the kunit_bus is not ever exposed to test code, nor really
is the driver (except via kunit_device_register_with_driver(), which
isn't actually being used anywhere yet, so it may make sense to cut it
out from the next version). So, ideally tests won't even be aware that
their devices are attached to the kunit_bus, nor that they have
drivers attached: it's mostly just to make these normal enough that
they show up nicely in sysfs and play well with the devm_ resource
management functions.

>
> Oh heck, let's point out the other issues as I'm already here...
>
> > @@ -7,7 +7,8 @@ kunit-objs +=                         test.o \
> >                                       assert.o \
> >                                       try-catch.o \
> >                                       executor.o \
> > -                                     attributes.o
> > +                                     attributes.o \
> > +                                     device.o
>
> Shouldn't this file be "bus.c" as you are creating a kunit bus?
>

I've sort-of grouped this as "device helpers", as it handles KUnit
devices, drivers, and the kunit_bus, but devices are the most
user-facing part. Indeed, the bus feels like an 'implementation
detail'. Happy to rename it if that makes things more consistent,
though.

> >
> >  ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
> >  kunit-objs +=                                debugfs.o
> > diff --git a/lib/kunit/device.c b/lib/kunit/device.c
> > new file mode 100644
> > index 000000000000..93ace1a2297d
> > --- /dev/null
> > +++ b/lib/kunit/device.c
> > @@ -0,0 +1,176 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * KUnit basic device implementation
>
> "basic bus/driver implementation", not device, right?
>

Given that the users of this really only care about getting their
"device", and the bus/driver are more implementation details, I'd
rather go with something like "KUnit-managed device implementation" or
"KUnit device-model helpers". How do those sound?

> > + *
> > + * Implementation of struct kunit_device helpers.
> > + *
> > + * Copyright (C) 2023, Google LLC.
> > + * Author: David Gow <davidgow@xxxxxxxxxx>
> > + */
> > +
> > +#include <linux/device.h>
> > +
> > +#include <kunit/test.h>
> > +#include <kunit/device.h>
> > +#include <kunit/resource.h>
> > +
> > +
> > +/* Wrappers for use with kunit_add_action() */
> > +KUNIT_DEFINE_ACTION_WRAPPER(device_unregister_wrapper, device_unregister, struct device *);
> > +KUNIT_DEFINE_ACTION_WRAPPER(driver_unregister_wrapper, driver_unregister, struct device_driver *);
> > +
> > +static struct device kunit_bus = {
> > +     .init_name = "kunit"
> > +};
>
> A static device as a bus?  This feels wrong, what is it for?  And where
> does this live?  If you _REALLY_ want a single device for the root of
> your bus (which is a good idea), then make it a dynamic variable (as it
> is reference counted), NOT a static struct device which should not be
> done if at all possible.

Will do. Would root_device_register() make more sense here?

>
> > +
> > +/* A device owned by a KUnit test. */
> > +struct kunit_device {
> > +     struct device dev;
> > +     struct kunit *owner;
> > +     /* Force binding to a specific driver. */
> > +     struct device_driver *driver;
> > +     /* The driver is managed by KUnit and unique to this device. */
> > +     bool cleanup_driver;
> > +};
>
> Wait, why isn't your "kunit" device above a struct kunit_device
> structure?  Why is it ok to be a "raw" struct device (hint, that's
> almost never a good idea.)
>
> > +static inline struct kunit_device *to_kunit_device(struct device *d)
> > +{
> > +     return container_of(d, struct kunit_device, dev);
>
> container_of_const()?  And to use that properly, why not make this a #define?
>
> > +}
> > +
> > +static int kunit_bus_match(struct device *dev, struct device_driver *driver)
> > +{
> > +     struct kunit_device *kunit_dev = to_kunit_device(dev);
> > +
> > +     if (kunit_dev->driver == driver)
> > +             return 1;
> > +
> > +     return 0;
>
> I don't understand, what are you trying to match here?
>

This is just to make sure that the match function will use whatever
driver is passed through. When I originally started writing this,
there were some resource cleanup problems if there was no driver
associated with a device, though that's fixed now.

> > +}
> > +
> > +static struct bus_type kunit_bus_type = {
> > +     .name           = "kunit",
> > +     .match          = kunit_bus_match
> > +};
> > +
> > +int kunit_bus_init(void)
> > +{
> > +     int error;
> > +
> > +     error = bus_register(&kunit_bus_type);
> > +     if (!error) {
> > +             error = device_register(&kunit_bus);
> > +             if (error)
> > +                     bus_unregister(&kunit_bus_type);
> > +     }
> > +     return error;
> > +}
> > +late_initcall(kunit_bus_init);
> > +
> > +static void kunit_device_release(struct device *d)
> > +{
> > +     kfree(to_kunit_device(d));
> > +}
> > +
> > +struct device_driver *kunit_driver_create(struct kunit *test, const char *name)
> > +{
> > +     struct device_driver *driver;
> > +     int err = -ENOMEM;
> > +
> > +     driver = kunit_kzalloc(test, sizeof(*driver), GFP_KERNEL);
> > +
> > +     if (!driver)
> > +             return ERR_PTR(err);
> > +
> > +     driver->name = name;
> > +     driver->bus = &kunit_bus_type;
> > +     driver->owner = THIS_MODULE;
> > +
> > +     err = driver_register(driver);
> > +     if (err) {
> > +             kunit_kfree(test, driver);
> > +             return ERR_PTR(err);
> > +     }
> > +
> > +     kunit_add_action(test, driver_unregister_wrapper, driver);
> > +     return driver;
> > +}
> > +EXPORT_SYMBOL_GPL(kunit_driver_create);
> > +
> > +struct kunit_device *__kunit_device_register_internal(struct kunit *test,
> > +                                                   const char *name,
> > +                                                   struct device_driver *drv)
> > +{
> > +     struct kunit_device *kunit_dev;
> > +     int err = -ENOMEM;
> > +
> > +     kunit_dev = kzalloc(sizeof(struct kunit_device), GFP_KERNEL);
> > +     if (!kunit_dev)
> > +             return ERR_PTR(err);
> > +
> > +     kunit_dev->owner = test;
> > +
> > +     err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
> > +     if (err) {
> > +             kfree(kunit_dev);
> > +             return ERR_PTR(err);
> > +     }
> > +
> > +     /* Set the expected driver pointer, so we match. */
> > +     kunit_dev->driver = drv;
>
> Ah, so this is the match function to pass above?  If so, why do you need
> it at all?

This is just to make sure there's a driver attached, so that
driver_detach() eventually gets called on the device, which used to be
required to clean up resources in some circumstances.
This has since been fixed in 699fb50d9903 ("drivers: base: Free devm
resources when unregistering a device"), but it seemed worth keeping
it both to make these devices seem "more normal", and potentially to
facilitate users providing a struct device_driver themselves later on,
should that one day be useful.

> > +
> > +     kunit_dev->dev.release = kunit_device_release;
> > +     kunit_dev->dev.bus = &kunit_bus_type;
> > +     kunit_dev->dev.parent = &kunit_bus;
> > +
> > +     err = device_register(&kunit_dev->dev);
> > +     if (err) {
> > +             put_device(&kunit_dev->dev);
> > +             return ERR_PTR(err);
> > +     }
> > +
> > +     kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
> > +
> > +     return kunit_dev;
> > +}
> > +
> > +struct device *kunit_device_register_with_driver(struct kunit *test,
> > +                                              const char *name,
> > +                                              struct device_driver *drv)
> > +{
> > +     struct kunit_device *kunit_dev = __kunit_device_register_internal(test, name, drv);
> > +
> > +     if (IS_ERR_OR_NULL(kunit_dev))
>
> This is almost always a sign that something is wrong with the api.

This can probably just be IS_ERR().

>
> > +             return (struct device *)kunit_dev; /* This is an error or NULL, so is compatible */
>
> Ick, the cast is odd, are you sure you need it?  Why would you return a
> struct device and not a kunit_device() anyway?
>

All the users of this only want the struct device, so it's more
convenient if that's all we return (and it lets us keep struct
kunit_device private). But it's a minor inconvenience at worst, so I
don't mind changing it.

> > +
> > +     return &kunit_dev->dev;
>
> Again, why this type, why not use the real type you have?

As above, to keep 'struct kunit_device' private.


Thanks again for looking at this; I'd definitely appreciate any
further input you may have on the design.

Cheers,
-- David

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature