Re: [PATCH 2/4] cpu: microblaze: remove unused ret variable

[Ovidiu Panait <ovpanait@gmail.com>]

Hi Michal,

On 8/25/22 11:59, Michal Simek wrote:
>
>
> On 8/25/22 08:41, Ovidiu Panait wrote:
>> Drop the unused ret variable from microblaze_cpu_get_desc().
>>
>> Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
>> ---
>>
>>   drivers/cpu/microblaze_cpu.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/cpu/microblaze_cpu.c b/drivers/cpu/microblaze_cpu.c
>> index 969a1047e5..4eae06a8a6 100644
>> --- a/drivers/cpu/microblaze_cpu.c
>> +++ b/drivers/cpu/microblaze_cpu.c
>> @@ -88,15 +88,14 @@ static int microblaze_cpu_get_desc(const struct 
>> udevice *dev, char *buf,
>>       struct microblaze_cpuinfo *ci = gd_cpuinfo();
>>       const char *cpu_ver, *fpga_family;
>>       u32 cpu_freq_mhz;
>> -    int ret;
>>         cpu_freq_mhz = ci->cpu_freq / 1000000;
>>       cpu_ver = microblaze_lookup_cpu_version_string(ci->ver_code);
>>       fpga_family = microblaze_lookup_fpga_family_string(ci->fpga_code);
>>   -    ret = snprintf(buf, size,
>> -               "MicroBlaze @ %uMHz, Rev: %s, FPGA family: %s",
>> -               cpu_freq_mhz, cpu_ver, fpga_family);
>> +    snprintf(buf, size,
>> +         "MicroBlaze @ %uMHz, Rev: %s, FPGA family: %s",
>> +         cpu_freq_mhz, cpu_ver, fpga_family);
>>         return 0;
>>   }
>
> First of all I think you can remove DECLARE_GLOBAL_DATA_PTR from 
> drivers/cpu/microblaze_cpu.c
>
gd_cpuinfo() macro expands to "(struct microblaze_cpuinfo 
*)&gd->arch.cpuinfo", so we need the declaration for gd.


> I looked at the code and I think that there are some things what 
> should happen.
> First of all I would memset by 0 that buf which is passed and I think 
> this should be done in uclass to make sure that you won't show 
> anything what it is on the stack where buf is allocated in cmd/cpu.c 
> for example.
>
I don't think that the memset is necessary, snprintf will overwrite any 
previous contents and append the terminating null character after the 
last byte that was written to the buffer. So no garbage previously 
present on the stack should be printed when buf is used afterwards, as 
the string is null-terminated.


> The second if snprintf fails we shouldn't ignore that error because if 
> you do that that means that buffer is valid and you can print content.
>
> For cpu_freq_mhz I think that make sense to allocate some space in 
> string. For example %4u gives you exact size GHz should be fine for 
> now. cpu_ver has max size 6 now and family string has 18 chars now.
>
>
> It means change string to this with some chars on the top should be 
> fine for me.
>     ret = snprintf(buf, size,
>          "MicroBlaze @ %5uMHz, Rev: %7s, FPGA family: %20s",
>          cpu_freq_mhz, cpu_ver, fpga_family);
>
>
> And then check that ret is equal XX size would be easy for checking 
> and returning 0 if they match.

I agree, the snprintf errors should be handled properly here.


Thanks,

Ovidiu

>
> Thanks,
> Michal
>
>