[PATCH 0/2] clk: kunit: Fix the lockdep warnings

[PATCH 0/2] clk: kunit: Fix the lockdep warnings

[Maxime Ripard]

Hi,

Here's a small series to address the lockdep warning we have when
running the clk kunit tests with lockdep enabled.

For the record, it can be tested with:

$ ./tools/testing/kunit/kunit.py run \
    --kunitconfig=drivers/clk \
    --cross_compile aarch64-linux-gnu- --arch arm64 \
    --kconfig_add CONFIG_DEBUG_KERNEL=y \
    --kconfig_add CONFIG_PROVE_LOCKING=y

Let me know what you think,
Maxime

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
Maxime Ripard (2):
      clk: Introduce kunit wrapper around clk_hw_init_rate_request
      clk: Introduce kunit wrapper around __clk_determine_rate

 drivers/clk/clk.c            | 51 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk_test.c       |  4 ++--
 include/linux/clk-provider.h | 21 ++++++++++++++++++
 3 files changed, 74 insertions(+), 2 deletions(-)
---
base-commit: c58c49dd89324b18a812762a2bfa5a0458e4f252
change-id: 20230721-clk-fix-kunit-lockdep-c5e62b221118

Best regards,
-- 
Maxime Ripard <mripard@kernel.org>

[PATCH 1/2] clk: Introduce kunit wrapper around clk_hw_init_rate_request

[Maxime Ripard]

Some kunit tests are meant to test the behaviour of providers (and
most notably __clk_determine_rate()), but don't run those functions with
the clk prepare_lock held which results in lockdep warning.

clk_hw_init_rate_request is one of the functions being executed from a
test which should have the lock held. Let's introduce a wrapper around
it meant to be used only by kunit tests.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Link: https://lore.kernel.org/linux-clk/2b594e50-2bbf-4a2d-88e6-49fc39f3957a@roeck-us.net/
Reported-by: kernel test robot <yujie.liu@intel.com>
Link: https://lore.kernel.org/oe-lkp/202301310919.b9d56ee3-yujie.liu@intel.com
Fixes: 262ca38f4b6e ("clk: Stop forwarding clk_rate_requests to the parent")
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/clk/clk.c            | 26 ++++++++++++++++++++++++++
 drivers/clk/clk_test.c       |  2 +-
 include/linux/clk-provider.h | 11 +++++++++++
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c249f9791ae8..8ee9bd02af76 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -6,6 +6,7 @@
  * Standard functionality for the common clock API.  See Documentation/driver-api/clk.rst
  */
 
+#include <kunit/test-bug.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/clk/clk-conf.h>
@@ -1556,6 +1557,31 @@ void clk_hw_init_rate_request(const struct clk_hw *hw,
 }
 EXPORT_SYMBOL_GPL(clk_hw_init_rate_request);
 
+#if IS_ENABLED(CONFIG_KUNIT)
+/**
+ * clk_kunit_init_rate_request - Initializes a clk_rate_request
+ * @hw: the clk for which we want to submit a rate request
+ * @req: the clk_rate_request structure we want to initialise
+ * @rate: the rate which is to be requested
+ *
+ * Initializes a clk_rate_request structure to submit to
+ * __clk_determine_rate() or similar functions. Only usable in kunit
+ * test contexts, use clk_hw_init_rate_request() otherwise.
+ */
+void clk_kunit_init_rate_request(const struct clk_hw *hw,
+				 struct clk_rate_request *req,
+				 unsigned long rate)
+{
+	if (!kunit_get_current_test())
+		return;
+
+	clk_prepare_lock();
+	clk_hw_init_rate_request(hw, req, rate);
+	clk_prepare_unlock();
+}
+EXPORT_SYMBOL_GPL(clk_kunit_init_rate_request);
+#endif
+
 /**
  * clk_hw_forward_rate_request - Forwards a clk_rate_request to a clock's parent
  * @hw: the original clock that got the rate request
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index a154ec9d0111..a64f7036baa3 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -2230,7 +2230,7 @@ static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
 
-	clk_hw_init_rate_request(hw, &req, DUMMY_CLOCK_RATE_2);
+	clk_kunit_init_rate_request(hw, &req, DUMMY_CLOCK_RATE_2);
 
 	ret = __clk_determine_rate(hw, &req);
 	KUNIT_ASSERT_EQ(test, ret, 0);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 0f0cd01906b4..efdebfa3fce9 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -67,6 +67,17 @@ struct clk_rate_request {
 void clk_hw_init_rate_request(const struct clk_hw *hw,
 			      struct clk_rate_request *req,
 			      unsigned long rate);
+#if IS_ENABLED(CONFIG_KUNIT)
+void clk_kunit_init_rate_request(const struct clk_hw *hw,
+				 struct clk_rate_request *req,
+				 unsigned long rate);
+#else
+static inline void clk_kunit_init_rate_request(const struct clk_hw *hw,
+					       struct clk_rate_request *req,
+					       unsigned long rate)
+{
+}
+#endif
 void clk_hw_forward_rate_request(const struct clk_hw *core,
 				 const struct clk_rate_request *old_req,
 				 const struct clk_hw *parent,

-- 
2.41.0

Re: [PATCH 1/2] clk: Introduce kunit wrapper around clk_hw_init_rate_request

[Guenter Roeck]

On Fri, Jul 21, 2023 at 09:09:32AM +0200, Maxime Ripard wrote:
> Some kunit tests are meant to test the behaviour of providers (and
> most notably __clk_determine_rate()), but don't run those functions with
> the clk prepare_lock held which results in lockdep warning.
> 
> clk_hw_init_rate_request is one of the functions being executed from a
> test which should have the lock held. Let's introduce a wrapper around
> it meant to be used only by kunit tests.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Link: https://lore.kernel.org/linux-clk/2b594e50-2bbf-4a2d-88e6-49fc39f3957a@roeck-us.net/
> Reported-by: kernel test robot <yujie.liu@intel.com>
> Link: https://lore.kernel.org/oe-lkp/202301310919.b9d56ee3-yujie.liu@intel.com
> Fixes: 262ca38f4b6e ("clk: Stop forwarding clk_rate_requests to the parent")
> Signed-off-by: Maxime Ripard <mripard@kernel.org>

Tested-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/clk/clk.c            | 26 ++++++++++++++++++++++++++
>  drivers/clk/clk_test.c       |  2 +-
>  include/linux/clk-provider.h | 11 +++++++++++
>  3 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index c249f9791ae8..8ee9bd02af76 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -6,6 +6,7 @@
>   * Standard functionality for the common clock API.  See Documentation/driver-api/clk.rst
>   */
>  
> +#include <kunit/test-bug.h>
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/clk/clk-conf.h>
> @@ -1556,6 +1557,31 @@ void clk_hw_init_rate_request(const struct clk_hw *hw,
>  }
>  EXPORT_SYMBOL_GPL(clk_hw_init_rate_request);
>  
> +#if IS_ENABLED(CONFIG_KUNIT)
> +/**
> + * clk_kunit_init_rate_request - Initializes a clk_rate_request
> + * @hw: the clk for which we want to submit a rate request
> + * @req: the clk_rate_request structure we want to initialise
> + * @rate: the rate which is to be requested
> + *
> + * Initializes a clk_rate_request structure to submit to
> + * __clk_determine_rate() or similar functions. Only usable in kunit
> + * test contexts, use clk_hw_init_rate_request() otherwise.
> + */
> +void clk_kunit_init_rate_request(const struct clk_hw *hw,
> +				 struct clk_rate_request *req,
> +				 unsigned long rate)
> +{
> +	if (!kunit_get_current_test())
> +		return;
> +
> +	clk_prepare_lock();
> +	clk_hw_init_rate_request(hw, req, rate);
> +	clk_prepare_unlock();
> +}
> +EXPORT_SYMBOL_GPL(clk_kunit_init_rate_request);
> +#endif
> +
>  /**
>   * clk_hw_forward_rate_request - Forwards a clk_rate_request to a clock's parent
>   * @hw: the original clock that got the rate request
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index a154ec9d0111..a64f7036baa3 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -2230,7 +2230,7 @@ static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
>  	rate = clk_get_rate(clk);
>  	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
>  
> -	clk_hw_init_rate_request(hw, &req, DUMMY_CLOCK_RATE_2);
> +	clk_kunit_init_rate_request(hw, &req, DUMMY_CLOCK_RATE_2);
>  
>  	ret = __clk_determine_rate(hw, &req);
>  	KUNIT_ASSERT_EQ(test, ret, 0);
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 0f0cd01906b4..efdebfa3fce9 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -67,6 +67,17 @@ struct clk_rate_request {
>  void clk_hw_init_rate_request(const struct clk_hw *hw,
>  			      struct clk_rate_request *req,
>  			      unsigned long rate);
> +#if IS_ENABLED(CONFIG_KUNIT)
> +void clk_kunit_init_rate_request(const struct clk_hw *hw,
> +				 struct clk_rate_request *req,
> +				 unsigned long rate);
> +#else
> +static inline void clk_kunit_init_rate_request(const struct clk_hw *hw,
> +					       struct clk_rate_request *req,
> +					       unsigned long rate)
> +{
> +}
> +#endif
>  void clk_hw_forward_rate_request(const struct clk_hw *core,
>  				 const struct clk_rate_request *old_req,
>  				 const struct clk_hw *parent,
> 
> -- 
> 2.41.0
> 

[PATCH 2/2] clk: Introduce kunit wrapper around __clk_determine_rate

[Maxime Ripard]

Some kunit tests are meant to test the behaviour of providers but don't
run those functions with the clk prepare_lock held which results in
lockdep warning.

__clk_determine_rate is one of the functions being executed from a test
which should have the lock held. Let's introduce a wrapper around it
meant to be used only by kunit tests.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Link: https://lore.kernel.org/linux-clk/2b594e50-2bbf-4a2d-88e6-49fc39f3957a@roeck-us.net/
Reported-by: kernel test robot <yujie.liu@intel.com>
Link: https://lore.kernel.org/oe-lkp/202301310919.b9d56ee3-yujie.liu@intel.com
Fixes: 262ca38f4b6e ("clk: Stop forwarding clk_rate_requests to the parent")
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/clk/clk.c            | 25 +++++++++++++++++++++++++
 drivers/clk/clk_test.c       |  2 +-
 include/linux/clk-provider.h | 10 ++++++++++
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8ee9bd02af76..8fdbfec6bd85 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1669,6 +1669,31 @@ int __clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
 }
 EXPORT_SYMBOL_GPL(__clk_determine_rate);
 
+#if IS_ENABLED(CONFIG_KUNIT)
+/**
+ * __clk_kunit_determine_rate - get the closest rate actually supported by a clock
+ * @hw: determine the rate of this clock
+ * @req: target rate request
+ *
+ * Useful for clk_ops such as .set_rate and .determine_rate. Only usable
+ * in a kunit test context. Use __clk_determine_rate() otherwise.
+ */
+int __clk_kunit_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
+{
+	int ret;
+
+	if (!kunit_get_current_test())
+		return -EINVAL;
+
+	clk_prepare_lock();
+	ret = __clk_determine_rate(hw, req);
+	clk_prepare_unlock();
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(__clk_kunit_determine_rate);
+#endif
+
 /**
  * clk_hw_round_rate() - round the given rate for a hw clk
  * @hw: the hw clk for which we are rounding a rate
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index a64f7036baa3..b835e08892b4 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -2232,7 +2232,7 @@ static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
 
 	clk_kunit_init_rate_request(hw, &req, DUMMY_CLOCK_RATE_2);
 
-	ret = __clk_determine_rate(hw, &req);
+	ret = __clk_kunit_determine_rate(hw, &req);
 	KUNIT_ASSERT_EQ(test, ret, 0);
 
 	KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index efdebfa3fce9..43049f2c1dd2 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -1352,6 +1352,16 @@ void clk_hw_get_rate_range(struct clk_hw *hw, unsigned long *min_rate,
 void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
 			   unsigned long max_rate);
 
+#if IS_ENABLED(CONFIG_KUNIT)
+int __clk_kunit_determine_rate(struct clk_hw *hw, struct clk_rate_request *req);
+#else
+static inline int __clk_kunit_determine_rate(struct clk_hw *hw,
+					     struct clk_rate_request *req)
+{
+	return -ENOENT;
+}
+#endif
+
 static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
 {
 	dst->clk = src->clk;

-- 
2.41.0

Re: [PATCH 2/2] clk: Introduce kunit wrapper around __clk_determine_rate

[Guenter Roeck]

On Fri, Jul 21, 2023 at 09:09:33AM +0200, Maxime Ripard wrote:
> Some kunit tests are meant to test the behaviour of providers but don't
> run those functions with the clk prepare_lock held which results in
> lockdep warning.
> 
> __clk_determine_rate is one of the functions being executed from a test
> which should have the lock held. Let's introduce a wrapper around it
> meant to be used only by kunit tests.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Link: https://lore.kernel.org/linux-clk/2b594e50-2bbf-4a2d-88e6-49fc39f3957a@roeck-us.net/
> Reported-by: kernel test robot <yujie.liu@intel.com>
> Link: https://lore.kernel.org/oe-lkp/202301310919.b9d56ee3-yujie.liu@intel.com
> Fixes: 262ca38f4b6e ("clk: Stop forwarding clk_rate_requests to the parent")
> Signed-off-by: Maxime Ripard <mripard@kernel.org>

Tested-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/clk/clk.c            | 25 +++++++++++++++++++++++++
>  drivers/clk/clk_test.c       |  2 +-
>  include/linux/clk-provider.h | 10 ++++++++++
>  3 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 8ee9bd02af76..8fdbfec6bd85 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1669,6 +1669,31 @@ int __clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
>  }
>  EXPORT_SYMBOL_GPL(__clk_determine_rate);
>  
> +#if IS_ENABLED(CONFIG_KUNIT)
> +/**
> + * __clk_kunit_determine_rate - get the closest rate actually supported by a clock
> + * @hw: determine the rate of this clock
> + * @req: target rate request
> + *
> + * Useful for clk_ops such as .set_rate and .determine_rate. Only usable
> + * in a kunit test context. Use __clk_determine_rate() otherwise.
> + */
> +int __clk_kunit_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
> +{
> +	int ret;
> +
> +	if (!kunit_get_current_test())
> +		return -EINVAL;
> +
> +	clk_prepare_lock();
> +	ret = __clk_determine_rate(hw, req);
> +	clk_prepare_unlock();
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(__clk_kunit_determine_rate);
> +#endif
> +
>  /**
>   * clk_hw_round_rate() - round the given rate for a hw clk
>   * @hw: the hw clk for which we are rounding a rate
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index a64f7036baa3..b835e08892b4 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -2232,7 +2232,7 @@ static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
>  
>  	clk_kunit_init_rate_request(hw, &req, DUMMY_CLOCK_RATE_2);
>  
> -	ret = __clk_determine_rate(hw, &req);
> +	ret = __clk_kunit_determine_rate(hw, &req);
>  	KUNIT_ASSERT_EQ(test, ret, 0);
>  
>  	KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2);
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index efdebfa3fce9..43049f2c1dd2 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -1352,6 +1352,16 @@ void clk_hw_get_rate_range(struct clk_hw *hw, unsigned long *min_rate,
>  void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
>  			   unsigned long max_rate);
>  
> +#if IS_ENABLED(CONFIG_KUNIT)
> +int __clk_kunit_determine_rate(struct clk_hw *hw, struct clk_rate_request *req);
> +#else
> +static inline int __clk_kunit_determine_rate(struct clk_hw *hw,
> +					     struct clk_rate_request *req)
> +{
> +	return -ENOENT;
> +}
> +#endif
> +
>  static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src)
>  {
>  	dst->clk = src->clk;
> 
> -- 
> 2.41.0
> 

Re: [PATCH 0/2] clk: kunit: Fix the lockdep warnings

[Guenter Roeck]

On Fri, Jul 21, 2023 at 09:09:31AM +0200, Maxime Ripard wrote:
> Hi,
> 
> Here's a small series to address the lockdep warning we have when
> running the clk kunit tests with lockdep enabled.
> 
> For the record, it can be tested with:
> 
> $ ./tools/testing/kunit/kunit.py run \
>     --kunitconfig=drivers/clk \
>     --cross_compile aarch64-linux-gnu- --arch arm64 \
>     --kconfig_add CONFIG_DEBUG_KERNEL=y \
>     --kconfig_add CONFIG_PROVE_LOCKING=y
> 
> Let me know what you think,

The series fixes the problem for me. I sent Tested-by: tags for
both patches individually.

Thanks,
Guenter

> Maxime
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
> Maxime Ripard (2):
>       clk: Introduce kunit wrapper around clk_hw_init_rate_request
>       clk: Introduce kunit wrapper around __clk_determine_rate
> 
>  drivers/clk/clk.c            | 51 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk_test.c       |  4 ++--
>  include/linux/clk-provider.h | 21 ++++++++++++++++++
>  3 files changed, 74 insertions(+), 2 deletions(-)
> ---
> base-commit: c58c49dd89324b18a812762a2bfa5a0458e4f252
> change-id: 20230721-clk-fix-kunit-lockdep-c5e62b221118
> 
> Best regards,
> -- 
> Maxime Ripard <mripard@kernel.org>
> 

Re: [PATCH 0/2] clk: kunit: Fix the lockdep warnings

[Stephen Boyd]

+kunit-dev

Quoting Maxime Ripard (2023-07-21 00:09:31)
> Hi,
> 
> Here's a small series to address the lockdep warning we have when
> running the clk kunit tests with lockdep enabled.
> 
> For the record, it can be tested with:
> 
> $ ./tools/testing/kunit/kunit.py run \
>     --kunitconfig=drivers/clk \
>     --cross_compile aarch64-linux-gnu- --arch arm64 \
>     --kconfig_add CONFIG_DEBUG_KERNEL=y \
>     --kconfig_add CONFIG_PROVE_LOCKING=y
> 
> Let me know what you think,

Thanks for doing this. I want to roll these helpers into the clk_kunit.c
file that I had created for some other clk tests[1]. That's mostly
because clk.c is already super long and adding kunit code there makes
that problem worse. I'll try to take that patch out of the rest of the
series and then add this series on top and resend.

I don't know what to do about the case where CONFIG_KUNIT=m though. We
have to export clk_prepare_lock/unlock()? I really don't want to do that
even if kunit is enabled (see EXPORT_SYMBOL_IF_KUNIT). Maybe if there
was a GPL version of that, so proprietary modules can't get at kernel
internals on kunit enabled kernels.

But I also like the approach taken here of adding a small stub around
the call to make sure a test is running. Maybe I'll make a kunit
namespaced exported gpl symbol that bails if a test isn't running and
calls the clk_prepare_lock/unlock functions inside clk.c and then move
the rest of the code to clk_kunit.c to get something more strict.

[1] https://lore.kernel.org/all/20230327222159.3509818-9-sboyd@kernel.org/

Re: [PATCH 0/2] clk: kunit: Fix the lockdep warnings

[Guenter Roeck]

On 8/9/23 16:21, Stephen Boyd wrote:
> +kunit-dev
> 
> Quoting Maxime Ripard (2023-07-21 00:09:31)
>> Hi,
>>
>> Here's a small series to address the lockdep warning we have when
>> running the clk kunit tests with lockdep enabled.
>>
>> For the record, it can be tested with:
>>
>> $ ./tools/testing/kunit/kunit.py run \
>>      --kunitconfig=drivers/clk \
>>      --cross_compile aarch64-linux-gnu- --arch arm64 \
>>      --kconfig_add CONFIG_DEBUG_KERNEL=y \
>>      --kconfig_add CONFIG_PROVE_LOCKING=y
>>
>> Let me know what you think,
> 
> Thanks for doing this. I want to roll these helpers into the clk_kunit.c
> file that I had created for some other clk tests[1]. That's mostly
> because clk.c is already super long and adding kunit code there makes
> that problem worse. I'll try to take that patch out of the rest of the
> series and then add this series on top and resend.
> 
> I don't know what to do about the case where CONFIG_KUNIT=m though. We
> have to export clk_prepare_lock/unlock()? I really don't want to do that
> even if kunit is enabled (see EXPORT_SYMBOL_IF_KUNIT). Maybe if there
> was a GPL version of that, so proprietary modules can't get at kernel
> internals on kunit enabled kernels.
> 

EXPORT_SYMBOL_IF_KUNIT defines a module namespace. You could go a step
further and define a CLK_KUNIT module namespace or similar. That would
of course still permit abuse, but it would have to be _very_ intentional.
There is an EXPORT_SYMBOL_NS_GPL(), so you could further restrict it
to GPL only.

Guenter

Re: [PATCH 0/2] clk: kunit: Fix the lockdep warnings

[Stephen Boyd]

Quoting Stephen Boyd (2023-08-09 16:21:50)
> +kunit-dev
> 
> Quoting Maxime Ripard (2023-07-21 00:09:31)
> > Hi,
> > 
> > Here's a small series to address the lockdep warning we have when
> > running the clk kunit tests with lockdep enabled.
> > 
> > For the record, it can be tested with:
> > 
> > $ ./tools/testing/kunit/kunit.py run \
> >     --kunitconfig=drivers/clk \
> >     --cross_compile aarch64-linux-gnu- --arch arm64 \
> >     --kconfig_add CONFIG_DEBUG_KERNEL=y \
> >     --kconfig_add CONFIG_PROVE_LOCKING=y
> > 
> > Let me know what you think,
> 
> Thanks for doing this. I want to roll these helpers into the clk_kunit.c
> file that I had created for some other clk tests[1]. That's mostly
> because clk.c is already super long and adding kunit code there makes
> that problem worse. I'll try to take that patch out of the rest of the
> series and then add this series on top and resend.
> 
> I don't know what to do about the case where CONFIG_KUNIT=m though. We
> have to export clk_prepare_lock/unlock()? I really don't want to do that
> even if kunit is enabled (see EXPORT_SYMBOL_IF_KUNIT). Maybe if there
> was a GPL version of that, so proprietary modules can't get at kernel
> internals on kunit enabled kernels.
> 
> But I also like the approach taken here of adding a small stub around
> the call to make sure a test is running. Maybe I'll make a kunit
> namespaced exported gpl symbol that bails if a test isn't running and
> calls the clk_prepare_lock/unlock functions inside clk.c and then move
> the rest of the code to clk_kunit.c to get something more strict.
> 

What if we don't try to do any wrapper or export symbols and test
__clk_determine_rate() how it is called from the clk framework? The
downside is the code is not as simple because we have to check things
from within the clk_ops::determine_rate(), but the upside is that we can
avoid exporting internal clk APIs or wrap them so certain preconditions
are met like requiring them to be called from within a clk_op.

I also find it very odd to call clk_mux_determine_rate_closest() from a
clk that has one parent. Maybe the clk op should call
clk_hw_forward_rate_request() followed by __clk_determine_rate() on the
parent so we can test what the test comment says it wants to test.

-----8<-----
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index a154ec9d0111..b5b4f504b284 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -2155,6 +2155,53 @@ static struct kunit_suite clk_range_minimize_test_suite = {
 struct clk_leaf_mux_ctx {
 	struct clk_multiple_parent_ctx mux_ctx;
 	struct clk_hw hw;
+	struct kunit *test;
+	bool determine_rate_called;
+};
+
+static int clk_leaf_mux_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
+{
+	struct clk_leaf_mux_ctx *ctx = container_of(hw, struct clk_leaf_mux_ctx, hw);
+	struct kunit *test = ctx->test;
+
+	KUNIT_ASSERT_EQ(test, req->rate, DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, 0, __clk_mux_determine_rate_closest(hw, req));
+
+	KUNIT_EXPECT_EQ(test, req->rate, DUMMY_CLOCK_RATE_2);
+	KUNIT_EXPECT_EQ(test, req->best_parent_rate, DUMMY_CLOCK_RATE_2);
+	KUNIT_EXPECT_PTR_EQ(test, req->best_parent_hw, &ctx->mux_ctx.hw);
+
+	ctx->determine_rate_called = true;
+
+	return 0;
+}
+
+static const struct clk_ops clk_leaf_mux_set_rate_parent_ops = {
+	.determine_rate = clk_leaf_mux_determine_rate,
+	.set_parent = clk_dummy_single_set_parent,
+	.get_parent = clk_dummy_single_get_parent,
+};
+
+/*
+ * Test that, for a clock that will forward any rate request to its
+ * parent, the rate request structure returned by __clk_determine_rate
+ * is sane and will be what we expect.
+ */
+static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
+{
+	struct clk_leaf_mux_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = clk_hw_get_clk(hw, NULL);
+
+	KUNIT_EXPECT_EQ(test, DUMMY_CLOCK_RATE_2, clk_round_rate(clk, DUMMY_CLOCK_RATE_2));
+	KUNIT_EXPECT_TRUE(test, ctx->determine_rate_called);
+
+	clk_put(clk);
+}
+
+static struct kunit_case clk_leaf_mux_set_rate_parent_test_cases[] = {
+	KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate),
+	{}
 };
 
 static int
@@ -2168,6 +2215,7 @@ clk_leaf_mux_set_rate_parent_test_init(struct kunit *test)
 	if (!ctx)
 		return -ENOMEM;
 	test->priv = ctx;
+	ctx->test = test;
 
 	ctx->mux_ctx.parents_ctx[0].hw.init = CLK_HW_INIT_NO_PARENT("parent-0",
 								    &clk_dummy_rate_ops,
@@ -2194,7 +2242,7 @@ clk_leaf_mux_set_rate_parent_test_init(struct kunit *test)
 		return ret;
 
 	ctx->hw.init = CLK_HW_INIT_HW("test-clock", &ctx->mux_ctx.hw,
-				      &clk_dummy_single_parent_ops,
+				      &clk_leaf_mux_set_rate_parent_ops,
 				      CLK_SET_RATE_PARENT);
 	ret = clk_hw_register(NULL, &ctx->hw);
 	if (ret)
@@ -2213,40 +2261,6 @@ static void clk_leaf_mux_set_rate_parent_test_exit(struct kunit *test)
 	clk_hw_unregister(&ctx->mux_ctx.parents_ctx[1].hw);
 }
 
-/*
- * Test that, for a clock that will forward any rate request to its
- * parent, the rate request structure returned by __clk_determine_rate
- * is sane and will be what we expect.
- */
-static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
-{
-	struct clk_leaf_mux_ctx *ctx = test->priv;
-	struct clk_hw *hw = &ctx->hw;
-	struct clk *clk = clk_hw_get_clk(hw, NULL);
-	struct clk_rate_request req;
-	unsigned long rate;
-	int ret;
-
-	rate = clk_get_rate(clk);
-	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
-
-	clk_hw_init_rate_request(hw, &req, DUMMY_CLOCK_RATE_2);
-
-	ret = __clk_determine_rate(hw, &req);
-	KUNIT_ASSERT_EQ(test, ret, 0);
-
-	KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2);
-	KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_2);
-	KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
-
-	clk_put(clk);
-}
-
-static struct kunit_case clk_leaf_mux_set_rate_parent_test_cases[] = {
-	KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate),
-	{}
-};
-
 /*
  * Test suite for a clock whose parent is a mux with multiple parents.
  * The leaf clock has CLK_SET_RATE_PARENT, and will forward rate

Re: [PATCH 0/2] clk: kunit: Fix the lockdep warnings

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3399 bytes --]

Hi Stephen,

On Wed, Aug 09, 2023 at 06:37:30PM -0700, Stephen Boyd wrote:
> Quoting Stephen Boyd (2023-08-09 16:21:50)
> > +kunit-dev
> > 
> > Quoting Maxime Ripard (2023-07-21 00:09:31)
> > > Hi,
> > > 
> > > Here's a small series to address the lockdep warning we have when
> > > running the clk kunit tests with lockdep enabled.
> > > 
> > > For the record, it can be tested with:
> > > 
> > > $ ./tools/testing/kunit/kunit.py run \
> > >     --kunitconfig=drivers/clk \
> > >     --cross_compile aarch64-linux-gnu- --arch arm64 \
> > >     --kconfig_add CONFIG_DEBUG_KERNEL=y \
> > >     --kconfig_add CONFIG_PROVE_LOCKING=y
> > > 
> > > Let me know what you think,
> > 
> > Thanks for doing this. I want to roll these helpers into the clk_kunit.c
> > file that I had created for some other clk tests[1]. That's mostly
> > because clk.c is already super long and adding kunit code there makes
> > that problem worse. I'll try to take that patch out of the rest of the
> > series and then add this series on top and resend.
> > 
> > I don't know what to do about the case where CONFIG_KUNIT=m though. We
> > have to export clk_prepare_lock/unlock()? I really don't want to do that
> > even if kunit is enabled (see EXPORT_SYMBOL_IF_KUNIT). Maybe if there
> > was a GPL version of that, so proprietary modules can't get at kernel
> > internals on kunit enabled kernels.
> > 
> > But I also like the approach taken here of adding a small stub around
> > the call to make sure a test is running. Maybe I'll make a kunit
> > namespaced exported gpl symbol that bails if a test isn't running and
> > calls the clk_prepare_lock/unlock functions inside clk.c and then move
> > the rest of the code to clk_kunit.c to get something more strict.
> > 
> 
> What if we don't try to do any wrapper or export symbols and test
> __clk_determine_rate() how it is called from the clk framework? The
> downside is the code is not as simple because we have to check things
> from within the clk_ops::determine_rate(), but the upside is that we can
> avoid exporting internal clk APIs or wrap them so certain preconditions
> are met like requiring them to be called from within a clk_op.

The main reason for that test was linked to commit 262ca38f4b6e ("clk:
Stop forwarding clk_rate_requests to the parent"). Prior to it, if a
clock had CLK_SET_RATE_PARENT, we could end up with its parent's parent
hw struct and rate in best_parent_*.

So that test was mostly about making sure that __clk_determine_rate will
properly set the best_parent fields to match the clock's parent.

If we do a proper clock that uses __clk_determine_rate, we lose the
ability to check the content of the clk_rate_request returned by
__clk_determine_rate. It's up to you to tell whether it's a bad thing or
not :)

> I also find it very odd to call clk_mux_determine_rate_closest() from a
> clk that has one parent.

Yeah, the behaviour difference between determine_rate and
determine_rate_closest is weird to me too. We discussed it recently here:
https://lore.kernel.org/linux-clk/mlgxmfim3poke2j45vwh2htkn66hrrjd6ozjebtqhbf4wwljwo@hzi4dyplhdqg/

> Maybe the clk op should call clk_hw_forward_rate_request() followed by
> __clk_determine_rate() on the parent so we can test what the test
> comment says it wants to test.

I guess that would work too :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 0/2] clk: kunit: Fix the lockdep warnings

[Stephen Boyd]

Quoting Maxime Ripard (2023-08-21 04:16:12)
> Hi Stephen,
> 
> On Wed, Aug 09, 2023 at 06:37:30PM -0700, Stephen Boyd wrote:
> > Quoting Stephen Boyd (2023-08-09 16:21:50)
> > > +kunit-dev
> > > 
> > > Quoting Maxime Ripard (2023-07-21 00:09:31)
> > > > Hi,
> > > > 
> > > > Here's a small series to address the lockdep warning we have when
> > > > running the clk kunit tests with lockdep enabled.
> > > > 
> > > > For the record, it can be tested with:
> > > > 
> > > > $ ./tools/testing/kunit/kunit.py run \
> > > >     --kunitconfig=drivers/clk \
> > > >     --cross_compile aarch64-linux-gnu- --arch arm64 \
> > > >     --kconfig_add CONFIG_DEBUG_KERNEL=y \
> > > >     --kconfig_add CONFIG_PROVE_LOCKING=y
> > > > 
> > > > Let me know what you think,
> > > 
> > > Thanks for doing this. I want to roll these helpers into the clk_kunit.c
> > > file that I had created for some other clk tests[1]. That's mostly
> > > because clk.c is already super long and adding kunit code there makes
> > > that problem worse. I'll try to take that patch out of the rest of the
> > > series and then add this series on top and resend.
> > > 
> > > I don't know what to do about the case where CONFIG_KUNIT=m though. We
> > > have to export clk_prepare_lock/unlock()? I really don't want to do that
> > > even if kunit is enabled (see EXPORT_SYMBOL_IF_KUNIT). Maybe if there
> > > was a GPL version of that, so proprietary modules can't get at kernel
> > > internals on kunit enabled kernels.
> > > 
> > > But I also like the approach taken here of adding a small stub around
> > > the call to make sure a test is running. Maybe I'll make a kunit
> > > namespaced exported gpl symbol that bails if a test isn't running and
> > > calls the clk_prepare_lock/unlock functions inside clk.c and then move
> > > the rest of the code to clk_kunit.c to get something more strict.
> > > 
> > 
> > What if we don't try to do any wrapper or export symbols and test
> > __clk_determine_rate() how it is called from the clk framework? The
> > downside is the code is not as simple because we have to check things
> > from within the clk_ops::determine_rate(), but the upside is that we can
> > avoid exporting internal clk APIs or wrap them so certain preconditions
> > are met like requiring them to be called from within a clk_op.
> 
> The main reason for that test was linked to commit 262ca38f4b6e ("clk:
> Stop forwarding clk_rate_requests to the parent"). Prior to it, if a
> clock had CLK_SET_RATE_PARENT, we could end up with its parent's parent
> hw struct and rate in best_parent_*.
> 
> So that test was mostly about making sure that __clk_determine_rate will
> properly set the best_parent fields to match the clock's parent.
> 
> If we do a proper clock that uses __clk_determine_rate, we lose the
> ability to check the content of the clk_rate_request returned by
> __clk_determine_rate. It's up to you to tell whether it's a bad thing or
> not :)

I'm a little confused here. Was the test trying to check the changed
lines in clk_core_round_rate_nolock() that were made in commit
262ca38f4b6e under the CLK_SET_RATE_PARENT condition? From what I can
tell that doesn't get tested here.

Instead, the test calls __clk_determine_rate() that calls
clk_core_round_rate_nolock() which falls into the clk_core_can_round()
condition because the hw has clk_dummy_single_parent_ops which has
.determine_rate op set to __clk_mux_determine_rate_closest(). Once we
find that the clk can round, we call __clk_mux_determine_rate_closest().
This patch still calls __clk_mux_determine_rate_closest() like
__clk_determine_rate() was doing in the test, and checks that the
request structure has the expected parent in the req->best_parent_hw.

If we wanted to test the changed lines in clk_core_round_rate_nolock()
we should have called __clk_determine_rate() on a clk_hw that didn't
have a .determine_rate or .round_rate clk_op. Then it would have fallen
into the if (core->flags & CLK_SET_RATE_PARENT) condition in
clk_core_round_rate_nolock() and made sure that the request structure
returned was properly forwarded to the parent.

We can still do that by making a child of the leaf, set clk_ops on that
to be this new determine_rate clk op that calls to the parent (the leaf
today), and make that leaf clk not have any determine_rate clk_op. Then
we will fall into the CLK_SET_RATE_PARENT condition and can make sure
the request structure returned points at the parent instead of the mux.

> 
> > I also find it very odd to call clk_mux_determine_rate_closest() from a
> > clk that has one parent.
> 
> Yeah, the behaviour difference between determine_rate and
> determine_rate_closest is weird to me too. We discussed it recently here:
> https://lore.kernel.org/linux-clk/mlgxmfim3poke2j45vwh2htkn66hrrjd6ozjebtqhbf4wwljwo@hzi4dyplhdqg/

Sure, but I'm saying that the clk has one parent, not more than one, so
by definition it isn't a mux. It can only choose one parent. It's odd
that "mux" is in the name.

> 
> > Maybe the clk op should call clk_hw_forward_rate_request() followed by
> > __clk_determine_rate() on the parent so we can test what the test
> > comment says it wants to test.
> 
> I guess that would work too :)
> 

Ok, but I think it doesn't test what was intended to be tested?

Re: [PATCH 0/2] clk: kunit: Fix the lockdep warnings

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5351 bytes --]

Hi Stephen,

On Wed, Aug 23, 2023 at 12:50:46PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2023-08-21 04:16:12)
> > Hi Stephen,
> > 
> > On Wed, Aug 09, 2023 at 06:37:30PM -0700, Stephen Boyd wrote:
> > > Quoting Stephen Boyd (2023-08-09 16:21:50)
> > > > +kunit-dev
> > > > 
> > > > Quoting Maxime Ripard (2023-07-21 00:09:31)
> > > > > Hi,
> > > > > 
> > > > > Here's a small series to address the lockdep warning we have when
> > > > > running the clk kunit tests with lockdep enabled.
> > > > > 
> > > > > For the record, it can be tested with:
> > > > > 
> > > > > $ ./tools/testing/kunit/kunit.py run \
> > > > >     --kunitconfig=drivers/clk \
> > > > >     --cross_compile aarch64-linux-gnu- --arch arm64 \
> > > > >     --kconfig_add CONFIG_DEBUG_KERNEL=y \
> > > > >     --kconfig_add CONFIG_PROVE_LOCKING=y
> > > > > 
> > > > > Let me know what you think,
> > > > 
> > > > Thanks for doing this. I want to roll these helpers into the clk_kunit.c
> > > > file that I had created for some other clk tests[1]. That's mostly
> > > > because clk.c is already super long and adding kunit code there makes
> > > > that problem worse. I'll try to take that patch out of the rest of the
> > > > series and then add this series on top and resend.
> > > > 
> > > > I don't know what to do about the case where CONFIG_KUNIT=m though. We
> > > > have to export clk_prepare_lock/unlock()? I really don't want to do that
> > > > even if kunit is enabled (see EXPORT_SYMBOL_IF_KUNIT). Maybe if there
> > > > was a GPL version of that, so proprietary modules can't get at kernel
> > > > internals on kunit enabled kernels.
> > > > 
> > > > But I also like the approach taken here of adding a small stub around
> > > > the call to make sure a test is running. Maybe I'll make a kunit
> > > > namespaced exported gpl symbol that bails if a test isn't running and
> > > > calls the clk_prepare_lock/unlock functions inside clk.c and then move
> > > > the rest of the code to clk_kunit.c to get something more strict.
> > > > 
> > > 
> > > What if we don't try to do any wrapper or export symbols and test
> > > __clk_determine_rate() how it is called from the clk framework? The
> > > downside is the code is not as simple because we have to check things
> > > from within the clk_ops::determine_rate(), but the upside is that we can
> > > avoid exporting internal clk APIs or wrap them so certain preconditions
> > > are met like requiring them to be called from within a clk_op.
> > 
> > The main reason for that test was linked to commit 262ca38f4b6e ("clk:
> > Stop forwarding clk_rate_requests to the parent"). Prior to it, if a
> > clock had CLK_SET_RATE_PARENT, we could end up with its parent's parent
> > hw struct and rate in best_parent_*.
> > 
> > So that test was mostly about making sure that __clk_determine_rate will
> > properly set the best_parent fields to match the clock's parent.
> > 
> > If we do a proper clock that uses __clk_determine_rate, we lose the
> > ability to check the content of the clk_rate_request returned by
> > __clk_determine_rate. It's up to you to tell whether it's a bad thing or
> > not :)
> 
> I'm a little confused here. Was the test trying to check the changed
> lines in clk_core_round_rate_nolock() that were made in commit
> 262ca38f4b6e under the CLK_SET_RATE_PARENT condition?

That's what I was trying to test, yeah. Not necessarily this function in
particular (there's several affected), but at least we would get
something sane in a common case.

> From what I can tell that doesn't get tested here.
> 
> Instead, the test calls __clk_determine_rate() that calls
> clk_core_round_rate_nolock() which falls into the clk_core_can_round()
> condition because the hw has clk_dummy_single_parent_ops which has
> .determine_rate op set to __clk_mux_determine_rate_closest(). Once we
> find that the clk can round, we call __clk_mux_determine_rate_closest().

clk_mux_determine_rate_flags was also affected by the same issue.

> This patch still calls __clk_mux_determine_rate_closest() like
> __clk_determine_rate() was doing in the test, and checks that the
> request structure has the expected parent in the req->best_parent_hw.
> 
> If we wanted to test the changed lines in clk_core_round_rate_nolock()
> we should have called __clk_determine_rate() on a clk_hw that didn't
> have a .determine_rate or .round_rate clk_op. Then it would have fallen
> into the if (core->flags & CLK_SET_RATE_PARENT) condition in
> clk_core_round_rate_nolock() and made sure that the request structure
> returned was properly forwarded to the parent.
>
> We can still do that by making a child of the leaf, set clk_ops on that
> to be this new determine_rate clk op that calls to the parent (the leaf
> today), and make that leaf clk not have any determine_rate clk_op. Then
> we will fall into the CLK_SET_RATE_PARENT condition and can make sure
> the request structure returned points at the parent instead of the mux.

But you're right clk_core_round_rate_nolock() wasn't properly tested. I
guess, if we find it worth it, we should add a test for that one too?

clk_mux_determine_rate_flags with multiple parents and
CLK_SET_RATE_PARENT was also affected and fixed in the commit mentioned
above.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 0/2] clk: kunit: Fix the lockdep warnings

[Stephen Boyd]

Quoting Maxime Ripard (2023-08-24 02:56:38)
> Hi Stephen,
> 
> On Wed, Aug 23, 2023 at 12:50:46PM -0700, Stephen Boyd wrote:
> > Quoting Maxime Ripard (2023-08-21 04:16:12)
> > > Hi Stephen,
> > > 
> > > The main reason for that test was linked to commit 262ca38f4b6e ("clk:
> > > Stop forwarding clk_rate_requests to the parent"). Prior to it, if a
> > > clock had CLK_SET_RATE_PARENT, we could end up with its parent's parent
> > > hw struct and rate in best_parent_*.
> > > 
> > > So that test was mostly about making sure that __clk_determine_rate will
> > > properly set the best_parent fields to match the clock's parent.
> > > 
> > > If we do a proper clock that uses __clk_determine_rate, we lose the
> > > ability to check the content of the clk_rate_request returned by
> > > __clk_determine_rate. It's up to you to tell whether it's a bad thing or
> > > not :)
> > 
> > I'm a little confused here. Was the test trying to check the changed
> > lines in clk_core_round_rate_nolock() that were made in commit
> > 262ca38f4b6e under the CLK_SET_RATE_PARENT condition?
> 
> That's what I was trying to test, yeah. Not necessarily this function in
> particular (there's several affected), but at least we would get
> something sane in a common case.

Cool. Thanks for confirming.

> 
> > From what I can tell that doesn't get tested here.
> > 
> > Instead, the test calls __clk_determine_rate() that calls
> > clk_core_round_rate_nolock() which falls into the clk_core_can_round()
> > condition because the hw has clk_dummy_single_parent_ops which has
> > .determine_rate op set to __clk_mux_determine_rate_closest(). Once we
> > find that the clk can round, we call __clk_mux_determine_rate_closest().
> 
> clk_mux_determine_rate_flags was also affected by the same issue.

Ok. I see.

> 
> > This patch still calls __clk_mux_determine_rate_closest() like
> > __clk_determine_rate() was doing in the test, and checks that the
> > request structure has the expected parent in the req->best_parent_hw.
> > 
> > If we wanted to test the changed lines in clk_core_round_rate_nolock()
> > we should have called __clk_determine_rate() on a clk_hw that didn't
> > have a .determine_rate or .round_rate clk_op. Then it would have fallen
> > into the if (core->flags & CLK_SET_RATE_PARENT) condition in
> > clk_core_round_rate_nolock() and made sure that the request structure
> > returned was properly forwarded to the parent.
> >
> > We can still do that by making a child of the leaf, set clk_ops on that
> > to be this new determine_rate clk op that calls to the parent (the leaf
> > today), and make that leaf clk not have any determine_rate clk_op. Then
> > we will fall into the CLK_SET_RATE_PARENT condition and can make sure
> > the request structure returned points at the parent instead of the mux.
> 
> But you're right clk_core_round_rate_nolock() wasn't properly tested. I
> guess, if we find it worth it, we should add a test for that one too?
> 
> clk_mux_determine_rate_flags with multiple parents and
> CLK_SET_RATE_PARENT was also affected and fixed in the commit mentioned
> above.
> 

Makes sense. I've made a set of these tests that call some function,
like __clk_determine_rate() or __clk_mux_determine_rate(), from the
determine_rate clk_op of the leaf. The clk_hw pointer passed to the
function under test is the parent of the clk (the intermediate empty
clk_ops clk). The parent of that intermediate clk is the mux.

I noticed that sometimes the parent_hw wasn't set to the mux_ctx if I
didn't call clk_hw_forward_rate_request() in the clk_op. That's because
functions like __clk_determine_rate() assume the best_parent_hw has been
set already and simply skip setting it at all. It's possible that a
driver may fail to call clk_hw_forward_rate_request() before calling
some determine rate function on the parent. Looks like a pitfall but I'm
not sure how to make it any better.

I have this as two patches in my local tree. I'll send it out tomorrow.

----8<----
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index a154ec9d0111..99b9f01ada71 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -2155,8 +2155,35 @@ static struct kunit_suite clk_range_minimize_test_suite = {
 struct clk_leaf_mux_ctx {
 	struct clk_multiple_parent_ctx mux_ctx;
 	struct clk_hw hw;
+	struct clk_hw parent;
+	struct clk_rate_request req;
+	int (*determine_rate_func)(struct clk_hw *, struct clk_rate_request *);
 };
 
+static int clk_leaf_mux_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
+{
+	struct clk_leaf_mux_ctx *ctx = container_of(hw, struct clk_leaf_mux_ctx, hw);
+	int ret;
+	struct clk_rate_request *parent_req = &ctx->req;
+
+	clk_hw_forward_rate_request(hw, req, req->best_parent_hw, parent_req, req->rate);
+	ret = ctx->determine_rate_func(req->best_parent_hw, parent_req);
+	if (ret)
+		return ret;
+
+	req->rate = parent_req->rate;
+
+	return 0;
+}
+
+static const struct clk_ops clk_leaf_mux_set_rate_parent_ops = {
+	.determine_rate = clk_leaf_mux_determine_rate,
+	.set_parent = clk_dummy_single_set_parent,
+	.get_parent = clk_dummy_single_get_parent,
+};
+
+static const struct clk_ops empty_clk_ops = { };
+
 static int
 clk_leaf_mux_set_rate_parent_test_init(struct kunit *test)
 {
@@ -2193,8 +2220,15 @@ clk_leaf_mux_set_rate_parent_test_init(struct kunit *test)
 	if (ret)
 		return ret;
 
-	ctx->hw.init = CLK_HW_INIT_HW("test-clock", &ctx->mux_ctx.hw,
-				      &clk_dummy_single_parent_ops,
+	ctx->parent.init = CLK_HW_INIT_HW("test-parent", &ctx->mux_ctx.hw,
+				      &empty_clk_ops,
+				      CLK_SET_RATE_PARENT);
+	ret = clk_hw_register(NULL, &ctx->parent);
+	if (ret)
+		return ret;
+
+	ctx->hw.init = CLK_HW_INIT_HW("test-clock", &ctx->parent,
+				      &clk_leaf_mux_set_rate_parent_ops,
 				      CLK_SET_RATE_PARENT);
 	ret = clk_hw_register(NULL, &ctx->hw);
 	if (ret)
@@ -2208,50 +2242,112 @@ static void clk_leaf_mux_set_rate_parent_test_exit(struct kunit *test)
 	struct clk_leaf_mux_ctx *ctx = test->priv;
 
 	clk_hw_unregister(&ctx->hw);
+	clk_hw_unregister(&ctx->parent);
 	clk_hw_unregister(&ctx->mux_ctx.hw);
 	clk_hw_unregister(&ctx->mux_ctx.parents_ctx[0].hw);
 	clk_hw_unregister(&ctx->mux_ctx.parents_ctx[1].hw);
 }
 
+struct clk_leaf_mux_set_rate_parent_determine_rate_test_case {
+	const char *desc;
+	int (*determine_rate_func)(struct clk_hw *, struct clk_rate_request *);
+};
+
+static void
+clk_leaf_mux_set_rate_parent_determine_rate_test_case_to_desc(
+		const struct clk_leaf_mux_set_rate_parent_determine_rate_test_case *t, char *desc)
+{
+	strcpy(desc, t->desc);
+}
+
+static const struct clk_leaf_mux_set_rate_parent_determine_rate_test_case
+clk_leaf_mux_set_rate_parent_determine_rate_test_cases[] = {
+	{
+		/*
+		 * Test that __clk_determine_rate() on the parent that can't
+		 * change rate doesn't return a clk_rate_request structure with
+		 * the best_parent_hw pointer pointing to the parent.
+		 */
+		.desc = "clk_leaf_mux_set_rate_parent__clk_determine_rate_proper_parent",
+		.determine_rate_func = __clk_determine_rate,
+	},
+	{
+		/*
+		 * Test that __clk_mux_determine_rate() on the parent that
+		 * can't change rate doesn't return a clk_rate_request
+		 * structure with the best_parent_hw pointer pointing to
+		 * the parent.
+		 */
+		.desc = "clk_leaf_mux_set_rate_parent__clk_mux_determine_rate_proper_parent",
+		.determine_rate_func = __clk_mux_determine_rate,
+	},
+	{
+		/*
+		 * Test that __clk_mux_determine_rate_closest() on the parent
+		 * that can't change rate doesn't return a clk_rate_request
+		 * structure with the best_parent_hw pointer pointing to
+		 * the parent.
+		 */
+		.desc = "clk_leaf_mux_set_rate_parent__clk_mux_determine_rate_closest_proper_parent",
+		.determine_rate_func = __clk_mux_determine_rate_closest,
+	},
+	{
+		/*
+		 * Test that clk_hw_determine_rate_no_reparent() on the parent
+		 * that can't change rate doesn't return a clk_rate_request
+		 * structure with the best_parent_hw pointer pointing to
+		 * the parent.
+		 */
+		.desc = "clk_leaf_mux_set_rate_parent_clk_hw_determine_rate_no_reparent_proper_parent",
+		.determine_rate_func = clk_hw_determine_rate_no_reparent,
+	},
+};
+
+KUNIT_ARRAY_PARAM(clk_leaf_mux_set_rate_parent_determine_rate_test,
+		  clk_leaf_mux_set_rate_parent_determine_rate_test_cases,
+		  clk_leaf_mux_set_rate_parent_determine_rate_test_case_to_desc)
+
 /*
- * Test that, for a clock that will forward any rate request to its
- * parent, the rate request structure returned by __clk_determine_rate
- * is sane and will be what we expect.
+ * Test that when a clk that can't change rate itself calls a function like
+ * __clk_determine_rate() on its parent it doesn't get back a clk_rate_request
+ * structure that has the best_parent_hw pointer point to the clk_hw passed
+ * into the determine rate function. See commit 262ca38f4b6e ("clk: Stop
+ * forwarding clk_rate_requests to the parent") for more background.
  */
-static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
+static void clk_leaf_mux_set_rate_parent_determine_rate_test(struct kunit *test)
 {
 	struct clk_leaf_mux_ctx *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
 	struct clk *clk = clk_hw_get_clk(hw, NULL);
-	struct clk_rate_request req;
+	struct clk_rate_request *req = &ctx->req;
 	unsigned long rate;
-	int ret;
+	const struct clk_leaf_mux_set_rate_parent_determine_rate_test_case *test_param;
+
+	test_param = test->param_value;
+	ctx->determine_rate_func = test_param->determine_rate_func;
 
 	rate = clk_get_rate(clk);
 	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+	KUNIT_ASSERT_EQ(test, DUMMY_CLOCK_RATE_2, clk_round_rate(clk, DUMMY_CLOCK_RATE_2));
 
-	clk_hw_init_rate_request(hw, &req, DUMMY_CLOCK_RATE_2);
-
-	ret = __clk_determine_rate(hw, &req);
-	KUNIT_ASSERT_EQ(test, ret, 0);
-
-	KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2);
-	KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_2);
-	KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
+	KUNIT_EXPECT_EQ(test, req->rate, DUMMY_CLOCK_RATE_2);
+	KUNIT_EXPECT_EQ(test, req->best_parent_rate, DUMMY_CLOCK_RATE_2);
+	KUNIT_EXPECT_PTR_EQ(test, req->best_parent_hw, &ctx->mux_ctx.hw);
 
 	clk_put(clk);
 }
 
 static struct kunit_case clk_leaf_mux_set_rate_parent_test_cases[] = {
-	KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate),
+	KUNIT_CASE_PARAM(clk_leaf_mux_set_rate_parent_determine_rate_test,
+			 clk_leaf_mux_set_rate_parent_determine_rate_test_gen_params),
 	{}
 };
 
 /*
- * Test suite for a clock whose parent is a mux with multiple parents.
- * The leaf clock has CLK_SET_RATE_PARENT, and will forward rate
- * requests to the mux, which will then select which parent is the best
- * fit for a given rate.
+ * Test suite for a clock whose parent is a pass-through clk whose parent is a
+ * mux with multiple parents. The leaf and pass-through clocks have the
+ * CLK_SET_RATE_PARENT flag, and will forward rate requests to the mux, which
+ * will then select which parent is the best fit for a given rate.
  *
  * These tests exercise the behaviour of muxes, and the proper selection
  * of parents.