Messages in this thread | | | Date | Mon, 26 Aug 2019 17:40:14 -0700 | From | Bjorn Andersson <> | Subject | Re: [PATCH] clk: Evict unregistered clks from parent caches |
| |
On Mon 26 Aug 16:43 PDT 2019, Stephen Boyd wrote:
> We leave a dangling pointer in each clk_core::parents array that has an > unregistered clk as a potential parent when that clk_core pointer is > freed by clk{_hw}_unregister(). It is impossible for the true parent of > a clk to be set with clk_set_parent() once the dangling pointer is left > in the cache because we compare parent pointers in > clk_fetch_parent_index() instead of checking for a matching clk name or > clk_hw pointer. > > Before commit ede77858473a ("clk: Remove global clk traversal on fetch > parent index"), we would check clk_hw pointers, which has a higher > chance of being the same between registration and unregistration, but it > can still be allocated and freed by the clk provider. In fact, this has > been a long standing problem since commit da0f0b2c3ad2 ("clk: Correct > lookup logic in clk_fetch_parent_index()") where we stopped trying to > compare clk names and skipped over entries in the cache that weren't > NULL. > > There are good (performance) reasons to not do the global tree lookup in > cases where the cache holds dangling pointers to parents that have been > unregistered. Let's take the performance hit on the uncommon > registration path instead. Loop through all the clk_core::parents arrays > when a clk is unregistered and set the entry to NULL when the parent > cache entry and clk being unregistered are the same pointer. This will > fix this problem and avoid the overhead for the "normal" case. > > Based on a patch by Bjorn Andersson. > > Fixes: da0f0b2c3ad2 ("clk: Correct lookup logic in clk_fetch_parent_index()") > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Thanks for writing this up.
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > --- > drivers/clk/clk.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index c0990703ce54..f3982bfa39d6 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -3737,6 +3737,34 @@ static const struct clk_ops clk_nodrv_ops = { > .set_parent = clk_nodrv_set_parent, > }; > > +static void clk_core_evict_parent_cache_subtree(struct clk_core *root, > + struct clk_core *target) > +{ > + int i; > + struct clk_core *child; > + > + for (i = 0; i < root->num_parents; i++) > + if (root->parents[i].core == target) > + root->parents[i].core = NULL; > + > + hlist_for_each_entry(child, &root->children, child_node) > + clk_core_evict_parent_cache_subtree(child, target); > +} > + > +/* Remove this clk from all parent caches */ > +static void clk_core_evict_parent_cache(struct clk_core *core) > +{ > + struct hlist_head **lists; > + struct clk_core *root; > + > + lockdep_assert_held(&prepare_lock); > + > + for (lists = all_lists; *lists; lists++) > + hlist_for_each_entry(root, *lists, child_node) > + clk_core_evict_parent_cache_subtree(root, core); > + > +} > + > /** > * clk_unregister - unregister a currently registered clock > * @clk: clock to unregister > @@ -3775,6 +3803,8 @@ void clk_unregister(struct clk *clk) > clk_core_set_parent_nolock(child, NULL); > } > > + clk_core_evict_parent_cache(clk->core); > + > hlist_del_init(&clk->core->child_node); > > if (clk->core->prepare_count) > -- > Sent by a computer through tubes >
| |