Skip to content

Commit cb1b1dd

Browse files
mripardbebarino
mripard
authored and
bebarino
committed
clk: Set req_rate on reparenting
If a non-rate clock started by default with a parent that never registered, core->req_rate will be 0. The expectation is that whenever the parent will be registered, req_rate will be updated with the new value that has just been computed. However, if that clock is a mux, clk_set_parent() can also make that clock no longer orphan. In this case however, we never update req_rate. The natural solution to this would be to update core->rate and core->req_rate in clk_reparent() by calling clk_recalc(). However, this doesn't work in all cases. Indeed, clk_recalc() is called by __clk_set_parent_before(), __clk_set_parent() and clk_core_reparent(). Both __clk_set_parent_before() and __clk_set_parent will call clk_recalc() with the enable_lock taken through a call to clk_enable_lock(), the underlying locking primitive being a spinlock. clk_recalc() calls the backing driver .recalc_rate hook, and that implementation might sleep if the underlying device uses a bus with accesses that might sleep, such as i2c. In such a situation, we would end up sleeping while holding a spinlock, and thus in an atomic section. In order to work around this, we can move the core->rate and core->req_rate update to the clk_recalc() calling sites, after the enable_lock has been released if it was taken. The only situation that could still be problematic is the clk_core_reparent() -> clk_reparent() case that doesn't have any locking. clk_core_reparent() is itself called by clk_hw_reparent(), which is then called by 4 drivers: * clk-stm32mp1.c, stm32/clk-stm32-core.c and tegra/clk-tegra210-emc.c use it in their set_parent implementation. The set_parent hook is only called by __clk_set_parent() and clk_change_rate(), both of them calling it without the enable_lock taken. * clk/tegra/clk-tegra124-emc.c calls it as part of its set_rate implementation. set_rate is only called by clk_change_rate(), again without the enable_lock taken. In both cases we can't end up in a situation where the clk_hw_reparent() caller would hold a spinlock, so it seems like this is a good workaround. Let's also add some unit tests to make sure we cover the original bug. Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com> # imx8mp Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> # exynos4210, meson g12b Signed-off-by: Maxime Ripard <maxime@cerno.tech> Link: https://lore.kernel.org/r/20220816112530.1837489-14-maxime@cerno.tech Tested-by: Linux Kernel Functional Testing <lkft@linaro.org> Tested-by: Naresh Kamboju <naresh.kamboju@linaro.org> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
1 parent 3afb072 commit cb1b1dd

File tree

2 files changed

+261
-0
lines changed

2 files changed

+261
-0
lines changed

‎drivers/clk/clk.c

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1765,6 +1765,23 @@ static void clk_core_update_orphan_status(struct clk_core *core, bool is_orphan)
17651765
clk_core_update_orphan_status(child, is_orphan);
17661766
}
17671767

1768+
/*
1769+
* Update the orphan rate and req_rate of @core and all its children.
1770+
*/
1771+
static void clk_core_update_orphan_child_rates(struct clk_core *core)
1772+
{
1773+
struct clk_core *child;
1774+
unsigned long parent_rate = 0;
1775+
1776+
if (core->parent)
1777+
parent_rate = core->parent->rate;
1778+
1779+
core->rate = core->req_rate = clk_recalc(core, parent_rate);
1780+
1781+
hlist_for_each_entry(child, &core->children, child_node)
1782+
clk_core_update_orphan_child_rates(child);
1783+
}
1784+
17681785
static void clk_reparent(struct clk_core *core, struct clk_core *new_parent)
17691786
{
17701787
bool was_orphan = core->orphan;
@@ -1834,6 +1851,8 @@ static struct clk_core *__clk_set_parent_before(struct clk_core *core,
18341851
clk_reparent(core, parent);
18351852
clk_enable_unlock(flags);
18361853

1854+
clk_core_update_orphan_child_rates(core);
1855+
18371856
return old_parent;
18381857
}
18391858

@@ -1878,6 +1897,8 @@ static int __clk_set_parent(struct clk_core *core, struct clk_core *parent,
18781897
flags = clk_enable_lock();
18791898
clk_reparent(core, old_parent);
18801899
clk_enable_unlock(flags);
1900+
1901+
clk_core_update_orphan_child_rates(core);
18811902
__clk_set_parent_after(core, old_parent, parent);
18821903

18831904
return ret;
@@ -2506,6 +2527,7 @@ static void clk_core_reparent(struct clk_core *core,
25062527
struct clk_core *new_parent)
25072528
{
25082529
clk_reparent(core, new_parent);
2530+
clk_core_update_orphan_child_rates(core);
25092531
__clk_recalc_accuracies(core);
25102532
__clk_recalc_rates(core, POST_RATE_CHANGE);
25112533
}

‎drivers/clk/clk_test.c

Lines changed: 239 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -594,6 +594,41 @@ clk_test_orphan_transparent_multiple_parent_mux_set_parent(struct kunit *test)
594594
clk_put(clk);
595595
}
596596

597+
/*
598+
* Test that, for a mux that started orphan but got switched to a valid
599+
* parent, calling clk_drop_range() on the mux won't affect the parent
600+
* rate.
601+
*/
602+
static void
603+
clk_test_orphan_transparent_multiple_parent_mux_set_parent_drop_range(struct kunit *test)
604+
{
605+
struct clk_multiple_parent_ctx *ctx = test->priv;
606+
struct clk_hw *hw = &ctx->hw;
607+
struct clk *clk = clk_hw_get_clk(hw, NULL);
608+
struct clk *parent;
609+
unsigned long parent_rate, new_parent_rate;
610+
int ret;
611+
612+
parent = clk_hw_get_clk(&ctx->parents_ctx[1].hw, NULL);
613+
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
614+
615+
parent_rate = clk_get_rate(parent);
616+
KUNIT_ASSERT_GT(test, parent_rate, 0);
617+
618+
ret = clk_set_parent(clk, parent);
619+
KUNIT_ASSERT_EQ(test, ret, 0);
620+
621+
ret = clk_drop_range(clk);
622+
KUNIT_ASSERT_EQ(test, ret, 0);
623+
624+
new_parent_rate = clk_get_rate(clk);
625+
KUNIT_ASSERT_GT(test, new_parent_rate, 0);
626+
KUNIT_EXPECT_EQ(test, parent_rate, new_parent_rate);
627+
628+
clk_put(parent);
629+
clk_put(clk);
630+
}
631+
597632
/*
598633
* Test that, for a mux that started orphan but got switched to a valid
599634
* parent, the rate of the mux and its new parent are consistent.
@@ -625,6 +660,39 @@ clk_test_orphan_transparent_multiple_parent_mux_set_parent_get_rate(struct kunit
625660
clk_put(clk);
626661
}
627662

663+
/*
664+
* Test that, for a mux that started orphan but got switched to a valid
665+
* parent, calling clk_put() on the mux won't affect the parent rate.
666+
*/
667+
static void
668+
clk_test_orphan_transparent_multiple_parent_mux_set_parent_put(struct kunit *test)
669+
{
670+
struct clk_multiple_parent_ctx *ctx = test->priv;
671+
struct clk *clk, *parent;
672+
unsigned long parent_rate, new_parent_rate;
673+
int ret;
674+
675+
parent = clk_hw_get_clk(&ctx->parents_ctx[1].hw, NULL);
676+
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
677+
678+
clk = clk_hw_get_clk(&ctx->hw, NULL);
679+
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, clk);
680+
681+
parent_rate = clk_get_rate(parent);
682+
KUNIT_ASSERT_GT(test, parent_rate, 0);
683+
684+
ret = clk_set_parent(clk, parent);
685+
KUNIT_ASSERT_EQ(test, ret, 0);
686+
687+
clk_put(clk);
688+
689+
new_parent_rate = clk_get_rate(parent);
690+
KUNIT_ASSERT_GT(test, new_parent_rate, 0);
691+
KUNIT_EXPECT_EQ(test, parent_rate, new_parent_rate);
692+
693+
clk_put(parent);
694+
}
695+
628696
/*
629697
* Test that, for a mux that started orphan but got switched to a valid
630698
* parent, calling clk_set_rate_range() will affect the parent state if
@@ -658,6 +726,43 @@ clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_range_modified(st
658726
clk_put(clk);
659727
}
660728

729+
/*
730+
* Test that, for a mux that started orphan but got switched to a valid
731+
* parent, calling clk_set_rate_range() won't affect the parent state if
732+
* its rate is within range.
733+
*/
734+
static void
735+
clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_range_untouched(struct kunit *test)
736+
{
737+
struct clk_multiple_parent_ctx *ctx = test->priv;
738+
struct clk_hw *hw = &ctx->hw;
739+
struct clk *clk = clk_hw_get_clk(hw, NULL);
740+
struct clk *parent;
741+
unsigned long parent_rate, new_parent_rate;
742+
int ret;
743+
744+
parent = clk_hw_get_clk(&ctx->parents_ctx[1].hw, NULL);
745+
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
746+
747+
parent_rate = clk_get_rate(parent);
748+
KUNIT_ASSERT_GT(test, parent_rate, 0);
749+
750+
ret = clk_set_parent(clk, parent);
751+
KUNIT_ASSERT_EQ(test, ret, 0);
752+
753+
ret = clk_set_rate_range(clk,
754+
DUMMY_CLOCK_INIT_RATE - 1000,
755+
DUMMY_CLOCK_INIT_RATE + 1000);
756+
KUNIT_ASSERT_EQ(test, ret, 0);
757+
758+
new_parent_rate = clk_get_rate(parent);
759+
KUNIT_ASSERT_GT(test, new_parent_rate, 0);
760+
KUNIT_EXPECT_EQ(test, parent_rate, new_parent_rate);
761+
762+
clk_put(parent);
763+
clk_put(clk);
764+
}
765+
661766
/*
662767
* Test that, for a mux whose current parent hasn't been registered yet,
663768
* calling clk_set_rate_range() will succeed, and will be taken into
@@ -724,8 +829,11 @@ clk_test_orphan_transparent_multiple_parent_mux_set_range_set_parent_get_rate(st
724829
static struct kunit_case clk_orphan_transparent_multiple_parent_mux_test_cases[] = {
725830
KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_get_parent),
726831
KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent),
832+
KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_drop_range),
727833
KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_get_rate),
834+
KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_put),
728835
KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_range_modified),
836+
KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_range_untouched),
729837
KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_range_round_rate),
730838
KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_range_set_parent_get_rate),
731839
{}
@@ -1021,6 +1129,136 @@ static struct kunit_suite clk_orphan_transparent_single_parent_test_suite = {
10211129
.test_cases = clk_orphan_transparent_single_parent_mux_test_cases,
10221130
};
10231131

1132+
struct clk_single_parent_two_lvl_ctx {
1133+
struct clk_dummy_context parent_parent_ctx;
1134+
struct clk_dummy_context parent_ctx;
1135+
struct clk_hw hw;
1136+
};
1137+
1138+
static int
1139+
clk_orphan_two_level_root_last_test_init(struct kunit *test)
1140+
{
1141+
struct clk_single_parent_two_lvl_ctx *ctx;
1142+
int ret;
1143+
1144+
ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
1145+
if (!ctx)
1146+
return -ENOMEM;
1147+
test->priv = ctx;
1148+
1149+
ctx->parent_ctx.hw.init =
1150+
CLK_HW_INIT("intermediate-parent",
1151+
"root-parent",
1152+
&clk_dummy_single_parent_ops,
1153+
CLK_SET_RATE_PARENT);
1154+
ret = clk_hw_register(NULL, &ctx->parent_ctx.hw);
1155+
if (ret)
1156+
return ret;
1157+
1158+
ctx->hw.init =
1159+
CLK_HW_INIT("test-clk", "intermediate-parent",
1160+
&clk_dummy_single_parent_ops,
1161+
CLK_SET_RATE_PARENT);
1162+
ret = clk_hw_register(NULL, &ctx->hw);
1163+
if (ret)
1164+
return ret;
1165+
1166+
ctx->parent_parent_ctx.rate = DUMMY_CLOCK_INIT_RATE;
1167+
ctx->parent_parent_ctx.hw.init =
1168+
CLK_HW_INIT_NO_PARENT("root-parent",
1169+
&clk_dummy_rate_ops,
1170+
0);
1171+
ret = clk_hw_register(NULL, &ctx->parent_parent_ctx.hw);
1172+
if (ret)
1173+
return ret;
1174+
1175+
return 0;
1176+
}
1177+
1178+
static void
1179+
clk_orphan_two_level_root_last_test_exit(struct kunit *test)
1180+
{
1181+
struct clk_single_parent_two_lvl_ctx *ctx = test->priv;
1182+
1183+
clk_hw_unregister(&ctx->hw);
1184+
clk_hw_unregister(&ctx->parent_ctx.hw);
1185+
clk_hw_unregister(&ctx->parent_parent_ctx.hw);
1186+
}
1187+
1188+
/*
1189+
* Test that, for a clock whose parent used to be orphan, clk_get_rate()
1190+
* will return the proper rate.
1191+
*/
1192+
static void
1193+
clk_orphan_two_level_root_last_test_get_rate(struct kunit *test)
1194+
{
1195+
struct clk_single_parent_two_lvl_ctx *ctx = test->priv;
1196+
struct clk_hw *hw = &ctx->hw;
1197+
struct clk *clk = clk_hw_get_clk(hw, NULL);
1198+
unsigned long rate;
1199+
1200+
rate = clk_get_rate(clk);
1201+
KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_INIT_RATE);
1202+
1203+
clk_put(clk);
1204+
}
1205+
1206+
/*
1207+
* Test that, for a clock whose parent used to be orphan,
1208+
* clk_set_rate_range() won't affect its rate if it is already within
1209+
* range.
1210+
*
1211+
* See (for Exynos 4210):
1212+
* https://lore.kernel.org/linux-clk/366a0232-bb4a-c357-6aa8-636e398e05eb@samsung.com/
1213+
*/
1214+
static void
1215+
clk_orphan_two_level_root_last_test_set_range(struct kunit *test)
1216+
{
1217+
struct clk_single_parent_two_lvl_ctx *ctx = test->priv;
1218+
struct clk_hw *hw = &ctx->hw;
1219+
struct clk *clk = clk_hw_get_clk(hw, NULL);
1220+
unsigned long rate;
1221+
int ret;
1222+
1223+
ret = clk_set_rate_range(clk,
1224+
DUMMY_CLOCK_INIT_RATE - 1000,
1225+
DUMMY_CLOCK_INIT_RATE + 1000);
1226+
KUNIT_ASSERT_EQ(test, ret, 0);
1227+
1228+
rate = clk_get_rate(clk);
1229+
KUNIT_ASSERT_GT(test, rate, 0);
1230+
KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_INIT_RATE);
1231+
1232+
clk_put(clk);
1233+
}
1234+
1235+
static struct kunit_case
1236+
clk_orphan_two_level_root_last_test_cases[] = {
1237+
KUNIT_CASE(clk_orphan_two_level_root_last_test_get_rate),
1238+
KUNIT_CASE(clk_orphan_two_level_root_last_test_set_range),
1239+
{}
1240+
};
1241+
1242+
/*
1243+
* Test suite for a basic, transparent, clock with a parent that is also
1244+
* such a clock. The parent's parent is registered last, while the
1245+
* parent and its child are registered in that order. The intermediate
1246+
* and leaf clocks will thus be orphan when registered, but the leaf
1247+
* clock itself will always have its parent and will never be
1248+
* reparented. Indeed, it's only orphan because its parent is.
1249+
*
1250+
* These tests exercise the behaviour of the consumer API when dealing
1251+
* with an orphan clock, and how we deal with the transition to a valid
1252+
* parent.
1253+
*/
1254+
static struct kunit_suite
1255+
clk_orphan_two_level_root_last_test_suite = {
1256+
.name = "clk-orphan-two-level-root-last-test",
1257+
.init = clk_orphan_two_level_root_last_test_init,
1258+
.exit = clk_orphan_two_level_root_last_test_exit,
1259+
.test_cases = clk_orphan_two_level_root_last_test_cases,
1260+
};
1261+
10241262
/*
10251263
* Test that clk_set_rate_range won't return an error for a valid range
10261264
* and that it will make sure the rate of the clock is within the
@@ -1729,6 +1967,7 @@ kunit_test_suites(
17291967
&clk_multiple_parents_mux_test_suite,
17301968
&clk_orphan_transparent_multiple_parent_mux_test_suite,
17311969
&clk_orphan_transparent_single_parent_test_suite,
1970+
&clk_orphan_two_level_root_last_test_suite,
17321971
&clk_range_test_suite,
17331972
&clk_range_maximize_test_suite,
17341973
&clk_range_minimize_test_suite,

0 commit comments

Comments
 (0)