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 <ovpan...@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
