lkml.org 
[lkml]   [2019]   [Aug]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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
    >

    \
     
     \ /
      Last update: 2019-08-27 02:39    [W:5.805 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site