Commit 1b7898ee276b "powerpc/boot: Use the pre-boot decompression API" breaks boot

Am 10.10.2016 um 08:10 schrieb Heiner Kallweit:
> Am 10.10.2016 um 06:41 schrieb Michael Ellerman:
>> Heiner Kallweit <hkallweit1@gmail.com> writes:
>>
>>> Am 07.10.2016 um 21:26 schrieb Heiner Kallweit:
>>>> Am 07.10.2016 um 07:51 schrieb Oliver O'Halloran:
>>>>> Hi, Heiner
>>>>>
>>>>> Could you send me a copy of the kernel .config (or which defconfig)
>>>>> that you're using, the name of the HW platform that you're using and
>>>>> if possible the kernel image itself?
>>>>>
>>>>> Thanks,
>>>>> Oliver
>>>>>
>>>> Thanks for the quick reply. Attached are .config and cuImage.
>>>> HW is a TP-Link TL-WDR4900 WiFi router (P1014-based) running OpenWRT.
>>>>
>>> After further checking I think I found the issue. The old gunzip code
>>> handled uncompressed data transparently whilst the new one bails out
>>> if it doesn't find a proper gzip header.
>>> And in my case the actual kernel image is uncompressed.
>>> With the following patch the system boots fine again (at least for me).
>>
>> Thanks for testing and tracking it down.
>>
>> I wonder why the actual image is uncompressed? Or alternately why do we
>> tell uboot the image is compressed when it's not?
>>
> Uboot is provided with a compressed image, but what gets compressed is
> not the pure kernel image but the resulting image incl. boot wrapper code,
> see this part of the wrapper script:
> 
> cuboot*)
>     gzip -n -f -9 "$ofile"
>     ${MKIMAGE} -A ppc -O linux -T kernel -C gzip -a "$base" -e "$entry" \
>             $uboot_version -d "$ofile".gz "$ofile"
> 
> And this resulting image is decompressed by uboot already during boot.
> Therefore the boot wrapper code sees an uncompressed kernel image.
> 
> IMHO in case of using cuboot no CONFIG_KERNEL_<COMPR TYPE> config option
> should be set and Makefile + code in arch/powerpc/boot should be able
> to deal with this situation:
> - don't copy and build the decompression stuff
> - use an alternative version of prep_kernel() in main.c which doesn't
>   attempt to decompress the kernel image
> 
> This should be a cleaner solution than probing the kernel image whether
> it's compressed or not.
> 

This would be the patch implementing the idea. Advantage is that all
the unnecessary decompression code isn't built. Works fine for me.

---
 arch/powerpc/boot/Makefile |  7 ++++++-
 arch/powerpc/boot/main.c   | 15 ++++++++++++---
 2 files changed, 18 insertions(+), 4 deletions(-)

On Tue, Oct 11, 2016 at 7:06 AM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> IMHO in case of using cuboot no CONFIG_KERNEL_<COMPR TYPE> config option
>> should be set and Makefile + code in arch/powerpc/boot should be able
>> to deal with this situation:
>> - don't copy and build the decompression stuff
>> - use an alternative version of prep_kernel() in main.c which doesn't
>>   attempt to decompress the kernel image
>>
>> This should be a cleaner solution than probing the kernel image whether
>> it's compressed or not.
>>
>
> This would be the patch implementing the idea. Advantage is that all
> the unnecessary decompression code isn't built. Works fine for me.

I don't think this approach is viable. The wrapper code is shared
among the various output image formats some of which *will* contain a
compressed kernel image so we can't simply remove the decompressor
from the wrapper. A random example I found in the makefile was
CONFIG_BAMBOO:

> image-$(CONFIG_BAMBOO) += treeImage.bamboo cuImage.bamboo

When building for this platform Kbuild will produce treeboot and a
cuboot image. Unlike uboot, Treeboot doesn't do any decompression so
the wrapper needs to decompress the kernel itself. The probing
solution more or less matches the old behaviour (which we know works)
so I think we should just stick with that.

- Oliver

Am 12.10.2016 um 06:26 schrieb Oliver O'Halloran:
> On Tue, Oct 11, 2016 at 7:06 AM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>> IMHO in case of using cuboot no CONFIG_KERNEL_<COMPR TYPE> config option
>>> should be set and Makefile + code in arch/powerpc/boot should be able
>>> to deal with this situation:
>>> - don't copy and build the decompression stuff
>>> - use an alternative version of prep_kernel() in main.c which doesn't
>>>   attempt to decompress the kernel image
>>>
>>> This should be a cleaner solution than probing the kernel image whether
>>> it's compressed or not.
>>>
>>
>> This would be the patch implementing the idea. Advantage is that all
>> the unnecessary decompression code isn't built. Works fine for me.
> 
> I don't think this approach is viable. The wrapper code is shared
> among the various output image formats some of which *will* contain a
> compressed kernel image so we can't simply remove the decompressor
> from the wrapper. A random example I found in the makefile was
> CONFIG_BAMBOO:
> 
>> image-$(CONFIG_BAMBOO) += treeImage.bamboo cuImage.bamboo
> 
> When building for this platform Kbuild will produce treeboot and a
> cuboot image. Unlike uboot, Treeboot doesn't do any decompression so
> the wrapper needs to decompress the kernel itself. The probing
> solution more or less matches the old behaviour (which we know works)
> so I think we should just stick with that.
> 
> - Oliver
> 
Indeed, I also figured that out later. As you say, then let's stick
with re-introducing the probing. I'll send the patch for this.

Heiner