[U-Boot] [PATCH v2] tools/env: fix environment alignment tests for block devices

[Max Krummenacher]

Hi Andreas

2016-11-28 10:47 GMT+01:00 Andreas Fenkart <andreas.fenkart at digitalstrom.com>:
> Hi Max,
>
> LGTM, see one nit below, can fixed later
>
>
> On 11/19/2016 01:58 PM, Max Krummenacher wrote:
>>
>> commit 183923d3e412500bdc597d1745e2fb6f7f679ec7 enforces that the
>> environment must start at an erase block boundary.
>>
>> For block devices the sample fw_env.config does not mandate a erase block
>> size
>> for block devices. A missing setting defaults to the full env size.
>>
>>   tools/env/fw_env.c | 60
...
>>                 }
>>                 DEVTYPE(dev) = mtdinfo.type;
>> +               if (DEVESIZE(dev) == 0)
>> +                       /* Assume the erase size is the same as the
>> env-size */
>> +                       DEVESIZE(dev) = ENVSIZE(dev);
>
> Since we already checked for character devices, we could use the
> mtdinfo.erasesize. I guess we can relay on mtdinfo on new kernels, and if
> not there is still the fallback to specify it in fw_env.config.

Agreed, however since that changes functionallity I think this must go
into a follow up patch.
Also, since the config file skeleton mandates a erase size I don't see
a pressing need.

>
>>         } else {
>>                 uint64_t size;
>>                 DEVTYPE(dev) = MTD_ABSENT;
>> +               if (DEVESIZE(dev) == 0)
>> +                       /* Assume the erase size to be 512 bytes */
>> +                       DEVESIZE(dev) = 0x200;
>
> It doesn't even matter. In case of MTD_ABSENT, erasize is never used itself,
> only the tuple (DEVESIZE * ENVSECTORS) matters.

That is not true. with the test for DEVOFFSET being aligned to
DEVESIZE the previous used default broke env with certain legal config
files.
Other than this new test I agree.

Max