On 05.05.20 18:02, Simon Glass wrote: > Hi Rayagonda, > > On Tue, 5 May 2020 at 06:13, Rayagonda Kokatanur > <rayagonda.kokata...@broadcom.com> wrote: >> >> From: Corneliu Doban <cdo...@broadcom.com> >> >> Add eMMC and GPT support. >> - GPT partition list and command to create the GPT added to u-boot >> environment >> - eMMC boot commands added to u-boot environment >> - new gpt commands (enumarate and setenv) that are used by broadcom >> update scripts and boot commands >> - eMMC specific u-boot configurations with environment saved in eMMC >> and GPT support >> >> Signed-off-by: Corneliu Doban <cdo...@broadcom.com> >> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokata...@broadcom.com> >> >> diff --git a/cmd/gpt.c b/cmd/gpt.c >> index b8d11c167d..c32e272b25 100644 >> --- a/cmd/gpt.c >> +++ b/cmd/gpt.c >> @@ -616,6 +616,87 @@ static int gpt_verify(struct blk_desc *blk_dev_desc, >> const char *str_part) >> return ret; >> } >> >> +/* >> + * Enumerate partition names into environment variable. >> + */ > > Can you check the style for these comments? You can see examples in > bootcount.h. Please fix for all functions.
Unfortunately bootcount.h itself is in bad shape. Please, use https://www.kernel.org/doc/html/v4.10/doc-guide/kernel-doc.html#function-documentation as reference. Best regards Heinrich > > It should mention the params and return value. > > Also in this case it should explain which environment variable is > updated and the format that is used in the variable. > >> +static int gpt_enumerate(struct blk_desc *blk_dev_desc) >> +{ >> + disk_partition_t pinfo; >> + struct part_driver *first_drv = >> + ll_entry_start(struct part_driver, part_driver); >> + const int n_drvs = ll_entry_count(struct part_driver, part_driver); > > Can you create functions in part.h to handle this? Something like: > > int part_driver_get_count() > struct part_driver *part_driver_get((int n); > > Then we don't have lots of places accessing this. > >> + struct part_driver *part_drv; >> + char part_list[2048]; >> + >> + part_list[0] = 0; >> + >> + for (part_drv = first_drv; part_drv != first_drv + n_drvs; >> part_drv++) { >> + int ret; >> + int i; >> + >> + for (i = 1; i < part_drv->max_entries; i++) { >> + ret = part_drv->get_info(blk_dev_desc, i, &pinfo); >> + if (ret != 0) { > > if (ret) > >> + /* no more entries in table */ >> + break; >> + } >> + strcat(part_list, (const char *)pinfo.name); >> + strcat(part_list, " "); > > Need to check that you don't go past sizeof(part_list). Can I suggest > that you have a ptr to the end of your string to avoid n^2 behaviour > here? > >> + } >> + } >> + if (strlen(part_list) > 0) > > if (*part_list) > >> + part_list[strlen(part_list) - 1] = 0; >> + debug("setenv gpt_partition_list %s\n", part_list); >> + env_set("gpt_partition_list", part_list); > > Please check return value > > > blank line here > >> + return 0; >> +} >> + >> +/* >> + * Dynamically setup environment variables for name, index, offset and size >> + * for partition in GPT table after running "gpt setenv" for a partition >> name. >> + * gpt_partition_name, gpt_partition_entry, gpt_partition_addr and >> + * gpt_partition_size environment variables will be set. >> + */ >> +static int gpt_setenv(struct blk_desc *blk_dev_desc, const char *name) >> +{ >> + disk_partition_t pinfo; >> + struct part_driver *first_drv = >> + ll_entry_start(struct part_driver, part_driver); >> + const int n_drvs = ll_entry_count(struct part_driver, part_driver); >> + struct part_driver *part_drv; >> + char buf[32]; > > put pinfo and buf inside loop? > >> + >> + for (part_drv = first_drv; part_drv != first_drv + n_drvs; >> part_drv++) { >> + int ret; >> + int i; >> + >> + for (i = 1; i < part_drv->max_entries; i++) { >> + ret = part_drv->get_info(blk_dev_desc, i, &pinfo); >> + if (ret != 0) { >> + /* no more entries in table */ >> + break; >> + } >> + if (strcmp(name, (const char *)pinfo.name) == 0) { >> + /* match found, setup environment variables >> */ >> + sprintf(buf, LBAF, pinfo.start); >> + debug("setenv gpt_partition_addr %s\n", buf); >> + env_set("gpt_partition_addr", buf); >> + sprintf(buf, LBAF, pinfo.size); >> + debug("setenv gpt_partition_size %s\n", buf); >> + env_set("gpt_partition_size", buf); >> + sprintf(buf, "%d", i); >> + debug("setenv gpt_partition_entry %s\n", >> buf); >> + env_set("gpt_partition_entry", buf); >> + sprintf(buf, "%s", pinfo.name); >> + debug("setenv gpt_partition_name %s\n", buf); >> + env_set("gpt_partition_name", buf); > > Need to check return values of env_set() > >> + return 0; >> + } >> + } >> + } >> + return -1; > > That is -EPERM. Perhaps -ENOENT? > >> +} >> + >> static int do_disk_guid(struct blk_desc *dev_desc, char * const namestr) >> { >> int ret; >> @@ -822,6 +903,10 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, >> char * const argv[]) >> } else if ((strcmp(argv[1], "verify") == 0)) { >> ret = gpt_verify(blk_dev_desc, argv[4]); >> printf("Verify GPT: "); >> + } else if ((strcmp(argv[1], "setenv") == 0)) { >> + ret = gpt_setenv(blk_dev_desc, argv[4]); >> + } else if ((strcmp(argv[1], "enumerate") == 0)) { >> + ret = gpt_enumerate(blk_dev_desc); >> } else if (strcmp(argv[1], "guid") == 0) { >> ret = do_disk_guid(blk_dev_desc, argv[4]); >> #ifdef CONFIG_CMD_GPT_RENAME >> @@ -852,7 +937,17 @@ U_BOOT_CMD(gpt, CONFIG_SYS_MAXARGS, 1, do_gpt, >> " to interface\n" >> " Example usage:\n" >> " gpt write mmc 0 $partitions\n" >> + " - write the GPT to device\n" >> " gpt verify mmc 0 $partitions\n" >> + " - verify the GPT on device against $partitions\n" >> + " gpt setenv mmc 0 $name\n" >> + " - setup environment variables for partition $name:\n" >> + " gpt_partition_addr, gpt_partition_size,\n" >> + " gpt_partition_name, gpt_partition_entry\n" >> + " gpt enumerate mmc 0\n" >> + " - store list of partitions to gpt_partition_list environment >> variable\n" >> + " read <interface> <dev>\n" >> + " - read GPT into a data structure for manipulation\n" >> " gpt guid <interface> <dev>\n" >> " - print disk GUID\n" >> " gpt guid <interface> <dev> <varname>\n" >> -- >> 2.17.1 >> > > It would be good to have a test for this, but I don't think anything > exists at present. > > +Heinrich Schuchardt might know > > Regards, > SImon >