Received-SPF: pass (google.com: domain of linux-kernel+bounces-204568-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <025da51b45c5234dc05c4bfc6b0570e5.sboyd@kernel.org>
Content-Type: text/plain; charset="utf-8"
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20240605234720.GA3441001-robh@kernel.org>
References: <20240603223811.3815762-1-sboyd@kernel.org> <20240603223811.3815762-6-sboyd@kernel.org> <20240605234720.GA3441001-robh@kernel.org>
Subject: Re: [PATCH v5 05/11] of: Add a KUnit test for overlays and test managed APIs
From: Stephen Boyd <sboyd@kernel.org>
Cc: Michael Turquette <mturquette@baylibre.com>, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org, patches@lists.linux.dev, kunit-dev@googlegroups.com, linux-kselftest@vger.kernel.org, devicetree@vger.kernel.org, Brendan Higgins <brendan.higgins@linux.dev>, David Gow <davidgow@google.com>, Rae Moar <rmoar@google.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Rafael J . Wysocki <rafael@kernel.org>, Saravana Kannan <saravanak@google.com>, Daniel Latypov <dlatypov@google.com>, Christian Marangi <ansuelsmth@gmail.com>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, Maxime Ripard <maxime@cerno.tech>
To: Rob Herring <robh@kernel.org>
Date: Thu, 06 Jun 2024 08:23:30 -0700
User-Agent: alot/0.10

Quoting Rob Herring (2024-06-05 16:47:20)
> On Mon, Jun 03, 2024 at 03:38:02PM -0700, Stephen Boyd wrote:
> > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > index 2ae909adde49..abd9c578343b 100644
> > diff --git a/drivers/of/overlay_test.c b/drivers/of/overlay_test.c
> > new file mode 100644
> > index 000000000000..9a8083c3a659
> > --- /dev/null
> > +++ b/drivers/of/overlay_test.c
> > @@ -0,0 +1,116 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * KUnit tests for device tree overlays
> > + */
> > +#include <linux/device/bus.h>
> > +#include <linux/kconfig.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <kunit/of.h>
> > +#include <kunit/test.h>
> > +
> > +static const char * const kunit_node_name =3D "kunit-test";
> > +static const char * const kunit_compatible =3D "test,empty";
> > +
> > +/* Test that of_overlay_apply_kunit() adds a node to the live tree */
> > +static void of_overlay_apply_kunit_apply(struct kunit *test)
> > +{
> > +     struct device_node *np;
> > +
> > +     KUNIT_ASSERT_EQ(test, 0,
> > +                     of_overlay_apply_kunit(test, kunit_overlay_test));
> > +
> > +     np =3D of_find_node_by_name(NULL, kunit_node_name);
> > +     KUNIT_EXPECT_NOT_ERR_OR_NULL(test, np);
> > +     of_node_put(np);
> > +}
> > +
> > +/*
> > + * Test that of_overlay_apply_kunit() creates platform devices with the
> > + * expected device_node
> > + */
> > +static void of_overlay_apply_kunit_platform_device(struct kunit *test)
> > +{
> > +     struct platform_device *pdev;
> > +     struct device_node *np;
> > +
> > +     KUNIT_ASSERT_EQ(test, 0,
> > +                     of_overlay_apply_kunit(test, kunit_overlay_test));
> > +
> > +     np =3D of_find_node_by_name(NULL, kunit_node_name);
> > +     of_node_put_kunit(test, np);
>=20
> Moving target, but we now have of_node_put() cleanups. Would that work=20
> here instead?

Do you mean cleanup.h? I don't think it will work. The assert logic is
like an exception handler. If the assertion fails we basically jump out
of the test and run any test exit code, including kunit resource exits.
I could introduce another kunit wrapper for of_find_node_by_name() and
use that here so that the reference is dropped when the test exits.

>=20
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> > +
> > +     pdev =3D of_find_device_by_node(np);
> > +     KUNIT_EXPECT_NOT_ERR_OR_NULL(test, pdev);
> > +     if (pdev)
> > +             put_device(&pdev->dev);
> > +}
> > +
> > +static int of_overlay_bus_match_compatible(struct device *dev, const v=
oid *data)
> > +{
> > +     return of_device_is_compatible(dev->of_node, data);
> > +}
> > +
> > +/* Test that of_overlay_apply_kunit() cleans up after the test is fini=
shed */
> > +static void of_overlay_apply_kunit_cleanup(struct kunit *test)
> > +{
> > +     struct kunit fake;
> > +     struct platform_device *pdev;
> > +     struct device *dev;
> > +     struct device_node *np;
> > +
> > +     if (!IS_ENABLED(CONFIG_OF_OVERLAY))
> > +             kunit_skip(test, "requires CONFIG_OF_OVERLAY");
> > +     if (!IS_ENABLED(CONFIG_OF_EARLY_FLATTREE))
> > +             kunit_skip(test, "requires CONFIG_OF_EARLY_FLATTREE for r=
oot node");
> > +
> > +     kunit_init_test(&fake, "fake test", NULL);
> > +     KUNIT_ASSERT_EQ(test, fake.status, KUNIT_SUCCESS);
> > +
> > +     KUNIT_ASSERT_EQ(test, 0,
> > +                     of_overlay_apply_kunit(&fake, kunit_overlay_test)=
);
> > +
> > +     np =3D of_find_node_by_name(NULL, kunit_node_name);
> > +     of_node_put(np); /* Not derefing 'np' after this */
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> > +
> > +     pdev =3D of_find_device_by_node(np);
>=20
> Don't you need to hold a ref on np until here?

Oh, good catch. We need an of_find_node_by_name_kunit() wrapper then.