[4/8] clk: Add test managed clk provider/consumer APIs

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>
---
 drivers/clk/Makefile    |   5 +
 drivers/clk/clk-kunit.c | 204 ++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk-kunit.h |  28 ++++++
 3 files changed, 237 insertions(+)
 create mode 100644 drivers/clk/clk-kunit.c
 create mode 100644 drivers/clk/clk-kunit.h

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.

>  drivers/clk/Makefile    |   5 +
>  drivers/clk/clk-kunit.c | 204 ++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk-kunit.h |  28 ++++++
>  3 files changed, 237 insertions(+)
>  create mode 100644 drivers/clk/clk-kunit.c
>  create mode 100644 drivers/clk/clk-kunit.h
>
> 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).

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?

> +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_).


> +{
> +       struct clk *clk = res->data;
> +
> +       clk_disable_unprepare(clk);
> +}
> +
> +/**
> + * kunit_clk_prepare_enable() - Test managed clk_prepare_enable()
> + * @test: The test context
> + * @clk: clk to prepare and enable
> + *
> + * Returns: 0 on success, or negative errno on failure.
> + */
> +int kunit_clk_prepare_enable(struct kunit *test, struct clk *clk)
> +{
> +       if (!kunit_alloc_resource(test, NULL, kunit_clk_disable_unprepare,
> +                                 GFP_KERNEL, clk))
> +               return -EINVAL;
> +
> +       return clk_prepare_enable(clk);
> +}
> +EXPORT_SYMBOL_GPL(kunit_clk_prepare_enable);
> +
> +static void kunit_clk_put(struct kunit_resource *res)
> +{
> +       struct clk *clk = res->data;
> +
> +       clk_put(clk);
> +}
> +
> +/**
> + * kunit_clk_get() - Test managed clk_get()
> + * @test: The test context
> + * @dev: device for clock "consumer"
> + * @id: clock consumer ID
> + *
> + * Returns: new clk consumer or ERR_PTR on failure
> + */
> +struct clk *
> +kunit_clk_get(struct kunit *test, struct device *dev, const char *con_id)
> +{
> +       struct clk *clk;
> +
> +       clk = clk_get(dev, con_id);
> +       if (IS_ERR(clk))
> +               return clk;
> +
> +       if (!kunit_alloc_resource(test, NULL, kunit_clk_put, GFP_KERNEL, clk)) {
> +               clk_put(clk);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       return clk;
> +}
> +EXPORT_SYMBOL_GPL(kunit_clk_get);
> +
> +/**
> + * kunit_of_clk_get() - Test managed of_clk_get()
> + * @test: The test context
> + * @np: device_node for clock "consumer"
> + * @index: index in 'clocks' property of @np
> + *
> + * Returns: new clk consumer or ERR_PTR on failure
> + */
> +struct clk *
> +kunit_of_clk_get(struct kunit *test, struct device_node *np, int index)
> +{
> +       struct clk *clk;
> +
> +       clk = of_clk_get(np, index);
> +       if (IS_ERR(clk))
> +               return clk;
> +
> +       if (!kunit_alloc_resource(test, NULL, kunit_clk_put, GFP_KERNEL, clk)) {
> +               clk_put(clk);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       return clk;
> +}
> +EXPORT_SYMBOL_GPL(kunit_of_clk_get);
> +
> +/**
> + * kunit_clk_hw_get_clk() - Test managed clk_hw_get_clk()
> + * @test: The test context
> + * @hw: clk_hw associated with the clk being consumed
> + * @con_id: connection ID string on device
> + *
> + * Returns: new clk consumer or ERR_PTR on failure
> + */
> +struct clk *
> +kunit_clk_hw_get_clk(struct kunit *test, struct clk_hw *hw, const char *con_id)
> +{
> +       struct clk *clk;
> +
> +       clk = clk_hw_get_clk(hw, con_id);
> +       if (IS_ERR(clk))
> +               return clk;
> +
> +       if (!kunit_alloc_resource(test, NULL, kunit_clk_put, GFP_KERNEL, clk)) {
> +               clk_put(clk);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       return clk;
> +}
> +EXPORT_SYMBOL_GPL(kunit_clk_hw_get_clk);
> +
> +/**
> + * kunit_clk_hw_get_clk_prepared_enabled() - Test managed clk_hw_get_clk() + clk_prepare_enable()
> + * @test: The test context
> + * @hw: clk_hw associated with the clk being consumed
> + * @con_id: connection ID string on device
> + *
> + * Returns: new clk consumer that is prepared and enabled or ERR_PTR on failure
> + */
> +struct clk *
> +kunit_clk_hw_get_clk_prepared_enabled(struct kunit *test, struct clk_hw *hw,
> +                                     const char *con_id)
> +{
> +       int ret;
> +       struct clk *clk;
> +
> +       clk = kunit_clk_hw_get_clk(test, hw, con_id);
> +       if (IS_ERR(clk))
> +               return clk;
> +
> +       ret = kunit_clk_prepare_enable(test, clk);
> +       if (ret)
> +               return ERR_PTR(ret);
> +
> +       return clk;
> +}
> +EXPORT_SYMBOL_GPL(kunit_clk_hw_get_clk_prepared_enabled);
> +
> +static void kunit_clk_hw_unregister(struct kunit_resource *res)
> +{
> +       struct clk_hw *hw = res->data;
> +
> +       clk_hw_unregister(hw);
> +}
> +
> +/**
> + * kunit_clk_hw_register() - Test managed clk_hw_register()
> + * @test: The test context
> + * @dev: device that is registering this clock
> + * @hw: link to hardware-specific clock data
> + *
> + * Returns: 0 on success or a negative errno value on failure
> + */
> +int kunit_clk_hw_register(struct kunit *test, struct device *dev, struct clk_hw *hw)
> +{
> +       int ret;
> +
> +       ret = clk_hw_register(dev, hw);
> +       if (ret)
> +               return ret;
> +
> +       if (!kunit_alloc_resource(test, NULL, kunit_clk_hw_unregister, GFP_KERNEL, hw)) {
> +               clk_hw_unregister(hw);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +/**
> + * kunit_of_clk_hw_register() - Test managed of_clk_hw_register()
> + * @test: The test context
> + * @node: device_node of device that is registering this clock
> + * @hw: link to hardware-specific clock data
> + *
> + * Returns: 0 on success or a negative errno value on failure
> + */
> +int kunit_of_clk_hw_register(struct kunit *test, struct device_node *node, struct clk_hw *hw)
> +{
> +       int ret;
> +
> +       ret = of_clk_hw_register(node, hw);
> +       if (ret)
> +               return ret;
> +
> +       if (!kunit_alloc_resource(test, NULL, kunit_clk_hw_unregister, GFP_KERNEL, hw)) {
> +               clk_hw_unregister(hw);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> diff --git a/drivers/clk/clk-kunit.h b/drivers/clk/clk-kunit.h
> new file mode 100644
> index 000000000000..153597d69269
> --- /dev/null
> +++ b/drivers/clk/clk-kunit.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _CLK_KUNIT_H
> +#define _CLK_KUNIT_H
> +
> +struct clk;
> +struct clk_hw;
> +struct device;
> +struct device_node;
> +struct kunit;
> +
> +struct clk *
> +kunit_clk_get(struct kunit *test, struct device *dev, const char *con_id);
> +struct clk *
> +kunit_of_clk_get(struct kunit *test, struct device_node *np, int index);
> +
> +struct clk *
> +kunit_clk_hw_get_clk(struct kunit *test, struct clk_hw *hw, const char *con_id);
> +struct clk *
> +kunit_clk_hw_get_clk_prepared_enabled(struct kunit *test, struct clk_hw *hw,
> +                                     const char *con_id);
> +
> +int kunit_clk_prepare_enable(struct kunit *test, struct clk *clk);
> +
> +int kunit_clk_hw_register(struct kunit *test, struct device *dev, struct clk_hw *hw);
> +int kunit_of_clk_hw_register(struct kunit *test, struct device_node *node,
> +                            struct clk_hw *hw);
> +
> +#endif
> --
> https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
> https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
>

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