Message-ID: <15345b349077f57935e8f1d3d69f66f9.sboyd@kernel.org>
Date: Thu, 02 May 2024 17:36:12 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: David Gow <davidgow@...gle.com>
Cc: Michael Turquette <mturquette@...libre.com>, linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org, patches@...ts.linux.dev, kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org, devicetree@...r.kernel.org, Brendan Higgins <brendan.higgins@...ux.dev>, Rae Moar <rmoar@...gle.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Rafael J . Wysocki <rafael@...nel.org>, Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>, Daniel Latypov <dlatypov@...gle.com>, Christian Marangi <ansuelsmth@...il.com>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, Maxime Ripard <maxime@...no.tech>
Subject: Re: [PATCH v4 01/10] of: Add test managed wrappers for of_overlay_apply()/of_node_put()

Quoting David Gow (2024-05-01 00:55:10)
> On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <sboyd@...nel.org> wrote:
> > diff --git a/Documentation/dev-tools/kunit/api/index.rst b/Documentation/dev-tools/kunit/api/index.rst
> > index 2d8f756aab56..282befa17edf 100644
> > --- a/Documentation/dev-tools/kunit/api/index.rst
> > +++ b/Documentation/dev-tools/kunit/api/index.rst
> > @@ -9,11 +9,15 @@ API Reference
> >         test
> >         resource
> >         functionredirection
> > +       of
> >
> >
> >  This page documents the KUnit kernel testing API. It is divided into the
> >  following sections:
> >
> > +Core KUnit API
> > +==============
> > +
> >  Documentation/dev-tools/kunit/api/test.rst
> >
> >   - Documents all of the standard testing API
> > @@ -25,3 +29,10 @@ Documentation/dev-tools/kunit/api/resource.rst
> >  Documentation/dev-tools/kunit/api/functionredirection.rst
> >
> >   - Documents the KUnit Function Redirection API
> > +
> > +Driver KUnit API
> > +================
> 
> If we're adding a separate 'Driver' section here, it's probably
> sensible to move the existing device/driver helper documentation here,
> rather than leaving it in resource.rst as-is. I'm happy to do that in
> a follow-up patch, though.

To clarify, you're talking about "Managed Devices"? Looks like that can
be a follow-up to split it into a new file and then put it here. If
you're happy to do that then I'll leave it to you.

> 
> > +
> > +Documentation/dev-tools/kunit/api/of.rst
> > +
> > + - Documents the KUnit device tree (OF) API
> > diff --git a/Documentation/dev-tools/kunit/api/of.rst b/Documentation/dev-tools/kunit/api/of.rst
> > new file mode 100644
> > index 000000000000..8587591c3e78
> > --- /dev/null
> > +++ b/Documentation/dev-tools/kunit/api/of.rst
> > @@ -0,0 +1,13 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +====================
> > +Device Tree (OF) API
> > +====================
> > +
> > +The KUnit device tree API is used to test device tree (of_*) dependent code.
> > +
> > +.. kernel-doc:: include/kunit/of.h
> > +   :internal:
> > +
> > +.. kernel-doc:: drivers/of/of_kunit.c
> > +   :export:
> > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > index 251d33532148..0dfd05079313 100644
> > --- a/drivers/of/Makefile
> > +++ b/drivers/of/Makefile
> > @@ -19,6 +19,7 @@ obj-y += kexec.o
> >  endif
> >  endif
> >
> > +obj-$(CONFIG_KUNIT) += of_kunit.o
> 
> I'm tempted to have this either live in lib/kunit, or be behind a
> separate Kconfig option, particularly since this will end up as a
> separate module, as-is.

Is the idea to have a single module that has all the kunit "stuff" in it
so we can just load one module and be done? Is there any discussion on
the list I can read to see the argument for this?

> 
> >  obj-$(CONFIG_OF_KUNIT_TEST) += of_test.o
> >
> >  obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> > diff --git a/drivers/of/of_kunit.c b/drivers/of/of_kunit.c
> > new file mode 100644
> > index 000000000000..f63527268a51
> > --- /dev/null
> > +++ b/drivers/of/of_kunit.c
> > @@ -0,0 +1,99 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Test managed device tree APIs
> > + */
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_fdt.h>
> > +
> > +#include <kunit/of.h>
> > +#include <kunit/test.h>
> > +#include <kunit/resource.h>
> > +
> > +static void of_overlay_fdt_apply_kunit_exit(void *ovcs_id)
> > +{
> > +       of_overlay_remove(ovcs_id);
> > +}
> > +
> > +/**
> > + * of_overlay_fdt_apply_kunit() - Test managed of_overlay_fdt_apply()
> > + * @test: test context
> > + * @overlay_fdt: device tree overlay to apply
> > + * @overlay_fdt_size: size in bytes of @overlay_fdt
> > + * @ovcs_id: identifier of overlay, used to remove the overlay
> > + *
> > + * Just like of_overlay_fdt_apply(), except the overlay is managed by the test
> > + * case and is automatically removed with of_overlay_remove() after the test
> > + * case concludes.
> > + *
> > + * Return: 0 on success, negative errno on failure
> > + */
> > +int of_overlay_fdt_apply_kunit(struct kunit *test, void *overlay_fdt,
> > +                              u32 overlay_fdt_size, int *ovcs_id)
> 
> We're using kunit_ as a prefix for the device helpers (e.g.
> kunit_device_register()), so it may make sense to do that here, too.
> It's not as important as with the platform_device helpers, which are
> very similar to the existing device ones, but if we want to treat
> these as "part of KUnit which deals with of_overlays", rather than
> "part of "of_overlay which deals with KUnit", this may fit better.
> 
> Thoughts?

I'm fine either way with the name. I recall that last time we put a
kunit postfix to make it easier to tab complete or something like that.

I find it hard to understand the distinction you're trying to make
though. I guess you're saying the difference is what subsystem maintains
the code, kunit or of. When they're simple wrappers it is easier to
extract them out to lib/kunit and thus they can (should?) have the kunit
prefix. Maybe that always holds true, because kunit wrappers are
typically another API consumer, and if the API is exported either in a
linux/ header or as an exported symbol it can be wrapped in lib/kunit
easily. Did I follow correctly? When would of_overlay ever deal with
KUnit?

> > diff --git a/include/kunit/of.h b/include/kunit/of.h
> > new file mode 100644
> > index 000000000000..9981442ba578
> > --- /dev/null
> > +++ b/include/kunit/of.h
> > @@ -0,0 +1,94 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _KUNIT_OF_H
> > +#define _KUNIT_OF_H
> > +
> > +#include <kunit/test.h>
> > +
> > +struct device_node;
> > +
> > +#ifdef CONFIG_OF
> 
> Do we also need to check for CONFIG_OF_OVERLAY here?
> 
> Also, how useful is it to compile but skip tests without
> CONFIG_OF{,_OVERLAY} enabled? The other option is a compile error,
> which may make it more obvious that these are disabled if it's
> unexpected.
> 
> Thoughts?

I've tried to make it so that tests skip if an option isn't enabled. I
suppose the CONFIG_OF_OVERLAY check can be hoisted up here as well so
that the skip isn't buried in lower levels.

Powered by blists - more mailing lists