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

[Heinrich Schuchardt]

On 05.05.20 18:02, Simon Glass wrote:
> 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.


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
>