[PATCH] xilinx: zynqmp: Do not use null as spl bss start address

[Stefan Herbrechtsmeier]

Am 03.06.2022 um 14:12 schrieb Michal Simek:
> 
> 
> On 6/1/22 16:03, Stefan Herbrechtsmeier wrote:
>> Hi,
>>
>> Am 01.06.2022 um 14:59 schrieb Michal Simek:
>>>
>>>
>>> On 6/1/22 14:27, Stefan Herbrechtsmeier wrote:
>>>> Hi,
>>>>
>>>> Am 01.06.2022 um 13:59 schrieb Michal Simek:
>>>>> Hi,
>>>>>
>>>>> first of all subject is not accurate. We are not using null as 
>>>>> start but address 0.
>>>>
>>>> I will replace null with 0.
>>>>
>>>>>
>>>>> On 6/1/22 12:55, Stefan Herbrechtsmeier wrote:
>>>>>> [CAUTION: External Email]
>>>>>>
>>>>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier at weidmueller.com>
>>>>>>
>>>>>> Do not use null as address for memory because of the special 
>>>>>> meaning for
>>>>>> pointers. Change the spl bss start address to the second page.
>>>>>>
>>>>>> Signed-off-by: Stefan Herbrechtsmeier 
>>>>>> <stefan.herbrechtsmeier at weidmueller.com>
>>>>>>
>>>>>> ---
>>>>>> The problem was discovered with a static char array initialized 
>>>>>> with an
>>>>>> empty string.
>>>>>
>>>>> It means your code is doing wrong pointer arithmeticians which 
>>>>> pointed to BSS section and overwrites there something. What is that 
>>>>> code doing?
>>>>
>>>> I like to call the zynqmp_get_silicon_idcode_name function from my 
>>>> board code and therefore rework the function to return static memory 
>>>> to avoid a memory allocation on each call.
>>>>
>>>> -    char name[ZYNQMP_VERSION_SIZE];
>>>> +    static char name[ZYNQMP_VERSION_SIZE] = "";
>>>
>>> Try to remove = "".
>>
>> The = "" is needed to skip the processing on the second run.
> 
> I looked if this really change anything. Static variable is placed to 
> BSS section which is zeroed by default at start. It means before you 
> jump to this function name array is already zeroed.

Without the empty default value it is placed in an other section.

General it is a bad idea to use memory address zero because if a pointer 
point to this place it is interpreted as NULL pointer.

>>> Undefined variables should be placed to bss section.
>>
>> A = "zu" will also move the variable to an other section.
>>
>>>
>>>>
>>>> +    if (name[0])
>>>> +        return name;
>>>>
>>>> -    return strdup(name);
>>>> +    return name;
> 
> this function is called only once now that's why I personally can't see 
> to do any change there unless you have good reason to call it more times.

I use the function to select the correct configuration (fpga image) from 
the u-boot.itb in the spl.

> But that being said I think that would make sense to move the whole 
> zynqmp_get_silicon_idcode_name() to soc_xilinx_zynqmp.c and do silicon 
> detection there. Only option there is soc_get_machine() which can return 
> silicon type.

That sounds good.

>>>> The name variable gets the address 0 which means that snprintf and 
>>>> strncat do nothing because the dest pointer and NULL are equal.
>>>
>>> Ok. I see. It become the first variable in bss section.
>>
>> Should I resend the patch with null replaced by 0?
> 
> Yes please.

I will resend the patch next week.

Regards
   Stefan