Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1697004

Re: [PATCH] ASoC: fix balance of of_node_get()/of_node_put()

From Mark Brown <broonie@kernel.org>
Newsgroups linux.kernel
Subject Re: [PATCH] ASoC: fix balance of of_node_get()/of_node_put()
Date 2017-07-26 13:40 +0200
Message-ID <u7sPw-89V-15@gated-at.bofh.it> (permalink)
References <u7g1Z-8nQ-31@gated-at.bofh.it>
Organization linux.* mail to news gateway

Show all headers | View raw


[Multipart message — attachments visible in raw view] - view raw

On Tue, Jul 25, 2017 at 11:49:52PM +0200, Antonio Borneo wrote:

> Fixed by:
> - removing of_node_put() in the body of of_for_each_phandle(){},
>   since already provided at each iteration. Add it in case the
>   loop is break out;
> - adding of_node_get() before calling of_graph_get_port_parent()
>   or asoc_graph_card_dai_link_of().

>  sound/soc/generic/audio-graph-card.c  | 14 +++++++++-----
>  sound/soc/generic/simple-card-utils.c |  5 +++++
>  sound/soc/soc-core.c                  |  5 +++++
>  3 files changed, 19 insertions(+), 5 deletions(-)

This is a series of different changes to fix different (although
related) problems which should be being submitted individually.  Sending
multiple changes in one patch makes it harder to review things and for
fixes like this makes it harder to backport the fixes where not all the
code being fixed was introduced in a single kernel version.

>  	of_for_each_phandle(&it, rc, node, "dais", NULL, 0) {
> +		/*
> +		 * asoc_graph_card_dai_link_of() will call
> +		 * of_node_put(). So, call of_node_get() here
> +		 */
> +		of_node_get(it.node);
>  		ret = asoc_graph_card_dai_link_of(it.node, priv, idx++);

Why is this the most sensible fix?  It is really not at all obvious why
asoc_graph_card_dai_link_of() would drop a reference, or in what
situations callers might have a reference they're OK with dropping.

> +	/*
> +	 * of_graph_get_port_parent() will call
> +	 * of_node_put(). So, call of_node_get() here
> +	 */
> +	of_node_get(ep);
>  	node = of_graph_get_port_parent(ep);

Same here, why does this make sense?  It is not in the least bit obvious
to me why looking up the parent of a node would cause us to drop the
reference to the current node, this seems like an error prone and
confusing API which would be better fixed.

Back to linux.kernel | Previous | NextPrevious in thread | Next in thread | Find similar | Unroll thread


Thread

[PATCH] ASoC: fix balance of of_node_get()/of_node_put() Antonio Borneo <borneo.antonio@gmail.com> - 2017-07-26 00:00 +0200
  Re: [PATCH] ASoC: fix balance of of_node_get()/of_node_put() Mark Brown <broonie@kernel.org> - 2017-07-26 13:40 +0200
    Re: [PATCH] ASoC: fix balance of of_node_get()/of_node_put() Antonio Borneo <borneo.antonio@gmail.com> - 2017-07-28 01:30 +0200
  [PATCH v2 0/3] ASoC: fix unbalanced of_node_get()/of_node_put() Antonio Borneo <borneo.antonio@gmail.com> - 2017-07-28 01:40 +0200
    Re: [PATCH v2 0/3] ASoC: fix unbalanced of_node_get()/of_node_put() Mark Brown <broonie@kernel.org> - 2017-07-28 12:10 +0200
      Re: [PATCH v2 0/3] ASoC: fix unbalanced of_node_get()/of_node_put() Antonio Borneo <borneo.antonio@gmail.com> - 2017-07-30 22:40 +0200

csiph-web