[U-Boot] [PATCH V2 1/4] disk: part_efi: range-check partition number

[U-Boot] [PATCH V2 1/4] disk: part_efi: range-check partition number

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Enhance get_partition_info_efi() to range-check the partition number.
This prevents invalid partitions being accessed, and prevents access
beyond the end of the gpt_pte[] array.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v2: New patch
---
 disk/part_efi.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/disk/part_efi.c b/disk/part_efi.c
index 02927a0..2962fd8 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -173,6 +173,13 @@ int get_partition_info_efi(block_dev_desc_t * dev_desc, int part,
 		return -1;
 	}
 
+	if (part > le32_to_int(gpt_head->num_partition_entries) ||
+	    !is_pte_valid(&gpt_pte[part - 1])) {
+		printf("%s: *** ERROR: Invalid partition number %d ***\n",
+			__func__, part);
+		return -1;
+	}
+
 	/* The ulong casting limits the maximum disk size to 2 TB */
 	info->start = (ulong) le64_to_int(gpt_pte[part - 1].starting_lba);
 	/* The ending LBA is inclusive, to calculate size, add 1 to it */
-- 
1.7.0.4

[U-Boot] [PATCH V2 2/4] disk: part_efi: parse and store partition UUID

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Each EFI partition table entry contains a UUID. Extend U-Boot's struct
disk_partition to be able to store this information, and modify
get_partition_info_efi() to fill it in.

The implementation of uuid_string() was stolen from the Linux kernel.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v2: Add #ifdef CONFIG_PARTITION_UUIDS around all new code and struct fields.
---
 disk/part.c     |    5 +++++
 disk/part_efi.c |   25 +++++++++++++++++++++++++
 include/part.h  |    3 +++
 3 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/disk/part.c b/disk/part.c
index 76f3939..db422c4 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -294,6 +294,11 @@ void init_part (block_dev_desc_t * dev_desc)
 int get_partition_info (block_dev_desc_t *dev_desc, int part
 					, disk_partition_t *info)
 {
+#ifdef CONFIG_PARTITION_UUIDS
+	/* The common case is no UUID support */
+	info->uuid[0] = 0;
+#endif
+
 	switch (dev_desc->part_type) {
 #ifdef CONFIG_MAC_PARTITION
 	case PART_TYPE_MAC:
diff --git a/disk/part_efi.c b/disk/part_efi.c
index 2962fd8..264ea9c 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -154,6 +154,28 @@ void print_part_efi(block_dev_desc_t * dev_desc)
 	return;
 }
 
+#ifdef CONFIG_PARTITION_UUIDS
+static void uuid_string(unsigned char *uuid, char *str)
+{
+	static const u8 le[16] = {3, 2, 1, 0, 5, 4, 7, 6, 8, 9, 10, 11,
+				  12, 13, 14, 15};
+	int i;
+
+	for (i = 0; i < 16; i++) {
+		sprintf(str, "%02x", uuid[le[i]]);
+		str += 2;
+		switch (i) {
+		case 3:
+		case 5:
+		case 7:
+		case 9:
+			*str++ = '-';
+			break;
+		}
+	}
+}
+#endif
+
 int get_partition_info_efi(block_dev_desc_t * dev_desc, int part,
 				disk_partition_t * info)
 {
@@ -190,6 +212,9 @@ int get_partition_info_efi(block_dev_desc_t * dev_desc, int part,
 	sprintf((char *)info->name, "%s",
 			print_efiname(&gpt_pte[part - 1]));
 	sprintf((char *)info->type, "U-Boot");
+#ifdef CONFIG_PARTITION_UUIDS
+	uuid_string(gpt_pte[part - 1].unique_partition_guid.b, info->uuid);
+#endif
 
 	debug("%s: start 0x%lX, size 0x%lX, name %s", __func__,
 		info->start, info->size, info->name);
diff --git a/include/part.h b/include/part.h
index e1478f4..fde320a 100644
--- a/include/part.h
+++ b/include/part.h
@@ -93,6 +93,9 @@ typedef struct disk_partition {
 	ulong	blksz;		/* block size in bytes			*/
 	uchar	name[32];	/* partition name			*/
 	uchar	type[32];	/* string type description		*/
+#ifdef CONFIG_PARTITION_UUIDS
+	char	uuid[37];	/* filesystem UUID as string, if exists	*/
+#endif
 } disk_partition_t;
 
 /* Misc _get_dev functions */
-- 
1.7.0.4

[U-Boot] [PATCH V2 2/4] disk: part_efi: parse and store partition UUID

[Tom Rini]

On Wed, Sep 05, 2012 at 04:03:42PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Each EFI partition table entry contains a UUID. Extend U-Boot's struct
> disk_partition to be able to store this information, and modify
> get_partition_info_efi() to fill it in.
> 
> The implementation of uuid_string() was stolen from the Linux kernel.

Don't say "stolen", say borrowed and cite a release and file path.
Thanks!

-- 
Tom

[U-Boot] [PATCH V2 3/4] disk: part_msdos: parse and store partition UUID

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

The MSDOS/MBR partition table includes a 32-bit unique ID, often referred
to as the NT disk signature. When combined with a partition number within
the table, this can form a unique ID similar in concept to EFI/GPT's
partition UUID.

This patch generates UUIDs in the format 0002dd75-01, which matches the
format expected by the Linux kernel.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v2: New patch.
---
 disk/part_dos.c |   15 ++++++++++++---
 disk/part_dos.h |    2 +-
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/disk/part_dos.c b/disk/part_dos.c
index a43dd9c..f9b7931 100644
--- a/disk/part_dos.c
+++ b/disk/part_dos.c
@@ -163,7 +163,8 @@ static void print_partition_extended (block_dev_desc_t *dev_desc, int ext_part_s
  */
 static int get_partition_info_extended (block_dev_desc_t *dev_desc, int ext_part_sector,
 				 int relative, int part_num,
-				 int which_part, disk_partition_t *info)
+				 int which_part, disk_partition_t *info,
+				 unsigned int disksig)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);
 	dos_partition_t *pt;
@@ -182,6 +183,11 @@ static int get_partition_info_extended (block_dev_desc_t *dev_desc, int ext_part
 		return -1;
 	}
 
+#ifdef CONFIG_PARTITION_UUIDS
+	if (!ext_part_sector)
+		disksig = le32_to_int(&buffer[DOS_PART_DISKSIG_OFFSET]);
+#endif
+
 	/* Print all primary/logical partitions */
 	pt = (dos_partition_t *) (buffer + DOS_PART_TBL_OFFSET);
 	for (i = 0; i < 4; i++, pt++) {
@@ -222,6 +228,9 @@ static int get_partition_info_extended (block_dev_desc_t *dev_desc, int ext_part
 			}
 			/* sprintf(info->type, "%d, pt->sys_ind); */
 			sprintf ((char *)info->type, "U-Boot");
+#ifdef CONFIG_PARTITION_UUIDS
+			sprintf(info->uuid, "%08x-%02x", disksig, part_num);
+#endif
 			return 0;
 		}
 
@@ -240,7 +249,7 @@ static int get_partition_info_extended (block_dev_desc_t *dev_desc, int ext_part
 
 			return get_partition_info_extended (dev_desc, lba_start,
 				 ext_part_sector == 0 ? lba_start : relative,
-				 part_num, which_part, info);
+				 part_num, which_part, info, disksig);
 		}
 	}
 	return -1;
@@ -254,7 +263,7 @@ void print_part_dos (block_dev_desc_t *dev_desc)
 
 int get_partition_info_dos (block_dev_desc_t *dev_desc, int part, disk_partition_t * info)
 {
-	return get_partition_info_extended (dev_desc, 0, 0, 1, part, info);
+	return get_partition_info_extended(dev_desc, 0, 0, 1, part, info, 0);
 }
 
 
diff --git a/disk/part_dos.h b/disk/part_dos.h
index de75542..7b77c1d 100644
--- a/disk/part_dos.h
+++ b/disk/part_dos.h
@@ -24,7 +24,7 @@
 #ifndef _DISK_PART_DOS_H
 #define _DISK_PART_DOS_H
 
-
+#define DOS_PART_DISKSIG_OFFSET	0x1b8
 #define DOS_PART_TBL_OFFSET	0x1be
 #define DOS_PART_MAGIC_OFFSET	0x1fe
 #define DOS_PBR_FSTYPE_OFFSET	0x36
-- 
1.7.0.4

[U-Boot] [PATCH V2 4/4] cmd_part: add partition-related command

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This implements the following:

part uuid mmc 0:1
  -> print partition UUID
part uuid mmc 0:1 uuid
  -> set environment variable to partition UUID

This can be useful when writing a bootcmd which searches all known
devices for something bootable, and then wants the kernel to use the
same partition as the root device, e.g.:

part uuid ${devtype} ${devnum}:${rootpart} uuid
setenv bootargs root=PARTUUID=${uuid} ...

It is expected that further part sub-commands will be added later, e.g.
to find which partition on a disk is marked bootable, to write new
partition tables to disk, etc.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v2: validate that CONFIG_PARTITION_UUID is defined when CONFIG_CMD_PART is

Note: If Rob Herring's proposed patch "disk/part: introduce
get_device_and_partition" is applied, the body of do_partuuid() should
be reworked to use Rob's new function get_device_and_partition().
---
 common/Makefile   |    1 +
 common/cmd_part.c |  104 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+), 0 deletions(-)
 create mode 100644 common/cmd_part.c

diff --git a/common/Makefile b/common/Makefile
index 3d62775..449b390 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -129,6 +129,7 @@ COBJS-$(CONFIG_CMD_NAND) += cmd_nand.o
 COBJS-$(CONFIG_CMD_NET) += cmd_net.o
 COBJS-$(CONFIG_CMD_ONENAND) += cmd_onenand.o
 COBJS-$(CONFIG_CMD_OTP) += cmd_otp.o
+COBJS-$(CONFIG_CMD_PART) += cmd_part.o
 ifdef CONFIG_PCI
 COBJS-$(CONFIG_CMD_PCI) += cmd_pci.o
 endif
diff --git a/common/cmd_part.c b/common/cmd_part.c
new file mode 100644
index 0000000..1b15ae9
--- /dev/null
+++ b/common/cmd_part.c
@@ -0,0 +1,104 @@
+/*
+ * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * made from cmd_ext2, which was:
+ *
+ * (C) Copyright 2004
+ * esd gmbh <www.esd-electronics.com>
+ * Reinhard Arlt <reinhard.arlt@esd-electronics.com>
+ *
+ * made from cmd_reiserfs by
+ *
+ * (C) Copyright 2003 - 2004
+ * Sysgo Real-Time Solutions, AG <www.elinos.com>
+ * Pavel Bartusek <pba@sysgo.com>
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <common.h>
+#include <config.h>
+#include <command.h>
+#include <part.h>
+#include <vsprintf.h>
+
+#ifndef CONFIG_PARTITION_UUIDS
+#error CONFIG_PARTITION_UUIDS must be enabled for CONFIG_CMD_PART to be enabled
+#endif
+
+int do_partuuid(int argc, char * const argv[])
+{
+	int dev;
+	int part;
+	char *ep;
+	block_dev_desc_t *dev_desc;
+	disk_partition_t info;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+	if (argc > 3)
+		return CMD_RET_USAGE;
+
+	dev = (int)simple_strtoul(argv[1], &ep, 16);
+	dev_desc = get_dev(argv[0], dev);
+	if (dev_desc == NULL) {
+		printf("Block device %s %d not supported\n", argv[0], dev);
+		return 1;
+	}
+
+	if (*ep) {
+		if (*ep != ':') {
+			puts("Invalid device; use dev[:part]\n");
+			return 1;
+		}
+		part = (int)simple_strtoul(++ep, NULL, 16);
+	} else {
+		part = 1;
+	}
+
+	if (get_partition_info(dev_desc, part, &info)) {
+		printf("Bad partition %d\n", part);
+		return 1;
+	}
+
+	if (argc > 2)
+		setenv(argv[2], info.uuid);
+	else
+		printf("%s\n", info.uuid);
+
+	return 0;
+}
+
+int do_part(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	if (!strcmp(argv[1], "uuid"))
+		return do_partuuid(argc - 2, argv + 2);
+
+	return CMD_RET_USAGE;
+}
+
+U_BOOT_CMD(
+	part,	5,	1,	do_part,
+	"disk partition related commands",
+	"part uuid <interface> <dev[:part]>\n"
+	"    - print partition UUID\n"
+	"part uuid <interface> <dev[:part]> <varname>\n"
+	"    - set environment variable to partition UUID"
+);
-- 
1.7.0.4

[U-Boot] [PATCH V2 4/4] cmd_part: add partition-related command

[Rob Herring]

On 09/05/2012 05:03 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> This implements the following:
> 
> part uuid mmc 0:1
>   -> print partition UUID
> part uuid mmc 0:1 uuid
>   -> set environment variable to partition UUID

What's the reason to not always both print out and set the uuid env var?

Perhaps the env name should be partuuid or part_uuid as you could have
uuid's for other purposes?

> 
> This can be useful when writing a bootcmd which searches all known
> devices for something bootable, and then wants the kernel to use the
> same partition as the root device, e.g.:
> 
> part uuid ${devtype} ${devnum}:${rootpart} uuid
> setenv bootargs root=PARTUUID=${uuid} ...
> 
> It is expected that further part sub-commands will be added later, e.g.
> to find which partition on a disk is marked bootable, to write new
> partition tables to disk, etc.

A list command would be useful and would be better located here than
under scsi or other interface commands. Perhaps instead of printing a
single part uuid, you should make a list command that prints all
partitions and their UUIDs. That would address my first question.

Rob

> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> v2: validate that CONFIG_PARTITION_UUID is defined when CONFIG_CMD_PART is
> 
> Note: If Rob Herring's proposed patch "disk/part: introduce
> get_device_and_partition" is applied, the body of do_partuuid() should
> be reworked to use Rob's new function get_device_and_partition().
> ---
>  common/Makefile   |    1 +
>  common/cmd_part.c |  104 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 105 insertions(+), 0 deletions(-)
>  create mode 100644 common/cmd_part.c
> 
> diff --git a/common/Makefile b/common/Makefile
> index 3d62775..449b390 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -129,6 +129,7 @@ COBJS-$(CONFIG_CMD_NAND) += cmd_nand.o
>  COBJS-$(CONFIG_CMD_NET) += cmd_net.o
>  COBJS-$(CONFIG_CMD_ONENAND) += cmd_onenand.o
>  COBJS-$(CONFIG_CMD_OTP) += cmd_otp.o
> +COBJS-$(CONFIG_CMD_PART) += cmd_part.o
>  ifdef CONFIG_PCI
>  COBJS-$(CONFIG_CMD_PCI) += cmd_pci.o
>  endif
> diff --git a/common/cmd_part.c b/common/cmd_part.c
> new file mode 100644
> index 0000000..1b15ae9
> --- /dev/null
> +++ b/common/cmd_part.c
> @@ -0,0 +1,104 @@
> +/*
> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * made from cmd_ext2, which was:
> + *
> + * (C) Copyright 2004
> + * esd gmbh <www.esd-electronics.com>
> + * Reinhard Arlt <reinhard.arlt@esd-electronics.com>
> + *
> + * made from cmd_reiserfs by
> + *
> + * (C) Copyright 2003 - 2004
> + * Sysgo Real-Time Solutions, AG <www.elinos.com>
> + * Pavel Bartusek <pba@sysgo.com>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <common.h>
> +#include <config.h>
> +#include <command.h>
> +#include <part.h>
> +#include <vsprintf.h>
> +
> +#ifndef CONFIG_PARTITION_UUIDS
> +#error CONFIG_PARTITION_UUIDS must be enabled for CONFIG_CMD_PART to be enabled
> +#endif
> +
> +int do_partuuid(int argc, char * const argv[])
> +{
> +	int dev;
> +	int part;
> +	char *ep;
> +	block_dev_desc_t *dev_desc;
> +	disk_partition_t info;
> +
> +	if (argc < 2)
> +		return CMD_RET_USAGE;
> +	if (argc > 3)
> +		return CMD_RET_USAGE;
> +
> +	dev = (int)simple_strtoul(argv[1], &ep, 16);
> +	dev_desc = get_dev(argv[0], dev);
> +	if (dev_desc == NULL) {
> +		printf("Block device %s %d not supported\n", argv[0], dev);
> +		return 1;
> +	}
> +
> +	if (*ep) {
> +		if (*ep != ':') {
> +			puts("Invalid device; use dev[:part]\n");
> +			return 1;
> +		}
> +		part = (int)simple_strtoul(++ep, NULL, 16);
> +	} else {
> +		part = 1;
> +	}
> +
> +	if (get_partition_info(dev_desc, part, &info)) {
> +		printf("Bad partition %d\n", part);
> +		return 1;
> +	}
> +
> +	if (argc > 2)
> +		setenv(argv[2], info.uuid);
> +	else
> +		printf("%s\n", info.uuid);
> +
> +	return 0;
> +}
> +
> +int do_part(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	if (argc < 2)
> +		return CMD_RET_USAGE;
> +
> +	if (!strcmp(argv[1], "uuid"))
> +		return do_partuuid(argc - 2, argv + 2);
> +
> +	return CMD_RET_USAGE;
> +}
> +
> +U_BOOT_CMD(
> +	part,	5,	1,	do_part,
> +	"disk partition related commands",
> +	"part uuid <interface> <dev[:part]>\n"
> +	"    - print partition UUID\n"
> +	"part uuid <interface> <dev[:part]> <varname>\n"
> +	"    - set environment variable to partition UUID"
> +);
> 

[U-Boot] [PATCH V2 4/4] cmd_part: add partition-related command

[Tom Rini]

On Wed, Sep 05, 2012 at 06:51:58PM -0500, Rob Herring wrote:
> On 09/05/2012 05:03 PM, Stephen Warren wrote:
> > From: Stephen Warren <swarren@nvidia.com>
> > 
> > This implements the following:
> > 
> > part uuid mmc 0:1
> >   -> print partition UUID
> > part uuid mmc 0:1 uuid
> >   -> set environment variable to partition UUID
> 
> What's the reason to not always both print out and set the uuid env var?
> 
> Perhaps the env name should be partuuid or part_uuid as you could have
> uuid's for other purposes?
> 
> > 
> > This can be useful when writing a bootcmd which searches all known
> > devices for something bootable, and then wants the kernel to use the
> > same partition as the root device, e.g.:
> > 
> > part uuid ${devtype} ${devnum}:${rootpart} uuid
> > setenv bootargs root=PARTUUID=${uuid} ...
> > 
> > It is expected that further part sub-commands will be added later, e.g.
> > to find which partition on a disk is marked bootable, to write new
> > partition tables to disk, etc.
> 
> A list command would be useful and would be better located here than
> under scsi or other interface commands. Perhaps instead of printing a
> single part uuid, you should make a list command that prints all
> partitions and their UUIDs. That would address my first question.

Sounds like a good idea to me as well.

[snip]
> > Signed-off-by: Stephen Warren <swarren@nvidia.com>
> > ---
> > v2: validate that CONFIG_PARTITION_UUID is defined when CONFIG_CMD_PART is
> > 
> > Note: If Rob Herring's proposed patch "disk/part: introduce
> > get_device_and_partition" is applied, the body of do_partuuid() should
> > be reworked to use Rob's new function get_device_and_partition().

I think the best idea here would be to make the next version just depend
on Rob's series.

-- 
Tom

[U-Boot] [PATCH V2 4/4] cmd_part: add partition-related command

[Stephen Warren]

On 09/05/2012 05:58 PM, Tom Rini wrote:
> On Wed, Sep 05, 2012 at 06:51:58PM -0500, Rob Herring wrote:
>> On 09/05/2012 05:03 PM, Stephen Warren wrote:
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> This implements the following:
>>>
>>> part uuid mmc 0:1
>>>   -> print partition UUID
>>> part uuid mmc 0:1 uuid
>>>   -> set environment variable to partition UUID
>>
>> What's the reason to not always both print out and set the uuid env var?
>>
>> Perhaps the env name should be partuuid or part_uuid as you could have
>> uuid's for other purposes?
>>
>>>
>>> This can be useful when writing a bootcmd which searches all known
>>> devices for something bootable, and then wants the kernel to use the
>>> same partition as the root device, e.g.:
>>>
>>> part uuid ${devtype} ${devnum}:${rootpart} uuid
>>> setenv bootargs root=PARTUUID=${uuid} ...
>>>
>>> It is expected that further part sub-commands will be added later, e.g.
>>> to find which partition on a disk is marked bootable, to write new
>>> partition tables to disk, etc.
>>
>> A list command would be useful and would be better located here than
>> under scsi or other interface commands. Perhaps instead of printing a
>> single part uuid, you should make a list command that prints all
>> partitions and their UUIDs. That would address my first question.
> 
> Sounds like a good idea to me as well.

In order to avoid too much feature creep in this patch-set, would it be
OK to implement a "part list" command that simply calls print_part(),
without changing what print_part() does right now, and then later send
patches to enhance print_part() to print the various partition UUIDs and
attributes?

>>> v2: validate that CONFIG_PARTITION_UUID is defined when CONFIG_CMD_PART is
>>>
>>> Note: If Rob Herring's proposed patch "disk/part: introduce
>>> get_device_and_partition" is applied, the body of do_partuuid() should
>>> be reworked to use Rob's new function get_device_and_partition().
> 
> I think the best idea here would be to make the next version just depend
> on Rob's series.

OK. Is it fairly certain that Rob's patches will be accepted then? There
hasn't been much feedback on them...

Rob, it sounds like we agreed to change the default partition
specification to use "auto" to mean "select a default partition". Were
you going to repost for that, or do you want me to make the modifications?

Thanks.

[U-Boot] [PATCH V2 4/4] cmd_part: add partition-related command

[Stephen Warren]

On 09/05/2012 05:58 PM, Tom Rini wrote:
> On Wed, Sep 05, 2012 at 06:51:58PM -0500, Rob Herring wrote:
>> On 09/05/2012 05:03 PM, Stephen Warren wrote:
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> This implements the following:
>>>
>>> part uuid mmc 0:1
>>>   -> print partition UUID
>>> part uuid mmc 0:1 uuid
>>>   -> set environment variable to partition UUID
>>
>> What's the reason to not always both print out and set the uuid env var?
>>
>> Perhaps the env name should be partuuid or part_uuid as you could have
>> uuid's for other purposes?
>>
>>>
>>> This can be useful when writing a bootcmd which searches all known
>>> devices for something bootable, and then wants the kernel to use the
>>> same partition as the root device, e.g.:
>>>
>>> part uuid ${devtype} ${devnum}:${rootpart} uuid
>>> setenv bootargs root=PARTUUID=${uuid} ...
>>>
>>> It is expected that further part sub-commands will be added later, e.g.
>>> to find which partition on a disk is marked bootable, to write new
>>> partition tables to disk, etc.
>>
>> A list command would be useful and would be better located here than
>> under scsi or other interface commands. Perhaps instead of printing a
>> single part uuid, you should make a list command that prints all
>> partitions and their UUIDs. That would address my first question.
> 
> Sounds like a good idea to me as well.
> 
> [snip]
>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>> ---
>>> v2: validate that CONFIG_PARTITION_UUID is defined when CONFIG_CMD_PART is
>>>
>>> Note: If Rob Herring's proposed patch "disk/part: introduce
>>> get_device_and_partition" is applied, the body of do_partuuid() should
>>> be reworked to use Rob's new function get_device_and_partition().
> 
> I think the best idea here would be to make the next version just depend
> on Rob's series.

Tom,

Rob's series depends on Wolfgang(?)'s u-boot/ext4 branch at present. I'm
not sure what the status of that branch is right now - is it something
that's ready to be submitted, or is more work there needed, so the
branch won't be pulled into u-boot/master in the near future? I'm mainly
asking so Rob and I know if Rob's patches should be rebased first onto
something else, before I rebase my patches on his.

Thanks.

[U-Boot] [PATCH V2 4/4] cmd_part: add partition-related command

[Lukasz Majewski]

Hi Stephen, Tom,


> Rob's series depends on Wolfgang(?)'s u-boot/ext4 branch at present.
> I'm not sure what the status of that branch is right now - is it
> something that's ready to be submitted, or is more work there needed,
> so the branch won't be pulled into u-boot/master in the near future?
> I'm mainly asking so Rob and I know if Rob's patches should be
> rebased first onto something else, before I rebase my patches on his.
> 
> Thanks.

I'd also like to know if those patches will be accepted soon. I'm
working on a GPT restoration support and those patches might be a
pre-requisite for my development (if were pulled into u-boot/master).


-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group

[U-Boot] [PATCH V2 4/4] cmd_part: add partition-related command

[Tom Rini]

On 09/12/2012 12:00 AM, Lukasz Majewski wrote:
> Hi Stephen, Tom,
> 
> 
>> Rob's series depends on Wolfgang(?)'s u-boot/ext4 branch at present.
>> I'm not sure what the status of that branch is right now - is it
>> something that's ready to be submitted, or is more work there needed,
>> so the branch won't be pulled into u-boot/master in the near future?
>> I'm mainly asking so Rob and I know if Rob's patches should be
>> rebased first onto something else, before I rebase my patches on his.
>>
>> Thanks.
> 
> I'd also like to know if those patches will be accepted soon. I'm
> working on a GPT restoration support and those patches might be a
> pre-requisite for my development (if were pulled into u-boot/master).

In short, yes, merged to master.  In longer form, please see the other
email I just sent in this thread.

-- 
Tom

[U-Boot] [PATCH V2 4/4] cmd_part: add partition-related command

[Tom Rini]

On 09/11/2012 03:52 PM, Stephen Warren wrote:
> On 09/05/2012 05:58 PM, Tom Rini wrote:
>> On Wed, Sep 05, 2012 at 06:51:58PM -0500, Rob Herring wrote:
>>> On 09/05/2012 05:03 PM, Stephen Warren wrote:
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> This implements the following:
>>>>
>>>> part uuid mmc 0:1
>>>>   -> print partition UUID
>>>> part uuid mmc 0:1 uuid
>>>>   -> set environment variable to partition UUID
>>>
>>> What's the reason to not always both print out and set the uuid env var?
>>>
>>> Perhaps the env name should be partuuid or part_uuid as you could have
>>> uuid's for other purposes?
>>>
>>>>
>>>> This can be useful when writing a bootcmd which searches all known
>>>> devices for something bootable, and then wants the kernel to use the
>>>> same partition as the root device, e.g.:
>>>>
>>>> part uuid ${devtype} ${devnum}:${rootpart} uuid
>>>> setenv bootargs root=PARTUUID=${uuid} ...
>>>>
>>>> It is expected that further part sub-commands will be added later, e.g.
>>>> to find which partition on a disk is marked bootable, to write new
>>>> partition tables to disk, etc.
>>>
>>> A list command would be useful and would be better located here than
>>> under scsi or other interface commands. Perhaps instead of printing a
>>> single part uuid, you should make a list command that prints all
>>> partitions and their UUIDs. That would address my first question.
>>
>> Sounds like a good idea to me as well.
>>
>> [snip]
>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>>> ---
>>>> v2: validate that CONFIG_PARTITION_UUID is defined when CONFIG_CMD_PART is
>>>>
>>>> Note: If Rob Herring's proposed patch "disk/part: introduce
>>>> get_device_and_partition" is applied, the body of do_partuuid() should
>>>> be reworked to use Rob's new function get_device_and_partition().
>>
>> I think the best idea here would be to make the next version just depend
>> on Rob's series.
> 
> Tom,
> 
> Rob's series depends on Wolfgang(?)'s u-boot/ext4 branch at present. I'm
> not sure what the status of that branch is right now - is it something
> that's ready to be submitted, or is more work there needed, so the
> branch won't be pulled into u-boot/master in the near future? I'm mainly
> asking so Rob and I know if Rob's patches should be rebased first onto
> something else, before I rebase my patches on his.

So, I want the ext4 work to make it into the next release.  At this
point I am aware there is an issue with large volumes but I need to
research a little more and make sure it's localized to ext4 only.  If
so, my feeling is that it's good enough to start with and then yes, it
will get merged to master, around -rc1 time.

-- 
Tom

[U-Boot] [PATCH V2 4/4] cmd_part: add partition-related command

[Stephen Warren]

On 09/05/2012 05:51 PM, Rob Herring wrote:
> On 09/05/2012 05:03 PM, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> This implements the following:
>>
>> part uuid mmc 0:1
>>   -> print partition UUID
>> part uuid mmc 0:1 uuid
>>   -> set environment variable to partition UUID
> 
> What's the reason to not always both print out and set the uuid env var?
> 
> Perhaps the env name should be partuuid or part_uuid as you could have
> uuid's for other purposes?

The idea is that if you're running the command interactively, you won't
pass a variable name on the command-line, so the command will print out
the UUID for you to read. In this case, it's pointless to set any
environment variable.

However, if you're writing a script, you want to capture the UUID into
an environment variable, and it's quite unlikely you want to litter
stdout with that content too. Hence, either-or, not both.

Note that in the second command above, the final parameter "uuid" is the
environment variable name, so you can save as many UUIDs for different
partitions into whatever variables you want; IIRC in the scripts I wrote
to use this, I did name the variable root_uuid or somesuch.

>> This can be useful when writing a bootcmd which searches all known
>> devices for something bootable, and then wants the kernel to use the
>> same partition as the root device, e.g.:
>>
>> part uuid ${devtype} ${devnum}:${rootpart} uuid
>> setenv bootargs root=PARTUUID=${uuid} ...
>>
>> It is expected that further part sub-commands will be added later, e.g.
>> to find which partition on a disk is marked bootable, to write new
>> partition tables to disk, etc.
> 
> A list command would be useful and would be better located here than
> under scsi or other interface commands. Perhaps instead of printing a
> single part uuid, you should make a list command that prints all
> partitions and their UUIDs. That would address my first question.

Yes, I wondered about "part list mmc 0", which would do essentially the
same thing as e.g. "mmc dev 0; mmc part", and also expanding the
partition printing functions to dump extra information, such as GPT's
partition type UUID, partition UUID, and attributes.

[U-Boot] [PATCH V2 4/4] cmd_part: add partition-related command

[Tom Rini]

On Wed, Sep 05, 2012 at 08:38:26PM -0600, Stephen Warren wrote:
> On 09/05/2012 05:51 PM, Rob Herring wrote:
> > On 09/05/2012 05:03 PM, Stephen Warren wrote:
> >> From: Stephen Warren <swarren@nvidia.com>
> >>
> >> This implements the following:
> >>
> >> part uuid mmc 0:1
> >>   -> print partition UUID
> >> part uuid mmc 0:1 uuid
> >>   -> set environment variable to partition UUID
> > 
> > What's the reason to not always both print out and set the uuid env var?
> > 
> > Perhaps the env name should be partuuid or part_uuid as you could have
> > uuid's for other purposes?
> 
> The idea is that if you're running the command interactively, you won't
> pass a variable name on the command-line, so the command will print out
> the UUID for you to read. In this case, it's pointless to set any
> environment variable.
> 
> However, if you're writing a script, you want to capture the UUID into
> an environment variable, and it's quite unlikely you want to litter
> stdout with that content too. Hence, either-or, not both.

Do other commands have a "I'm being scripted, probably, don't stdout"
and "I'm being interactive, use stdout" distinction like this?  IMHO,
always printing out makes sense so you can "see" that your script is
working as you expect.

-- 
Tom

[U-Boot] [PATCH V2 4/4] cmd_part: add partition-related command

[Stephen Warren]

On 09/06/2012 11:12 AM, Tom Rini wrote:
> On Wed, Sep 05, 2012 at 08:38:26PM -0600, Stephen Warren wrote:
>> On 09/05/2012 05:51 PM, Rob Herring wrote:
>>> On 09/05/2012 05:03 PM, Stephen Warren wrote:
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> This implements the following:
>>>>
>>>> part uuid mmc 0:1
>>>>   -> print partition UUID
>>>> part uuid mmc 0:1 uuid
>>>>   -> set environment variable to partition UUID
>>>
>>> What's the reason to not always both print out and set the uuid env var?
>>>
>>> Perhaps the env name should be partuuid or part_uuid as you could have
>>> uuid's for other purposes?
>>
>> The idea is that if you're running the command interactively, you won't
>> pass a variable name on the command-line, so the command will print out
>> the UUID for you to read. In this case, it's pointless to set any
>> environment variable.
>>
>> However, if you're writing a script, you want to capture the UUID into
>> an environment variable, and it's quite unlikely you want to litter
>> stdout with that content too. Hence, either-or, not both.
> 
> Do other commands have a "I'm being scripted, probably, don't stdout"
> and "I'm being interactive, use stdout" distinction like this?  IMHO,
> always printing out makes sense so you can "see" that your script is
> working as you expect.

In general, as a script writer, yes you do have the ability to choose.
Typically, I'd write:

part uuid ....

vs.

var=`part uuid ....`

in order to control this. However, U-Boot's shell doesn't support
backticks. As a script writer, I certainly desire the ability to control
what commands spam to the console, and really don't think it's useful to
print the UUID from a script (does the user really care, and any script
developer can just echo it for debugging if they need it).

I'm not aware of other U-Boot commands whose purpose it is to set
environment variables, so can't really compare.

Still, if you're insistent on this point, I can change the code to
always print, and optionally write an environment variable.

[U-Boot] [PATCH V2 4/4] cmd_part: add partition-related command

[Tom Rini]

On 09/06/2012 11:46 AM, Stephen Warren wrote:
> On 09/06/2012 11:12 AM, Tom Rini wrote:
>> On Wed, Sep 05, 2012 at 08:38:26PM -0600, Stephen Warren wrote:
>>> On 09/05/2012 05:51 PM, Rob Herring wrote:
>>>> On 09/05/2012 05:03 PM, Stephen Warren wrote:
>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>>
>>>>> This implements the following:
>>>>>
>>>>> part uuid mmc 0:1
>>>>>   -> print partition UUID
>>>>> part uuid mmc 0:1 uuid
>>>>>   -> set environment variable to partition UUID
>>>>
>>>> What's the reason to not always both print out and set the uuid env var?
>>>>
>>>> Perhaps the env name should be partuuid or part_uuid as you could have
>>>> uuid's for other purposes?
>>>
>>> The idea is that if you're running the command interactively, you won't
>>> pass a variable name on the command-line, so the command will print out
>>> the UUID for you to read. In this case, it's pointless to set any
>>> environment variable.
>>>
>>> However, if you're writing a script, you want to capture the UUID into
>>> an environment variable, and it's quite unlikely you want to litter
>>> stdout with that content too. Hence, either-or, not both.
>>
>> Do other commands have a "I'm being scripted, probably, don't stdout"
>> and "I'm being interactive, use stdout" distinction like this?  IMHO,
>> always printing out makes sense so you can "see" that your script is
>> working as you expect.
> 
> In general, as a script writer, yes you do have the ability to choose.
> Typically, I'd write:
> 
> part uuid ....
> 
> vs.
> 
> var=`part uuid ....`
> 
> in order to control this. However, U-Boot's shell doesn't support
> backticks. As a script writer, I certainly desire the ability to control
> what commands spam to the console, and really don't think it's useful to
> print the UUID from a script (does the user really care, and any script
> developer can just echo it for debugging if they need it).
> 
> I'm not aware of other U-Boot commands whose purpose it is to set
> environment variables, so can't really compare.
> 
> Still, if you're insistent on this point, I can change the code to
> always print, and optionally write an environment variable.

No, you make a good point.  Thanks!

-- 
Tom