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: <20141105213925.3790BC41C40@trevor.secretlab.ca>
Date:	Wed, 05 Nov 2014 21:39:25 +0000
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Pantelis Antoniou <pantelis.antoniou@...sulko.com>
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@...r.kernel.org,
	Pete Popov <pete.popov@...sulko.com>,
	Dan Malek <dan.malek@...sulko.com>,
	Georgi Vlaev <georgi.vlaev@...sulko.com>,
	Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
	Pantelis Antoniou <panto@...oniou-consulting.com>
Subject: Re: [PATCH 3/5] of: Only call notifiers when node is attached

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.

g.

> 
> 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/