RE: [PATCH V3 3/3] board: dhelectronics: Sync env variable dh_som_serial_number with SN

[Christoph Niedermaier <cniedermaier@dh-electronics.com>]

From: Marek Vasut <marex@denx.de>
Sent: Sunday, November 24, 2024 11:11 PM
> On 11/24/24 10:10 PM, Marek Vasut wrote:
>> On 11/21/24 6:21 PM, Christoph Niedermaier wrote:
>>> The env variable "SN" is used to store the serial number on DH
>>> electronics
>>> SoMs. New SoMs will use the variable "dh_som_serial_number". To ensure
>>> compatibility, these env variables are synchronized. This is achieved
>>> using callback functions. To avoid recursive calls, these are locked
>>> against each other.
>>
>> What kind of recursive calls?
>>
>> It would be good to expand the commit message and elaborate on the
>> recursive calls problem .
>>
>>> diff --git a/board/dhelectronics/common/dh_common.c b/board/
>>> dhelectronics/common/dh_common.c
>>> index a7b0472a09..2e3e5c483b 100644
>>> --- a/board/dhelectronics/common/dh_common.c
>>> +++ b/board/dhelectronics/common/dh_common.c
>>> @@ -45,6 +45,32 @@ struct eeprom_id_page {
>>>   #define DH_EEPROM_ID_PAGE_V1_0_DATA_LEN    (sizeof(struct
>>> eeprom_id_page) - \
>>>                       offsetof(struct eeprom_id_page, mac0))
>>> +static bool in_dh_som_serial_number;
>>> +static bool in_SN;
>>> +
>>> +static int on_dh_som_serial_number(const char *name, const char
>>> *value, enum env_op op,
>>> +                   int flags)
>>> +{
>>> +    in_dh_som_serial_number = true;
>>> +    if (!in_SN)
>>> +        env_set("SN", value);
>>> +    in_dh_som_serial_number = false;
>>> +    return 0;
>>> +}
> Maybe a more generic solution is:
> 
> diff --git a/lib/hashtable.c b/lib/hashtable.c
> index e8a59e2dcac..75c263b5053 100644
> --- a/lib/hashtable.c
> +++ b/lib/hashtable.c
> @@ -221,11 +221,32 @@ static int
>   do_callback(const struct env_entry *e, const char *name, const char
> *value,
>              enum env_op op, int flags)
>   {
> +       int ret = 0;
> +
>   #ifndef CONFIG_XPL_BUILD
> -       if (e->callback)
> -               return e->callback(name, value, op, flags);
> +       static bool in_callback;
> +
> +       if (!e->callback || in_callback)
> +               return 0;
> +
> +       /*
> +        * In case there are two variables which each implement env callback
> +        * that performs env_set() on the other variable, the callbacks will
> +        * call each other recursively until the stack runs out. Prevent such
> +        * a recursion from happening.
> +        *
> +        * Example which triggers this behavior:
> +        * static int on_foo(...) { env_set("bar", 0); ... }
> +        * static int on_bar(...) { env_set("foo", 0); ... }
> +        * U_BOOT_ENV_CALLBACK(foo, on_foo);
> +        * U_BOOT_ENV_CALLBACK(bar, on_bar);
> +        */
> +       in_callback = true;
> +       ret = e->callback(name, value, op, flags);
> +       in_callback = false;
>   #endif
> -       return 0;
> +
> +       return ret;
>   }
> 
>   /*

Looks good, I will integrate it in V4.

Regards
Christoph