All of lore.kernel.org
 help / color / mirror / Atom feed
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
To: Mark Brown <broonie@kernel.org>
Cc: Jean-Francois Moine <moinejf@free.fr>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Lars-Peter Clausen <lars@metafoo.de>,
	alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] ASoC: Remove 'const' from the device_node pointers
Date: Mon, 10 Nov 2014 19:51:22 +0000	[thread overview]
Message-ID: <20141110195122.GY4042@n2100.arm.linux.org.uk> (raw)
In-Reply-To: <20141110193704.GL3815@sirena.org.uk>

On Mon, Nov 10, 2014 at 07:37:04PM +0000, Mark Brown wrote:
> On Sun, Nov 09, 2014 at 08:33:45PM +0100, Jean-Francois Moine wrote:
> 
> > index 7ba7130..405f967 100644
> > --- a/include/sound/soc.h
> > +++ b/include/sound/soc.h
> > @@ -886,7 +886,7 @@ struct snd_soc_platform_driver {
> >  
> >  struct snd_soc_dai_link_component {
> >  	const char *name;
> > -	const struct device_node *of_node;
> > +	struct device_node *of_node;
> >  	const char *dai_name;
> >  };
> >  
> 
> The changelog talked about of_node_put() but I'm not seeing anything in
> the code which calls that except snd_soc_of_get_dai_name()?  Everything
> else just does comparisons of the pointer AFIACT from a quick scan
> through.

I think it comes from these files (from a previous patch from Jean
adding a load of inappropriate const's to places where they do not
belong):

 sound/soc/generic/simple-card.c       | 7 ++-----
 sound/soc/samsung/odroidx2_max98090.c | 4 ++--
 sound/soc/ux500/mop500.c              | 6 ++----

That patch was trying to do this to those three files:

-               np = (struct device_node *) dai_link->cpu_of_node;
-               of_node_put(np);
-               np = (struct device_node *) dai_link->codec_of_node;
-               of_node_put(np);
+               of_node_put(dai_link->cpu_of_node);
+               of_node_put(dai_link->codec_of_node);
...
-       of_node_put((struct device_node *)odroidx2_dai[0].cpu_of_node);
-       of_node_put((struct device_node *)odroidx2_dai[0].codec_of_node);
+       of_node_put(odroidx2_dai[0].cpu_of_node);
+       of_node_put(odroidx2_dai[0].codec_of_node);
...
                if (mop500_dai_links[i].cpu_of_node)
-                       of_node_put((struct device_node *)
-                               mop500_dai_links[i].cpu_of_node);
+                       of_node_put(mop500_dai_links[i].cpu_of_node);
                if (mop500_dai_links[i].codec_of_node)
-                       of_node_put((struct device_node *)
-                               mop500_dai_links[i].codec_of_node);
+                       of_node_put(mop500_dai_links[i].codec_of_node);

having made of_node_put() take a const pointer.  Since of_node_put()
ultimately modifies the data pointed to by that pointer, making its
argument "const" is incorrect.

Moreover, Jean's latest patch is absolutely the right thing to do.
Had he quoted me fully, then the reason would become clear.

struct device_node is a ref-counted structure.  That means if you
store a reference to it, you should "get" it, and you should "put"
it once you've done.  The act of "put"ing the pointed-to structure
involves writing to that structure, so it is totally unappropriate
to store a device_node structure as a const pointer.  It forces you
to have to cast it back to a non-const pointer at various points
in time to use various OF function calls.

So, what you have is a pointer that you would /like/ to be const, but
ultimately you violate its const-ness at some point in time when you
need to call of_node_put() on it.  So it isn't really const at all.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

      reply	other threads:[~2014-11-10 19:51 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-09 19:40 [PATCH v2 0/2] Fix some 'const' problems Jean-Francois Moine
2014-11-09 19:40 ` Jean-Francois Moine
2014-11-09 11:38 ` [PATCH v2 2/2] ASoC: simple-card: Remove useless casts Jean-Francois Moine
2014-11-10 19:02   ` Mark Brown
2014-11-09 19:33 ` [PATCH v2 1/2] ASoC: Remove 'const' from the device_node pointers Jean-Francois Moine
2014-11-10 19:03   ` Mark Brown
2014-11-10 19:12     ` Jean-Francois Moine
2014-11-10 19:25       ` Mark Brown
2014-11-10 19:37   ` Mark Brown
2014-11-10 19:51     ` Russell King - ARM Linux [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20141110195122.GY4042@n2100.arm.linux.org.uk \
    --to=linux@arm.linux.org.uk \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=lars@metafoo.de \
    --cc=lgirdwood@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=moinejf@free.fr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.