| Message ID | 20220825164245.1606958-5-quic_jaehyoo@quicinc.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | cmd/fru: move FRU handling support to common region | expand |
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
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
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. Thanks, Michal
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
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" " 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" ; #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" +}; + 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); }
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(-)