[U-Boot,RFC,v2,6/8] cmd: mtdparts: update command to support GPT over MTD

add new subcommand :
mtdparts gpt <mtd-dev> [<GUID>]

extract mtd partition from GPT header present in MTD device
> mtdparts gpt nand0
> mtdparts gpt nor0

extract mtd partitions only for some GUID
> mtdparts gpt nand0 data
> mtdparts gpt nor0 0FC63DAF-8483-4772-8E79-3D69D8477DE4

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
Signed-off-by: Patrick Delaunay <patrick.delaunay73@gmail.com>
---

Changes in v2: None

 cmd/mtdparts.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 119 insertions(+), 2 deletions(-)

Hi Patrick,

On 30 November 2016 at 04:01, Patrick Delaunay
<patrick.delaunay73@gmail.com> wrote:
> add new subcommand :
> mtdparts gpt <mtd-dev> [<GUID>]
>
> extract mtd partition from GPT header present in MTD device
>> mtdparts gpt nand0
>> mtdparts gpt nor0
>
> extract mtd partitions only for some GUID
>> mtdparts gpt nand0 data
>> mtdparts gpt nor0 0FC63DAF-8483-4772-8E79-3D69D8477DE4
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> Signed-off-by: Patrick Delaunay <patrick.delaunay73@gmail.com>
> ---
>
> Changes in v2: None
>
>  cmd/mtdparts.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 119 insertions(+), 2 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

But please see nits below.

> diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
> index b9b160d..035a7ec 100644
> --- a/cmd/mtdparts.c
> +++ b/cmd/mtdparts.c
> @@ -121,6 +121,8 @@ extern void board_mtdparts_default(const char **mtdids, const char **mtdparts);
>  static const char *mtdids_default = MTDIDS_DEFAULT;
>  static const char *mtdparts_default = MTDPARTS_DEFAULT;
>
> +#define PART_ADD_DESC_MAXLEN 64
> +

This change should really be in a new patch, and it would be good to
have a comment as to what this is.

>  /* copies of last seen 'mtdids', 'mtdparts' and 'partition' env variables */
>  #define MTDIDS_MAXLEN          128
>  #define MTDPARTS_MAXLEN                512
> @@ -1891,6 +1893,90 @@ static struct part_info* mtd_part_info(struct mtd_device *dev, unsigned int part
>         return NULL;
>  }
>
> +#ifdef CONFIG_EFI_PARTITION_MTD
> +/**
> + * extract MTD info from GPT information
> + *
> + * @param device string to select GPT device
> + * @param p_guid_filter guid for filtering add
> + *
> + * @return 0 on success, 1 otherwise

I think it would be better to return a useful error number from
errno.h and have the caller turn it into CMD_RET_FAILURE.

> + */
> +
> +static int mtdparts_gpt(char *device, char *p_guid_filter)
> +{
> +       char tmpbuf[PART_ADD_DESC_MAXLEN];
> +       u8 type, num;
> +       struct mtdids *id;
> +       int part_id;
> +       struct mtd_info *mtd;
> +       struct mtd_device *dev;
> +       struct mtd_device *dev_tmp;
> +       disk_partition_t info;
> +       struct part_info *p;
> +
> +       if (mtd_id_parse(device, NULL, &type, &num) != 0) {
> +               printf("invalid MTD device %s\n", device);
> +               return 1;
> +       }
> +
> +       /* this may be the first run, initialize lists if needed
> +          and make sure we are in sync with env variables */
> +       mtdparts_init();
> +       /* don't treat error (mtdparts can be empty) */
> +
> +       id = id_find(type, num);
> +       if (id == NULL) {
> +               printf("no such device %s defined in mtdids variable\n",
> +                      device);
> +               return 1;
> +       }
> +
> +       if (get_mtd_info(type, num, &mtd) || (mtd == NULL)) {
> +               printf("no such MTD device %s\n", device);
> +               return 1;
> +       }
> +
> +       for (part_id = 1; part_id <= GPT_ENTRY_NUMBERS; part_id++) {
> +               if (part_get_info_efi_mtd(mtd, part_id, &info))
> +                       break; /* Stop for first non valid partition */
> +
> +#ifdef CONFIG_PARTITION_TYPE_GUID
> +               if (p_guid_filter &&
> +                   strcmp(p_guid_filter, info.type_guid))
> +                       continue;
> +#endif
> +
> +               sprintf(tmpbuf, "%s:0x%llx@0x%llx(%s)",

Can you use snprintf() to be safe?

> +                       id->mtd_id,
> +                       (u64)(info.size * info.blksz),
> +                       (u64)(info.start * info.blksz),
> +                       info.name);
> +               debug("add tmpbuf: %s\n", tmpbuf);
> +
> +               if ((device_parse(tmpbuf, NULL, &dev) != 0) || (!dev))
> +                       return 1;
> +
> +               debug("+ %s\t%d\t%s\n", MTD_DEV_TYPE(type), num,
> +                     id->mtd_id);
> +
> +               p = list_entry(dev->parts.next, struct part_info, link);
> +
> +               dev_tmp = device_find(type, num);
> +
> +               if (dev_tmp == NULL)
> +                       device_add(dev);
> +               else if (part_add(dev_tmp, p) != 0)
> +                       return 1;
> +       }
> +       if (generate_mtdparts_save(last_parts, MTDPARTS_MAXLEN) != 0) {
> +               printf("generated mtdparts too long, resetting to null\n");
> +               return 1;
> +       }
> +       return 0;
> +}
> +#endif
> +
>  /***************************************************/
>  /* U-Boot commands                                */
>  /***************************************************/
> @@ -1966,6 +2052,30 @@ static int do_mtdparts(cmd_tbl_t *cmdtp, int flag, int argc,
>                 }
>         }
>
> +#ifdef CONFIG_EFI_PARTITION_MTD
> +       /* mtdparts gpt <mtd-dev> [GUID] */
> +       if ((argc == 3 || argc == 4) && (strncmp(argv[1], "gpt", 3) == 0)) {
> +#ifdef CONFIG_PARTITION_TYPE_GUID
> +               char guid_str[UUID_STR_LEN + 1];
> +               char *p_guid_filter = NULL;
> +#endif
> +
> +               if (argc == 4) {
> +#ifdef CONFIG_PARTITION_TYPE_GUID
> +                       p_guid_filter = guid_str;
> +                       if (uuid_guid_parse_str(argv[3], guid_str)) {
> +                               printf("invalid type gui %s\n", argv[3]);
> +                               return 1;
> +                       }
> +                       debug("filtered type gui %s (%s)\n", argv[3], guid_str);
> +#else
> +                       puts("type GUI not support\n");

printf()

Should this be GUID?

> +#endif
> +               }
> +
> +               return mtdparts_gpt(argv[2], p_guid_filter);
> +       }
> +#endif
>         /* make sure we are in sync with env variables */
>         if (mtdparts_init() != 0)
>                 return 1;
> @@ -1977,7 +2087,6 @@ static int do_mtdparts(cmd_tbl_t *cmdtp, int flag, int argc,
>
>         /* mtdparts add <mtd-dev> <size>[@<offset>] <name> [ro] */
>         if (((argc == 5) || (argc == 6)) && (strncmp(argv[1], "add", 3) == 0)) {
> -#define PART_ADD_DESC_MAXLEN 64
>                 char tmpbuf[PART_ADD_DESC_MAXLEN];
>  #if defined(CONFIG_CMD_MTDPARTS_SPREAD)
>                 struct mtd_info *mtd;
> @@ -2083,6 +2192,10 @@ static char mtdparts_help_text[] =
>         "mtdparts add.spread <mtd-dev> <size>[@<offset>] [<name>] [ro]\n"
>         "    - add partition, padding size by skipping bad blocks\n"
>  #endif
> +#ifdef CONFIG_EFI_PARTITION_MTD
> +       "mtdparts gpt <mtd-dev> [<GUID>]\n"
> +       "    - add partitions for device from gpt, filtered by GUID type\n"
> +#endif
>         "mtdparts default\n"
>         "    - reset partition table to defaults\n"
>  #if defined(CONFIG_CMD_MTDPARTS_SPREAD)
> @@ -2112,7 +2225,11 @@ static char mtdparts_help_text[] =
>         "<size>     := standard linux memsize OR '-' to denote all remaining space\n"
>         "<offset>   := partition start offset within the device\n"
>         "<name>     := '(' NAME ')'\n"
> -       "<ro-flag>  := when set to 'ro' makes partition read-only (not used, passed to kernel)";
> +       "<ro-flag>  := when set to 'ro' makes partition read-only (not used, passed to kernel)"
> +#ifdef CONFIG_EFI_PARTITION_MTD
> +       "<GUID>     := partition guid"
> +#endif
> +       ;
>  #endif
>
>  U_BOOT_CMD(
> --
> 1.9.1
>

Regards,
Simon