Hi Denis,
On 2026-06-05T03:38:59, None <[email protected]> wrote:
> bootcount: gpt: introduce GPT-backed bootcount device
>
> Introduce bootcount device backed by GPT PTE attributes (bits 48:63).
>
> Signed-off-by: Denis Mukhin <[email protected]>
>
> drivers/bootcount/Kconfig | 7 ++
> drivers/bootcount/Makefile | 1 +
> drivers/bootcount/bootcount-gpt.c | 140
> ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 148 insertions(+)
> diff --git a/drivers/bootcount/bootcount-gpt.c
> b/drivers/bootcount/bootcount-gpt.c
> @@ -0,0 +1,140 @@
> +static int bootcount_gpt_get(struct udevice *dev, u32 *val)
> +{
> + struct bootcount_gpt_priv *priv = dev_get_priv(dev);
> + int rc;
> +
> + if (!priv->blk_desc) {
> + rc = bootcount_gpt_find(dev);
> + if (rc < 0)
> + return rc;
> + }
> +
> + rc = read_disk_flags(priv->blk_desc, priv->partnum, (u16 *)val);
Not portable. read_disk_flags() writes 16 bits at the address; on
big-endian that lands in the high half of *val, and the other half is
left uninitialised either way. Please use a local u16, then *val =
local
> diff --git a/drivers/bootcount/bootcount-gpt.c
> b/drivers/bootcount/bootcount-gpt.c
> @@ -0,0 +1,140 @@
> +static int bootcount_gpt_set(struct udevice *dev, const u32 val)
> +{
> + struct bootcount_gpt_priv *priv = dev_get_priv(dev);
> + int rc;
> +
> + if (!priv->blk_desc) {
> + rc = bootcount_gpt_find(dev);
> + if (rc < 0)
> + return rc;
> + }
> +
> + pr_debug("partnum %d: write GPT flags 0x%04x\n", priv->partnum, val);
> +
> + rc = write_disk_flags(priv->blk_desc, priv->partnum, (u16)val);
The driver passes the raw 16 bits straight through but does not use
the cros_flags helpers added in patch 1. The standard generic path is
roughly:
val = bootcount_load();
bootcount_store(val + 1);
Against this driver, that increments the whole 16-bit attribute word,
so priority (bits 0..3), successful (bit 8) etc. all get clobbered the
moment the tries field overflows nibble 1. Either the get/set ops
should isolate the tries field via
cros_flags_get_tries()/cros_flags_set_tries(), or the commit message
needs to spell out that this driver is intended only for callers that
know the cgpt layout and that bootcount_inc()/bootcount_error() must
not be used (which is what patch 8 enforces). As it stands the
relationship between this driver and patch 1 is not obvious.
> diff --git a/drivers/bootcount/bootcount-gpt.c
> b/drivers/bootcount/bootcount-gpt.c
> @@ -0,0 +1,140 @@
> +static int bootcount_gpt_probe(struct udevice *dev)
> +{
> + struct bootcount_gpt_priv *priv = dev_get_priv(dev);
> + const char *str;
> + int uclass_id, devnum;
> +
> + str = dev_read_string(dev, 'device-type');
> + if (!str)
> + return -ENOSYS;
> +
> + uclass_id = uclass_get_by_name(str);
> + if (uclass_id == UCLASS_INVALID)
> + return -ENOSYS;
> +
> + devnum = dev_read_s32_default(dev, 'devnum', 0);
> + if (devnum < 0)
> + return -ENOSYS;
> +
> + str = dev_read_string(dev, 'partition-name');
> + if (!str)
> + return -ENOSYS;
Please move the DT parsing into an of_to_plat() callback (and a plat
struct), matching bootcount-syscon and the rest of DM. Also -ENOSYS is
the wrong errno for missing/invalid DT properties - use -EINVAL.
dev_err() rather than pr_err()/pr_warn() throughout would identify
which bootcount device is complaining.
> diff --git a/drivers/bootcount/bootcount-gpt.c
> b/drivers/bootcount/bootcount-gpt.c
> @@ -0,0 +1,140 @@
> + if (info.bootable & PART_BOOTABLE) {
> + pr_warn("partition %s: legacy boot flag enabled, no GPT
> accounting\n",
> + priv->partition_name);
> + return -EPERM;
> + }
Why does the legacy-BIOS bootable bit disqualify the partition from
holding a counter in the top 16 bits of attributes? Those bits are
independent of bit 2. If there is a real reason please put it in a
comment; otherwise drop the check. Also -EPERM reads oddly here.
> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
> @@ -171,6 +171,13 @@ config DM_BOOTCOUNT_ZYNQMP
> +config DM_BOOTCOUNT_GPT
> + bool "Support GPT PTE-backed boot counter"
> + depends on EFI_PARTITION
> + help
> + Add support for maintaining boot count within bits 48:63 of GUID
> + partition table entry attributes (similarly to ChromiumOS).
The commit message could explain the motivation (why GPT, why bits
48:63, relationship to the cgpt layout) rather than restating the
subject line.
> diff --git a/drivers/bootcount/bootcount-gpt.c
> b/drivers/bootcount/bootcount-gpt.c
> @@ -0,0 +1,140 @@
> + priv->blk_desc = dev_get_uclass_plat(bdev);
> + priv->partnum = partnum;
> +
> + return 0;
> +}
blk_desc and partnum are cached on first get/set and never refreshed.
If the underlying block device is re-probed (or the disk
re-partitioned) the driver keeps using a stale descriptor. At minimum
worth a comment; ideally a .remove that clears the cache, or simply
re-resolve on every call since this is not a hot path.
Regards,
Simon
