Messages in this thread | | | Date | Thu, 24 Aug 2023 11:56:38 +0200 | From | Maxime Ripard <> | Subject | Re: [PATCH 0/2] clk: kunit: Fix the lockdep warnings |
| |
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 [unhandled content-type:application/pgp-signature] | |