[coreboot] [PATCH] superiotool: libsuperiodetect
Stefan Reinauer
stefan.reinauer at coresystems.de
Wed Jun 30 14:51:43 CEST 2010
On 6/30/10 2:35 PM, Carl-Daniel Hailfinger wrote:
> Using the superiotool code to detect SuperI/O chips can be really useful
> even for applications besides superiotool, e.g. flashrom.
> The biggest hurdle right now is the excessive size of the superiotool
> binary (>2.2 MB) which is still ~15 kB after lzma compression. That's
> simply unacceptable for a library.
>
Why? There is no size constraints on libraries in Unix like systems.
Assuming we're talking about a static library
>From my system:
20M /usr/lib64/libbfd.a
20M /usr/lib64/libc.a
11M /usr/lib64/libcrypto.a
19M /usr/lib64/libdb-4.5.a
3.9M /usr/lib64/libfreetype.a
5.1M /usr/lib64/libgio-2.0.a
3.6M /usr/lib64/libglib-2.0.a
3.3M /usr/lib64/libgmp.a
2.8M /usr/lib64/libm.a
2.8M /usr/lib64/libncurses.a
3.2M /usr/lib64/libncursesw.a
4.9M /usr/lib64/libopcodes.a
4.2M /usr/lib64/libruby-static.a
3.0M /usr/lib64/libssl.a
6.9M /usr/lib64/libxml2.a
Also, keep in mind that the size of the library in the filesystem is not
the size increase when linking it.
15kb lzma compressed is nothing.
> This minimally invasive patch makes all dumping code optional and
> reduces binary size for the probe-only case (libsuperiodetect) a lot.
> Binary sizes:
> 2268258 (before)
> 31600 (after) 98.6% reduction
> LZMA compressed and stripped binary sizes:
> 14778 (before)
> 6709 (after) 54.6% reduction
>
Not worth the additional unreadability of the code in my opinion
> To test compilation of superiotool with probe-only functionality, run
> make CONFIG_LIB=yes
>
> This patch is the first step towards libsuperiodetect, and more will
> have to follow.
>
I think the idea for libsuperio is great.
I'll gladly ack this without the very intrusive "we save 6700 bytes"
part of the patch.
> IMHO the {EOT} markers should be killed in libsuperiodetect, at least
> for the LDNs. Such an operation would have increased the size of the
> patch and would have reduced readability as well. Due to that, I skipped
> it, but I plan to do that in a later operation.
>
What would you use instead? The array members all have different sizes
as far as I can tell, so some kind of end marker will be needed.
> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
>
> /* Init: 0x55, 0x55. Exit: 0xaa. Port: 0x3f0. */
> @@ -765,7 +803,9 @@
> uint8_t revreg)
> {
> uint8_t id, rev;
> +#ifndef LIBSUPERIODETECT
> uint16_t runtime_base;
> +#endif /* ! LIBSUPERIODETECT */
>
Maybe this could be moved downwards to where it is used, to save an ifndef?
> Index: superiotool_libsuperiodetect/superiotool.h
> ===================================================================
> --- superiotool_libsuperiodetect/superiotool.h (Revision 5651)
> +++ superiotool_libsuperiodetect/superiotool.h (Arbeitskopie)
> @@ -98,12 +98,19 @@
> struct superio_registers {
> int32_t superio_id; /* Signed, as we need EOT. */
> const char *name; /* Super I/O name */
> +#ifndef LIBSUPERIODETECT
> struct {
> int8_t ldn;
> const char *name; /* LDN name */
> int16_t idx[IDXSIZE];
> int16_t def[IDXSIZE];
> } ldn[LDNSIZE];
> +#else /* LIBSUPERIODETECT */
> + /* Ugly hack to avoid messing with EOT markers. */
> + struct {
> + int8_t dummy;
> + } dummy[1];
> +#endif /* LIBSUPERIODETECT */
> };
>
hm ... the comment is very terse.
--
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
Tel.: +49 761 7668825 • Fax: +49 761 7664613
Email: info at coresystems.de • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866
More information about the coreboot
mailing list