Skip to contentSkip to conversation

add consts where appropriate in sound/pci/hda/*

PATCH In review Denys Vlasenko 6 participants 13 messages Sep 14, 2007 – Sep 17, 2007 Sound

lore
Share & tools
lore

Denys Vlasenko

author
#0 lore raw

PATCH 1/1 add consts where appropriate in sound/pci/hda/*

Hi Takashi,

There is a lot of data structures in that code,
and most of them seems to be read-only.

I added const modifiers to most of such places:

text data bss dec hex filename

 106315  179564      36  285915   45cdb snd-hda-intel.o
 283051    2624      36  285711   45c0f snd-hda-intel_patched.o

Patch is attached.

It moves "static struct hda_codec_preset *hda_preset_tables[]" from hda_patch.h to hda_codec.c, and then adds #include "hda_patch.h"
in a few .c files so that definitions of e.g.
const struct hda_codec_preset snd_hda_preset_analog[] are checked to match declarations in hda_patch.h

The rest of the patch (bulk of it) adds "const"
in many places.

Patch is compile tested. Please apply.

Signed-off-by Denys Vlasenko <[email protected]>

--
vda

#1 lore raw
Quoted 1 line · Denys Vlasenko

Patch is attached.

The SND_HDA_PRESETS define doesn't seem useful.
It's only used once.

sound/pci/hda/hda_codec.c +4 -0
diff -urp linux-2.6.23-rc6/sound/pci/hda/hda_codec.c linux-2.6.23-rc6.bigdata/sound/pci/hda/hda_codec.c
@@ -57,6 +57,10 @@ static struct hda_vendor_id hda_vendor_i
 
 /* codec presets */
 #include "hda_patch.h"
+static const struct hda_codec_preset *const hda_preset_tables[] = {
+	SND_HDA_PRESETS,
+	NULL
+};
sound/pci/hda/hda_patch.h +18 -20
diff -urp linux-2.6.23-rc6/sound/pci/hda/hda_patch.h linux-2.6.23-rc6.bigdata/sound/pci/hda/hda_patch.h
@@ -1,32 +1,30 @@
 /*
- * HDA Patches - included by hda_codec.c
+ * HDA Patches
  */
 
 /* Realtek codecs */
-extern struct hda_codec_preset snd_hda_preset_realtek[];
+extern const struct hda_codec_preset snd_hda_preset_realtek[];
 /* C-Media codecs */
-extern struct hda_codec_preset snd_hda_preset_cmedia[];
+extern const struct hda_codec_preset snd_hda_preset_cmedia[];
 /* Analog Devices codecs */
-extern struct hda_codec_preset snd_hda_preset_analog[];
+extern const struct hda_codec_preset snd_hda_preset_analog[];
 /* SigmaTel codecs */
-extern struct hda_codec_preset snd_hda_preset_sigmatel[];
+extern const struct hda_codec_preset snd_hda_preset_sigmatel[];
 /* SiLabs 3054/3055 modem codecs */
-extern struct hda_codec_preset snd_hda_preset_si3054[];
+extern const struct hda_codec_preset snd_hda_preset_si3054[];
 /* ATI HDMI codecs */
-extern struct hda_codec_preset snd_hda_preset_atihdmi[];
+extern const struct hda_codec_preset snd_hda_preset_atihdmi[];
 /* Conexant audio codec */
-extern struct hda_codec_preset snd_hda_preset_conexant[];
+extern const struct hda_codec_preset snd_hda_preset_conexant[];
 /* VIA codecs */
-extern struct hda_codec_preset snd_hda_preset_via[];
+extern const struct hda_codec_preset snd_hda_preset_via[];
 
-static const struct hda_codec_preset *hda_preset_tables[] = {
-	snd_hda_preset_realtek,
-	snd_hda_preset_cmedia,
-	snd_hda_preset_analog,
-	snd_hda_preset_sigmatel,
-	snd_hda_preset_si3054,
-	snd_hda_preset_atihdmi,
-	snd_hda_preset_conexant,
-	snd_hda_preset_via,
-	NULL
-};
+#define SND_HDA_PRESETS \
+	snd_hda_preset_realtek, \
+	snd_hda_preset_cmedia, \
+	snd_hda_preset_analog, \
+	snd_hda_preset_sigmatel, \
+	snd_hda_preset_si3054, \
+	snd_hda_preset_atihdmi, \
+	snd_hda_preset_conexant, \
+	snd_hda_preset_via
author
#2 lore raw
Quoted 6 lines · Joe Perches
Quoted 1 line · Denys Vlasenko

Patch is attached.

The SND_HDA_PRESETS define doesn't seem useful.
It's only used once.

It is defined in .h file and used in .c file.

It is made so because defining static data variables in .h file is a bad style in general and in this case will result in build-time warnings in particular.

Therefore definition of hda_preset_tables[] is moved to .c file.

It looks like this:

static const struct hda_codec_preset *const hda_preset_tables[] = {
       snd_hda_preset_realtek,
       snd_hda_preset_cmedia,
       snd_hda_preset_analog,
       snd_hda_preset_sigmatel,
       snd_hda_preset_si3054,
       snd_hda_preset_atihdmi,
       snd_hda_preset_conexant,
       snd_hda_preset_via,
       NULL
};

I want to make it easier for people to add new snd_hda_preset_XXX.

I don't want them to be forced to add it in hda_patch.h first, and then go to hda_codec.c and add it there too.

Therefore I turned this list into a #define which sits in hda_patch.h:

#define SND_HDA_PRESETS \
       snd_hda_preset_realtek, \
       snd_hda_preset_cmedia, \
       snd_hda_preset_analog, \
       snd_hda_preset_sigmatel, \
       snd_hda_preset_si3054, \
       snd_hda_preset_atihdmi, \
       snd_hda_preset_conexant, \
       snd_hda_preset_via

Now if you want to add yet another snd_hda_preset_XXX, you don't need to touch hda_codec.c.

Original code was achieving the same by cheating: it has

static const struct hda_codec_preset *hda_preset_tables[] = {...}

in hda_patch.h.
--
vda

author
#3 lore raw
In reply to Denys Vlasenko
Quoted 18 lines · Denys Vlasenko

There is a lot of data structures in that code,
and most of them seems to be read-only.

I added const modifiers to most of such places:

text data bss dec hex filename

 106315  179564      36  285915   45cdb snd-hda-intel.o
 283051    2624      36  285711   45c0f snd-hda-intel_patched.o

Patch is attached.

It moves "static struct hda_codec_preset *hda_preset_tables[]" from hda_patch.h to hda_codec.c, and then adds #include "hda_patch.h"
in a few .c files so that definitions of e.g.
const struct hda_codec_preset snd_hda_preset_analog[] are checked to match declarations in hda_patch.h

Quoted 4 lines

The rest of the patch (bulk of it) adds "const"
in many places.

Patch is compile tested. Please apply.

After additional testing I found a place where I forgot to add 'const', and build throws warnings at me.

Updated patch is attached.

Signed-off-by Denys Vlasenko <[email protected]>

--
vda

#4 lore raw
In reply to Denys Vlasenko
Quoted 10 lines · Denys Vlasenko

Hi Takashi,

There is a lot of data structures in that code,
and most of them seems to be read-only.

I added const modifiers to most of such places:

text data bss dec hex filename

106315  179564      36  285915   45cdb snd-hda-intel.o
283051    2624      36  285711   45c0f snd-hda-intel_patched.o

This is kinda odd. Why did the _text_ size increase by constifying?

#5 lore raw
Quoted 7 lines · Jan Engelhardt
Quoted 5 lines · Denys Vlasenko

Hi Takashi,

There is a lot of data structures in that code,
and most of them seems to be read-only.

Quoted 7 lines
Quoted 5 lines

I added const modifiers to most of such places:

text data bss dec hex filename

106315  179564      36  285915   45cdb snd-hda-intel.o
283051    2624      36  285711   45c0f snd-hda-intel_patched.o

This is kinda odd. Why did the _text_ size increase by constifying?

The data got converted from data to text because they were made const. Making stuff const unfortunately does not make them disappear albeit the embedded folks would have liked that.

Sam

#6 lore raw
Quoted 7 lines · Sam Ravnborg
Quoted 5 lines
Quoted 3 lines

text data bss dec hex filename

106315  179564      36  285915   45cdb snd-hda-intel.o
283051    2624      36  285711   45c0f snd-hda-intel_patched.o

This is kinda odd. Why did the _text_ size increase by constifying?

The data got converted from data to text because they were made const.

Which is odd. How can data become code? Or does 'text' actually include .rodata?

Quoted 2 lines

Making stuff const unfortunately does not make them disappear albeit the embedded folks would have liked that.

author
#7 lore raw
Quoted 11 lines · Jan Engelhardt
Quoted 7 lines · Sam Ravnborg
Quoted 5 lines
Quoted 3 lines

text data bss dec hex filename

106315  179564      36  285915   45cdb snd-hda-intel.o
283051    2624      36  285711   45c0f snd-hda-intel_patched.o

This is kinda odd. Why did the _text_ size increase by constifying?

The data got converted from data to text because they were made const.

Which is odd. How can data become code? Or does 'text' actually include .rodata?

More precisely: size utility adds up all readonly code and readonly data sections it sees and shows it as "text".

ELF is not as rigid as old a.out (which had only one text, one data and one bss segment per .o file IIRC), but size was born in a.out days, so it sort of "translates" ELF into a.out.
--
vda

#8 lore raw
Quoted 3 lines · Denys Vlasenko

ELF is not as rigid as old a.out (which had only one text, one data and one bss segment per .o file IIRC), but size was born in a.out days, so it sort of "translates" ELF into a.out.

Try size -A instead.

Andreas.

Signature · 4 lines
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
author
#9 lore raw
Quoted 7 lines · Andreas Schwab
Quoted 3 lines · Denys Vlasenko

ELF is not as rigid as old a.out (which had only one text, one data and one bss segment per .o file IIRC), but size was born in a.out days, so it sort of "translates" ELF into a.out.

Try size -A instead.

thanks, didn't know that.

$ size -A busybox
busybox  :
section       size        addr
.init           28   134512788
.text       633051   134512816
.fini           23   135145867
.rodata     139422   135145904
.eh_frame      156   135285328
.ctors           8   135286784
.dtors           8   135286792
.jcr             4   135286800
.got.plt        12   135286804
.data         1019   135286816
.bss         10724   135287840
Total       784455

addr in decimal?! wow...
--
vda

#10 lore raw
Quoted 9 lines · Denys Vlasenko
Quoted 1 line

Try size -A instead.

thanks, didn't know that.

$ size -A busybox
busybox  :
section       size        addr
.init           28   134512788
addr in decimal?! wow...

Try size -x instead.

#11 lore raw
In reply to Denys Vlasenko

At Fri, 14 Sep 2007 18:48:05 +0100,

Quoted 21 lines · Denys Vlasenko

Hi Takashi,

There is a lot of data structures in that code,
and most of them seems to be read-only.

I added const modifiers to most of such places:

text data bss dec hex filename

 106315  179564      36  285915   45cdb snd-hda-intel.o
 283051    2624      36  285711   45c0f snd-hda-intel_patched.o

Patch is attached.

It moves "static struct hda_codec_preset *hda_preset_tables[]" from hda_patch.h to hda_codec.c, and then adds #include "hda_patch.h"
in a few .c files so that definitions of e.g.
const struct hda_codec_preset snd_hda_preset_analog[] are checked to match declarations in hda_patch.h

Quoted 6 lines

The rest of the patch (bulk of it) adds "const"
in many places.

Patch is compile tested. Please apply.

Signed-off-by Denys Vlasenko <[email protected]>

Sorry for the late reply.

First, thanks for your patch. Although I have also a similar patch pending on my tree, but it wasn't applied, because I'd like to mark these functions/data rather as __devinit*. And, sadly, init and const don't like with each other. So, my plan is to apply __devinit but without const.

But, please don't hurry up to mark everything init yet now. I've been working on the dynamic codec parser on user-space and this would involve with some functions that are now apparently init functions.

Since this isn't a critical issue, let's postpone it as a post-2.6.24 stuff.

thanks,

Takashi

author
#12 lore raw
Quoted 23 lines · Takashi Iwai
Quoted 23 lines

There is a lot of data structures in that code,
and most of them seems to be read-only.

I added const modifiers to most of such places:

text data bss dec hex filename

 106315  179564      36  285915   45cdb snd-hda-intel.o
 283051    2624      36  285711   45c0f snd-hda-intel_patched.o

Patch is attached.

It moves "static struct hda_codec_preset *hda_preset_tables[]" from hda_patch.h to hda_codec.c, and then adds #include "hda_patch.h"
in a few .c files so that definitions of e.g.
const struct hda_codec_preset snd_hda_preset_analog[] are checked to match declarations in hda_patch.h

The rest of the patch (bulk of it) adds "const"
in many places.

Patch is compile tested. Please apply.

Quoted 8 lines
Quoted 1 line
Signed-off-by Denys Vlasenko <[email protected]>

Sorry for the late reply.

First, thanks for your patch. Although I have also a similar patch pending on my tree, but it wasn't applied, because I'd like to mark these functions/data rather as __devinit*. And, sadly, init and const don't like with each other.

Unless we will go to the pains of implementing __devrodata, which doesn't sound encouraging.

Quoted 2 lines

So, my plan is to apply __devinit but
without const.

Yes, I see. const as it stands is not very useful in kernel anyway (only a small code reduction sometimes).
ro or rw, the data is still taking space.

Well, maybe someday ld will be sooo clever that it will actually merge rodata which is identical, but so far it is not implemented. --
vda