[PATCH 01/12] env: Complete generic support for writable list

[Jan Kiszka]

On 06.06.22 08:42, Marek Vasut wrote:
> On 5/29/22 17:37, Jan Kiszka wrote:
>> On 29.05.22 03:46, Marek Vasut wrote:
>>> On 5/28/22 15:02, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka at siemens.com>
>>>>
>>>> This completes what 890feecaab72 started by selecting ENV_APPEND and
>>>> ENV_IS_NOWHERE and by moving this driver to top if the list. This
>>>
>>> s at if the list at of the list@
>>>
>>>> ensures that load operations pick up both the default env and the
>>>> permitted parts of the next-prio location. When writing though, we must
>>>> not use NOWHERE and rather need to forward directly to the first
>>>> external location.
>>>
>>> Isn't the env load order a board-specific setting ?
>>>
>>
>> There can always be special cases, but I don't see why it should be
>> _always_ board-specific.
> 
> Because this is a special case, right ?

It's no longer special when you request the feature by turning on the
related config options.

> 
>>>> With this change, boards only need to define the list of writable
>>>> variables but no longer have to provide a custom env_get_location
>>>> implementation.
>>>
>>> This also brings in a lot of ifdeffery and extra complexity. If you
>>> implement this as a board-specific env_get_location() override, you can
>>> avoid that. Try:
>>>
>>> enum env_location env_get_location(enum env_operation op, int prio)
>>> {
>>>           if (op == ENVOP_SAVE) {
>>>                   if (prio == 0)
>>>                           return ENVL_SPI_FLASH;
>>>           } else {
>>>                   if (prio == 0)
>>>                           return ENVL_NOWHERE;
>>>                   if (prio == 1)
>>>                           return ENVL_SPI_FLASH;
>>>           }
>>>
>>>           return ENVL_UNKNOWN;
>>> }
>>
>> This is exactly what I would like to avoid, having to carry such a
>> generic function (minus that hard-coded the choice of storage - this can
>> be controlled via .config) in every board.
> 
> Just for a quick statistics -- how many boards would make use of this
> generic function ?

I don't have stats for how many boards are turning on ENV_WRITEABLE_LIST
except for our boards. Here, I have about 3-5 boards (depending on how
you count variants) that would all be fine with this switch and could
then drop their custom handler (or never introduce one - IOT2050).

> 
> If you want to make this into a generic patch, can you somehow reduce
> the ever-growing ifdeffery, so that the patch won't add to it so much ?
> I suspect the code above can help with that, maybe it can be used to
> remove at least the env_locations[] reordering ifdeffery ?

Your code is not generic enough as it ignores the config-selected
storage device. It would only happen to cover all our boards, but we are
not the world. That's why I had to add some ifdeffery.

I could try write an alternative arch_env_get_location that does the
required logic with a single ifdef. The prize might be some code
duplication, though.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux