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

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

Hello Michal,

On 9/22/2022 12:19 AM, Michal Simek wrote:
> Hi,
> 
> On 9/22/22 08:39, Jae Hyun Yoo wrote:
>> 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.
> 
> No doubt that that it can stay here. It is more about to have just small 
> patches which is much much easier to review. It means if one patch moves 
> this structure to common location for using for product area generation. 
> I need to review 20 LOC. And this patch is smaller which is again easier 
> to review without distraction from obvious/easy going changes.

Okay. I'll split this small piece as a separate one.

Thanks,

Jae