Fix section mismatch in wm8995.c

Fix section mismatch in wm8995.c

[Takashi Iwai]

FYI,

I noticed the section mismatch in wm8995.c, and fixed now on topic/asoc
branch with the patch below.


thanks,

Takashi

===
>From 03cfbdf9f7a1d392146718f67e50fa9ab2844f22 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Tue, 11 Jan 2011 17:58:26 +0100
Subject: [PATCH] ASoC: Fix section mismatch in wm8995.c

__devinitconst can't be used for data referred in driver struct.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm8995.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/soc/codecs/wm8995.c b/sound/soc/codecs/wm8995.c
index 3d2110c..6045cbd 100644
--- a/sound/soc/codecs/wm8995.c
+++ b/sound/soc/codecs/wm8995.c
@@ -30,7 +30,7 @@
 
 #include "wm8995.h"
 
-static const u16 wm8995_reg_defs[WM8995_MAX_REGISTER + 1] __devinitconst = {
+static const u16 wm8995_reg_defs[WM8995_MAX_REGISTER + 1] = {
 	[0]     = 0x8995, [5]     = 0x0100, [16]    = 0x000b, [17]    = 0x000b,
 	[24]    = 0x02c0, [25]    = 0x02c0, [26]    = 0x02c0, [27]    = 0x02c0,
 	[28]    = 0x000f, [32]    = 0x0005, [33]    = 0x0005, [40]    = 0x0003,
-- 
1.7.3.4

Re: Fix section mismatch in wm8995.c

[Mark Brown]

On Tue, Jan 11, 2011 at 06:02:54PM +0100, Takashi Iwai wrote:

> I noticed the section mismatch in wm8995.c, and fixed now on topic/asoc
> branch with the patch below.

Hrm, it'd be better to find some annotation for the pointer which lets
us have the reference.  We at a higher level only dereference the
pointer from the probe path, preventing the discard isn't a great
solution.

Re: Fix section mismatch in wm8995.c

[Takashi Iwai]

At Tue, 11 Jan 2011 17:10:22 +0000,
Mark Brown wrote:
> 
> On Tue, Jan 11, 2011 at 06:02:54PM +0100, Takashi Iwai wrote:
> 
> > I noticed the section mismatch in wm8995.c, and fixed now on topic/asoc
> > branch with the patch below.
> 
> Hrm, it'd be better to find some annotation for the pointer which lets
> us have the reference.  We at a higher level only dereference the
> pointer from the probe path, preventing the discard isn't a great
> solution.

Nah, that's way too complex for the time being.  I really prefer a
warning-less code for such a case.

The whole __init stuff have lots of potential improvements.  But,
improving it would cost pretty much.  And we don't want yet another
new annotation...


thanks,

Takashi

Re: Fix section mismatch in wm8995.c

[Mark Brown]

On Tue, Jan 11, 2011 at 06:18:49PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > Hrm, it'd be better to find some annotation for the pointer which lets
> > us have the reference.  We at a higher level only dereference the
> > pointer from the probe path, preventing the discard isn't a great
> > solution.

> Nah, that's way too complex for the time being.  I really prefer a
> warning-less code for such a case.

The array is ~25k so it's not a completely trivial amount of memory,
unfortunately.

> The whole __init stuff have lots of potential improvements.  But,
> improving it would cost pretty much.  And we don't want yet another
> new annotation...

Very true.  The other option is to refactor all the data structures to
appease it which is painful.

Re: Fix section mismatch in wm8995.c

[Takashi Iwai]

At Tue, 11 Jan 2011 17:28:40 +0000,
Mark Brown wrote:
> 
> On Tue, Jan 11, 2011 at 06:18:49PM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > Hrm, it'd be better to find some annotation for the pointer which lets
> > > us have the reference.  We at a higher level only dereference the
> > > pointer from the probe path, preventing the discard isn't a great
> > > solution.
> 
> > Nah, that's way too complex for the time being.  I really prefer a
> > warning-less code for such a case.
> 
> The array is ~25k so it's not a completely trivial amount of memory,
> unfortunately.

Hm.  But, it's same for (some) other codecs, no?
If we do annotate something, we should mark all these at once.

> > The whole __init stuff have lots of potential improvements.  But,
> > improving it would cost pretty much.  And we don't want yet another
> > new annotation...
> 
> Very true.  The other option is to refactor all the data structures to
> appease it which is painful.

Agreed.


Takashi

Re: Fix section mismatch in wm8995.c

[Mark Brown]

On Tue, Jan 11, 2011 at 06:32:32PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > The array is ~25k so it's not a completely trivial amount of memory,
> > unfortunately.

> Hm.  But, it's same for (some) other codecs, no?
> If we do annotate something, we should mark all these at once.

In principle, though since it requires per driver checking for
references and so on it's not as simple as just going through and
annotating.  It's partly tied in with the cache stuff, things that are
fully using that can be converted over easily.  And of course many of
the devices have much smaller register maps so it's much less of an
issue.

Re: Fix section mismatch in wm8995.c

[Takashi Iwai]

At Tue, 11 Jan 2011 17:33:49 +0000,
Mark Brown wrote:
> 
> On Tue, Jan 11, 2011 at 06:32:32PM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > The array is ~25k so it's not a completely trivial amount of memory,
> > > unfortunately.
> 
> > Hm.  But, it's same for (some) other codecs, no?
> > If we do annotate something, we should mark all these at once.
> 
> In principle, though since it requires per driver checking for
> references and so on it's not as simple as just going through and
> annotating.  It's partly tied in with the cache stuff, things that are
> fully using that can be converted over easily.  And of course many of
> the devices have much smaller register maps so it's much less of an
> issue.

IMO, such a data should be uniquely handled -- either init-only or
not.  Through a quick look, snd_soc_cache_sync() may still refer to
reg_cache_default.  So, it's still risky to blindly set
__devinitconst.  (Yeah, I know it's not used right now, though ;)

That is, the data that shall be deleted shouldn't be kept in a
structure that might be referred later from other places.


thanks,

Takashi

Re: Fix section mismatch in wm8995.c

[Mark Brown]

On Tue, Jan 11, 2011 at 06:48:30PM +0100, Takashi Iwai wrote:

> IMO, such a data should be uniquely handled -- either init-only or
> not.  Through a quick look, snd_soc_cache_sync() may still refer to
> reg_cache_default.  So, it's still risky to blindly set
> __devinitconst.  (Yeah, I know it's not used right now, though ;)

That's a bug in the flat cache, it should be stashing a copy of it in
the codec instance.  Though it only really makes a difference for
devices where you'd want to use a compressed cache anyway.

> That is, the data that shall be deleted shouldn't be kept in a
> structure that might be referred later from other places.

On the other hand you don't want to make stuff too much of a song and
dance to use.

Re: Fix section mismatch in wm8995.c

[Takashi Iwai]

At Tue, 11 Jan 2011 18:14:11 +0000,
Mark Brown wrote:
> 
> On Tue, Jan 11, 2011 at 06:48:30PM +0100, Takashi Iwai wrote:
> 
> > IMO, such a data should be uniquely handled -- either init-only or
> > not.  Through a quick look, snd_soc_cache_sync() may still refer to
> > reg_cache_default.  So, it's still risky to blindly set
> > __devinitconst.  (Yeah, I know it's not used right now, though ;)
> 
> That's a bug in the flat cache, it should be stashing a copy of it in
> the codec instance.

Heh, this is a drawback of keeping such stuff in struct :)

>  Though it only really makes a difference for
> devices where you'd want to use a compressed cache anyway.
> 
> > That is, the data that shall be deleted shouldn't be kept in a
> > structure that might be referred later from other places.
> 
> On the other hand you don't want to make stuff too much of a song and
> dance to use.

True, too.  


Takashi

Re: Fix section mismatch in wm8995.c

[Dimitris Papastamos]

On Tue, 2011-01-11 at 18:14 +0000, Mark Brown wrote:
> On Tue, Jan 11, 2011 at 06:48:30PM +0100, Takashi Iwai wrote:
> 
> > IMO, such a data should be uniquely handled -- either init-only or
> > not.  Through a quick look, snd_soc_cache_sync() may still refer to
> > reg_cache_default.  So, it's still risky to blindly set
> > __devinitconst.  (Yeah, I know it's not used right now, though ;)
> 
> That's a bug in the flat cache, it should be stashing a copy of it in
> the codec instance.  Though it only really makes a difference for
> devices where you'd want to use a compressed cache anyway.

The flat cache is intentionally *not* making a copy of the defaults
cache as it makes no difference in terms of memory usage.  The flat
cache has codec->reg_def_copy = NULL; and can work with either NULL
default caches or filled in.

Thanks,
Dimitris

Re: Fix section mismatch in wm8995.c

[Mark Brown]

On Wed, Jan 12, 2011 at 09:58:10AM +0000, Dimitris Papastamos wrote:
> On Tue, 2011-01-11 at 18:14 +0000, Mark Brown wrote:

> > That's a bug in the flat cache, it should be stashing a copy of it in
> > the codec instance.  Though it only really makes a difference for
> > devices where you'd want to use a compressed cache anyway.

> The flat cache is intentionally *not* making a copy of the defaults
> cache as it makes no difference in terms of memory usage.  The flat
> cache has codec->reg_def_copy = NULL; and can work with either NULL
> default caches or filled in.

It can make a difference to memory usage - if multiple CODEC drivers are
built in but only one is in use then if we take a copy all the defaults
can be discared, meaning that we don't need to keep the defaults for the
unused drivers and only keep the defaults for the one that's in use
instead of keeping the defaults for all drivers.

Re: Fix section mismatch in wm8995.c

[Dimitris Papastamos]

On Wed, 2011-01-12 at 10:46 +0000, Mark Brown wrote:
> On Wed, Jan 12, 2011 at 09:58:10AM +0000, Dimitris Papastamos wrote:
> > On Tue, 2011-01-11 at 18:14 +0000, Mark Brown wrote:
> 
> > > That's a bug in the flat cache, it should be stashing a copy of it in
> > > the codec instance.  Though it only really makes a difference for
> > > devices where you'd want to use a compressed cache anyway.
> 
> > The flat cache is intentionally *not* making a copy of the defaults
> > cache as it makes no difference in terms of memory usage.  The flat
> > cache has codec->reg_def_copy = NULL; and can work with either NULL
> > default caches or filled in.
> 
> It can make a difference to memory usage - if multiple CODEC drivers are
> built in but only one is in use then if we take a copy all the defaults
> can be discared, meaning that we don't need to keep the defaults for the
> unused drivers and only keep the defaults for the one that's in use
> instead of keeping the defaults for all drivers.

Aha, indeed.  The patch I've prepared should be sufficient for this.

Thanks,
Dimitris

Re: Fix section mismatch in wm8995.c

[Dimitris Papastamos]

On Tue, 2011-01-11 at 18:48 +0100, Takashi Iwai wrote:
> At Tue, 11 Jan 2011 17:33:49 +0000,
> Mark Brown wrote:
> > 
> > On Tue, Jan 11, 2011 at 06:32:32PM +0100, Takashi Iwai wrote:
> > > Mark Brown wrote:
> > 
> > > > The array is ~25k so it's not a completely trivial amount of memory,
> > > > unfortunately.
> > 
> > > Hm.  But, it's same for (some) other codecs, no?
> > > If we do annotate something, we should mark all these at once.
> > 
> > In principle, though since it requires per driver checking for
> > references and so on it's not as simple as just going through and
> > annotating.  It's partly tied in with the cache stuff, things that are
> > fully using that can be converted over easily.  And of course many of
> > the devices have much smaller register maps so it's much less of an
> > issue.
> 
> IMO, such a data should be uniquely handled -- either init-only or
> not.  Through a quick look, snd_soc_cache_sync() may still refer to
> reg_cache_default.  So, it's still risky to blindly set
> __devinitconst.  (Yeah, I know it's not used right now, though ;)
> 
> That is, the data that shall be deleted shouldn't be kept in a
> structure that might be referred later from other places.

For the LZO and the rbtree compression, a copy of the defaults cache is
kept and that is used in the syncing functionality.  Only the flat cache
is using reg_cache_default directly which is never marked as
__devinitconst.

Thanks,
Dimitris

Re: Fix section mismatch in wm8995.c

[Takashi Iwai]

At Wed, 12 Jan 2011 10:00:59 +0000,
Dimitris Papastamos wrote:
> 
> On Tue, 2011-01-11 at 18:48 +0100, Takashi Iwai wrote:
> > At Tue, 11 Jan 2011 17:33:49 +0000,
> > Mark Brown wrote:
> > > 
> > > On Tue, Jan 11, 2011 at 06:32:32PM +0100, Takashi Iwai wrote:
> > > > Mark Brown wrote:
> > > 
> > > > > The array is ~25k so it's not a completely trivial amount of memory,
> > > > > unfortunately.
> > > 
> > > > Hm.  But, it's same for (some) other codecs, no?
> > > > If we do annotate something, we should mark all these at once.
> > > 
> > > In principle, though since it requires per driver checking for
> > > references and so on it's not as simple as just going through and
> > > annotating.  It's partly tied in with the cache stuff, things that are
> > > fully using that can be converted over easily.  And of course many of
> > > the devices have much smaller register maps so it's much less of an
> > > issue.
> > 
> > IMO, such a data should be uniquely handled -- either init-only or
> > not.  Through a quick look, snd_soc_cache_sync() may still refer to
> > reg_cache_default.  So, it's still risky to blindly set
> > __devinitconst.  (Yeah, I know it's not used right now, though ;)
> > 
> > That is, the data that shall be deleted shouldn't be kept in a
> > structure that might be referred later from other places.
> 
> For the LZO and the rbtree compression, a copy of the defaults cache is
> kept and that is used in the syncing functionality.  Only the flat cache
> is using reg_cache_default directly which is never marked as
> __devinitconst.

But in theory, the cache method can fall back to flat if no
corresponding compression is available.  Thus no strong-type check is
possible in such a case.


Takashi

Re: Fix section mismatch in wm8995.c

[Dimitris Papastamos]

On Wed, 2011-01-12 at 11:03 +0100, Takashi Iwai wrote:
> At Wed, 12 Jan 2011 10:00:59 +0000,
> Dimitris Papastamos wrote:
> > 
> > On Tue, 2011-01-11 at 18:48 +0100, Takashi Iwai wrote:
> > > At Tue, 11 Jan 2011 17:33:49 +0000,
> > > Mark Brown wrote:
> > > > 
> > > > On Tue, Jan 11, 2011 at 06:32:32PM +0100, Takashi Iwai wrote:
> > > > > Mark Brown wrote:
> > > > 
> > > > > > The array is ~25k so it's not a completely trivial amount of memory,
> > > > > > unfortunately.
> > > > 
> > > > > Hm.  But, it's same for (some) other codecs, no?
> > > > > If we do annotate something, we should mark all these at once.
> > > > 
> > > > In principle, though since it requires per driver checking for
> > > > references and so on it's not as simple as just going through and
> > > > annotating.  It's partly tied in with the cache stuff, things that are
> > > > fully using that can be converted over easily.  And of course many of
> > > > the devices have much smaller register maps so it's much less of an
> > > > issue.
> > > 
> > > IMO, such a data should be uniquely handled -- either init-only or
> > > not.  Through a quick look, snd_soc_cache_sync() may still refer to
> > > reg_cache_default.  So, it's still risky to blindly set
> > > __devinitconst.  (Yeah, I know it's not used right now, though ;)
> > > 
> > > That is, the data that shall be deleted shouldn't be kept in a
> > > structure that might be referred later from other places.
> > 
> > For the LZO and the rbtree compression, a copy of the defaults cache is
> > kept and that is used in the syncing functionality.  Only the flat cache
> > is using reg_cache_default directly which is never marked as
> > __devinitconst.
> 
> But in theory, the cache method can fall back to flat if no
> corresponding compression is available.  Thus no strong-type check is
> possible in such a case.

Aw, yes that was added in a patch recently.  I will prepare a patch that
handles that.

Thanks,
Dimitris