Date: Wed, 20 May 2015 01:05:05 +0200
From: Giuliano Pochini <pochini@shiny.it>
To: Takashi Iwai <tiwai@suse.de>
Cc: Denys Vlasenko <dvlasenk@redhat.com>, LKML <linux-kernel@vger.kernel.org>
Subject: Re: Object code duplication in sound/pci/echoaudio/
Message-ID: <20150520010505.02fcf5e7@wc1>
In-Reply-To: <s5h617q2rjy.wl-tiwai@suse.de>
References: <55510DE0.3080909@redhat.com>
	<20150514223206.04d6b79e@wc1>
	<5555F599.60908@redhat.com>
	<s5h617q2rjy.wl-tiwai@suse.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3302
Lines: 83

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/