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?