| Message ID | 20220825064131.673958-2-ovpanait@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Michal Simek |
| Headers | show |
| Series | [1/4] microblaze: drop CONFIG_SYS_INIT_RAM_ADDR and CONFIG_SYS_INIT_RAM_SIZE | expand |
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 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. 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. Thanks, Michal
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 > >
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; }
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(-)