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

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

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

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@kernel.org>
Suggested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---

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@sony.com>

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

[Alan Tull]

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

Hi Frank,

> From: Frank Rowand <frank.rowand@sony.com>
>
> 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@kernel.org>
> Suggested-by: Alan Tull <atull@kernel.org>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
>
> 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).

Thanks for the very fast response!  Monday I'll be able to test it and
review it.

Thanks,
Alan

>
>  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@sony.com>
>

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

[Alan Tull]

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

Hi Frank,

> From: Frank Rowand <frank.rowand@sony.com>
>
> 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@kernel.org>
> Suggested-by: Alan Tull <atull@kernel.org>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
>
> 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).

Thanks for the very fast response!  Monday I'll be able to test it and
review it.

Thanks,
Alan

>
>  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@sony.com>
>

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

[Alan Tull]

On Sun, Jun 17, 2018 at 11:03 AM,  <frowand.list@gmail.com> 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@sony.com>
>
> 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@kernel.org>
> Suggested-by: Alan Tull <atull@kernel.org>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
Tested-by: Alan Tull <atull@kernel.org>
Reviewed-by: Alan Tull <atull@kernel.org>

> ---
>
> 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@sony.com>
>

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

[Alan Tull]

On Sun, Jun 17, 2018 at 11:03 AM,  <frowand.list@gmail.com> 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@sony.com>
>
> 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@kernel.org>
> Suggested-by: Alan Tull <atull@kernel.org>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
Tested-by: Alan Tull <atull@kernel.org>
Reviewed-by: Alan Tull <atull@kernel.org>

> ---
>
> 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@sony.com>
>

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

[Rob Herring]

On Sun, Jun 17, 2018 at 10:03 AM,  <frowand.list@gmail.com> wrote:
> From: Frank Rowand <frank.rowand@sony.com>
>
> 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@kernel.org>
> Suggested-by: Alan Tull <atull@kernel.org>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
>
> Compiles for one configuration.
> NOT boot tested.
> Not run through my normal process to check for new warnings, etc.

I'm assuming you will resend a non-RFC version for me to apply.

I think it would be a bit better if callers didn't have to do free and
populate themselves, but just made an invalidate call (like a normal
cache) and re-populating the cache could happen on demand. Or if it
was done as a single call, you could just copy the old entries to the
new larger array. But maybe there would be a race condition in doing
that? In any case, all that could be a subsequent patch.

Rob

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

[Frank Rowand]

On 06/20/18 11:23, Rob Herring wrote:
> On Sun, Jun 17, 2018 at 10:03 AM,  <frowand.list@gmail.com> wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> 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@kernel.org>
>> Suggested-by: Alan Tull <atull@kernel.org>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>>
>> Compiles for one configuration.
>> NOT boot tested.
>> Not run through my normal process to check for new warnings, etc.
> 
> I'm assuming you will resend a non-RFC version for me to apply.

Yes, I will.

> 
> I think it would be a bit better if callers didn't have to do free and
> populate themselves, but just made an invalidate call (like a normal
> cache) and re-populating the cache could happen on demand. Or if it
> was done as a single call, you could just copy the old entries to the
> new larger array. But maybe there would be a race condition in doing
> that? In any case, all that could be a subsequent patch.

Yes, the unspoken, underlying issue is a race condition.  I'll update
the commit comment to explain the race issues a little bit.  And maybe
add a code comment if I can be concise enough.

-Frank

> 
> Rob
>