Re: [PATCH v4 4/6] cmd: fru: add product info area parsing support

[Michal Simek <michal.simek@amd.com>]

On 8/25/22 18:42, Jae Hyun Yoo wrote:
> Add product info area parsing support. Custom board fields can be
> added dynamically using linked list so that each board support can
> utilize them in their own custom way.
> 
> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
> ---
> Changes from v3:
>   * None.
> 
> Changes from v2:
>   * Changed 'struct fru_board_info_member' to 'struct fru_common_info_member'.
> 
> Changes from v1:
>   * Refactored using linked list instead of calling a custom parsing callback.
> 
> Changes from RFC:
>   * Added manufacturer custom product info fields parsing flow.
> 
>   cmd/fru.c     |  28 ++++--
>   include/fru.h |  34 ++++++-
>   lib/fru_ops.c | 244 +++++++++++++++++++++++++++++++++++++++++++++++---
>   3 files changed, 286 insertions(+), 20 deletions(-)
> 
> diff --git a/cmd/fru.c b/cmd/fru.c
> index b2cadbec9780..42bdaae09449 100644
> --- a/cmd/fru.c
> +++ b/cmd/fru.c
> @@ -43,11 +43,22 @@ static int do_fru_display(struct cmd_tbl *cmdtp, int flag, int argc,
>   static int do_fru_generate(struct cmd_tbl *cmdtp, int flag, int argc,
>   			   char *const argv[])
>   {
> +	int (*fru_generate)(const void *addr, int argc, char *const argv[]);
>   	unsigned long addr;
>   	const void *buf;
> -	int ret;
> +	int ret, maxargs;
> +
> +	if (!strncmp(argv[2], "-b", 3)) {
> +		fru_generate = fru_board_generate;
> +		maxargs = cmdtp->maxargs + FRU_BOARD_AREA_TOTAL_FIELDS;
> +	} else if (!strncmp(argv[2], "-p", 3)) {
> +		fru_generate = fru_product_generate;
> +		maxargs = cmdtp->maxargs + FRU_PRODUCT_AREA_TOTAL_FIELDS;
> +	} else {
> +		return CMD_RET_USAGE;
> +	}
>   
> -	if (argc < cmdtp->maxargs)
> +	if (argc < maxargs)
>   		return CMD_RET_USAGE;
>   
>   	addr = hextoul(argv[3], NULL);
> @@ -62,7 +73,7 @@ static int do_fru_generate(struct cmd_tbl *cmdtp, int flag, int argc,
>   static struct cmd_tbl cmd_fru_sub[] = {
>   	U_BOOT_CMD_MKENT(capture, 3, 0, do_fru_capture, "", ""),
>   	U_BOOT_CMD_MKENT(display, 2, 0, do_fru_display, "", ""),
> -	U_BOOT_CMD_MKENT(board_gen, 8, 0, do_fru_generate, "", ""),
> +	U_BOOT_CMD_MKENT(generate, 4, 0, do_fru_generate, "", ""),
>   };
>   
>   static int do_fru(struct cmd_tbl *cmdtp, int flag, int argc,
> @@ -90,11 +101,16 @@ static char fru_help_text[] =
>   	"capture <addr> - Parse and capture FRU table present at address.\n"
>   	"fru display - Displays content of FRU table that was captured using\n"
>   	"              fru capture command\n"
> -	"fru board_gen <addr> <manufacturer> <board name> <serial number>\n"
> -	"              <part number> <file id> [custom ...] - Generate FRU\n"
> -	"              format with board info area filled based on\n"
> +	"fru generate -b <addr> <manufacturer> <board name> <serial number>\n"
> +	"                <part number> <file id> [custom ...] - Generate FRU\n"
> +	"                format with board info area filled based on\n"

this simply means that all previous user custom scripts will stop to work.
No problem to deprecated board_gen but with any transition time. It means keep 
origin option, create new one and add deprecate message to origin that scripts 
should be converted. After 3-4 releases we can remove it which should be enough 
time for users to do transition.

>   	"                parameters. <addr> is pointing to place where FRU is\n"
>   	"                generated.\n"
> +	"fru generate -p <addr> <manufacturer> <product name> <part number>\n"
> +	"                <version number> <serial number> <asset number>\n"
> +	"                <file id> [custom ...] - Generate FRU format with\n"
> +	"                product info area filled based on parameters. <addr>\n"
> +	"                is pointing to place where FRU is generated.\n"

Are you going to use this product generation. I mean it is fine how it is but 
maybe in future would make sense to have -b and -p together to be able to 
generate both of these fields.
Definitely showing product information is key part here at least for me.

>   	;
>   #endif
>   
> diff --git a/include/fru.h b/include/fru.h
> index b158b80b5866..2b19033a3843 100644
> --- a/include/fru.h
> +++ b/include/fru.h
> @@ -31,7 +31,13 @@ struct fru_board_info_header {
>   	u8 time[3];
>   } __packed;
>   
> -struct fru_board_info_member {
> +struct fru_product_info_header {
> +	u8 ver;
> +	u8 len;
> +	u8 lang_code;
> +} __packed;
> +
> +struct fru_common_info_member {
>   	u8 type_len;
>   	u8 *name;
>   } __packed;
> @@ -64,6 +70,27 @@ struct fru_board_data {
>   	struct list_head custom_fields;
>   };
>   
> +struct fru_product_data {
> +	u8 ver;
> +	u8 len;
> +	u8 lang_code;
> +	u8 manufacturer_type_len;
> +	u8 manufacturer_name[FRU_INFO_FIELD_LEN_MAX];
> +	u8 product_name_type_len;
> +	u8 product_name[FRU_INFO_FIELD_LEN_MAX];
> +	u8 part_number_type_len;
> +	u8 part_number[FRU_INFO_FIELD_LEN_MAX];
> +	u8 version_number_type_len;
> +	u8 version_number[FRU_INFO_FIELD_LEN_MAX];
> +	u8 serial_number_type_len;
> +	u8 serial_number[FRU_INFO_FIELD_LEN_MAX];
> +	u8 asset_number_type_len;
> +	u8 asset_number[FRU_INFO_FIELD_LEN_MAX];
> +	u8 file_id_type_len;
> +	u8 file_id[FRU_INFO_FIELD_LEN_MAX];
> +	struct list_head custom_fields;
> +};
> +
>   struct fru_multirec_hdr {
>   	u8 rec_type;
>   	u8 type;
> @@ -85,6 +112,7 @@ struct fru_multirec_node {
>   struct fru_table {
>   	struct fru_common_hdr hdr;
>   	struct fru_board_data brd;
> +	struct fru_product_data prd;
>   	struct list_head multi_recs;
>   	bool captured;
>   };
> @@ -102,13 +130,15 @@ struct fru_table {
>   
>   /* This should be minimum of fields */
>   #define FRU_BOARD_AREA_TOTAL_FIELDS	5
> +#define FRU_PRODUCT_AREA_TOTAL_FIELDS	7
>   #define FRU_TYPELEN_TYPE_SHIFT		6
>   #define FRU_TYPELEN_TYPE_BINARY		0
>   #define FRU_TYPELEN_TYPE_ASCII8		3
>   
>   int fru_display(int verbose);
>   int fru_capture(const void *addr);
> -int fru_generate(const void *addr, int argc, char *const argv[]);
> +int fru_board_generate(const void *addr, int argc, char *const argv[]);
> +int fru_product_generate(const void *addr, int argc, char *const argv[]);
>   u8 fru_checksum(u8 *addr, u8 len);
>   int fru_check_type_len(u8 type_len, u8 language, u8 *type);
>   const struct fru_table *fru_get_fru_data(void);
> diff --git a/lib/fru_ops.c b/lib/fru_ops.c
> index a25ebccd110d..978d5f8e8a19 100644
> --- a/lib/fru_ops.c
> +++ b/lib/fru_ops.c
> @@ -16,9 +16,18 @@
>   
>   struct fru_table fru_data __section(".data") = {
>   	.brd.custom_fields = LIST_HEAD_INIT(fru_data.brd.custom_fields),
> +	.prd.custom_fields = LIST_HEAD_INIT(fru_data.prd.custom_fields),
>   	.multi_recs = LIST_HEAD_INIT(fru_data.multi_recs),
>   };
>   
> +static const char * const fru_typecode_str[] = {
> +	"Binary/Unspecified",
> +	"BCD plus",
> +	"6-bit ASCII",
> +	"8-bit ASCII",
> +	"2-byte UNICODE"
> +};

This can be done via separate patch to make this one smaller and easier to review.

> +
>   static u16 fru_cal_area_len(u8 len)
>   {
>   	return len * FRU_COMMON_HDR_LEN_MULTIPLIER;
> @@ -77,9 +86,9 @@ int fru_check_type_len(u8 type_len, u8 language, u8 *type)
>   static u8 fru_gen_type_len(u8 *addr, char *name)
>   {
>   	int len = strlen(name);
> -	struct fru_board_info_member *member;
> +	struct fru_common_info_member *member;
>   
> -	member = (struct fru_board_info_member *)addr;
> +	member = (struct fru_common_info_member *)addr;
>   	member->type_len = FRU_TYPELEN_TYPE_ASCII8 << FRU_TYPELEN_TYPE_SHIFT;
>   	member->type_len |= len;
>   
> @@ -91,7 +100,7 @@ static u8 fru_gen_type_len(u8 *addr, char *name)
>   	return 1 + len;
>   }
>   
> -int fru_generate(const void *addr, int argc, char *const argv[])
> +int fru_board_generate(const void *addr, int argc, char *const argv[])
>   {
>   	struct fru_common_hdr *header = (struct fru_common_hdr *)addr;
>   	struct fru_board_info_header *board_info;
> @@ -161,6 +170,74 @@ int fru_generate(const void *addr, int argc, char *const argv[])
>   	return 0;
>   }
>   
> +int fru_product_generate(const void *addr, int argc, char *const argv[])
> +{
> +	struct fru_common_hdr *header = (struct fru_common_hdr *)addr;
> +	struct fru_product_info_header *product_info;
> +	u8 *member;
> +	u8 len, pad, modulo;
> +
> +	header->version = 1; /* Only version 1.0 is supported now */
> +	header->off_internal = 0; /* not present */
> +	header->off_chassis = 0; /* not present */
> +	header->off_board = 0; /* not present */
> +	header->off_product = (sizeof(*header)) / 8; /* Starting offset 8 */
> +	header->off_multirec = 0; /* not present */
> +	header->pad = 0;
> +	/*
> +	 * This unsigned byte can be used to calculate a zero checksum
> +	 * for the data area following the header. I.e. the modulo 256 sum of
> +	 * the record data bytes plus the checksum byte equals zero.
> +	 */
> +	header->crc = 0; /* Clear before calculation */
> +	header->crc = 0 - fru_checksum((u8 *)header, sizeof(*header));
> +
> +	/* board info is just right after header */
> +	product_info = (void *)((u8 *)header + sizeof(*header));
> +
> +	debug("header %lx, product_info %lx\n", (ulong)header,
> +	      (ulong)product_info);
> +
> +	product_info->ver = 1; /* 1.0 spec */
> +	product_info->lang_code = 0; /* English */
> +
> +	/* Member fields are just after board_info header */
> +	member = (u8 *)product_info + sizeof(*product_info);
> +
> +	while (--argc > 0 && ++argv) {
> +		len = fru_gen_type_len(member, *argv);
> +		member += len;
> +	}
> +
> +	*member++ = 0xc1; /* Indication of no more fields */
> +
> +	len = member - (u8 *)product_info; /* Find current length */
> +	len += 1; /* Add checksum there too for calculation */
> +
> +	modulo = len % 8;
> +
> +	if (modulo) {
> +		/* Do not fill last item which is checksum */
> +		for (pad = 0; pad < 8 - modulo; pad++)
> +			*member++ = 0;
> +
> +		/* Increase structure size */
> +		len += 8 - modulo;
> +	}
> +
> +	product_info->len = len / 8; /* Size in multiples of 8 bytes */
> +
> +	*member = 0; /* Clear before calculation */
> +	*member = 0 - fru_checksum((u8 *)product_info, len);
> +
> +	debug("checksum %x(addr %x)\n", *member, len);
> +
> +	env_set_hex("fru_addr", (ulong)addr);
> +	env_set_hex("filesize", (ulong)member - (ulong)addr + 1);
> +
> +	return 0;
> +}
> +
>   static void fru_delete_custom_fields(struct list_head *custom_fields)
>   {
>   	struct fru_custom_field_node *node, *next;
> @@ -258,6 +335,74 @@ static int fru_parse_board(const void *addr)
>   	return 0;
>   }
>   
> +static int fru_parse_product(const void *addr)
> +{
> +	u8 i, type;
> +	int len;
> +	u8 *data, *term, *limit;
> +
> +	memcpy(&fru_data.prd.ver, (void *)addr, 6);
> +	addr += 3;
> +	data = (u8 *)&fru_data.prd.manufacturer_type_len;
> +
> +	/* Record max structure limit not to write data over allocated space */
> +	limit = (u8 *)&fru_data.prd + sizeof(struct fru_product_data) -
> +		sizeof(struct fru_custom_field_node *);
> +
> +	for (i = 0; ; i++, data += FRU_INFO_FIELD_LEN_MAX) {
> +		len = fru_check_type_len(*(u8 *)addr, fru_data.prd.lang_code,
> +					 &type);
> +		/*
> +		 * Stop cature if it end of fields
> +		 */
> +		if (len == -EINVAL)
> +			break;
> +
> +		if (i < FRU_PRODUCT_AREA_TOTAL_FIELDS) {
> +			/* Stop when length is more then fields to record */
> +			if (data + len > limit)
> +				break;
> +
> +			/* This record type/len field */
> +			*data++ = *(u8 *)addr;
> +		}
> +
> +		/* Add offset to match data */
> +		addr += 1;
> +
> +		/* If len is 0 it means empty field that's why skip writing */
> +		if (!len)
> +			continue;
> +
> +		if (i < FRU_PRODUCT_AREA_TOTAL_FIELDS) {
> +			/* Record data field */
> +			memcpy(data, (u8 *)addr, len);
> +			term = data + (u8)len;
> +			*term = 0;
> +		} else {
> +			struct list_head *custom_fields;
> +
> +			custom_fields = &fru_data.prd.custom_fields;
> +
> +			/* Add a custom info field */
> +			if (fru_append_custom_info(addr - 1, custom_fields)) {
> +				fru_delete_custom_fields(custom_fields);
> +				return -EINVAL;
> +			}
> +		}
> +
> +		addr += len;
> +	}
> +
> +	if (i < FRU_PRODUCT_AREA_TOTAL_FIELDS) {
> +		printf("Product area require minimum %d fields\n",
> +		       FRU_PRODUCT_AREA_TOTAL_FIELDS);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>   static void fru_delete_multirecs(struct list_head *multi_recs)
>   {
>   	struct fru_multirec_node *node, *next;
> @@ -320,9 +465,11 @@ int fru_capture(const void *addr)
>   
>   	hdr = (struct fru_common_hdr *)addr;
>   	fru_delete_custom_fields(&fru_data.brd.custom_fields);
> +	fru_delete_custom_fields(&fru_data.prd.custom_fields);
>   	fru_delete_multirecs(&fru_data.multi_recs);
>   	memset((void *)&fru_data, 0, sizeof(fru_data));
>   	INIT_LIST_HEAD(&fru_data.brd.custom_fields);
> +	INIT_LIST_HEAD(&fru_data.prd.custom_fields);
>   	INIT_LIST_HEAD(&fru_data.multi_recs);
>   	memcpy((void *)&fru_data, (void *)hdr, sizeof(struct fru_common_hdr));
>   
> @@ -331,6 +478,9 @@ int fru_capture(const void *addr)
>   	if (hdr->off_board)
>   		fru_parse_board(addr + fru_cal_area_len(hdr->off_board));
>   
> +	if (hdr->off_product)
> +		fru_parse_product(addr + fru_cal_area_len(hdr->off_product));
> +
>   	if (hdr->off_multirec)
>   		fru_parse_multirec(addr + fru_cal_area_len(hdr->off_multirec));
>   
> @@ -341,13 +491,6 @@ int fru_capture(const void *addr)
>   
>   static int fru_display_board(struct fru_board_data *brd, int verbose)
>   {
> -	static const char * const typecode[] = {
> -		"Binary/Unspecified",
> -		"BCD plus",
> -		"6-bit ASCII",
> -		"8-bit ASCII",
> -		"2-byte UNICODE"
> -	};
>   	static const char * const boardinfo[] = {
>   		"Manufacturer Name",
>   		"Product Name",
> @@ -389,9 +532,9 @@ static int fru_display_board(struct fru_board_data *brd, int verbose)
>   		if (type <= FRU_TYPELEN_TYPE_ASCII8 &&
>   		    (brd->lang_code == FRU_LANG_CODE_ENGLISH ||
>   		     brd->lang_code == FRU_LANG_CODE_ENGLISH_1))
> -			debug("Type code: %s\n", typecode[type]);
> +			debug("Type code: %s\n", fru_typecode_str[type]);
>   		else
> -			debug("Type code: %s\n", typecode[type + 1]);
> +			debug("Type code: %s\n", fru_typecode_str[type + 1]);
>   
>   		if (!len) {
>   			debug("%s not found\n", boardinfo[i]);
> @@ -424,6 +567,79 @@ static int fru_display_board(struct fru_board_data *brd, int verbose)
>   	return 0;
>   }
>   
> +static int fru_display_product(struct fru_product_data *prd, int verbose)
> +{
> +	static const char * const productinfo[] = {
> +		"Manufacturer Name",
> +		"Product Name",
> +		"Part Number",
> +		"Version Number",
> +		"Serial Number",
> +		"Asset Number",
> +		"File ID",
> +	};
> +	struct fru_custom_field_node *node;
> +	u8 *data;
> +	u8 type;
> +	int len;
> +
> +	if (verbose) {
> +		printf("*****PRODUCT INFO*****\n");
> +		printf("Version:%d\n", fru_version(prd->ver));
> +		printf("Product Area Length:%d\n", fru_cal_area_len(prd->len));
> +	}
> +
> +	if (fru_check_language(prd->lang_code))
> +		return -EINVAL;
> +
> +	data = (u8 *)&prd->manufacturer_type_len;
> +
> +	for (u8 i = 0; i < (sizeof(productinfo) / sizeof(*productinfo)); i++) {
> +		len = fru_check_type_len(*data++, prd->lang_code,
> +					 &type);
> +		if (len == -EINVAL) {
> +			printf("**** EOF for Product Area ****\n");
> +			break;
> +		}
> +
> +		if (type <= FRU_TYPELEN_TYPE_ASCII8 &&
> +		    (prd->lang_code == FRU_LANG_CODE_ENGLISH ||
> +		     prd->lang_code == FRU_LANG_CODE_ENGLISH_1))
> +			debug("Type code: %s\n", fru_typecode_str[type]);
> +		else
> +			debug("Type code: %s\n", fru_typecode_str[type + 1]);
> +
> +		if (!len) {
> +			debug("%s not found\n", productinfo[i]);
> +			continue;
> +		}
> +
> +		switch (type) {
> +		case FRU_TYPELEN_TYPE_BINARY:
> +			debug("Length: %d\n", len);
> +			printf(" %s: 0x%x\n", productinfo[i], *data);
> +			break;
> +		case FRU_TYPELEN_TYPE_ASCII8:
> +			debug("Length: %d\n", len);
> +			printf(" %s: %s\n", productinfo[i], data);
> +			break;
> +		default:
> +			debug("Unsupported type %x\n", type);
> +		}
> +
> +		data += FRU_INFO_FIELD_LEN_MAX;
> +	}
> +
> +	list_for_each_entry(node, &prd->custom_fields, list) {
> +		printf(" Custom Type/Length: 0x%x\n", node->info.type_len);
> +		print_hex_dump_bytes("  ", DUMP_PREFIX_OFFSET, node->info.data,
> +				     node->info.type_len &
> +				     FRU_TYPELEN_LEN_MASK);
> +	}
> +
> +	return 0;
> +}
> +
>   static int fru_display_multirec(struct list_head *multi_recs, int verbose)
>   {
>   	struct fru_multirec_node *node;
> @@ -495,6 +711,10 @@ int fru_display(int verbose)
>   	if (ret)
>   		return ret;
>   
> +	ret = fru_display_product(&fru_data.prd, verbose);
> +	if (ret)
> +		return ret;
> +
>   	return fru_display_multirec(&fru_data.multi_recs, verbose);
>   }
>   

M