From fa9b30447f7766ac7f95746d68341d38c990f68a Mon Sep 17 00:00:00 2001 From: David Collins Date: Mon, 25 Feb 2019 16:11:42 -0800 Subject: [PATCH] clk: correct vdd_class voting scheme used during clock rate changes Correct the method used to account for vdd_class voltage level requirements for each clock during a clk_set_rate() call. This ensures that the ref count for each level does not become out of sync during reentrant clk_set_rate() calls or during complex rate change and re-parenting situations. Implement the following behavior for clocks which have a vdd_class specified: clk_core_prepare(): - Calculate the voltage level needed for core->rate - Vote for this voltage level - Set both vdd_class_vote and new_vdd_class_vote to this level clk_core_unprepare(): - Unvote for core->vdd_class_vote - Set both vdd_class_vote and new_vdd_class_vote to 0 clk_core_set_rate_nolock() (and the helper functions it calls): - While traversing the clock tree and calculating core->new_rate for each affected clock: - Calculate next_level that is needed for core->new_rate - If next_level != core->new_vdd_class_vote: - Vote for next_level - Insert core->rate_change_node into clk_rate_change_list - If rate_change_node is already in the list, then Unvote core->new_vdd_class_vote since it is being changed - Set core->new_vdd_class_vote = next_level - If an error is encountered *before* any physical clock rates are changed, then loop over all clocks in clk_rate_change_list: - Unvote for core->new_vdd_class_vote - Set core->new_vdd_class_vote = core->vdd_class_vote - Remove core->rate_change_node from the list - If an error is encountered *after* any physical clock rates are changed, then loop over all clocks in clk_rate_change_list: - If core->vdd_class_vote > core->new_vdd_class_vote: - Vote for core->vdd_class_vote - Unvote for core->new_vdd_class_vote - Set core->new_vdd_class_vote = core->vdd_class_vote - This ensures that the maximum voltage level needed for either core->rate or core->new_rate is configured since the physical state of the clock isn't well known after an error. - Unvote for core->vdd_class_vote - Set core->vdd_class_vote = core->new_vdd_class_vote - Remove core->rate_change_node from the list - If all physical clock rates are changed successfully (including in reentrant clk_core_set_rate_nolock() calls), then loop over all clocks in clk_rate_change_list: - Unvote for core->vdd_class_vote - Set core->vdd_class_vote = core->new_vdd_class_vote - Remove core->rate_change_node from the list Since clk_rate_change_list is shared between the original invocation of clk_core_set_rate_nolock() and reentrant calls to the function resulting from set_rate() callback ops that explicitly call clk_set_rate(), special care must be taken when inserting and removing elements. Insertions must occur atomically with new_vdd_class_vote voting. Removals must only occur from the original invocation of clk_core_set_rate_nolock() and not in the reentrant calls. Use a static nesting count to tell the original and reentrant calls apart. Change-Id: I3bfebf20101a0bf22a79a33188a63dfff0af11cc Signed-off-by: David Collins --- drivers/clk/clk.c | 278 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 195 insertions(+), 83 deletions(-) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index fce7ab4e28d7..1cb305fc775f 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -50,6 +50,17 @@ struct clk_handoff_vdd { }; static LIST_HEAD(clk_handoff_vdd_list); +static bool vdd_class_handoff_completed; +static DEFINE_MUTEX(vdd_class_list_lock); +/* + * clk_rate_change_list is used during clk_core_set_rate_nolock() calls to + * handle vdd_class vote tracking. core->rate_change_node is added to + * clk_rate_change_list when core->new_rate requires a different voltage level + * (core->new_vdd_class_vote) than core->vdd_class_vote. Elements are removed + * from the list after unvoting core->vdd_class_vote immediately before + * returning from clk_core_set_rate_nolock(). + */ +static LIST_HEAD(clk_rate_change_list); /*** private data structures ***/ @@ -67,9 +78,7 @@ struct clk_core { unsigned long rate; unsigned long req_rate; unsigned long new_rate; - unsigned long old_rate; struct clk_core *new_parent; - struct clk_core *old_parent; struct clk_core *new_child; unsigned long flags; bool orphan; @@ -93,6 +102,9 @@ struct clk_core { #endif struct kref ref; struct clk_vdd_class *vdd_class; + int vdd_class_vote; + int new_vdd_class_vote; + struct list_head rate_change_node; unsigned long *rate_max; int num_rate_max; }; @@ -708,9 +720,11 @@ static int clk_unvote_vdd_level(struct clk_vdd_class *vdd_class, int level) mutex_lock(&vdd_class->lock); if (WARN(!vdd_class->level_votes[level], - "Reference counts are incorrect for %s level %d\n", - vdd_class->class_name, level)) + "Reference counts are incorrect for %s level %d\n", + vdd_class->class_name, level)) { + rc = -EINVAL; goto out; + } vdd_class->level_votes[level]--; @@ -774,29 +788,43 @@ static bool clk_is_rate_level_valid(struct clk_core *core, unsigned long rate) static int clk_vdd_class_init(struct clk_vdd_class *vdd) { struct clk_handoff_vdd *v; + int ret = 0; if (vdd->skip_handoff) return 0; + mutex_lock(&vdd_class_list_lock); + list_for_each_entry(v, &clk_handoff_vdd_list, list) { if (v->vdd_class == vdd) - return 0; + goto done; } - pr_debug("voting for vdd_class %s\n", vdd->class_name); + if (!vdd_class_handoff_completed) { + pr_debug("voting for vdd_class %s\n", vdd->class_name); - if (clk_vote_vdd_level(vdd, vdd->num_levels - 1)) - pr_err("failed to vote for %s\n", vdd->class_name); + ret = clk_vote_vdd_level(vdd, vdd->num_levels - 1); + if (ret) { + pr_err("failed to vote for %s, ret=%d\n", + vdd->class_name, ret); + goto done; + } + } v = kmalloc(sizeof(*v), GFP_KERNEL); - if (!v) - return -ENOMEM; + if (!v) { + ret = -ENOMEM; + goto done; + } v->vdd_class = vdd; list_add_tail(&v->list, &clk_handoff_vdd_list); - return 0; +done: + mutex_unlock(&vdd_class_list_lock); + + return ret; } /*** clk api ***/ @@ -967,7 +995,11 @@ static void clk_core_unprepare(struct clk_core *core) trace_clk_unprepare_complete(core); - clk_unvote_rate_vdd(core, core->rate); + if (core->vdd_class) { + clk_unvote_vdd_level(core->vdd_class, core->vdd_class_vote); + core->vdd_class_vote = 0; + core->new_vdd_class_vote = 0; + } clk_core_unprepare(core->parent); } @@ -1024,6 +1056,11 @@ static int clk_core_prepare(struct clk_core *core) clk_core_unprepare(core->parent); return ret; } + if (core->vdd_class) { + core->vdd_class_vote + = clk_find_vdd_level(core, core->rate); + core->new_vdd_class_vote = core->vdd_class_vote; + } if (core->ops->prepare) ret = core->ops->prepare(core->hw); @@ -1032,6 +1069,8 @@ static int clk_core_prepare(struct clk_core *core) if (ret) { clk_unvote_rate_vdd(core, core->rate); + core->vdd_class_vote = 0; + core->new_vdd_class_vote = 0; goto unprepare; } } @@ -1372,12 +1411,15 @@ static int clk_disable_unused(void) hlist_for_each_entry(core, &clk_orphan_list, child_node) clk_unprepare_unused_subtree(core); + mutex_lock(&vdd_class_list_lock); list_for_each_entry_safe(v, v_temp, &clk_handoff_vdd_list, list) { clk_unvote_vdd_level(v->vdd_class, v->vdd_class->num_levels - 1); list_del(&v->list); kfree(v); } + vdd_class_handoff_completed = true; + mutex_unlock(&vdd_class_list_lock); clk_prepare_unlock(); @@ -1915,12 +1957,59 @@ static int __clk_speculate_rates(struct clk_core *core, return ret; } -static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate, +/* + * Vote for the voltage level required for core->new_rate. Keep track of all + * clocks with a changed voltage level in clk_rate_change_list. + */ +static int clk_vote_new_rate_vdd(struct clk_core *core) +{ + int cur_level, next_level; + int ret; + + if (IS_ERR_OR_NULL(core) || !core->vdd_class) + return 0; + + if (!clk_core_is_prepared(core)) + return 0; + + cur_level = core->new_vdd_class_vote; + next_level = clk_find_vdd_level(core, core->new_rate); + if (cur_level == next_level) + return 0; + + ret = clk_vote_vdd_level(core->vdd_class, next_level); + if (ret) + return ret; + + core->new_vdd_class_vote = next_level; + + if (list_empty(&core->rate_change_node)) { + list_add(&core->rate_change_node, &clk_rate_change_list); + } else { + /* + * A different new_rate has been determined for a clock that + * was already encountered in the clock tree traversal so the + * level that was previously voted for it should be removed. + */ + ret = clk_unvote_vdd_level(core->vdd_class, cur_level); + if (ret) + return ret; + } + + return 0; +} + +static int clk_calc_subtree(struct clk_core *core, unsigned long new_rate, struct clk_core *new_parent, u8 p_index) { struct clk_core *child; + int ret; core->new_rate = new_rate; + ret = clk_vote_new_rate_vdd(core); + if (ret) + return ret; + core->new_parent = new_parent; core->new_parent_index = p_index; /* include clk in new parent's PRE_RATE_CHANGE notifications */ @@ -1930,8 +2019,12 @@ static void clk_calc_subtree(struct clk_core *core, unsigned long new_rate, hlist_for_each_entry(child, &core->children, child_node) { child->new_rate = clk_recalc(child, new_rate); - clk_calc_subtree(child, child->new_rate, NULL, 0); + ret = clk_calc_subtree(child, child->new_rate, NULL, 0); + if (ret) + return ret; } + + return 0; } /* @@ -2024,7 +2117,9 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core, if (!clk_is_rate_level_valid(core, rate)) return NULL; - clk_calc_subtree(core, new_rate, parent, p_index); + ret = clk_calc_subtree(core, new_rate, parent, p_index); + if (ret) + return NULL; return top; } @@ -2083,7 +2178,7 @@ static int clk_change_rate(struct clk_core *core) struct clk_core *parent = NULL; int rc = 0; - core->old_rate = old_rate = core->rate; + old_rate = core->rate; if (core->new_parent) { parent = core->new_parent; @@ -2097,8 +2192,6 @@ static int clk_change_rate(struct clk_core *core) if (rc) return rc; - core->old_parent = core->parent; - if (core->flags & CLK_SET_RATE_UNGATE) { unsigned long flags; @@ -2112,7 +2205,6 @@ static int clk_change_rate(struct clk_core *core) if (core->new_parent && core->new_parent != core->parent) { old_parent = __clk_set_parent_before(core, core->new_parent); - core->old_parent = old_parent; trace_clk_set_parent(core, core->new_parent); if (core->ops->set_rate_and_parent) { @@ -2211,72 +2303,68 @@ static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core, return ret ? 0 : req.rate; } -static int vote_vdd_up(struct clk_core *core) +/* + * Unvote for the voltage level required for each core->new_vdd_class_vote in + * clk_rate_change_list. This is used when undoing voltage requests after an + * error is encountered before any physical rate changing. + */ +static void clk_unvote_new_rate_vdd(void) { - struct clk_core *parent = NULL; - int ret, cur_level, next_level; + struct clk_core *core; - /* sanity */ - if (IS_ERR_OR_NULL(core)) - return 0; - - if (core->vdd_class) { - cur_level = clk_find_vdd_level(core, core->rate); - next_level = clk_find_vdd_level(core, core->new_rate); - if (cur_level == next_level) - return 0; + list_for_each_entry(core, &clk_rate_change_list, rate_change_node) { + clk_unvote_vdd_level(core->vdd_class, core->new_vdd_class_vote); + core->new_vdd_class_vote = core->vdd_class_vote; } +} - /* save parent rate, if it exists */ - if (core->new_parent) - parent = core->new_parent; - else if (core->parent) - parent = core->parent; +/* + * Unvote for the voltage level required for each core->vdd_class_vote in + * clk_rate_change_list. + */ +static int clk_unvote_old_rate_vdd(void) +{ + struct clk_core *core; + int ret; - if (core->prepare_count && core->new_rate) { - ret = clk_vote_rate_vdd(core, core->new_rate); + list_for_each_entry(core, &clk_rate_change_list, rate_change_node) { + ret = clk_unvote_vdd_level(core->vdd_class, + core->vdd_class_vote); if (ret) return ret; } - vote_vdd_up(parent); - return 0; } -static int vote_vdd_down(struct clk_core *core) +/* + * In the case that rate setting fails, apply the max voltage level needed + * by either the old or new rate for each changed clock. + */ +static void clk_vote_safe_vdd(void) { - struct clk_core *parent; - unsigned long rate; - int cur_level, old_level; + struct clk_core *core; - /* sanity */ - if (IS_ERR_OR_NULL(core)) - return 0; - - rate = core->old_rate; - - /* New rate set was a failure */ - if (DIV_ROUND_CLOSEST(core->rate, 1000) != - DIV_ROUND_CLOSEST(core->new_rate, 1000)) - rate = core->new_rate; - - if (core->vdd_class) { - cur_level = clk_find_vdd_level(core, core->rate); - old_level = clk_find_vdd_level(core, core->old_rate); - if ((cur_level == old_level) - || !core->vdd_class->level_votes[old_level]) - return 0; + list_for_each_entry(core, &clk_rate_change_list, rate_change_node) { + if (core->vdd_class_vote > core->new_vdd_class_vote) { + clk_vote_vdd_level(core->vdd_class, + core->vdd_class_vote); + clk_unvote_vdd_level(core->vdd_class, + core->new_vdd_class_vote); + core->new_vdd_class_vote = core->vdd_class_vote; + } } +} - parent = core->old_parent; +static void clk_cleanup_vdd_votes(void) +{ + struct clk_core *core, *temp; - if (core->prepare_count && rate) - clk_unvote_rate_vdd(core, rate); - - vote_vdd_down(parent); - - return 0; + list_for_each_entry_safe(core, temp, &clk_rate_change_list, + rate_change_node) { + core->vdd_class_vote = core->new_vdd_class_vote; + list_del_init(&core->rate_change_node); + } } static int clk_core_set_rate_nolock(struct clk_core *core, @@ -2285,6 +2373,15 @@ static int clk_core_set_rate_nolock(struct clk_core *core, struct clk_core *top, *fail_clk; unsigned long rate; int ret = 0; + /* + * The prepare lock ensures mutual exclusion with other tasks. + * set_rate_nesting_count is a static so that it can be incremented in + * the case of reentrancy caused by a set_rate() ops callback itself + * calling clk_set_rate(). That way, the voltage level votes for the + * old rates are safely removed when the original invocation of this + * function completes. + */ + static unsigned int set_rate_nesting_count; if (!core) return 0; @@ -2301,12 +2398,14 @@ static int clk_core_set_rate_nolock(struct clk_core *core, /* calculate new rates and get the topmost changed clock */ top = clk_calc_new_rates(core, req_rate); - if (!top) - return -EINVAL; + if (!top) { + ret = -EINVAL; + goto pre_rate_change_err; + } ret = clk_pm_runtime_get(core); if (ret) - return ret; + goto pre_rate_change_err; /* notify that we are about to change rates */ fail_clk = clk_propagate_rate_change(top, PRE_RATE_CHANGE); @@ -2315,32 +2414,44 @@ static int clk_core_set_rate_nolock(struct clk_core *core, fail_clk->name, req_rate); clk_propagate_rate_change(top, ABORT_RATE_CHANGE); ret = -EBUSY; - goto err; + clk_pm_runtime_put(core); + goto pre_rate_change_err; } - /* Enforce the VDD for new frequency */ - ret = vote_vdd_up(core); - if (ret) - goto err; - /* change the rates */ + set_rate_nesting_count++; ret = clk_change_rate(top); + set_rate_nesting_count--; if (ret) { pr_err("%s: failed to set %s clock to run at %lu\n", __func__, top->name, req_rate); clk_propagate_rate_change(top, ABORT_RATE_CHANGE); - /* Release vdd requirements for new frequency. */ - vote_vdd_down(core); - goto err; + clk_vote_safe_vdd(); + goto post_rate_change_err; } core->req_rate = req_rate; - /* Release vdd requirements for old frequency. */ - vote_vdd_down(core); -err: +post_rate_change_err: + /* + * Only remove vdd_class level votes for old clock rates after all + * nested clk_set_rate() calls have completed. + */ + if (set_rate_nesting_count == 0) { + ret |= clk_unvote_old_rate_vdd(); + clk_cleanup_vdd_votes(); + } + clk_pm_runtime_put(core); + return ret; + +pre_rate_change_err: + if (set_rate_nesting_count == 0) { + clk_unvote_new_rate_vdd(); + clk_cleanup_vdd_votes(); + } + return ret; } @@ -4204,6 +4315,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) }; INIT_HLIST_HEAD(&core->clks); + INIT_LIST_HEAD(&core->rate_change_node); hw->clk = __clk_create_clk(hw, NULL, NULL); if (IS_ERR(hw->clk)) {