[PATCH] mx6qsabrelite: increase offset for environment on SPI

[Denis Pynkin]

14.09.2020 21:50, Tom Rini wrote:
> On Mon, Sep 14, 2020 at 08:08:48PM +0300, Denis Pynkin wrote:
>> 14.09.2020 17:11, Tom Rini wrote:
>>> On Mon, Sep 14, 2020 at 09:29:18AM -0400, Tom Rini wrote:
>>>> On Mon, Sep 14, 2020 at 04:20:20PM +0300, Denis Pynkin wrote:
>>>>> 14.09.2020 15:33, Tom Rini wroteт:
>>>>>> On Mon, Sep 14, 2020 at 08:03:33AM -0300, Fabio Estevam wrote:
>>>>>
>>>>>>>> The size of the binary created with the default U-boot config is much
>>>>>>>> greater than the default offset for environment `0x60000`. If the new
>>>>>>>> version is flashed onto SPI it overlaps with the stored environment.
>>>>>>>> This leads to U-Boot corruption in case of saving environment onto
>>>>>>>> SPI and non-bootable SabreLite board.
>>>>>>>>
>>>>>>>> Signed-off-by: Denis Pynkin <denis.pynkin at collabora.com>
>>>>>>>
>>>>>>> Reviewed-by: Fabio Estevam <festevam at gmail.com>
>>>>>>>
>>>>>>> In case you want to detect errors like this again in the future, you could add
>>>>>>> CONFIG_BOARD_SIZE_LIMIT that detects such overlaps in build-time.
>>>>>>>
>>>>>>> Please check commit 033f6ea5fa5f ("mx53loco: Fix U-Boot corruption
>>>>>>> after saving the environment")
>>>>>>>
>>>>>>> The addition of CONFIG_BOARD_SIZE_LIMIT can be a separate patch though.
>>>>>>
>>>>>> Hold on.  Can we shrink the board back down so that we don't need to
>>>>>> move the environment?  Moving the environment is bad as it will also
>>>>>> break existing users.  Thanks!
>>>>>
>>>>> I don't think so to be honest.
>>>>> Current sizes even for `u-boot-nodtb.imx` in my builds are:
>>>>> - 595308 (0x9156C) -- cross-compilation
>>>>> - 470780 (0x72EFC) -- native build
>>>>
>>>> I'd like to have one thread where we see what on earth is going on
>>>> there.  That's a rather crazy size difference and very troubling.
>>>>
>>>>> which is much larger than current offset 393216 (0x60000)
>>>>>
>>>>> The offset for environment `CONFIG_ENV_OFFSET=0x60000` were added in
>>>>> commit a09fea1 just a year ago.
>>>>> Not sure if it was tested with SabreLite board tbh -- this is a bulk
>>>>> commit aimed for ARMv8 and sizes of binaries produced with pre-a09fea1
>>>>> commit are also larger than the current offset.
>>>>
>>>> Ah, so here's what's going on then.  Commit a09fea1 wasn't about ARMv8,
>>>> it was about migrating options to the defconfig files.  In this case, it
>>>> should have been set to the value you're changing it to now, at least
>>>> from a read of the patch (include/configs/mx6sabre_common.h would set it
>>>> to 0xC0000 if CONFIG_ENV_IS_IN_MMC), but I thought I had done that
>>>> migration with my hack to make a tool that had the in-use value be
>>>> printed.  So I'm going to re-check that whole thing to see what else
>>>> might be wrong as well.  Thanks!
>>>
>>> Ah, so, mx6qsabrelite falls in to the "nitrogen6x" family and not the
>>> rest of the "sabre" board families.  As such, it's always had the env
>>> offset of 0x60000.  Jumping back somewhat arbitrarily to v2014.10, I see
>>> with gcc-4.9 a size of 404480 on u-boot.imx which is still bigger.
>>>
>>> The commit message isn't clear however, as environment is in MMC and not
>>> SPI, so SPI booting should be fine.  But MMC/eMMC is where this case can
>>> happen (I'm not sure which device is SD slot and which is eMMC on this
>>> hardware off-hand).  Thanks!
>>
>> To my shame -- I didn't even thought about the booting from MMC, but yes
>> -- should be the same issue in case if U-Boot is placed on SD-card.
>>
>> This device have 2MB SPI NOR flash there the U-Boot could be flashed
>> with an environment. We are using this approach, so I attempt to
>> describe the issue from this point of view.
>>
>> Should I try to re-word the description? Or may I ask you to add a
>> better description?
> 
> Well, there's a few things going on here then.  The defconfig has
> ENV_IS_IN_MMC, not ENV_IS_IN_SPI.  So SPI booting should be fine we're
> putting U-Boot and environment on entirely different media.  There's no
> conflict.  Booting from the first MMC device can be broken as that's
> where we say the environment should be and 'saveenv' will scribble all
> over it.  So can you confirm what the breaking condition you see is?
> The commit message talks about SPI, but SPI should work as-is.

You are absolutely right.
My fault -- copy-paste the patch from my project as is without
additional testing.
Did an additional tests with default value 0x60000:
- works well with the combination U-Boot on SPI NOR flash + environment
on MMC
- corrupted U-Boot on MMC after `saveenv` if used U-Boot on MMC
- corrupted U-Boot after `saveenv` if it is configured
  with `CONFIG_ENV_IS_IN_SPI_FLASH=y` and flashed on SPI

The value `0xC0000` fixes the issue for both corrupted cases.

Thanks a lot for pointing to incorrect description and misunderstanding!

-- 
wbr, Denis