Hi Denis,
On 2026-06-05T03:38:59, None <[email protected]> wrote:
> cmd: gpt: enable extension to manipulate GPT PTE attributes
>
> Enable extension for existing 'gpt' command to manipulate top 16
> bits of GPT PTE attributes to assist ChromiumOS-style boot counters.
>
> Follow the existing syntax for specifying <interface, device, partition>
> tuple.
>
> Signed-off-by: Denis Mukhin <[email protected]>
>
> cmd/Kconfig | 8 +++++
> cmd/gpt.c | 97
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 105 insertions(+)
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> @@ -11,6 +11,9 @@
> */
>
> #include <blk.h>
> +#ifdef CONFIG_CMD_GPT_FLAGS
> +#include <cros_flags.h>
> +#endif
> #include <env.h>
Please drop the #ifdef - header guards make it harmless to include
unconditionally, and U-Boot generally avoids conditional includes for
plain headers.
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> @@ -1116,6 +1119,64 @@ out:
> +static int do_set_gpt_flags(struct blk_desc *desc, int partnum,
> + const char *key, int value)
The continuation here is indented with spaces instead of a tab plus
spaces. Same problem in the pr_err() call below and at the closing
brace:
> + pr_err("ERROR: partnum %d: failed to write GPT flags: %d\n",
> + partnum, rc);
> + return CMD_RET_FAILURE;
> + }
Spaces again.
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> @@ -1116,6 +1119,64 @@ out:
> + rc = write_disk_flags(desc, partnum, guid_flags);
> + if (rc < 0) {
> + pr_err("ERROR: partnum %d: failed to write GPT flags: %d\n",
> + partnum, rc);
Why pr_err() here, but printf() for the read-flags error a few lines
up and inside do_get_gpt_flags()? Please pick one - this is a command
handler, so printf() seems more consistent with the rest of the file.
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> @@ -1116,6 +1119,64 @@ out:
> + if (!strcmp(key, 'set-flags')) {
> + guid_flags = (u16)(value & 0xffff);
> + } else if (!strcmp(key, 'set-priority')) {
> + guid_flags = cros_flags_set_priority(guid_flags, value);
> + } else if (!strcmp(key, 'set-successful')) {
> + guid_flags = cros_flags_set_successful(guid_flags, value);
> + } else if (!strcmp(key, 'set-tries')) {
> + guid_flags = cros_flags_set_tries(guid_flags, value);
The help text advertises ranges like [0..15] and {0,1}, but nothing
here rejects out-of-range input...the cros_flags_set_*() helpers
silently mask to 4 bits, and set-flags silently truncates to 16 bits.
Please validate value against the documented range and return
CMD_RET_USAGE on bad input.
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> @@ -1176,6 +1237,23 @@ static int do_gpt(struct cmd_tbl *cmdtp, int flag, int
> argc, char *const argv[])
> + } else if (!strcmp(argv[1], 'get-flags')) {
> + if (argc < 5)
> + return CMD_RET_FAILURE;
> + ret = do_get_gpt_flags(blk_dev_desc,
> + simple_strtol(argv[4], NULL, 0));
> + } else if (!strcmp(argv[1], 'set-flags') ||
> + !strcmp(argv[1], 'set-priority') ||
> + !strcmp(argv[1], 'set-successful') ||
> + !strcmp(argv[1], 'set-tries')) {
> + if (argc < 6)
> + return CMD_RET_FAILURE;
The other branches fall through to CMD_RET_USAGE on bad argument
counts (see the top of do_gpt()). Returning CMD_RET_FAILURE here means
the user gets 'error!' instead of the usage hint. Please use
CMD_RET_USAGE
Also, do_gpt() already gates argc at 4..5 (or 4..6 with
CMD_GPT_RENAME) before any dispatch. From what I can tell, with
CMD_GPT_FLAGS but not CMD_GPT_RENAME, 'gpt set-flags <if> <dev> <part>
<val>' has argc == 6 and will be rejected up front. Please extend the
early argc check so the new subcommands can actually be reached.
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> @@ -1176,6 +1237,23 @@ static int do_gpt(struct cmd_tbl *cmdtp, int flag, int
> argc, char *const argv[])
> + ret = do_get_gpt_flags(blk_dev_desc,
> + simple_strtol(argv[4], NULL, 0));
simple_strtol() has no failure indication - a typo in the partition
number silently becomes 0 and writes flags to the wrong partition. The
existing code at the top of do_gpt() uses dectoul() with an endptr
check for argv[3] - please do the same for argv[4] (and argv[5] in the
set case) and reject non-numeric input.
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> @@ -1176,6 +1237,23 @@ static int do_gpt(struct cmd_tbl *cmdtp, int flag, int
> argc, char *const argv[])
> + ret = do_set_gpt_flags(blk_dev_desc, /* argv[2,3] */
> + simple_strtol(argv[4], NULL, 0),
> + argv[1],
> + simple_strtol(argv[5], NULL, 0));
The /* argv[2,3] */ comment is cryptic - argv[2] and argv[3] have
already been consumed via blk_get_dev() above. Please drop it.
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> @@ -1237,4 +1315,23 @@ U_BOOT_CMD(gpt, CONFIG_SYS_MAXARGS, 1, do_gpt,
> +#ifdef CONFIG_CMD_GPT_FLAGS
> + "gpt partition boot flag manipulation:\n"
> + " gpt get-flags <interface> <dev> <partid>\n"
> + " - read top 16-bit of partition attributes (bits 48:63)\n"
> + " gpt set-flags <interface> <dev> <partid> <flags-in-hex>\n"
> + " - write top 16-bit of partition attributes (bits 48:63)\n"
Minor: 'top 16-bit' reads as singular - please use 'top 16 bits'. The
flags occupy bits 48..63 in the on-disk attribute, but the helpers and
the command operate on a u16 - worth saying so explicitly so users do
not try to pass a 64-bit value.
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> @@ -1319,6 +1319,14 @@ config CMD_GPT
> +config CMD_GPT_FLAGS
> + bool "GPT partition flags commands"
> + depends on CMD_GPT
> + select PARTITION_TYPE_GUID
> + help
> + Enables the GUID partition table attributes manipulation (bits 48:63)
> + in a ChromiumOS-style manner.
Just to check - why select PARTITION_TYPE_GUID? The new subcommands
operate on the attribute u64 via {read,write}_disk_flags() and do not
appear to look at the partition-type GUID, even though I assume other
things would break without it. If the dependency is real, please add a
sentence to the help text; if not, drop the select.
Regards,
Simon
