Hi Denis,
On 2026-06-05T03:38:59, None <[email protected]> wrote:
> cros: add common helpers for GPT flags
>
> Add common helper to decode/encode ChromuimOS-style GPT attributes
> similarly to [1].
>
> [1]
> https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/master/cgpt
> Signed-off-by: Denis Mukhin <[email protected]>
>
> include/cros_flags.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 74 insertions(+)
> diff --git a/include/cros_flags.h b/include/cros_flags.h
> @@ -0,0 +1,74 @@
> + * Bit definitions and masks for GPT attributes for Chromium OS.
> + *
> + * 63-61 -- (reserved)
> + * 60 -- read-only
> + * 59-58 -- (reserved)
> + * 57 -- error counter
> + * 56 -- success
> + * 55-52 -- tries
> + * 51-48 -- priority
The table describes bit positions in the full 64-bit GPT attribute
field, but the masks and helpers below operate on a 16-bit value (the
top 16 bits, already shifted down to bits 0-15). So priority lives at
bits 0-3 here, not 48-51. Please reconcile the two - either
re-document the table in terms of the 16-bit value the helpers
consume, or have the helpers take the full 64-bit attribute and shift
internally. As written, a reader who trusts the comment writes wrong
code.
> diff --git a/include/cros_flags.h b/include/cros_flags.h
> @@ -0,0 +1,74 @@
> +#ifndef _CROS_FLAGS_H
> +#define _CROS_FLAGS_H
> +
> +#define CROS_FLAG_PRIORITY_MASK 0x000fU
The header uses bool without including <stdbool.h> or <linux/types.h>,
so it is not self-contained - it only compiles where the includer
happens to have pulled those in first. Please add the include.
> diff --git a/include/cros_flags.h b/include/cros_flags.h
> @@ -0,0 +1,74 @@
> + * 57 -- error counter
> + * 56 -- success
Re bit 57 the cover letter talks about counters/flags logic, but there
are no get/set helpers for it. Does that mean it is not actually used?
> diff --git a/include/cros_flags.h b/include/cros_flags.h
> @@ -0,0 +1,74 @@
> +static inline unsigned int
> +cros_flags_set_priority(unsigned int flags, unsigned int priority)
You can likely use uint here if it is only compiled for U-Boot and not
the tools.
> +{
> + flags &= ~CROS_FLAG_PRIORITY_MASK;
> + flags |= (priority & 0xf) << CROS_FLAG_PRIORITY_SHIFT;
> + return flags;
> +}
The bare 0xf (and 0xf in set_tries()) duplicates the field width
already encoded in CROS_FLAG_PRIORITY_MASK. Please express this as
(CROS_FLAG_PRIORITY_MASK >> CROS_FLAG_PRIORITY_SHIFT) so the width
lives in one place.
> diff --git a/include/cros_flags.h b/include/cros_flags.h
> @@ -0,0 +1,74 @@
> +static inline bool
> +cros_flags_get_successful(unsigned int flags)
> +{
> + return (flags & CROS_FLAG_SUCCESSFUL_MASK) >>
> CROS_FLAG_SUCCESSFUL_SHIFT;
> +}
Simpler as !!(flags & CROS_FLAG_SUCCESSFUL_MASK) - no shift needed,
and the bool conversion is explicit.
> Add common helper to decode/encode ChromuimOS-style GPT attributes
> similarly to [1].
Typo - ChromuimOS should be ChromiumOS (or the old Chromium OS, which
is what the header uses).
Regards,
Simon
