Re: [PATCH v2 2/4] cmd: provide command to display SMBIOS information

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Heinrich,

A few nits below

On Wed, Jan 17, 2024 at 04:33:45PM +0100, Heinrich Schuchardt wrote:
> U-Boot can either generated an SMBIOS table or copy it from a prior boot
> stage, e.g. QEMU.
>
> Provide a command to display the SMBIOS information.
>
> Currently only type 1 and 2 are translated to human readable text.
> Other types may be added later. Currently only a hexdump and the list of
> strings is provided for these.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
> v2:

[...]

> 	email address updated
> +static struct smbios_header *next_table(struct smbios_header *table)
> +{
> +	const char *str;
> +
> +	if (table->type == SMBIOS_END_OF_TABLE)
> +		return NULL;
> +
> +	str = smbios_get_string(table, 0);
> +	return (struct smbios_header *)(++str);

That can lead to unaligned access when we dereference that pointer, do we
care?

> +}
> +
> +static void smbios_print_generic(struct smbios_header *table)
> +{
> +	char *str = (char *)table + table->length;
> +

Do we want the header below printed if there are no strings?
IOW can we exit early if (!*str) ?

> +	if (CONFIG_IS_ENABLED(HEXDUMP)) {
> +		printf("Header and Data:\n");
> +		print_hex_dump("\t", DUMP_PREFIX_OFFSET, 16, 1,
> +			       table, table->length, false);
> +	}
> +	if (*str) {
> +		printf("Strings:\n");
> +		for (int index = 1; *str; ++index) {
> +			printf("\tString %u: %s\n", index, str);
> +			str += strlen(str) + 1;
> +		}
> +	}
> +}
> +
> +void smbios_print_str(const char *label, void *table, u8 index)
> +{
> +	printf("\t%s: %s\n", label, smbios_get_string(table, index));
> +}
> +


Other than that, LGTM

Thanks
/Ilias