All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/2] 'part name' subcommand and some robustification
@ 2025-11-10 20:54 Rasmus Villemoes
  2025-11-10 20:54 ` [PATCH v3 1/2] disk/part.c: ensure strings in struct disk_partition are valid after successful get_info Rasmus Villemoes
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Rasmus Villemoes @ 2025-11-10 20:54 UTC (permalink / raw)
  To: u-boot; +Cc: Quentin Schulz, Tom Rini, Rasmus Villemoes

Implement a "part name" subcommand, mirroring the existing "part
number" subcommand.

In the discussion for v1 of that, it came up that there's a bit of
inconsistency in how much and what one can assume to be initialized in
'struct disk_partition' after a successful call of one of the
get_info* family of functions. Patch 1/2 tries to consolidate
that by making sure all ->get_info invocations go through a common
helper that at least always initializes the string members.

Quentin, I've taken the liberty of including your Acks, as the
incremental diff in patch 1 is quite minor, but do speak up if I
should not have done that.

v3: Make sure part_get_type_by_name() can only ever return 0, -ENOSYS
(in case ->get_info is not implemented) and -ENOENT, rather than
anything that ->get_info might return.

v2: https://lore.kernel.org/u-boot/20251020121100.1742812-1-ravi@prevas.dk/

Rasmus Villemoes (2):
  disk/part.c: ensure strings in struct disk_partition are valid after
    successful get_info
  cmd/part.c: implement "part name" subcommand

 cmd/gpt.c              |  4 +--
 cmd/part.c             | 16 ++++++++++-
 disk/part.c            | 63 +++++++++++++++++++++++++-----------------
 doc/usage/cmd/part.rst | 13 +++++++++
 include/part.h         | 16 +++++++++++
 5 files changed, 83 insertions(+), 29 deletions(-)

-- 
2.51.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3 1/2] disk/part.c: ensure strings in struct disk_partition are valid after successful get_info
  2025-11-10 20:54 [PATCH v3 0/2] 'part name' subcommand and some robustification Rasmus Villemoes
@ 2025-11-10 20:54 ` Rasmus Villemoes
  2025-11-11  5:18   ` Anshul Dalal
  2025-11-10 20:54 ` [PATCH v3 2/2] cmd/part.c: implement "part name" subcommand Rasmus Villemoes
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Rasmus Villemoes @ 2025-11-10 20:54 UTC (permalink / raw)
  To: u-boot; +Cc: Quentin Schulz, Tom Rini, Rasmus Villemoes

Not all ->get_info implementations necessarily populate all the string
members of struct disk_partition.

Currently, only part_get_info_by_type() (and thereby part_get_info)
ensure that the uuid strings are initialized; part_get_info_by_type()
and part_get_info_by_uuid() do not. In fact, the latter could lead to
a false positive match - if the ->get_info backend does not populate
info->uuid, stale contents in info could cause the strncasecmp() to
succeed.

None of the functions currently ensure that the ->name and ->type
strings are initialized.

Instead of forcing all callers of any of these functions to
pre-initialize info, or all implementations of the ->get_info method
to ensure there are valid C strings in all four fields, create a small
helper function and factor all invocations of ->get_info through that.

This also consolidates the -ENOSYS check and standardizes on using
log_debug() for reporting absence, instead of the current mix of
PRINTF and log_debug(). It does mean we have to special-case -ENOSYS
in the error cases inside the loops in the _by_uuid() and _by_name()
functions, but it's still a net win in #LOC.

Acked-by: Quentin Schulz <quentin.schulz@cherry.de>
Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 cmd/gpt.c      |  4 ++--
 disk/part.c    | 63 +++++++++++++++++++++++++++++---------------------
 include/part.h | 16 +++++++++++++
 3 files changed, 55 insertions(+), 28 deletions(-)

diff --git a/cmd/gpt.c b/cmd/gpt.c
index e18e5036a06..84221881c39 100644
--- a/cmd/gpt.c
+++ b/cmd/gpt.c
@@ -724,7 +724,7 @@ static int gpt_enumerate(struct blk_desc *desc)
 			continue;
 
 		for (i = 1; i < part_drv->max_entries; i++) {
-			ret = part_drv->get_info(desc, i, &pinfo);
+			ret = part_driver_get_info(part_drv, desc, i, &pinfo);
 			if (ret)
 				continue;
 
@@ -820,7 +820,7 @@ static int gpt_setenv(struct blk_desc *desc, const char *name)
 		int i;
 
 		for (i = 1; i < part_drv->max_entries; i++) {
-			ret = part_drv->get_info(desc, i, &pinfo);
+			ret = part_driver_get_info(part_drv, desc, i, &pinfo);
 			if (ret)
 				continue;
 
diff --git a/disk/part.c b/disk/part.c
index be2b45d5a29..49a0fca6b89 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -72,6 +72,28 @@ struct part_driver *part_driver_lookup_type(struct blk_desc *desc)
 	return NULL;
 }
 
+static void disk_partition_clr(struct disk_partition *info)
+{
+	/* The common case is no UUID support */
+	disk_partition_clr_uuid(info);
+	disk_partition_clr_type_guid(info);
+	info->name[0] = '\0';
+	info->type[0] = '\0';
+}
+
+int part_driver_get_info(struct part_driver *drv, struct blk_desc *desc, int part,
+			 struct disk_partition *info)
+{
+	if (!drv->get_info) {
+		log_debug("## Driver %s does not have the get_info() method\n",
+			  drv->name);
+		return -ENOSYS;
+	}
+
+	disk_partition_clr(info);
+	return drv->get_info(desc, part, info);
+}
+
 int part_get_type_by_name(const char *name)
 {
 	struct part_driver *drv =
@@ -322,12 +344,9 @@ int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
 			  struct disk_partition *info)
 {
 	struct part_driver *drv;
+	int ret = -ENOENT;
 
 	if (blk_enabled()) {
-		/* The common case is no UUID support */
-		disk_partition_clr_uuid(info);
-		disk_partition_clr_type_guid(info);
-
 		if (part_type == PART_TYPE_UNKNOWN) {
 			drv = part_driver_lookup_type(desc);
 		} else {
@@ -339,18 +358,16 @@ int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
 			      desc->part_type);
 			return -EPROTONOSUPPORT;
 		}
-		if (!drv->get_info) {
-			PRINTF("## Driver %s does not have the get_info() method\n",
-			       drv->name);
-			return -ENOSYS;
-		}
-		if (drv->get_info(desc, part, info) == 0) {
+
+		ret = part_driver_get_info(drv, desc, part, info);
+		if (ret && ret != -ENOSYS) {
+			ret = -ENOENT;
+		} else {
 			PRINTF("## Valid %s partition found ##\n", drv->name);
-			return 0;
 		}
 	}
 
-	return -ENOENT;
+	return ret;
 }
 
 int part_get_info(struct blk_desc *desc, int part,
@@ -657,15 +674,12 @@ int part_get_info_by_name(struct blk_desc *desc, const char *name,
 	if (!part_drv)
 		return -1;
 
-	if (!part_drv->get_info) {
-		log_debug("## Driver %s does not have the get_info() method\n",
-			  part_drv->name);
-		return -ENOSYS;
-	}
-
 	for (i = 1; i < part_drv->max_entries; i++) {
-		ret = part_drv->get_info(desc, i, info);
+		ret = part_driver_get_info(part_drv, desc, i, info);
 		if (ret != 0) {
+			/* -ENOSYS means no ->get_info method. */
+			if (ret == -ENOSYS)
+				return ret;
 			/*
 			 * Partition with this index can't be obtained, but
 			 * further partitions might be, so keep checking.
@@ -695,15 +709,12 @@ int part_get_info_by_uuid(struct blk_desc *desc, const char *uuid,
 	if (!part_drv)
 		return -1;
 
-	if (!part_drv->get_info) {
-		log_debug("## Driver %s does not have the get_info() method\n",
-			  part_drv->name);
-		return -ENOSYS;
-	}
-
 	for (i = 1; i < part_drv->max_entries; i++) {
-		ret = part_drv->get_info(desc, i, info);
+		ret = part_driver_get_info(part_drv, desc, i, info);
 		if (ret != 0) {
+			/* -ENOSYS means no ->get_info method. */
+			if (ret == -ENOSYS)
+				return ret;
 			/*
 			 * Partition with this index can't be obtained, but
 			 * further partitions might be, so keep checking.
diff --git a/include/part.h b/include/part.h
index 6caaa6526aa..d940f8b5d0e 100644
--- a/include/part.h
+++ b/include/part.h
@@ -509,6 +509,22 @@ struct part_driver {
 	int (*test)(struct blk_desc *desc);
 };
 
+/**
+ * part_driver_get_info() - Call the part_driver's get_info method
+ *
+ * On success, string members of info are guaranteed to be properly
+ * initialized, though they may be empty.
+ *
+ * @drv:	Partition driver
+ * @desc:	Block device descriptor
+ * @part:	Partition number to read
+ * @info:	Returned partition information
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int part_driver_get_info(struct part_driver *drv, struct blk_desc *desc, int part,
+			 struct disk_partition *info);
+
 /* Declare a new U-Boot partition 'driver' */
 #define U_BOOT_PART_TYPE(__name)					\
 	ll_entry_declare(struct part_driver, __name, part_driver)
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 2/2] cmd/part.c: implement "part name" subcommand
  2025-11-10 20:54 [PATCH v3 0/2] 'part name' subcommand and some robustification Rasmus Villemoes
  2025-11-10 20:54 ` [PATCH v3 1/2] disk/part.c: ensure strings in struct disk_partition are valid after successful get_info Rasmus Villemoes
@ 2025-11-10 20:54 ` Rasmus Villemoes
  2025-11-11  5:25   ` Anshul Dalal
  2025-11-11  5:07 ` [PATCH v3 0/2] 'part name' subcommand and some robustification Anshul Dalal
  2025-11-18 20:32 ` Tom Rini
  3 siblings, 1 reply; 10+ messages in thread
From: Rasmus Villemoes @ 2025-11-10 20:54 UTC (permalink / raw)
  To: u-boot; +Cc: Quentin Schulz, Tom Rini, Rasmus Villemoes

This is a natural buddy to the existing "part number", allowing one to
get the partition name for a given partition number.

Acked-by: Quentin Schulz <quentin.schulz@cherry.de>
Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 cmd/part.c             | 16 +++++++++++++++-
 doc/usage/cmd/part.rst | 13 +++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/cmd/part.c b/cmd/part.c
index db7bc5819c0..975a0a08a99 100644
--- a/cmd/part.c
+++ b/cmd/part.c
@@ -25,7 +25,8 @@
 enum cmd_part_info {
 	CMD_PART_INFO_START = 0,
 	CMD_PART_INFO_SIZE,
-	CMD_PART_INFO_NUMBER
+	CMD_PART_INFO_NUMBER,
+	CMD_PART_INFO_NAME,
 };
 
 static int do_part_uuid(int argc, char *const argv[])
@@ -154,6 +155,9 @@ static int do_part_info(int argc, char *const argv[], enum cmd_part_info param)
 	case CMD_PART_INFO_NUMBER:
 		snprintf(buf, sizeof(buf), "0x%x", part);
 		break;
+	case CMD_PART_INFO_NAME:
+		snprintf(buf, sizeof(buf), "%s", info.name);
+		break;
 	default:
 		printf("** Unknown cmd_part_info value: %d\n", param);
 		return 1;
@@ -182,6 +186,11 @@ static int do_part_number(int argc, char *const argv[])
 	return do_part_info(argc, argv, CMD_PART_INFO_NUMBER);
 }
 
+static int do_part_name(int argc, char *const argv[])
+{
+	return do_part_info(argc, argv, CMD_PART_INFO_NAME);
+}
+
 static int do_part_set(int argc, char *const argv[])
 {
 	const char *devname, *partstr, *typestr;
@@ -273,6 +282,8 @@ static int do_part(struct cmd_tbl *cmdtp, int flag, int argc,
 		return do_part_size(argc - 2, argv + 2);
 	else if (!strcmp(argv[1], "number"))
 		return do_part_number(argc - 2, argv + 2);
+	else if (!strcmp(argv[1], "name"))
+		return do_part_name(argc - 2, argv + 2);
 	else if (!strcmp(argv[1], "types"))
 		return do_part_types(argc - 2, argv + 2);
 	else if (!strcmp(argv[1], "set"))
@@ -305,6 +316,9 @@ U_BOOT_CMD(
 	"part number <interface> <dev> <part> <varname>\n"
 	"    - set environment variable to the partition number using the partition name\n"
 	"      part must be specified as partition name\n"
+	"part name <interface> <dev> <part> <varname>\n"
+	"    - set environment variable to the partition name using the partition number\n"
+	"      part must be specified as partition number\n"
 #ifdef CONFIG_PARTITION_TYPE_GUID
 	"part type <interface> <dev>:<part>\n"
 	"    - print partition type\n"
diff --git a/doc/usage/cmd/part.rst b/doc/usage/cmd/part.rst
index 72f5d8b8de7..299f2ac15c5 100644
--- a/doc/usage/cmd/part.rst
+++ b/doc/usage/cmd/part.rst
@@ -16,6 +16,7 @@ Synopsis
     part start <interface> <dev> <part> <varname>
     part size <interface> <dev> <part> <varname>
     part number <interface> <dev> <part> <varname>
+    part name <interface> <dev> <part> <varname>
     part set <interface> <dev> <part> <type>
     part type <interface> <dev>:<part> [varname]
     part types
@@ -86,6 +87,18 @@ part must be specified as partition name.
     varname
         a variable to store the current partition number value into
 
+The 'part name' command sets an environment variable to the partition name using the partition number,
+part must be specified as partition number.
+
+    interface
+        interface for accessing the block device (mmc, sata, scsi, usb, ....)
+    dev
+        device number
+    part
+        partition number
+    varname
+        a variable to store the current partition name into
+
 The 'part set' command sets the type of a partition. This is useful when
 autodetection fails or does not do the correct thing:
 
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/2] 'part name' subcommand and some robustification
  2025-11-10 20:54 [PATCH v3 0/2] 'part name' subcommand and some robustification Rasmus Villemoes
  2025-11-10 20:54 ` [PATCH v3 1/2] disk/part.c: ensure strings in struct disk_partition are valid after successful get_info Rasmus Villemoes
  2025-11-10 20:54 ` [PATCH v3 2/2] cmd/part.c: implement "part name" subcommand Rasmus Villemoes
@ 2025-11-11  5:07 ` Anshul Dalal
  2025-11-18 20:32 ` Tom Rini
  3 siblings, 0 replies; 10+ messages in thread
From: Anshul Dalal @ 2025-11-11  5:07 UTC (permalink / raw)
  To: Rasmus Villemoes, u-boot; +Cc: Quentin Schulz, Tom Rini

On Tue Nov 11, 2025 at 2:24 AM IST, Rasmus Villemoes wrote:
> Implement a "part name" subcommand, mirroring the existing "part
> number" subcommand.
>
> In the discussion for v1 of that, it came up that there's a bit of
> inconsistency in how much and what one can assume to be initialized in
> 'struct disk_partition' after a successful call of one of the
> get_info* family of functions. Patch 1/2 tries to consolidate
> that by making sure all ->get_info invocations go through a common
> helper that at least always initializes the string members.
>
> Quentin, I've taken the liberty of including your Acks, as the
> incremental diff in patch 1 is quite minor, but do speak up if I
> should not have done that.
>
> v3: Make sure part_get_type_by_name() can only ever return 0, -ENOSYS
> (in case ->get_info is not implemented) and -ENOENT, rather than
> anything that ->get_info might return.
>
> v2: https://lore.kernel.org/u-boot/20251020121100.1742812-1-ravi@prevas.dk/
>

For the entire series:

Tested-by: Anshul Dalal <anshuld@ti.com>

Logs (on TI AM62x):

	=> part
	part - disk partition related commands

	Usage:
	part uuid <interface> <dev>:<part>
	    - print partition UUID
	part uuid <interface> <dev>:<part> <varname>
	    - set environment variable to partition UUID
	part list <interface> <dev>
	    - print a device's partition table
	part list <interface> <dev> [flags] <varname>
	    - set environment variable to the list of partitions
	      flags can be -bootable (list only bootable partitions)
	part start <interface> <dev> <part> <varname>
	    - set environment variable to the start of the partition (in blocks)
	      part can be either partition number or partition name
	part size <interface> <dev> <part> <varname>
	    - set environment variable to the size of the partition (in blocks)
	      part can be either partition number or partition name
	part number <interface> <dev> <part> <varname>
	    - set environment variable to the partition number using the partition name
	      part must be specified as partition name
	part name <interface> <dev> <part> <varname>
	    - set environment variable to the partition name using the partition number
	      part must be specified as partition number
	part set <interface> <dev> type
	    - set partition type for a device
	part types
	    - list supported partition table types
	=> part list mmc 1

	Partition Map for mmc device 1  --   Partition Type: DOS

	Part    Start Sector    Num Sectors     UUID            Type
	  1     2048            262144          076c4a2a-01     0c Boot
	  2     264192          417336          076c4a2a-02     83
	=> part name mmc 1 1
	mmcsdb1
	=> part name mmc 1 2
	mmcsdb2

Regards,
Anshul


> Rasmus Villemoes (2):
>   disk/part.c: ensure strings in struct disk_partition are valid after
>     successful get_info
>   cmd/part.c: implement "part name" subcommand
>
>  cmd/gpt.c              |  4 +--
>  cmd/part.c             | 16 ++++++++++-
>  disk/part.c            | 63 +++++++++++++++++++++++++-----------------
>  doc/usage/cmd/part.rst | 13 +++++++++
>  include/part.h         | 16 +++++++++++
>  5 files changed, 83 insertions(+), 29 deletions(-)


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 1/2] disk/part.c: ensure strings in struct disk_partition are valid after successful get_info
  2025-11-10 20:54 ` [PATCH v3 1/2] disk/part.c: ensure strings in struct disk_partition are valid after successful get_info Rasmus Villemoes
@ 2025-11-11  5:18   ` Anshul Dalal
  2025-11-11  7:58     ` Rasmus Villemoes
  0 siblings, 1 reply; 10+ messages in thread
From: Anshul Dalal @ 2025-11-11  5:18 UTC (permalink / raw)
  To: Rasmus Villemoes, u-boot; +Cc: Quentin Schulz, Tom Rini

Hi Rasmus,

On Tue Nov 11, 2025 at 2:24 AM IST, Rasmus Villemoes wrote:
> Not all ->get_info implementations necessarily populate all the string
> members of struct disk_partition.
>
> Currently, only part_get_info_by_type() (and thereby part_get_info)
> ensure that the uuid strings are initialized; part_get_info_by_type()
> and part_get_info_by_uuid() do not. In fact, the latter could lead to
> a false positive match - if the ->get_info backend does not populate
> info->uuid, stale contents in info could cause the strncasecmp() to
> succeed.
>
> None of the functions currently ensure that the ->name and ->type
> strings are initialized.
>
> Instead of forcing all callers of any of these functions to
> pre-initialize info, or all implementations of the ->get_info method
> to ensure there are valid C strings in all four fields, create a small
> helper function and factor all invocations of ->get_info through that.
>
> This also consolidates the -ENOSYS check and standardizes on using
> log_debug() for reporting absence, instead of the current mix of
> PRINTF and log_debug(). It does mean we have to special-case -ENOSYS
> in the error cases inside the loops in the _by_uuid() and _by_name()
> functions, but it's still a net win in #LOC.
>
> Acked-by: Quentin Schulz <quentin.schulz@cherry.de>
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
> ---
>  cmd/gpt.c      |  4 ++--
>  disk/part.c    | 63 +++++++++++++++++++++++++++++---------------------
>  include/part.h | 16 +++++++++++++
>  3 files changed, 55 insertions(+), 28 deletions(-)
>
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> index e18e5036a06..84221881c39 100644
> --- a/cmd/gpt.c
> +++ b/cmd/gpt.c
> @@ -724,7 +724,7 @@ static int gpt_enumerate(struct blk_desc *desc)
>  			continue;
>  
>  		for (i = 1; i < part_drv->max_entries; i++) {
> -			ret = part_drv->get_info(desc, i, &pinfo);
> +			ret = part_driver_get_info(part_drv, desc, i, &pinfo);
>  			if (ret)
>  				continue;
>  
> @@ -820,7 +820,7 @@ static int gpt_setenv(struct blk_desc *desc, const char *name)
>  		int i;
>  
>  		for (i = 1; i < part_drv->max_entries; i++) {
> -			ret = part_drv->get_info(desc, i, &pinfo);
> +			ret = part_driver_get_info(part_drv, desc, i, &pinfo);
>  			if (ret)
>  				continue;
>  
> diff --git a/disk/part.c b/disk/part.c
> index be2b45d5a29..49a0fca6b89 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -72,6 +72,28 @@ struct part_driver *part_driver_lookup_type(struct blk_desc *desc)
>  	return NULL;
>  }
>  
> +static void disk_partition_clr(struct disk_partition *info)
> +{
> +	/* The common case is no UUID support */
> +	disk_partition_clr_uuid(info);
> +	disk_partition_clr_type_guid(info);
> +	info->name[0] = '\0';
> +	info->type[0] = '\0';
> +}
> +
> +int part_driver_get_info(struct part_driver *drv, struct blk_desc *desc, int part,
> +			 struct disk_partition *info)
> +{
> +	if (!drv->get_info) {
> +		log_debug("## Driver %s does not have the get_info() method\n",
> +			  drv->name);
> +		return -ENOSYS;
> +	}
> +
> +	disk_partition_clr(info);
> +	return drv->get_info(desc, part, info);
> +}
> +
>  int part_get_type_by_name(const char *name)
>  {
>  	struct part_driver *drv =
> @@ -322,12 +344,9 @@ int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
>  			  struct disk_partition *info)
>  {
>  	struct part_driver *drv;
> +	int ret = -ENOENT;
>  
>  	if (blk_enabled()) {
> -		/* The common case is no UUID support */
> -		disk_partition_clr_uuid(info);
> -		disk_partition_clr_type_guid(info);
> -
>  		if (part_type == PART_TYPE_UNKNOWN) {
>  			drv = part_driver_lookup_type(desc);
>  		} else {
> @@ -339,18 +358,16 @@ int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
>  			      desc->part_type);
>  			return -EPROTONOSUPPORT;
>  		}
> -		if (!drv->get_info) {
> -			PRINTF("## Driver %s does not have the get_info() method\n",
> -			       drv->name);
> -			return -ENOSYS;
> -		}
> -		if (drv->get_info(desc, part, info) == 0) {
> +
> +		ret = part_driver_get_info(drv, desc, part, info);
> +		if (ret && ret != -ENOSYS) {
> +			ret = -ENOENT;

Why are we overwriting the err code from part_driver_get_info here?

Regards,
Anshul

> +		} else {
>  			PRINTF("## Valid %s partition found ##\n", drv->name);
> -			return 0;
>  		}
>  	}
>  
> -	return -ENOENT;
> +	return ret;
>  }
>  
>  int part_get_info(struct blk_desc *desc, int part,
> @@ -657,15 +674,12 @@ int part_get_info_by_name(struct blk_desc *desc, const char *name,
>  	if (!part_drv)
>  		return -1;
>  
> -	if (!part_drv->get_info) {
> -		log_debug("## Driver %s does not have the get_info() method\n",
> -			  part_drv->name);
> -		return -ENOSYS;
> -	}
> -
>  	for (i = 1; i < part_drv->max_entries; i++) {
> -		ret = part_drv->get_info(desc, i, info);
> +		ret = part_driver_get_info(part_drv, desc, i, info);
>  		if (ret != 0) {
> +			/* -ENOSYS means no ->get_info method. */
> +			if (ret == -ENOSYS)
> +				return ret;
>  			/*
>  			 * Partition with this index can't be obtained, but
>  			 * further partitions might be, so keep checking.
> @@ -695,15 +709,12 @@ int part_get_info_by_uuid(struct blk_desc *desc, const char *uuid,
>  	if (!part_drv)
>  		return -1;
>  
> -	if (!part_drv->get_info) {
> -		log_debug("## Driver %s does not have the get_info() method\n",
> -			  part_drv->name);
> -		return -ENOSYS;
> -	}
> -
>  	for (i = 1; i < part_drv->max_entries; i++) {
> -		ret = part_drv->get_info(desc, i, info);
> +		ret = part_driver_get_info(part_drv, desc, i, info);
>  		if (ret != 0) {
> +			/* -ENOSYS means no ->get_info method. */
> +			if (ret == -ENOSYS)
> +				return ret;
>  			/*
>  			 * Partition with this index can't be obtained, but
>  			 * further partitions might be, so keep checking.
> diff --git a/include/part.h b/include/part.h
> index 6caaa6526aa..d940f8b5d0e 100644
> --- a/include/part.h
> +++ b/include/part.h
> @@ -509,6 +509,22 @@ struct part_driver {
>  	int (*test)(struct blk_desc *desc);
>  };
>  
> +/**
> + * part_driver_get_info() - Call the part_driver's get_info method
> + *
> + * On success, string members of info are guaranteed to be properly
> + * initialized, though they may be empty.
> + *
> + * @drv:	Partition driver
> + * @desc:	Block device descriptor
> + * @part:	Partition number to read
> + * @info:	Returned partition information
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int part_driver_get_info(struct part_driver *drv, struct blk_desc *desc, int part,
> +			 struct disk_partition *info);
> +
>  /* Declare a new U-Boot partition 'driver' */
>  #define U_BOOT_PART_TYPE(__name)					\
>  	ll_entry_declare(struct part_driver, __name, part_driver)


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/2] cmd/part.c: implement "part name" subcommand
  2025-11-10 20:54 ` [PATCH v3 2/2] cmd/part.c: implement "part name" subcommand Rasmus Villemoes
@ 2025-11-11  5:25   ` Anshul Dalal
  2025-11-11  8:00     ` Rasmus Villemoes
  0 siblings, 1 reply; 10+ messages in thread
From: Anshul Dalal @ 2025-11-11  5:25 UTC (permalink / raw)
  To: Rasmus Villemoes, u-boot; +Cc: Quentin Schulz, Tom Rini

On Tue Nov 11, 2025 at 2:24 AM IST, Rasmus Villemoes wrote:
> This is a natural buddy to the existing "part number", allowing one to
> get the partition name for a given partition number.
>
> Acked-by: Quentin Schulz <quentin.schulz@cherry.de>
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
> ---
>  cmd/part.c             | 16 +++++++++++++++-
>  doc/usage/cmd/part.rst | 13 +++++++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/cmd/part.c b/cmd/part.c
> index db7bc5819c0..975a0a08a99 100644
> --- a/cmd/part.c
> +++ b/cmd/part.c
> @@ -25,7 +25,8 @@
>  enum cmd_part_info {
>  	CMD_PART_INFO_START = 0,
>  	CMD_PART_INFO_SIZE,
> -	CMD_PART_INFO_NUMBER
> +	CMD_PART_INFO_NUMBER,
> +	CMD_PART_INFO_NAME,
>  };
>  
>  static int do_part_uuid(int argc, char *const argv[])
> @@ -154,6 +155,9 @@ static int do_part_info(int argc, char *const argv[], enum cmd_part_info param)
>  	case CMD_PART_INFO_NUMBER:
>  		snprintf(buf, sizeof(buf), "0x%x", part);
>  		break;
> +	case CMD_PART_INFO_NAME:
> +		snprintf(buf, sizeof(buf), "%s", info.name);
> +		break;
>  	default:
>  		printf("** Unknown cmd_part_info value: %d\n", param);
>  		return 1;
> @@ -182,6 +186,11 @@ static int do_part_number(int argc, char *const argv[])
>  	return do_part_info(argc, argv, CMD_PART_INFO_NUMBER);
>  }
>  
> +static int do_part_name(int argc, char *const argv[])
> +{
> +	return do_part_info(argc, argv, CMD_PART_INFO_NAME);
> +}
> +
>  static int do_part_set(int argc, char *const argv[])
>  {
>  	const char *devname, *partstr, *typestr;
> @@ -273,6 +282,8 @@ static int do_part(struct cmd_tbl *cmdtp, int flag, int argc,
>  		return do_part_size(argc - 2, argv + 2);
>  	else if (!strcmp(argv[1], "number"))
>  		return do_part_number(argc - 2, argv + 2);
> +	else if (!strcmp(argv[1], "name"))
> +		return do_part_name(argc - 2, argv + 2);
>  	else if (!strcmp(argv[1], "types"))
>  		return do_part_types(argc - 2, argv + 2);
>  	else if (!strcmp(argv[1], "set"))
> @@ -305,6 +316,9 @@ U_BOOT_CMD(
>  	"part number <interface> <dev> <part> <varname>\n"
>  	"    - set environment variable to the partition number using the partition name\n"
>  	"      part must be specified as partition name\n"
> +	"part name <interface> <dev> <part> <varname>\n"

Nit: Shouldn't this be part name <interface> <dev> <part> [varname]
instead?

> +	"    - set environment variable to the partition name using the partition number\n"
> +	"      part must be specified as partition number\n"
>  #ifdef CONFIG_PARTITION_TYPE_GUID
>  	"part type <interface> <dev>:<part>\n"
>  	"    - print partition type\n"
> diff --git a/doc/usage/cmd/part.rst b/doc/usage/cmd/part.rst
> index 72f5d8b8de7..299f2ac15c5 100644
> --- a/doc/usage/cmd/part.rst
> +++ b/doc/usage/cmd/part.rst
> @@ -16,6 +16,7 @@ Synopsis
>      part start <interface> <dev> <part> <varname>
>      part size <interface> <dev> <part> <varname>
>      part number <interface> <dev> <part> <varname>
> +    part name <interface> <dev> <part> <varname>

Same here.

Regards,
Anshul

>      part set <interface> <dev> <part> <type>
>      part type <interface> <dev>:<part> [varname]
>      part types
> @@ -86,6 +87,18 @@ part must be specified as partition name.
>      varname
>          a variable to store the current partition number value into
>  
> +The 'part name' command sets an environment variable to the partition name using the partition number,
> +part must be specified as partition number.
> +
> +    interface
> +        interface for accessing the block device (mmc, sata, scsi, usb, ....)
> +    dev
> +        device number
> +    part
> +        partition number
> +    varname
> +        a variable to store the current partition name into
> +
>  The 'part set' command sets the type of a partition. This is useful when
>  autodetection fails or does not do the correct thing:
>  


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 1/2] disk/part.c: ensure strings in struct disk_partition are valid after successful get_info
  2025-11-11  5:18   ` Anshul Dalal
@ 2025-11-11  7:58     ` Rasmus Villemoes
  0 siblings, 0 replies; 10+ messages in thread
From: Rasmus Villemoes @ 2025-11-11  7:58 UTC (permalink / raw)
  To: Anshul Dalal; +Cc: u-boot, Quentin Schulz, Tom Rini

On Tue, Nov 11 2025, Anshul Dalal <anshuld@ti.com> wrote:

>>  int part_get_type_by_name(const char *name)
>>  {
>>  	struct part_driver *drv =
>> @@ -322,12 +344,9 @@ int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
>>  			  struct disk_partition *info)
>>  {
>>  	struct part_driver *drv;
>> +	int ret = -ENOENT;
>>  
>>  	if (blk_enabled()) {
>> -		/* The common case is no UUID support */
>> -		disk_partition_clr_uuid(info);
>> -		disk_partition_clr_type_guid(info);
>> -
>>  		if (part_type == PART_TYPE_UNKNOWN) {
>>  			drv = part_driver_lookup_type(desc);
>>  		} else {
>> @@ -339,18 +358,16 @@ int part_get_info_by_type(struct blk_desc *desc, int part, int part_type,
>>  			      desc->part_type);
>>  			return -EPROTONOSUPPORT;
>>  		}
>> -		if (!drv->get_info) {
>> -			PRINTF("## Driver %s does not have the get_info() method\n",
>> -			       drv->name);
>> -			return -ENOSYS;
>> -		}
>> -		if (drv->get_info(desc, part, info) == 0) {
>> +
>> +		ret = part_driver_get_info(drv, desc, part, info);
>> +		if (ret && ret != -ENOSYS) {
>> +			ret = -ENOENT;
>
> Why are we overwriting the err code from part_driver_get_info here?

That's essentially what the code did originally (it only returned 0 in
case ->get_info returned 0, otherwise it fell through to the "return
-ENOENT" at the bottom). And it turns out that that behaviour is
expected by lots of tests; when I inadvertently changed that logic in v2
to actually propagate whatever error ->get_info returned, lots of tests
broke:

https://lore.kernel.org/u-boot/20251107201927.GA2243313@bill-the-cat/

And this is the only difference between v2 and v3, I tweaked this so it
would preserve that "either success or -ENOENT [or -ENOSYS]" behaviour.

Rasmus

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/2] cmd/part.c: implement "part name" subcommand
  2025-11-11  5:25   ` Anshul Dalal
@ 2025-11-11  8:00     ` Rasmus Villemoes
  2025-11-11 17:27       ` Quentin Schulz
  0 siblings, 1 reply; 10+ messages in thread
From: Rasmus Villemoes @ 2025-11-11  8:00 UTC (permalink / raw)
  To: Anshul Dalal; +Cc: u-boot, Quentin Schulz, Tom Rini

On Tue, Nov 11 2025, Anshul Dalal <anshuld@ti.com> wrote:

>>  static int do_part_set(int argc, char *const argv[])
>>  {
>>  	const char *devname, *partstr, *typestr;
>> @@ -273,6 +282,8 @@ static int do_part(struct cmd_tbl *cmdtp, int flag, int argc,
>>  		return do_part_size(argc - 2, argv + 2);
>>  	else if (!strcmp(argv[1], "number"))
>>  		return do_part_number(argc - 2, argv + 2);
>> +	else if (!strcmp(argv[1], "name"))
>> +		return do_part_name(argc - 2, argv + 2);
>>  	else if (!strcmp(argv[1], "types"))
>>  		return do_part_types(argc - 2, argv + 2);
>>  	else if (!strcmp(argv[1], "set"))
>> @@ -305,6 +316,9 @@ U_BOOT_CMD(
>>  	"part number <interface> <dev> <part> <varname>\n"
>>  	"    - set environment variable to the partition number using the partition name\n"
>>  	"      part must be specified as partition name\n"
>> +	"part name <interface> <dev> <part> <varname>\n"
>
> Nit: Shouldn't this be part name <interface> <dev> <part> [varname]
> instead?
>

Yes, probably, I did notice that, but I preferred being consistent with
the existing cases which also do not indicate varname as optional.

Rasmus

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/2] cmd/part.c: implement "part name" subcommand
  2025-11-11  8:00     ` Rasmus Villemoes
@ 2025-11-11 17:27       ` Quentin Schulz
  0 siblings, 0 replies; 10+ messages in thread
From: Quentin Schulz @ 2025-11-11 17:27 UTC (permalink / raw)
  To: Rasmus Villemoes, Anshul Dalal; +Cc: u-boot, Tom Rini

Hi Rasmus,

On 11/11/25 9:00 AM, Rasmus Villemoes wrote:
> On Tue, Nov 11 2025, Anshul Dalal <anshuld@ti.com> wrote:
> 
>>>   static int do_part_set(int argc, char *const argv[])
>>>   {
>>>   	const char *devname, *partstr, *typestr;
>>> @@ -273,6 +282,8 @@ static int do_part(struct cmd_tbl *cmdtp, int flag, int argc,
>>>   		return do_part_size(argc - 2, argv + 2);
>>>   	else if (!strcmp(argv[1], "number"))
>>>   		return do_part_number(argc - 2, argv + 2);
>>> +	else if (!strcmp(argv[1], "name"))
>>> +		return do_part_name(argc - 2, argv + 2);
>>>   	else if (!strcmp(argv[1], "types"))
>>>   		return do_part_types(argc - 2, argv + 2);
>>>   	else if (!strcmp(argv[1], "set"))
>>> @@ -305,6 +316,9 @@ U_BOOT_CMD(
>>>   	"part number <interface> <dev> <part> <varname>\n"
>>>   	"    - set environment variable to the partition number using the partition name\n"
>>>   	"      part must be specified as partition name\n"
>>> +	"part name <interface> <dev> <part> <varname>\n"
>>
>> Nit: Shouldn't this be part name <interface> <dev> <part> [varname]
>> instead?
>>
> 
> Yes, probably, I did notice that, but I preferred being consistent with
> the existing cases which also do not indicate varname as optional.
> 

Let's not be consistent with a mistake if it can be fixed without 
breaking compatibility. From what I gathered, it's a simple help text 
(and maybe doc) update, so i think it's better to fix it.

Cheers,
Quentin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/2] 'part name' subcommand and some robustification
  2025-11-10 20:54 [PATCH v3 0/2] 'part name' subcommand and some robustification Rasmus Villemoes
                   ` (2 preceding siblings ...)
  2025-11-11  5:07 ` [PATCH v3 0/2] 'part name' subcommand and some robustification Anshul Dalal
@ 2025-11-18 20:32 ` Tom Rini
  3 siblings, 0 replies; 10+ messages in thread
From: Tom Rini @ 2025-11-18 20:32 UTC (permalink / raw)
  To: u-boot, Rasmus Villemoes; +Cc: Quentin Schulz

On Mon, 10 Nov 2025 21:54:09 +0100, Rasmus Villemoes wrote:

> Implement a "part name" subcommand, mirroring the existing "part
> number" subcommand.
> 
> In the discussion for v1 of that, it came up that there's a bit of
> inconsistency in how much and what one can assume to be initialized in
> 'struct disk_partition' after a successful call of one of the
> get_info* family of functions. Patch 1/2 tries to consolidate
> that by making sure all ->get_info invocations go through a common
> helper that at least always initializes the string members.
> 
> [...]

Applied to u-boot/next, thanks!

[1/2] disk/part.c: ensure strings in struct disk_partition are valid after successful get_info
      commit: 3c2a9475334ae32997b3bce8d24b39c8ffa9fc67
[2/2] cmd/part.c: implement "part name" subcommand
      commit: 30890051ab23a0293f6404c9a49e86f33e45df66
-- 
Tom



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-11-18 20:32 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-10 20:54 [PATCH v3 0/2] 'part name' subcommand and some robustification Rasmus Villemoes
2025-11-10 20:54 ` [PATCH v3 1/2] disk/part.c: ensure strings in struct disk_partition are valid after successful get_info Rasmus Villemoes
2025-11-11  5:18   ` Anshul Dalal
2025-11-11  7:58     ` Rasmus Villemoes
2025-11-10 20:54 ` [PATCH v3 2/2] cmd/part.c: implement "part name" subcommand Rasmus Villemoes
2025-11-11  5:25   ` Anshul Dalal
2025-11-11  8:00     ` Rasmus Villemoes
2025-11-11 17:27       ` Quentin Schulz
2025-11-11  5:07 ` [PATCH v3 0/2] 'part name' subcommand and some robustification Anshul Dalal
2025-11-18 20:32 ` Tom Rini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.