[alsa-devel] Fix section mismatch in wm8995.c
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,
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.
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
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.
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
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.
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
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.
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
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
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.
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
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
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
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
participants (3)
-
Dimitris Papastamos
-
Mark Brown
-
Takashi Iwai