* [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.