Object code duplication in sound/pci/echoaudio/

Object code duplication in sound/pci/echoaudio/

[Denys Vlasenko]

There are fourteen files in sound/pci/echoaudio/, namely:

darla20.c darla24.c echo3g.c gina20.c gina24.c indigo.c
indigodj.c indigodjx.c indigoio.c indigoiox.c layla20.c
layla24.c mia.c mona.c

Which use the following method of "code reuse":

#include "echoaudio_dsp.c"
#include "echoaudio_gml.c"
#include "echoaudio.c"

echoaudio.c is not a header file, it contains a bunch of
static functions, some of a considerable size.
This makes those functions to be duplicated many times over.

For instance, there are fourteen instances of init_engine(),
each 1117 bytes long. Fourteen instances of pcm_open(), each 556 bytes
long.

11 get_firmware
10 free_firmware
13 audiopipe_free
14 init_hw
14 hw_rule_capture_format_by_channels
14 hw_rule_capture_channels_by_format
14 hw_rule_playback_format_by_channels
14 hw_rule_playback_channels_by_format

and so on.

In my humble opinion, this is not a good coding practice.
You should not duplicate functions like this.
Where possible, you need to reuse a single instance of a function.

According to git, author of these drivers is Giuliano Pochini
<pochini@shiny.it>.

Re: Object code duplication in sound/pci/echoaudio/

[Giuliano Pochini]

On Mon, 11 May 2015 22:15:28 +0200
Denys Vlasenko <dvlasenk@redhat.com> wrote:

> There are fourteen files in sound/pci/echoaudio/, namely:
> 
> darla20.c darla24.c echo3g.c gina20.c gina24.c indigo.c
> indigodj.c indigodjx.c indigoio.c indigoiox.c layla20.c
> layla24.c mia.c mona.c
> 
> Which use the following method of "code reuse":
> 
> #include "echoaudio_dsp.c"
> #include "echoaudio_gml.c"
> #include "echoaudio.c"
> 
> echoaudio.c is not a header file, it contains a bunch of
> static functions, some of a considerable size.
> This makes those functions to be duplicated many times over.

Not really. It is quite unlikely that there are two different Echoaudio cards installed.


> For instance, there are fourteen instances of init_engine(),
> each 1117 bytes long. Fourteen instances of pcm_open(), each 556 bytes
> long.
> 
> 11 get_firmware
> 10 free_firmware
> 13 audiopipe_free
> 14 init_hw
> 14 hw_rule_capture_format_by_channels
> 14 hw_rule_capture_channels_by_format
> 14 hw_rule_playback_format_by_channels
> 14 hw_rule_playback_channels_by_format
> 
> and so on.
> 
> In my humble opinion, this is not a good coding practice.
> You should not duplicate functions like this.
> Where possible, you need to reuse a single instance of a function.

One option is to make a single driver which supports all the cards. There is not any duplicated code, but there is a lot of unused code. The other way (the one I choosed) is to build many specialized drivers. There is duplicated code overall, but given a single installed card there is neither duplicated not unused code.


-- 
Giuliano.

Re: Object code duplication in sound/pci/echoaudio/

[Denys Vlasenko]

On 05/14/2015 10:32 PM, Giuliano Pochini wrote:
> On Mon, 11 May 2015 22:15:28 +0200
> Denys Vlasenko <dvlasenk@redhat.com> wrote:
> 
>> There are fourteen files in sound/pci/echoaudio/, namely:
>>
>> darla20.c darla24.c echo3g.c gina20.c gina24.c indigo.c
>> indigodj.c indigodjx.c indigoio.c indigoiox.c layla20.c
>> layla24.c mia.c mona.c
>>
>> Which use the following method of "code reuse":
>>
>> #include "echoaudio_dsp.c"
>> #include "echoaudio_gml.c"
>> #include "echoaudio.c"
>>
>> echoaudio.c is not a header file, it contains a bunch of
>> static functions, some of a considerable size.
>> This makes those functions to be duplicated many times over.
> 
> Not really. It is quite unlikely that there are two different Echoaudio cards installed.

Distros tend to build almost all drivers in the tree.
On Fedora, kernel has a config where the following drivers
are built from sources in sound/pci/echoaudio/:

$ ls /lib/modules/4.0.0/kernel/sound/pci/echoaudio/
snd-darla20.ko	snd-gina20.ko	 snd-indigodjx.ko  snd-indigo.ko   snd-mia.ko
snd-darla24.ko	snd-gina24.ko	 snd-indigoio.ko   snd-layla20.ko  snd-mona.ko
snd-echo3g.ko	snd-indigodj.ko  snd-indigoiox.ko  snd-layla24.ko

>> For instance, there are fourteen instances of init_engine(),
>> each 1117 bytes long. Fourteen instances of pcm_open(), each 556 bytes
>> long.
>>
>> 11 get_firmware
>> 10 free_firmware
>> 13 audiopipe_free
>> 14 init_hw
>> 14 hw_rule_capture_format_by_channels
>> 14 hw_rule_capture_channels_by_format
>> 14 hw_rule_playback_format_by_channels
>> 14 hw_rule_playback_channels_by_format
>>
>> and so on.
>>
>> In my humble opinion, this is not a good coding practice.
>> You should not duplicate functions like this.
>> Where possible, you need to reuse a single instance of a function.
> 
> One option is to make a single driver which supports all the cards.
> There is not any duplicated code, but there is a lot of unused code.
> The other way (the one I choosed) is to build many specialized drivers.

If you have a lot of common code, you just need to have it factored out
into a separate module, say, echoaudio-core.c,
and make individual drivers depend on it.

This is done all over the kernel (there are nearly 400 *core*.c files
in the tree).

Re: Object code duplication in sound/pci/echoaudio/

[Takashi Iwai]

At Fri, 15 May 2015 15:33:13 +0200,
Denys Vlasenko wrote:
> 
> On 05/14/2015 10:32 PM, Giuliano Pochini wrote:
> > On Mon, 11 May 2015 22:15:28 +0200
> > Denys Vlasenko <dvlasenk@redhat.com> wrote:
> > 
> >> There are fourteen files in sound/pci/echoaudio/, namely:
> >>
> >> darla20.c darla24.c echo3g.c gina20.c gina24.c indigo.c
> >> indigodj.c indigodjx.c indigoio.c indigoiox.c layla20.c
> >> layla24.c mia.c mona.c
> >>
> >> Which use the following method of "code reuse":
> >>
> >> #include "echoaudio_dsp.c"
> >> #include "echoaudio_gml.c"
> >> #include "echoaudio.c"
> >>
> >> echoaudio.c is not a header file, it contains a bunch of
> >> static functions, some of a considerable size.
> >> This makes those functions to be duplicated many times over.
> > 
> > Not really. It is quite unlikely that there are two different Echoaudio cards installed.
> 
> Distros tend to build almost all drivers in the tree.
> On Fedora, kernel has a config where the following drivers
> are built from sources in sound/pci/echoaudio/:
> 
> $ ls /lib/modules/4.0.0/kernel/sound/pci/echoaudio/
> snd-darla20.ko	snd-gina20.ko	 snd-indigodjx.ko  snd-indigo.ko   snd-mia.ko
> snd-darla24.ko	snd-gina24.ko	 snd-indigoio.ko   snd-layla20.ko  snd-mona.ko
> snd-echo3g.ko	snd-indigodj.ko  snd-indigoiox.ko  snd-layla24.ko
> 
> >> For instance, there are fourteen instances of init_engine(),
> >> each 1117 bytes long. Fourteen instances of pcm_open(), each 556 bytes
> >> long.
> >>
> >> 11 get_firmware
> >> 10 free_firmware
> >> 13 audiopipe_free
> >> 14 init_hw
> >> 14 hw_rule_capture_format_by_channels
> >> 14 hw_rule_capture_channels_by_format
> >> 14 hw_rule_playback_format_by_channels
> >> 14 hw_rule_playback_channels_by_format
> >>
> >> and so on.
> >>
> >> In my humble opinion, this is not a good coding practice.
> >> You should not duplicate functions like this.
> >> Where possible, you need to reuse a single instance of a function.
> > 
> > One option is to make a single driver which supports all the cards.
> > There is not any duplicated code, but there is a lot of unused code.
> > The other way (the one I choosed) is to build many specialized drivers.
> 
> If you have a lot of common code, you just need to have it factored out
> into a separate module, say, echoaudio-core.c,
> and make individual drivers depend on it.
> 
> This is done all over the kernel (there are nearly 400 *core*.c files
> in the tree).

Yes, we know of such problems well.  But practically seen, the only
problem is the disk space, so it's been in a low priority of TODO
list.  There are a few other drivers having the very same issue, and
the biggest problem is the lack of test hardware, as they tend to be
legacy boards (either ISA or old PCI).


thanks,

Takashi

Re: Object code duplication in sound/pci/echoaudio/

[Giuliano Pochini]

On Mon, 18 May 2015 10:55:29 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> At Fri, 15 May 2015 15:33:13 +0200,
> Denys Vlasenko wrote:
> > 
> > On 05/14/2015 10:32 PM, Giuliano Pochini wrote:
> > > On Mon, 11 May 2015 22:15:28 +0200
> > > Denys Vlasenko <dvlasenk@redhat.com> wrote:
> > > 
> > >> There are fourteen files in sound/pci/echoaudio/, namely:
> > >>
> > >> darla20.c darla24.c echo3g.c gina20.c gina24.c indigo.c
> > >> indigodj.c indigodjx.c indigoio.c indigoiox.c layla20.c
> > >> layla24.c mia.c mona.c
> > >>
> > >> Which use the following method of "code reuse":
> > >>
> > >> #include "echoaudio_dsp.c"
> > >> #include "echoaudio_gml.c"
> > >> #include "echoaudio.c"
> > >>
> > >> echoaudio.c is not a header file, it contains a bunch of
> > >> static functions, some of a considerable size.
> > >> This makes those functions to be duplicated many times over.
> > > 
> > > Not really. It is quite unlikely that there are two different Echoaudio cards installed.
> > 
> > Distros tend to build almost all drivers in the tree.
> > On Fedora, kernel has a config where the following drivers
> > are built from sources in sound/pci/echoaudio/:
> > 
> > $ ls /lib/modules/4.0.0/kernel/sound/pci/echoaudio/
> > snd-darla20.ko	snd-gina20.ko	 snd-indigodjx.ko  snd-indigo.ko   snd-mia.ko
> > snd-darla24.ko	snd-gina24.ko	 snd-indigoio.ko   snd-layla20.ko  snd-mona.ko
> > snd-echo3g.ko	snd-indigodj.ko  snd-indigoiox.ko  snd-layla24.ko
> > 
> > >> For instance, there are fourteen instances of init_engine(),
> > >> each 1117 bytes long. Fourteen instances of pcm_open(), each 556 bytes
> > >> long.
> > >>
> > >> 11 get_firmware
> > >> 10 free_firmware
> > >> 13 audiopipe_free
> > >> 14 init_hw
> > >> 14 hw_rule_capture_format_by_channels
> > >> 14 hw_rule_capture_channels_by_format
> > >> 14 hw_rule_playback_format_by_channels
> > >> 14 hw_rule_playback_channels_by_format
> > >>
> > >> and so on.
> > >>
> > >> In my humble opinion, this is not a good coding practice.
> > >> You should not duplicate functions like this.
> > >> Where possible, you need to reuse a single instance of a function.
> > > 
> > > One option is to make a single driver which supports all the cards.
> > > There is not any duplicated code, but there is a lot of unused code.
> > > The other way (the one I choosed) is to build many specialized drivers.
> > 
> > If you have a lot of common code, you just need to have it factored out
> > into a separate module, say, echoaudio-core.c,
> > and make individual drivers depend on it.
> > 
> > This is done all over the kernel (there are nearly 400 *core*.c files
> > in the tree).
> 
> Yes, we know of such problems well.  But practically seen, the only
> problem is the disk space, so it's been in a low priority of TODO
> list.  There are a few other drivers having the very same issue, and
> the biggest problem is the lack of test hardware, as they tend to be
> legacy boards (either ISA or old PCI).

Ok, I'll try to find the time to split the drivers. Do not expect it will be ready tomorrow :)


-- 
Giuliano.