Message-ID: <ysws7wkfqxfve52yiy5eiiez65tch3qefs3a6o36dipicl7tvq@ettzrxh2lhpk>
Date:   Thu, 8 Jun 2023 09:59:53 +0200
From:   Maxime Ripard <mripard@...nel.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Brendan Higgins <brendan.higgins@...ux.dev>,
        David Gow <davidgow@...gle.com>,
        linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND 1/2] drivers: base: Add basic devm tests for root
 devices

On Wed, Jun 07, 2023 at 09:14:15PM +0200, Greg Kroah-Hartman wrote:
> On Sun, Jun 04, 2023 at 10:31:42AM +0200, Maxime Ripard wrote:
> > Hi,
> > 
> > On Sat, Jun 03, 2023 at 04:43:51PM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Jun 02, 2023 at 05:20:43PM +0200, Maxime Ripard wrote:
> > > > From: Maxime Ripard <maxime@...no.tech>
> > > > 
> > > > The root devices show some odd behaviours compared to regular "bus" devices
> > > > that have been probed through the usual mechanism, so let's create kunit
> > > > tests to exercise those paths and odd cases.
> > > > 
> > > > Signed-off-by: Maxime Ripard <maxime@...no.tech>
> > > > ---
> > > >  drivers/base/test/.kunitconfig       |   2 +
> > > >  drivers/base/test/Kconfig            |   4 ++
> > > >  drivers/base/test/Makefile           |   2 +
> > > >  drivers/base/test/root-device-test.c | 120 +++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 128 insertions(+)
> > > > 
> > > > diff --git a/drivers/base/test/.kunitconfig b/drivers/base/test/.kunitconfig
> > > > new file mode 100644
> > > > index 000000000000..473923f0998b
> > > > --- /dev/null
> > > > +++ b/drivers/base/test/.kunitconfig
> > > > @@ -0,0 +1,2 @@
> > > > +CONFIG_KUNIT=y
> > > > +CONFIG_DM_KUNIT_TEST=y
> > > > diff --git a/drivers/base/test/Kconfig b/drivers/base/test/Kconfig
> > > > index 610a1ba7a467..9d42051f8f8e 100644
> > > > --- a/drivers/base/test/Kconfig
> > > > +++ b/drivers/base/test/Kconfig
> > > > @@ -9,6 +9,10 @@ config TEST_ASYNC_DRIVER_PROBE
> > > >  
> > > >  	  If unsure say N.
> > > >  
> > > > +config DM_KUNIT_TEST
> > > > +	tristate "KUnit Tests for the device model" if !KUNIT_ALL_TESTS
> > > > +	depends on KUNIT
> > > > +
> > > >  config DRIVER_PE_KUNIT_TEST
> > > >  	bool "KUnit Tests for property entry API" if !KUNIT_ALL_TESTS
> > > >  	depends on KUNIT=y
> > > > diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
> > > > index 7f76fee6f989..d589ca3fa8fc 100644
> > > > --- a/drivers/base/test/Makefile
> > > > +++ b/drivers/base/test/Makefile
> > > > @@ -1,5 +1,7 @@
> > > >  # SPDX-License-Identifier: GPL-2.0
> > > >  obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE)	+= test_async_driver_probe.o
> > > >  
> > > > +obj-$(CONFIG_DM_KUNIT_TEST)	+= root-device-test.o
> > > > +
> > > >  obj-$(CONFIG_DRIVER_PE_KUNIT_TEST) += property-entry-test.o
> > > >  CFLAGS_property-entry-test.o += $(DISABLE_STRUCTLEAK_PLUGIN)
> > > > diff --git a/drivers/base/test/root-device-test.c b/drivers/base/test/root-device-test.c
> > > > new file mode 100644
> > > > index 000000000000..fcb55d8882aa
> > > > --- /dev/null
> > > > +++ b/drivers/base/test/root-device-test.c
> > > > @@ -0,0 +1,120 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +// Copyright 2023 Maxime Ripard <mripard@...nel.org>
> > > > +
> > > > +#include <kunit/resource.h>
> > > > +
> > > > +#include <linux/device.h>
> > > > +
> > > > +#define DEVICE_NAME "test"
> > > > +
> > > > +struct test_priv {
> > > > +	bool probe_done;
> > > > +	bool release_done;
> > > > +	wait_queue_head_t release_wq;
> > > > +	struct device *dev;
> > > > +};
> > > > +
> > > > +static void devm_device_action(void *ptr)
> > > > +{
> > > > +	struct test_priv *priv = ptr;
> > > > +
> > > > +	priv->release_done = true;
> > > > +	wake_up_interruptible(&priv->release_wq);
> > > > +}
> > > > +
> > > > +static void devm_put_device_action(void *ptr)
> > > > +{
> > > > +	struct test_priv *priv = ptr;
> > > > +
> > > > +	put_device(priv->dev);
> > > > +	priv->release_done = true;
> > > > +	wake_up_interruptible(&priv->release_wq);
> > > > +}
> > > > +
> > > > +#define RELEASE_TIMEOUT_MS	500
> > > > +
> > > > +static void root_device_devm_register_unregister_test(struct kunit *test)
> > > > +{
> > > > +	struct test_priv *priv;
> > > > +	int ret;
> > > > +
> > > > +	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> > > > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
> > > > +	init_waitqueue_head(&priv->release_wq);
> > > > +
> > > > +	priv->dev = root_device_register(DEVICE_NAME);
> > > > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
> > > > +
> > > > +	ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
> > > > +	KUNIT_ASSERT_EQ(test, ret, 0);
> > > > +
> > > > +	root_device_unregister(priv->dev);
> > > > +
> > > > +	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
> > > > +					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> > > > +	KUNIT_EXPECT_GT(test, ret, 0);
> > > > +}
> > > > +
> > > > +static void root_device_devm_register_get_put_unregister_test(struct kunit *test)
> > > > +{
> > > > +	struct test_priv *priv;
> > > > +	int ret;
> > > > +
> > > > +	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> > > > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
> > > > +	init_waitqueue_head(&priv->release_wq);
> > > > +
> > > > +	priv->dev = root_device_register(DEVICE_NAME);
> > > > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
> > > > +
> > > > +	get_device(priv->dev);
> > > 
> > > Why are you incrementing the reference here?
> > > 
> > > > +
> > > > +	ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
> > > > +	KUNIT_ASSERT_EQ(test, ret, 0);
> > > > +
> > > > +	put_device(priv->dev);
> > > 
> > > And then dropping it here?
> > > 
> > > What did that accomplish?  You shouldn't have needed to do that at all,
> > > right?
> > > 
> > > THat's all the difference from the previous function?  What is this
> > > testing?
> > > 
> > > 
> > > > +
> > > > +	root_device_unregister(priv->dev);
> > > > +
> > > > +	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
> > > > +					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> > > > +	KUNIT_EXPECT_GT(test, ret, 0);
> > > > +}
> > > > +
> > > > +static void root_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
> > > > +{
> > > > +	struct test_priv *priv;
> > > > +	int ret;
> > > > +
> > > > +	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> > > > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
> > > > +	init_waitqueue_head(&priv->release_wq);
> > > > +
> > > > +	priv->dev = root_device_register(DEVICE_NAME);
> > > > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
> > > > +
> > > > +	get_device(priv->dev);
> > > > +
> > > > +	ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
> > > > +	KUNIT_ASSERT_EQ(test, ret, 0);
> > > > +
> > > > +	root_device_unregister(priv->dev);
> > > > +
> > > > +	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
> > > > +					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> > > > +	KUNIT_EXPECT_GT(test, ret, 0);
> > > > +}
> > > > +
> > > > +static struct kunit_case root_device_devm_tests[] = {
> > > > +	KUNIT_CASE(root_device_devm_register_unregister_test),
> > > > +	KUNIT_CASE(root_device_devm_register_get_put_unregister_test),
> > > > +	KUNIT_CASE(root_device_devm_register_get_unregister_with_devm_test),
> > > 
> > > I can't figure out what you are trying to test here at all, which
> > > doesn't bode well for this patchset.
> > > 
> > > Can you document it better?  What should be happening (or not happening)
> > > that you are trying to ensure works properly?
> > > 
> > > All I see is a register/devm_something/unregister sequence and then wait
> > > for the device to be freed.  Am I missing something else?
> > 
> > So I guess most of the context was dropped since I first posted that
> > series (and I believe that the following will also answer the comment on
> > the other patch).
> 
> You have to have the context in the patch changelog itself, otherwise it
> is useless (remember, some of us review hundreds of patches a week, and
> have the short-term-memory of a squirrel.)

It wasn't really supposed to be merged either (in my mind at least). It
started as a PoC showing the inconcistencies and then you asked for it
to be submitted so you could play with it more easily.

> > It spawned from the discussion here:
> > https://lore.kernel.org/linux-kselftest/20230324123157.bbwvfq4gsxnlnfwb@houat/
> > 
> > Basically, depending on the bus (platform vs root devices), and whether
> > a driver was bound to the device or not, the device managed actions
> > might or might not run.
> 
> And is that correct?  I don't remember if we said it was or not.
> 
> So why test something we don't know if it should be?

You never really confirmed it was, and David and I couldn't really judge
whether it was expected or not. I think it's one, David too:
https://lore.kernel.org/linux-kselftest/CABVgOSn8H=9pQfY7Exc-e37Nm6u299AJLYup6R-_97v5Fb4bpQ@mail.gmail.com/

I don't have much experience with the core kernel, so I'm not definitive.

> > This lead us in DRM to create helpers that will register a platform
> > device and bind it to a dumb driver so that we can have the proper
> > behaviour (ie, when we free the device, the device managed actions are
> > executed).
> > 
> > We wanted to create generic helpers for kunit to create a new device
> > instance to run a test on, and you were (not surprisingly) not really
> > along with it. We discussed the above fact that the bus and bind-ness of
> > a device was affecting device managed actions, I provided a bunch of
> > kunit tests showing the inconsistencies that led to what we did in DRM,
> > and you offered to fix it if I submitted the tests.
> > 
> > https://lore.kernel.org/linux-kselftest/ZB2a291P5abeah6s@kroah.com/
> > 
> > And so here we are :)
> > 
> > Those tests are not doing much indeed but checking whether a device
> > managed action would run in various scenarii. If you run them, you'll
> > end up with:
> 
> Then document them please.  You can't have tests that aren't obvious
> what they are actually supposed to be testing, otherwise we have no idea
> if the test is correct or not (or if the code it is testing is correct.)
> 
> > $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/base/test/
> > [10:28:39] Configuring KUnit Kernel ...
> > Regenerating .config ...
> > Populating config with:
> > $ make ARCH=um O=.kunit olddefconfig
> > [10:28:40] Building KUnit Kernel ...
> > Populating config with:
> > $ make ARCH=um O=.kunit olddefconfig
> > Building with:
> > $ make ARCH=um O=.kunit --jobs=32
> > [10:28:50] Starting KUnit Kernel (1/1)...
> > [10:28:50] ============================================================
> > [10:28:50] ============== root-device-devm (3 subtests) ===============
> > [10:28:50] [PASSED] root_device_devm_register_unregister_test
> > [10:28:50] [PASSED] root_device_devm_register_get_put_unregister_test
> > [10:28:50] # root_device_devm_register_get_unregister_with_devm_test: EXPECTATION FAILED at drivers/base/test/root-device-test.c:105
> > [10:28:50] Expected ret > 0, but
> > [10:28:50]     ret == 0 (0x0)
> > [10:28:50] [FAILED] root_device_devm_register_get_unregister_with_devm_test
> > [10:28:50] # root-device-devm: pass:2 fail:1 skip:0 total:3
> > [10:28:50] # Totals: pass:2 fail:1 skip:0 total:3
> > [10:28:50] ================ [FAILED] root-device-devm =================
> > [10:28:50] ============ platform-device-devm (6 subtests) =============
> > [10:28:50] [PASSED] platform_device_devm_register_unregister_test
> > [10:28:51] [PASSED] platform_device_devm_register_get_put_unregister_test
> > [10:28:51] # platform_device_devm_register_get_unregister_with_devm_test: EXPECTATION FAILED at drivers/base/test/platform-device-test.c:123
> > [10:28:51] Expected ret > 0, but
> > [10:28:51]     ret == 0 (0x0)
> > [10:28:51] [FAILED] platform_device_devm_register_get_unregister_with_devm_test
> > [10:28:51] [PASSED] probed_platform_device_devm_register_unregister_test
> > [10:28:51] [PASSED] probed_platform_device_devm_register_get_put_unregister_test
> > [10:28:51] [PASSED] probed_platform_device_devm_register_get_unregister_with_devm_test
> > [10:28:51] # platform-device-devm: pass:5 fail:1 skip:0 total:6
> > [10:28:51] # Totals: pass:5 fail:1 skip:0 total:6
> > [10:28:51] ============== [FAILED] platform-device-devm ===============
> > [10:28:51] ============================================================
> > [10:28:51] Testing complete. Ran 9 tests: passed: 7, failed: 2
> > [10:28:51] Elapsed time: 11.701s total, 0.979s configuring, 9.601s building, 1.087s running
> > 
> > So you can see (and test) those inconsistencies: if you're using devm,
> > you need to have a "bus" device bound to a driver. Failing that, devm
> > actions will not run, which we all believed was a bug in that thread
> > above.
> 
> So, what is the correct thing to do here?  Fix the driver core?  Don't
> fix the driver core but document it?  Something else?

I have no idea. I have a very limited knowledge of the driver core so I
certainly can't say whether it was intentional or not, and thus how we
should address it.

It certainly look to me like we should fix it though.

> I don't think it is to create an undocumented test :)

I mean, yeah, definitely. But it was supposed to be patches for you to
test so you could see these issues for yourself after you asked for it,
not really something that should be merged as is.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists