Received-SPF: pass (google.com: domain of linux-kernel+bounces-178295-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <31d4dfdf8bc4b866a2b6d45fc4de0c2d.sboyd@kernel.org>
Content-Type: text/plain; charset="utf-8"
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20240422232404.213174-6-sboyd@kernel.org>
References: <20240422232404.213174-1-sboyd@kernel.org> <20240422232404.213174-6-sboyd@kernel.org>
Subject: Re: [PATCH v4 05/10] platform: Add test managed platform_device/driver APIs
From: Stephen Boyd <sboyd@kernel.org>
Cc: 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>, Rob Herring <robh@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: Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>
Date: Mon, 13 May 2024 20:35:49 -0700
User-Agent: alot/0.10

Quoting Stephen Boyd (2024-04-22 16:23:58)
> diff --git a/drivers/base/test/platform_kunit.c b/drivers/base/test/platf=
orm_kunit.c
> new file mode 100644
> index 000000000000..54af6db2a6d8
> --- /dev/null
> +++ b/drivers/base/test/platform_kunit.c
> @@ -0,0 +1,174 @@
[...]
> +struct platform_device *
> +platform_device_alloc_kunit(struct kunit *test, const char *name, int id)
> +{
> +       struct platform_device *pdev;
> +
> +       pdev =3D platform_device_alloc(name, id);
> +       if (!pdev)
> +               return NULL;
> +
> +       if (kunit_add_action_or_reset(test, (kunit_action_t *)&platform_d=
evice_put, pdev))
> +               return NULL;
> +
> +       return pdev;
> +}
> +EXPORT_SYMBOL_GPL(platform_device_alloc_kunit);
> +
> +static void platform_device_add_kunit_exit(struct kunit_resource *res)
> +{
> +       struct platform_device *pdev =3D res->data;
> +
> +       platform_device_unregister(pdev);
> +}
> +
> +static bool
> +platform_device_alloc_kunit_match(struct kunit *test,
> +                                 struct kunit_resource *res, void *match=
_data)
> +{
> +       struct platform_device *pdev =3D match_data;
> +
> +       return res->data =3D=3D pdev;
> +}
> +
> +/**
> + * platform_device_add_kunit() - Register a KUnit test managed platform =
device
> + * @test: test context
> + * @pdev: platform device to add
> + *
> + * Register a test managed platform device. The device is unregistered w=
hen the
> + * test completes.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int platform_device_add_kunit(struct kunit *test, struct platform_device=
 *pdev)
> +{
> +       struct kunit_resource *res;
> +       int ret;
> +
> +       ret =3D platform_device_add(pdev);
> +       if (ret)
> +               return ret;
> +
> +       res =3D kunit_find_resource(test, platform_device_alloc_kunit_mat=
ch, pdev);

This doesn't work because platform_device_alloc_kunit() used
kunit_add_action_or_reset() which has a chained free routine and data
pointer. I've added a test to make sure the platform device is removed
from the bus. It's not super great though because when this code fails
to find a match it will still remove the device by calling
platform_device_unregister() when the test ends. It will follow that up
with a call to platform_device_put(), which is the problem as that
causes an underflow and operates on an already freed device.

I couldn't come up with anything better than searching the platform bus.
Maybe if there was a way to allocate the memory or redirect where
platform_device_alloc_kunit() got memory from we could hold the device
memory around after it should have been freed and make sure the kref for
the device kobject is 0. That seems pretty invasive to do though so I'm
just going to leave it for now and add this test to make sure it cleans
up.