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

[Jae Hyun Yoo <quic_jaehyoo@quicinc.com>]

Hello Michal,

On 9/21/2022 6:52 AM, Michal Simek wrote:
> 
> 
> 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.

I agree with you. I'll add the old command back in the next version and
will keep for 3-4 releases before deprecating the command.

>>       "                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.

Yes, that makes sense. I'll change these commands to 'gen_board' and
'gen_product' with adding back the previous command 'board_gen' to keep
its compatibility. A command which can generate both board and product
into a single FRU could be added later using a separate series.

>>       ;
>>   #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.

It's just moved out from 'fru_display_board' function because it can be
used for both 'fru_display_board' and 'fru_display_product' functions.
Definately it's a part of this patch and I don't think that it should be
submitted using a seperate patch.

Thank you,

Jae

> 
>> +
>>   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 = (structfru_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 byteequals 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 currentlength */
>> +    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 overallocated 
>> 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 requireminimum %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