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

čt 25. 8. 2022 v 8:42 odesílatel Ovidiu Panait ovpanait@gmail.com napsal:

Add bdinfo_print_size() helper to display size variables (such as cache sizes) in bdinfo format. The size is printed as "xxx Bytes", "xxx KiB", "xxx MiB", "xxx GiB", etc as needed;

Signed-off-by: Ovidiu Panait ovpanait@gmail.com

cmd/bdinfo.c | 7 +++++++ include/init.h | 13 +++++++++++++ 2 files changed, 20 insertions(+)

diff --git a/cmd/bdinfo.c b/cmd/bdinfo.c index 37cd8a57eb..9e23c4dd8f 100644 --- a/cmd/bdinfo.c +++ b/cmd/bdinfo.c @@ -16,9 +16,16 @@ #include <vsprintf.h> #include <asm/cache.h> #include <asm/global_data.h> +#include <display_options.h>

DECLARE_GLOBAL_DATA_PTR;

+void bdinfo_print_size(const char *name, uint64_t size) +{

•   printf("%-12s= ", name);
  

•   print_size(size, "\n");
  

+}

void bdinfo_print_num_l(const char *name, ulong value) { printf("%-12s= 0x%0*lx\n", name, 2 * (int)sizeof(value), value); diff --git a/include/init.h b/include/init.h index 7b8f62c121..02bb4ce13e 100644 --- a/include/init.h +++ b/include/init.h @@ -343,6 +343,19 @@ void bdinfo_print_num_ll(const char *name, unsigned long long value); /* Print a clock speed in MHz */ void bdinfo_print_mhz(const char *name, unsigned long hz);

+/**

• • bdinfo_print_size - print size variables in bdinfo format
• • @name: string to print before the size
• • @size: size to print
• • Helper function for displaying size variables as properly formatted bdinfo
• • entries. The size is printed as "xxx Bytes", "xxx KiB", "xxx MiB",
• • "xxx GiB", etc. as needed;
• • For use in arch_print_bdinfo().
• */

+void bdinfo_print_size(const char *name, uint64_t size);

/* Show arch-specific information for the 'bd' command */ void arch_print_bdinfo(void);

-- 2.25.1

No problem with this code. If there is any problem you can include it directly to mb implementation but make sense to have it in generic location.

Thanks, Michal