[PATCH] add consts where appropriate in sound/pci/hda/*

[PATCH] add consts where appropriate in sound/pci/hda/*

[Denys Vlasenko]

[-- Attachment #1: Type: text/plain, Size: 793 bytes --]

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 <vda.linux@googlemail.com>
--
vda

[-- Attachment #2: constify_hda_codec.diff.bz2 --]
[-- Type: application/x-bzip2, Size: 18868 bytes --]

Re: [PATCH] add consts where appropriate in sound/pci/hda/*

[Joe Perches]

On Fri, 2007-09-14 at 18:48 +0100, Denys Vlasenko wrote:

> Patch is attached.

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

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
--- linux-2.6.23-rc6/sound/pci/hda/hda_codec.c	2007-07-09 00:32:17.000000000 +0100
+++ linux-2.6.23-rc6.bigdata/sound/pci/hda/hda_codec.c	2007-09-14 18:32:24.000000000 +0100
@@ -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
+};
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
--- linux-2.6.23-rc6/sound/pci/hda/hda_patch.h	2007-07-09 00:32:17.000000000 +0100
+++ linux-2.6.23-rc6.bigdata/sound/pci/hda/hda_patch.h	2007-09-14 18:30:17.000000000 +0100
@@ -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

Re: [PATCH] add consts where appropriate in sound/pci/hda/*

[Denys Vlasenko]

On Friday 14 September 2007 19:09, Joe Perches wrote:
> On Fri, 2007-09-14 at 18:48 +0100, Denys Vlasenko wrote:
> 
> > 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

Re: [PATCH] add consts where appropriate in sound/pci/hda/*

[Denys Vlasenko]

[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]

On Friday 14 September 2007 18:48, Denys Vlasenko wrote:
> 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.

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 <vda.linux@googlemail.com>
--
vda

[-- Attachment #2: constify_hda_codec_v2.diff.bz2 --]
[-- Type: application/x-bzip2, Size: 18943 bytes --]

Re: [PATCH] add consts where appropriate in sound/pci/hda/*

[Jan Engelhardt]

On Sep 14 2007 18:48, Denys Vlasenko wrote:
>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?

Re: [PATCH] add consts where appropriate in sound/pci/hda/*

[Sam Ravnborg]

On Sat, Sep 15, 2007 at 11:43:40AM +0200, Jan Engelhardt wrote:
> 
> On Sep 14 2007 18:48, Denys Vlasenko wrote:
> >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?

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

Re: [PATCH] add consts where appropriate in sound/pci/hda/*

[Jan Engelhardt]

On Sep 15 2007 12:18, Sam Ravnborg wrote:
>> >   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?

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

Re: [PATCH] add consts where appropriate in sound/pci/hda/*

[Denys Vlasenko]

On Saturday 15 September 2007 11:29, Jan Engelhardt wrote:
 
> On Sep 15 2007 12:18, Sam Ravnborg wrote:
> >> >   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

Re: [PATCH] add consts where appropriate in sound/pci/hda/*

[Andreas Schwab]

Denys Vlasenko <vda.linux@googlemail.com> writes:

> 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.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
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."

Re: [PATCH] add consts where appropriate in sound/pci/hda/*

[Denys Vlasenko]

On Saturday 15 September 2007 13:40, Andreas Schwab wrote:
> Denys Vlasenko <vda.linux@googlemail.com> writes:
> 
> > 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

Re: [PATCH] add consts where appropriate in sound/pci/hda/*

[Jan Engelhardt]

On Sep 15 2007 14:47, Denys Vlasenko wrote:
>> 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.

Re: [PATCH] add consts where appropriate in sound/pci/hda/*

[Takashi Iwai]

At Fri, 14 Sep 2007 18:48:05 +0100,
Denys Vlasenko wrote:
> 
> 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 <vda.linux@googlemail.com>

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

Re: [PATCH] add consts where appropriate in sound/pci/hda/*

[Denys Vlasenko]

On Monday 17 September 2007 11:01, Takashi Iwai wrote:
> > 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 <vda.linux@googlemail.com>
> 
> 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.

> 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