drivers/usb/misc/emi*.c have the biggest data objects in the whole tree

drivers/usb/misc/emi*.c have the biggest data objects in the whole tree

[Denys Vlasenko]

Hi Tapio,

You are the author of these files. Are you still maintaining them?
If not, do you know who is the current maintainer?

These two object files hold the biggest data objects in the whole Linux kernel
after lockdep:

   text    data     bss     dec     hex filename
   1258  160516       0  161774   277ee ./drivers/usb/misc/emi26.o
   1504  209296       0  210800   33770 ./drivers/usb/misc/emi62.o

Basically, these are big arrays of the following structures:

typedef struct _INTEL_HEX_RECORD
{
        __u32   length;
        __u32   address;
        __u32   type;
        __u8    data[MAX_INTEL_HEX_RECORD_LENGTH];
} INTEL_HEX_RECORD;

I suggest the following optimizations:

Change structure to

typedef struct _INTEL_HEX_RECORD
{
        __u8   type;
        __u8   length;
        __u16   address;
        __u8    data[MAX_INTEL_HEX_RECORD_LENGTH];
} INTEL_HEX_RECORD __attribute__((__packed__));

Store gzip compressed tables and unpack them at load time.

Declare them const and __initdata.

I can do it if there is no active maintainer.
--
vda

Re: drivers/usb/misc/emi*.c have the biggest data objects in the whole tree

[Clemens Ladisch]

Denys Vlasenko wrote:
> Hi Tapio,
>
> You are the author of these files. Are you still maintaining them?

His newer email address that I found with Google is dead, too.

> These two object files hold the biggest data objects in the whole   
> Linux kernel
>
> Basically, these are big arrays of the following structures:
>
> typedef struct _INTEL_HEX_RECORD
> {
>         __u32   length;
>         __u32   address;
>         __u32   type;
>         __u8    data[MAX_INTEL_HEX_RECORD_LENGTH];
> } INTEL_HEX_RECORD;
>
> I suggest the following optimizations:
>
> Change structure to
>
> typedef struct _INTEL_HEX_RECORD
> {
>         __u8   type;
>         __u8   length;
>         __u16   address;
>         __u8    data[MAX_INTEL_HEX_RECORD_LENGTH];
> } INTEL_HEX_RECORD __attribute__((__packed__));
>
> Store gzip compressed tables and unpack them at load time.
>
> Declare them const and __initdata.

I have a patch somewhere that moves the firmware code to userspace and changes
the drivers to use request_firmware().


Regards,
Clemens

Re: drivers/usb/misc/emi*.c have the biggest data objects in the whole tree

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

On Fri, 14 Sep 2007 11:35:34 BST, Denys Vlasenko said:
> Hi Tapio,
> 
> You are the author of these files. Are you still maintaining them?
> If not, do you know who is the current maintainer?

> These two object files hold the biggest data objects in the whole Linux kernel
> after lockdep:
> 
>    text    data     bss     dec     hex filename
>    1258  160516       0  161774   277ee ./drivers/usb/misc/emi26.o
>    1504  209296       0  210800   33770 ./drivers/usb/misc/emi62.o
> 
> Basically, these are big arrays of the following structures:
> 
> typedef struct _INTEL_HEX_RECORD
> {
>         __u32   length;
>         __u32   address;
>         __u32   type;
>         __u8    data[MAX_INTEL_HEX_RECORD_LENGTH];
> } INTEL_HEX_RECORD;
> 
> I suggest the following optimizations:
> 
> Change structure to

I suggest moving those out of the kernel entirely and use the firmware loader
support to bring it in from userspace like all the *other* firmware blobs.

'INTEL_HEX_RECORD' just *screams* 'microcode' ;)

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: drivers/usb/misc/emi*.c have the biggest data objects in the whole tree

[Greg KH]

On Fri, Sep 14, 2007 at 11:35:34AM +0100, Denys Vlasenko wrote:
> Hi Tapio,
> 
> You are the author of these files. Are you still maintaining them?
> If not, do you know who is the current maintainer?
> 
> These two object files hold the biggest data objects in the whole Linux kernel
> after lockdep:
> 
>    text    data     bss     dec     hex filename
>    1258  160516       0  161774   277ee ./drivers/usb/misc/emi26.o
>    1504  209296       0  210800   33770 ./drivers/usb/misc/emi62.o
> 
> Basically, these are big arrays of the following structures:
> 
> typedef struct _INTEL_HEX_RECORD
> {
>         __u32   length;
>         __u32   address;
>         __u32   type;
>         __u8    data[MAX_INTEL_HEX_RECORD_LENGTH];
> } INTEL_HEX_RECORD;
> 
> I suggest the following optimizations:
> 
> Change structure to
> 
> typedef struct _INTEL_HEX_RECORD
> {
>         __u8   type;
>         __u8   length;
>         __u16   address;
>         __u8    data[MAX_INTEL_HEX_RECORD_LENGTH];
> } INTEL_HEX_RECORD __attribute__((__packed__));

Only if you redo the whole firmware image too :)

What is this really hurting?  It's only relevant if you load the
specific module, if you have this device type.  It's a firmware blob,
nothing really interesting at all.

thanks,

greg k-h

Re: drivers/usb/misc/emi*.c have the biggest data objects in the whole tree

[Denys Vlasenko]

On Friday 28 September 2007 05:41, Greg KH wrote:
> On Fri, Sep 14, 2007 at 11:35:34AM +0100, Denys Vlasenko wrote:
> > Hi Tapio,
> > 
> > You are the author of these files. Are you still maintaining them?
> > If not, do you know who is the current maintainer?
> > 
> > These two object files hold the biggest data objects in the whole Linux kernel
> > after lockdep:
> > 
> >    text    data     bss     dec     hex filename
> >    1258  160516       0  161774   277ee ./drivers/usb/misc/emi26.o
> >    1504  209296       0  210800   33770 ./drivers/usb/misc/emi62.o
> > 
> > Basically, these are big arrays of the following structures:
> > 
> > typedef struct _INTEL_HEX_RECORD
> > {
> >         __u32   length;
> >         __u32   address;
> >         __u32   type;
> >         __u8    data[MAX_INTEL_HEX_RECORD_LENGTH];
> > } INTEL_HEX_RECORD;
> > 
> > I suggest the following optimizations:
> > 
> > Change structure to
> > 
> > typedef struct _INTEL_HEX_RECORD
> > {
> >         __u8   type;
> >         __u8   length;
> >         __u16   address;
> >         __u8    data[MAX_INTEL_HEX_RECORD_LENGTH];
> > } INTEL_HEX_RECORD __attribute__((__packed__));
> 
> Only if you redo the whole firmware image too :)

I did. It wasn't hard.

> What is this really hurting?  It's only relevant if you load the
> specific module

By this logic, no space wastage in modules is worth fixing.
--
vda

Re: drivers/usb/misc/emi*.c have the biggest data objects in the whole tree

[Greg KH]

On Fri, Sep 28, 2007 at 12:34:29PM +0100, Denys Vlasenko wrote:
> On Friday 28 September 2007 05:41, Greg KH wrote:
> > On Fri, Sep 14, 2007 at 11:35:34AM +0100, Denys Vlasenko wrote:
> > > Hi Tapio,
> > > 
> > > You are the author of these files. Are you still maintaining them?
> > > If not, do you know who is the current maintainer?
> > > 
> > > These two object files hold the biggest data objects in the whole Linux kernel
> > > after lockdep:
> > > 
> > >    text    data     bss     dec     hex filename
> > >    1258  160516       0  161774   277ee ./drivers/usb/misc/emi26.o
> > >    1504  209296       0  210800   33770 ./drivers/usb/misc/emi62.o
> > > 
> > > Basically, these are big arrays of the following structures:
> > > 
> > > typedef struct _INTEL_HEX_RECORD
> > > {
> > >         __u32   length;
> > >         __u32   address;
> > >         __u32   type;
> > >         __u8    data[MAX_INTEL_HEX_RECORD_LENGTH];
> > > } INTEL_HEX_RECORD;
> > > 
> > > I suggest the following optimizations:
> > > 
> > > Change structure to
> > > 
> > > typedef struct _INTEL_HEX_RECORD
> > > {
> > >         __u8   type;
> > >         __u8   length;
> > >         __u16   address;
> > >         __u8    data[MAX_INTEL_HEX_RECORD_LENGTH];
> > > } INTEL_HEX_RECORD __attribute__((__packed__));
> > 
> > Only if you redo the whole firmware image too :)
> 
> I did. It wasn't hard.

Great, send me a patch then :)

> > What is this really hurting?  It's only relevant if you load the
> > specific module
> 
> By this logic, no space wastage in modules is worth fixing.

No, that's not true.  Be realistic here please.

thanks,

greg k-h

testers needed for drivers/usb/misc/emi*.c

[Denys Vlasenko]

On Tuesday 02 October 2007 17:35, Greg KH wrote:
> > > > > > I suggest the following optimizations:
> > > > > > 
> > > > > > Change structure to
> > > > > > 
> > > > > > typedef struct _INTEL_HEX_RECORD
> > > > > > {
> > > > > >         __u8   type;
> > > > > >         __u8   length;
> > > > > >         __u16   address;
> > > > > >         __u8    data[MAX_INTEL_HEX_RECORD_LENGTH];
> > > > > > } INTEL_HEX_RECORD __attribute__((__packed__));
> > > > > 
> > > > > Only if you redo the whole firmware image too :)
> > > > 
> > > > I did. It wasn't hard.
> > > 
> > > Great, send me a patch then :)
> > 
> > Here is it. Sending off-list due to size.
> > 
> > emi.code.patch contains changes to firmware loading code.
> > emi.fw.patch.bz2 contains modified firmware images.
> > 
> > I don't have this hardware, thus I wrote test programs which
> > mimic firmware loading in userspace (they dump filrmware
> > in hex to stdout). See testing.tar.bz2.
> 
> Can you find someone with this hardware to test this patch?  I'll not be
> able to take it until you do as the device might be assuming some of
> these firmware requirements that you changed.

No, I don't know any users who have this device.

Changing subject and CCing lkml, just in case.
--
vda