Re: [RFC PATCH] of: overlay: update phandle cache on overlay apply and remove

From: Alan Tull
Date: Mon Jun 18 2018 - 12:50:58 EST


On Sun, Jun 17, 2018 at 11:03 AM, <frowand.list@xxxxxxxxx> wrote:

Hi Frank,

Thanks again for the fast response while traveling. The RFC looks
good in my testing and review.

> From: Frank Rowand <frank.rowand@xxxxxxxx>
>
> A comment in the review of the patch adding the phandle cache said that
> the cache would have to be updated when modules are applied and removed.
> This patch implements the cache updates.
>
> Fixes: 0b3ce78e90fc ("of: cache phandle nodes to reduce cost of of_find_node_by_phandle()")
> Reported-by: Alan Tull <atull@xxxxxxxxxx>
> Suggested-by: Alan Tull <atull@xxxxxxxxxx>
> Signed-off-by: Frank Rowand <frank.rowand@xxxxxxxx>
Tested-by: Alan Tull <atull@xxxxxxxxxx>
Reviewed-by: Alan Tull <atull@xxxxxxxxxx>

> ---
>
> Compiles for one configuration.
> NOT boot tested.
> Not run through my normal process to check for new warnings, etc.
>
> It is late, I'm tired, my brain is fuzzy. I need to review this more to have
> any confidence in it. But I wanted to get a version out for Alan to see (and
> test if he wants).
>
> drivers/of/base.c | 6 +++---
> drivers/of/of_private.h | 2 ++
> drivers/of/overlay.c | 12 ++++++++++++
> 3 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 848f549164cd..466e3c8582f0 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -102,7 +102,7 @@ static u32 phandle_cache_mask;
> * - the phandle lookup overhead reduction provided by the cache
> * will likely be less
> */
> -static void of_populate_phandle_cache(void)
> +void of_populate_phandle_cache(void)
> {
> unsigned long flags;
> u32 cache_entries;
> @@ -134,8 +134,7 @@ static void of_populate_phandle_cache(void)
> raw_spin_unlock_irqrestore(&devtree_lock, flags);
> }
>
> -#ifndef CONFIG_MODULES
> -static int __init of_free_phandle_cache(void)
> +int of_free_phandle_cache(void)
> {
> unsigned long flags;
>
> @@ -148,6 +147,7 @@ static int __init of_free_phandle_cache(void)
>
> return 0;
> }
> +#if !defined(CONFIG_MODULES)
> late_initcall_sync(of_free_phandle_cache);
> #endif
>
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 891d780c076a..216175d11d3d 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -79,6 +79,8 @@ int of_resolve_phandles(struct device_node *tree);
> #if defined(CONFIG_OF_OVERLAY)
> void of_overlay_mutex_lock(void);
> void of_overlay_mutex_unlock(void);
> +int of_free_phandle_cache(void);
> +void of_populate_phandle_cache(void);
> #else
> static inline void of_overlay_mutex_lock(void) {};
> static inline void of_overlay_mutex_unlock(void) {};
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 7baa53e5b1d7..3f76e58fbec4 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -804,6 +804,8 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
> goto err_free_overlay_changeset;
> }
>
> + of_populate_phandle_cache();
> +
> ret = __of_changeset_apply_notify(&ovcs->cset);
> if (ret)
> pr_err("overlay changeset entry notify error %d\n", ret);
> @@ -1046,8 +1048,18 @@ int of_overlay_remove(int *ovcs_id)
>
> list_del(&ovcs->ovcs_list);
>
> + /*
> + * Empty and disable phandle cache. Must empty here so that
> + * changeset notifiers do not use stale cache entry for a removed
> + * phandle.
> + */
> + of_free_phandle_cache();
> +
> ret_apply = 0;
> ret = __of_changeset_revert_entries(&ovcs->cset, &ret_apply);
> +
> + of_populate_phandle_cache();
> +
> if (ret) {
> if (ret_apply)
> devicetree_state_flags |= DTSF_REVERT_FAIL;
> --
> Frank Rowand <frank.rowand@xxxxxxxx>
>