| Message ID | 20220408091037.2041955-17-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | clk: More clock rate fixes and tests | expand |
Quoting Maxime Ripard (2022-04-08 02:10:31) > Since all the users of clk_core_round_rate_nolock() will now properly > initialize, there's no need for it to initialize the request itself. Probably this needs to be combined with the previous patch. > > This is even dangerous, as if the clock cannot change its rate by itself > and has CLK_SET_RATE_PARENT, clk_core_round_rate_nolock() will call > itself with the parent clock but the client clk_rate_request structure. > I think the next sentence is part of the single sentence paragraph above. > We will then reinitialize the child request with the parent context > (parent, boundaries, etc.), which is an issue if the parent ever changes > its own parent or parent rate. The parent of the parent can't be the parent of the child, i.e. itself. I guess this is only a problem if clk_core_init_rate_req() starts setting min/max? We want to leave those members unchanged so that the rate request can flow up through the tree and be modified when rounding rates from a grandchild. That's why the child req is passed up to the parent if the child can't round itself. The boundary of the child is moved to the parent. Definitely the rate should be clamped through a parent to the grandparent taking into account any of their constraints. Perhaps the bug is that __clk_determine_rate() doesn't clamp to boundaries like clk_hw_round_rate() does by calling clk_core_get_boundaries() and then mixing in the new requests boundaries.
On Fri, Apr 22, 2022 at 09:02:48PM -0700, Stephen Boyd wrote: > Quoting Maxime Ripard (2022-04-08 02:10:31) > > Since all the users of clk_core_round_rate_nolock() will now properly > > initialize, there's no need for it to initialize the request itself. > > Probably this needs to be combined with the previous patch. > > > > > This is even dangerous, as if the clock cannot change its rate by itself > > and has CLK_SET_RATE_PARENT, clk_core_round_rate_nolock() will call > > itself with the parent clock but the client clk_rate_request structure. > > > > I think the next sentence is part of the single sentence paragraph > above. > > > We will then reinitialize the child request with the parent context > > (parent, boundaries, etc.), which is an issue if the parent ever changes > > its own parent or parent rate. > > The parent of the parent can't be the parent of the child, i.e. itself. I already explained this a bit more in my answer to patch 15. > I guess this is only a problem if clk_core_init_rate_req() starts > setting min/max? The bugs I were seeing were mostly about the parent related infos (best_parent_rate and best_parent_hw) leaking into the child request. > We want to leave those members unchanged so that the > rate request can flow up through the tree and be modified when rounding > rates from a grandchild. That's why the child req is passed up to the > parent if the child can't round itself. The boundary of the child is > moved to the parent. > > Definitely the rate should be clamped through a parent to the > grandparent taking into account any of their constraints. Perhaps the > bug is that __clk_determine_rate() doesn't clamp to boundaries like > clk_hw_round_rate() does by calling clk_core_get_boundaries() and then > mixing in the new requests boundaries. That should work fine though, I've added some tests to make sure this is working properly after a few fixes (clk_test_single_parent_mux_set_range_round_rate_child_smaller in patch 5, clk_test_single_parent_mux_set_range_round_rate_parent_only and clk_test_single_parent_mux_set_range_round_rate_parent_smaller in patch 19) Maxime
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 3a59152b06b8..ccb6e9686fb8 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1431,8 +1431,6 @@ static int clk_core_round_rate_nolock(struct clk_core *core, return 0; } - clk_core_init_rate_req(core, req, req->rate); - if (clk_core_can_round(core)) return clk_core_determine_round_nolock(core, req); else if (core->flags & CLK_SET_RATE_PARENT)
Since all the users of clk_core_round_rate_nolock() will now properly initialize, there's no need for it to initialize the request itself. This is even dangerous, as if the clock cannot change its rate by itself and has CLK_SET_RATE_PARENT, clk_core_round_rate_nolock() will call itself with the parent clock but the client clk_rate_request structure. We will then reinitialize the child request with the parent context (parent, boundaries, etc.), which is an issue if the parent ever changes its own parent or parent rate. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/clk/clk.c | 2 -- 1 file changed, 2 deletions(-)