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

[Pantelis Antoniou]

Hi Grant,

> On Nov 5, 2014, at 23:39 , Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:
> 
> On Tue, 28 Oct 2014 22:33:51 +0200
> , Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
> 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.
> 

Valid points. In fact I performed some tests and with this reverted things
still work.

The rationale behind this is for when nodes/properties are removed in the overlay,
but since we donât support this for now, we never hit the case where it was
needed.

Please remove from the patch series, Iâll revisit this when I add the removal
functionality.

> g.
> 

Regards

â Pantelis

>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
>> ---
>> 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
>> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/