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

[Takashi Iwai <tiwai@suse.de>]

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