Re: [PATCH 3/5] of: Only call notifiers when node is attached

[Grant Likely <grant.likely@secretlab.ca>]

On Tue, 28 Oct 2014 22:33:51 +0200
, Pantelis Antoniou <pantelis.antoniou@konsulko.com>
 wrote:
> Make sure we call notifier only when the node is attached.
> When a detatched tree is being constructed we do not want the
> notifiers to fire at all.

The description does not match what the patch does. The patch moves the
test into of_{add,remove,update}_property() and out of
of_property_notify() itself. That leaves one other caller of
of_property_notify(); __of_changeset_entry_notify(). The effect of this
patch is that applying a changeset will cause notifiers to be fired for
each property modified in a changeset. The comment says nothing about
the change in behaviour and it sounds like it is a bug fix when it
doesn't actually change the behaviour at all for the
of_{add,remove,update}_property() paths.

This needs a better changelog. It needs to describe what the effects of
the patch are and why the change is being made. When someone is
bisecting a problem and they land on this change, the changelog needs to
give them a good idea about what is going on and why.

g.

> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  drivers/of/base.c    | 9 ++++++---
>  drivers/of/dynamic.c | 5 +----
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 2305dc0..a79d4ee 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1695,7 +1695,8 @@ int of_add_property(struct device_node *np, struct property *prop)
>  
>  	mutex_unlock(&of_mutex);
>  
> -	if (!rc)
> +	/* only call notifiers if the node is attached and no error occurred */
> +	if (of_node_is_attached(np) && !rc)
>  		of_property_notify(OF_RECONFIG_ADD_PROPERTY, np, prop, NULL);
>  
>  	return rc;
> @@ -1754,7 +1755,8 @@ int of_remove_property(struct device_node *np, struct property *prop)
>  
>  	mutex_unlock(&of_mutex);
>  
> -	if (!rc)
> +	/* only call notifiers if the node is attached and no error occurred */
> +	if (of_node_is_attached(np) && !rc)
>  		of_property_notify(OF_RECONFIG_REMOVE_PROPERTY, np, prop, NULL);
>  
>  	return rc;
> @@ -1830,7 +1832,8 @@ int of_update_property(struct device_node *np, struct property *newprop)
>  
>  	mutex_unlock(&of_mutex);
>  
> -	if (!rc)
> +	/* only call notifiers if the node is attached and no error occurred */
> +	if (of_node_is_attached(np) && !rc)
>  		of_property_notify(OF_RECONFIG_UPDATE_PROPERTY, np, newprop, oldprop);
>  
>  	return rc;
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index f297891..8e8b614 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -90,10 +90,6 @@ int of_property_notify(int action, struct device_node *np,
>  {
>  	struct of_prop_reconfig pr;
>  
> -	/* only call notifiers if the node is attached */
> -	if (!of_node_is_attached(np))
> -		return 0;
> -
>  	pr.dn = np;
>  	pr.prop = prop;
>  	pr.old_prop = oldprop;
> @@ -138,6 +134,7 @@ int of_attach_node(struct device_node *np)
>  	__of_attach_node_sysfs(np);
>  	mutex_unlock(&of_mutex);
>  
> +	/* node is guaranteed to be attached at this point */
>  	of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, np);
>  
>  	return 0;
> -- 
> 1.7.12
>