Re: [coreboot] [PATCH] superiotool: libsuperiodetect

[Stefan Reinauer]

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
> /* 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.