[PATCH] cmd: gpt: add eMMC and GPT support

[Simon Glass]

Hi Rayagonda,

On Tue, 5 May 2020 at 06:13, Rayagonda Kokatanur
<rayagonda.kokatanur at broadcom.com> wrote:
>
> From: Corneliu Doban <cdoban at 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 <cdoban at broadcom.com>
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur at 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.

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