On Sat, 11 Mar 2023 at 07:21, Stephen Boyd sboyd@kernel.org wrote:
Quoting David Gow (2023-03-02 23:15:35)
On Thu, 2 Mar 2023 at 09:38, Stephen Boyd sboyd@kernel.org wrote:
Unit tests are more ergonomic and simpler to understand if they don't have to hoist a bunch of code into the test harness init and exit functions. Add some test managed wrappers for the clk APIs so that clk unit tests can write more code in the actual test and less code in the harness.
Only add APIs that are used for now. More wrappers can be added in the future as necessary.
Cc: Brendan Higgins brendan.higgins@linux.dev Cc: David Gow davidgow@google.com Signed-off-by: Stephen Boyd sboyd@kernel.org
Looks good, modulo bikeshedding below.
Cool!
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index e3ca0d058a25..7efce649b0d3 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -17,6 +17,11 @@ ifeq ($(CONFIG_OF), y) obj-$(CONFIG_COMMON_CLK) += clk-conf.o endif
+# KUnit specific helpers +ifeq ($(CONFIG_COMMON_CLK), y) +obj-$(CONFIG_KUNIT) += clk-kunit.o
Do we want to compile these in whenever KUnit is enabled, or only when we're building clk tests specifically? I suspect this would be served better by being under a CLK_KUNIT config option, which all of the tests then depend on. (Whether that's the existing CONFIG_CLK_KUNIT_TEST, and all of the clk tests live under the same config option, or a separate parent option would be up to you).
I was thinking of building it in with whatever mode CONFIG_KUNIT is built as. If this is a module because CONFIG_KUNIT=m, then unit tests would depend on that, and this would be a module as well. modprobe would know that some unit test module depends on symbols provided by clk-kunit.ko and thus load clk-kunit.ko first.
Personally, I'd rather have this behind CONFIG_CLK_KUNIT_TEST if possible, if only to avoid needlessly building these if someone just wants to test some other subsystem (but needs CONFIG_COMMON_CLK enabled anyway). I doubt it'd be a problem in practice in this case, but we definitely want to keep build (and hence iteration) times down as much as possible, so it's probably good practice to keep all tests behind at least some sort of "test this subsystem" option.
Equally, this could be a bit interesting if CONFIG_KUNIT=m. Given CONFIG_COMMON_CLK=y, this would end up as a clk-kunit module, no?
Yes, that is the intent.
+endif
# hardware specific clock types # please keep this section sorted lexicographically by file path name obj-$(CONFIG_COMMON_CLK_APPLE_NCO) += clk-apple-nco.o diff --git a/drivers/clk/clk-kunit.c b/drivers/clk/clk-kunit.c new file mode 100644 index 000000000000..78d85b3a7a4a --- /dev/null +++ b/drivers/clk/clk-kunit.c @@ -0,0 +1,204 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- KUnit helpers for clk tests
- */
+#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/err.h> +#include <linux/kernel.h> +#include <linux/slab.h>
+#include <kunit/resource.h>
+#include "clk-kunit.h"
+static void kunit_clk_disable_unprepare(struct kunit_resource *res)
We need to decide on the naming scheme of these, and in particular if they should be kunit_clk or clk_kunit (or something else).
I'd lean to clk_kunit, if only to match DRM's KUnit helpers being drm_kunit_helper better, and so that these are more tightly bound to the subsystem being tested. (i.e., so I don't have to scroll through every subsystem's helpers when autocompleting kunit_).
Ok, got it. I was trying to match kunit_kzalloc() style. It makes it easy to slap the 'kunit_' prefix on existing auto-completed function names like kzalloc() or clk_prepare_enable().
Yeah: my rule of thumb at the moment is to keep the kunit_ prefix for things which are generic across the whole kernel (and tend to be implemented in lib/kunit), and to use suffixes or infixes (whichever works best) for things which are subsystem-specific.
I wasn't aware of drm_kunit_helper. That's a mouthful! We don't call it slab_kunit_helper_kzalloc(). Maybe to satisfy all conditions it should be:
clk_prepare_enable_kunit()so that kunit_ autocomplete doesn't have a big scroll list, and clk subsystem autocompletes, and we know it is kunit specific.
Sounds good to me.
Cheers, -- David
On Sat, Mar 11, 2023 at 02:32:04PM +0800, David Gow wrote:
diff --git a/drivers/clk/clk-kunit.c b/drivers/clk/clk-kunit.c new file mode 100644 index 000000000000..78d85b3a7a4a --- /dev/null +++ b/drivers/clk/clk-kunit.c @@ -0,0 +1,204 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- KUnit helpers for clk tests
- */
+#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/err.h> +#include <linux/kernel.h> +#include <linux/slab.h>
+#include <kunit/resource.h>
+#include "clk-kunit.h"
+static void kunit_clk_disable_unprepare(struct kunit_resource *res)
We need to decide on the naming scheme of these, and in particular if they should be kunit_clk or clk_kunit (or something else).
I'd lean to clk_kunit, if only to match DRM's KUnit helpers being drm_kunit_helper better, and so that these are more tightly bound to the subsystem being tested. (i.e., so I don't have to scroll through every subsystem's helpers when autocompleting kunit_).
Ok, got it. I was trying to match kunit_kzalloc() style. It makes it easy to slap the 'kunit_' prefix on existing auto-completed function names like kzalloc() or clk_prepare_enable().
Yeah: my rule of thumb at the moment is to keep the kunit_ prefix for things which are generic across the whole kernel (and tend to be implemented in lib/kunit), and to use suffixes or infixes (whichever works best) for things which are subsystem-specific.
A suffix is kind of weird though when any other managed call is using a prefix: devm is always using a prefix including for clocks, kunit for some calls too (like kzalloc).
Having clk_get vs devm_clk_get vs clk_get_kunit would be very inconsistent and throws me off completely :)
Maxime
linux-kselftest-mirror@lists.linaro.org
-
David Gow -
Maxime Ripard