[U-Boot] [PATCH] Add support for LZ4 decompression algorithm

[Simon Glass]

Hi Julius,

On 2 October 2015 at 23:18, Julius Werner <jwerner at chromium.org> wrote:
>>
>> I get this build error with sandbox.
>>
>> test/built-in.o: In function `uncompress_using_lz4':
>> /home/sjg/c/src/third_party/u-boot/files/test/compression.c:278:
>> undefined reference to `ulz4fn'
>
>
> Yeah... that's because you didn't configure it in. I'm not really sure how this is supposed to work... there should really be some sort of dependency between selecting the compression tests and selecting the algorithm, or the algorithms should be #ifdefed out like in bootm.c. But neither of those seems to be in place right now... include/configs/sandbox.h just enables all the other compression algorithms, that's why it works for them.
>
> I could add a 'select LZ4' to config UNIT_TEST in the Kconfig to create such a dependency, I think that would be the best option?

Sounds good. I think it makes sense to enable all possible options in
sandbox - since it helps with build testing too.

>
>> You should be able to run the tests using:
>>
>> make O=sandbox defconfig all
>>  ./sandbox/u-boot -c "ut_compression"
>
>
> I tried, but I can't find myself through the SDL dependency hell... sorry. I installed Ubuntu's libsdl2-dev but apparently that wasn't enough... I still get "make[2]: sdl-config: Command not found" and "../arch/sandbox/cpu/sdl.c:9:21: fatal error: SDL/SDL.h: No such file or directory" when I'm trying to build. (I ran it with make -k now so I confirmed that it builds both lib/lz4_wrapper.o and test/compression.o without errors. I just can't test the linking step.)

You can build U-Boot with NO_SDL=1

I have this on my goobuntu laptop:

~> dpkg -l |grep -i sdl
ii  libsdl1.2-dev
1.2.15-8ubuntu1.1                                   amd64
Simple DirectMedia Layer development files
ii  libsdl1.2debian:amd64
1.2.15-8ubuntu1.1                                   amd64
Simple DirectMedia Layer

>
>>
>> Can you instead add this option to lib/Kconfig and put your help
>> there? We are moving away from the old CONFIGS.
>
>
> Done in next version.
>
>> You should be able to replace this license with
>>
>> SPDX-License-Identifier: BSD-2-Clause
>> [...]
>> Should be able to use:
>>
>>  SPDX-License-Identifier: GPL-2.0+ BSD-2-Clause
>
>
> Done in next version.
>
>>
>> I can see why you want to keep lz4.c as is. But this file is written
>> by you, isn't it? If so, can you fix the checkpatch errors that are
>> fixable (e.g. run 'patman').
>>
>> warning: lib/lz4_wrapper.c,41: do not add new typedefs
>> warning: lib/lz4_wrapper.c,42: do not add new typedefs
>> warning: lib/lz4_wrapper.c,43: do not add new typedefs
>> warning: lib/lz4_wrapper.c,44: do not add new typedefs
>> warning: lib/lz4_wrapper.c,45: do not add new typedefs
>> warning: lib/lz4_wrapper.c,47: storage class should be at the
>> beginning of the declaration
>> error: lib/lz4_wrapper.c,59: spaces prohibited around that ':' (ctx:WxW)
>> error: lib/lz4_wrapper.c,60: spaces prohibited around that ':' (ctx:WxW)
>> error: lib/lz4_wrapper.c,61: spaces prohibited around that ':' (ctx:WxW)
>> error: lib/lz4_wrapper.c,62: spaces prohibited around that ':' (ctx:WxW)
>> error: lib/lz4_wrapper.c,63: spaces prohibited around that ':' (ctx:WxW)
>> error: lib/lz4_wrapper.c,64: spaces prohibited around that ':' (ctx:WxW)
>> error: lib/lz4_wrapper.c,70: spaces prohibited around that ':' (ctx:WxW)
>> error: lib/lz4_wrapper.c,71: spaces prohibited around that ':' (ctx:WxW)
>> error: lib/lz4_wrapper.c,72: spaces prohibited around that ':' (ctx:WxW)
>> warning: lib/lz4_wrapper.c,77: __packed is preferred over
>> __attribute__((packed))
>> warning: lib/lz4_wrapper.c,77: Adding new packed members is to be done with care
>> error: lib/lz4_wrapper.c,83: spaces prohibited around that ':' (ctx:WxW)
>> error: lib/lz4_wrapper.c,84: spaces prohibited around that ':' (ctx:WxW)
>> warning: lib/lz4_wrapper.c,89: __packed is preferred over
>> __attribute__((packed))
>> warning: lib/lz4_wrapper.c,89: Adding new packed members is to be done with care
>> error: test/compression.c,282: return is not a function, parentheses
>> are not required
>
>
> Okay, I replaced __attribute__((packed)) with __packed and fixed the whitespace for bit fields. I think those are the only actionable ones... the camel case comes from names inside lz4.c, and I need the typedefs to map U-Boot's types to the ones lz4.c expects.

Right.

>
>>
>> Could you return int here, and use proper error numbers below? Like
>> -EINVAL, -EPROTONOSUPPORT, -ENOSPC, etc.
>
>
> Okay, I switched it to the model where it returns an int and the size is an output pointer, like the other algorithms.

Sounds good.

Regards,
Simon