Re: [PATCH v5 05/11] of: Add a KUnit test for overlays and test managed APIs

[Stephen Boyd <sboyd@kernel.org>]

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 = "kunit-test";
> > +static const char * const kunit_compatible = "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 = 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 = of_find_node_by_name(NULL, kunit_node_name);
> > +     of_node_put_kunit(test, np);
> 
> Moving target, but we now have of_node_put() cleanups. Would that work 
> 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.

> 
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
> > +
> > +     pdev = 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 void *data)
> > +{
> > +     return of_device_is_compatible(dev->of_node, data);
> > +}
> > +
> > +/* Test that of_overlay_apply_kunit() cleans up after the test is finished */
> > +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 root 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 = 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 = of_find_device_by_node(np);
> 
> 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.