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

[David Gow <davidgow@google.com>]

[-- Attachment #1: Type: text/plain, Size: 2897 bytes --]

On Fri, 24 Mar 2023 at 18:17, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
> On 3/24/23 12:05, Matti Vaittinen wrote:
> > On 3/24/23 11:52, David Gow wrote:
> >> On Fri, 24 Mar 2023 at 14:51, Matti Vaittinen
> >> <mazziesaccount@gmail.com> wrote:
> >>>
> >>> On 3/24/23 08:34, David Gow wrote:
> >>>> On Fri, 24 Mar 2023 at 14:11, Matti Vaittinen
> >>>> <mazziesaccount@gmail.com> wrote:
> >
> >>>> 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.
>
> There's one thing though. If the goal is to do a direct replacement and
> if automatic device deletion upon test completion / test abort is
> planned - then it should be there also for these initial wrappers.
>

Yeah, that's an excellent point. It's a pretty subtle change in
behaviour to suddenly introduce that, so changing it behind the scenes
is probably unwise.

> If these wrappers don't yet include the automatic device clean-up - then
> it probably makes more sense to just do the kunit_root_device_* defines
> because the tests are likely to need removing the explicit device
> clean-ups when proper APIs are finished.
>

I sent out my prototype implementation of this here, which does do the
automatic cleanup:
https://lore.kernel.org/linux-kselftest/20230325043104.3761770-1-davidgow@google.com/T/#mf797239a8bce11630875fdf60aab9ed627add1f0

It's probably overkill to squeeze into your patch series, though,
given it also adds and uses a whole new kunit_defer() API.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]