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

[Giuliano Pochini <pochini@shiny.it>]

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.