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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <EE6F3801-A76A-4D95-B841-A7935FA7FCF9@konsulko.com>
Date:	Thu, 6 Nov 2014 14:33:29 +0200
From:	Pantelis Antoniou <pantelis.antoniou@...sulko.com>
To:	Grant Likely <grant.likely@...retlab.ca>
Cc:	Rob Herring <robherring2@...il.com>,
	Stephen Warren <swarren@...dotorg.org>,
	Matt Porter <matt.porter@...aro.org>,
	Koen Kooi <koen@...inion.thruhere.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Alison Chaiken <Alison_Chaiken@...tor.com>,
	Dinh Nguyen <dinh.linux@...il.com>,
	Jan Lubbe <jluebbe@...net.de>,
	Alexander Sverdlin <alexander.sverdlin@....com>,
	Michael Stickel <ms@...able.de>,
	Guenter Roeck <linux@...ck-us.net>,
	Dirk Behme <dirk.behme@...il.com>,
	Alan Tull <delicious.quinoa@...il.com>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Michael Bohan <mbohan@...eaurora.org>,
	Ionut Nicu <ioan.nicu.ext@....com>,
	Michal Simek <monstr@...str.eu>,
	Matt Ranostay <mranostay@...il.com>,
	devicetree@...r.kernel.org,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Pete Popov <pete.popov@...sulko.com>,
	Dan Malek <dan.malek@...sulko.com>,
	Georgi Vlaev <georgi.vlaev@...sulko.com>
Subject: Re: [PATCH 3/5] of: Only call notifiers when node is attached

Hi Grant,

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

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@...sulko.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
>> 
> 

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