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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABVgOSmx3A4Vwos2_8xO-XQrQAw5gvY0nc5zLpLmcJ7FtA-dTQ@mail.gmail.com>
Date:   Fri, 24 Mar 2023 17:52:01 +0800
From:   David Gow <davidgow@...gle.com>
To:     Matti Vaittinen <mazziesaccount@...il.com>
Cc:     Maxime Ripard <maxime@...no.tech>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Brendan Higgins <brendan.higgins@...ux.dev>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        kunit-dev@...glegroups.com, Stephen Boyd <sboyd@...nel.org>,
        Jonathan Cameron <jic23@...nel.org>, linux-iio@...r.kernel.org
Subject: Re: [PATCH v5 1/8] drivers: kunit: Generic helpers for test device creation

On Fri, 24 Mar 2023 at 14:51, Matti Vaittinen <mazziesaccount@...il.com> wrote:
>
> On 3/24/23 08:34, David Gow wrote:
> > On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen <mazziesaccount@...il.com> wrote:
> >>
> >> On 3/23/23 18:36, Maxime Ripard wrote:
> >>> On Thu, Mar 23, 2023 at 03:02:03PM +0200, Matti Vaittinen wrote:
> >>>> On 3/23/23 14:29, Maxime Ripard wrote:
> >>>>> On Thu, Mar 23, 2023 at 02:16:52PM +0200, Matti Vaittinen wrote:
>
> >> Ok. Fair enough. Besides, if the root-device was sufficient - then I
> >> would actually not see the need for a helper. People could in that case
> >> directly use the root_device_register(). So, if helpers are provided
> >> they should be backed up by a device with a bus then.
> >
> > I think there is _some_ value in helpers even without a bus, but it's
> > much more limited:
> > - It's less confusing if KUnit test devices are using kunit labelled
> > structs and functions.
> > - Helpers could use KUnit's resource management API to ensure any
> > device created is properly unregistered and removed when the test
> > exits (particularly if it exits early due to, e.g., an assertion).
>
> Ah. That's true. Being able to abort the test on error w/o being forced
> to do a clean-up dance for the dummy device would be convenient.
>
> > I've played around implementing those with a proper struct
> > kunit_device and the automatic cleanup on test failure, and thus far
> > it -- like root_device_register -- works for all of the tests except
> > the drm-test-managed one.
> >
> > So if we really wanted to, we could use KUnit-specific helpers for
> > just those tests which currently work with root_device_register(), but
> > if we're going to try to implement a KUnit bus -- which I think is at
> > least worth investigating -- I'd rather not either hold up otherwise
> > good tests on helper development, or rush a helper out only to have to
> > change it a lot when we see exactly what the bus implementation would
> > look like.
>
> It's easy for me to agree.
>
> >> As I said, in my very specific IIO related test the test device does not
> >> need a bus. Hence I'll drop the 'generic helpers' from this series.
> >>
> >
> > I think that sounds like a good strategy for now, and we can work on a
> > set of 'generic helpers' which have an associated bus and struct
> > kunit_device in the meantime. If we can continue to use
> > root_device_register until those are ready, that'd be very convenient.
>
> Would it be a tiny bit more acceptable if we did add a very simple:
>
> #define kunit_root_device_register(name) root_device_register(name)
> #define kunit_root_device_unregister(dev) root_device_unregister(dev)
>
> to include/kunit/device.h (or somesuch)
>
> This should help us later to at least spot the places where
> root_device_[un]register() is abused and (potentially mass-)covert them
> to use the proper helpers when they're available.
>

Great idea.

The code I've been playing with has the following in include/kunit/device.h:

/* Register a new device against a KUnit test. */
struct device *kunit_device_register(struct kunit *test, const char *name);
/* Unregister a device created by kunit_device_register() early (i.e.,
before test cleanup). */
void kunit_device_unregister(struct kunit *test, struct device *dev);

If we used the same names, and just forwarded them to
root_device_register() and root_device_unregister() for now
(discarding the struct kunit pointer), then I expect we could just
swap out the implementation to gain the extra functionality.

It's a little less explicit, though, so I could see the value in using
macros with "root_device" in the name to make the current
implementation clearer, and the eventual change more obvious.

Cheers,
-- David

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4003 bytes)