From: Martin Schwan <M.Schwan@phytec.de>
To: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>,
"u-boot@lists.denx.de" <u-boot@lists.denx.de>,
"upstream@lists.phytec.de" <upstream@lists.phytec.de>
Subject: Re: [PATCH 1/2] bootstd: Add implementation for bootmeth rauc
Date: Thu, 24 Apr 2025 12:43:51 +0000 [thread overview]
Message-ID: <4aef8b9ffd41d4e36550806f4688fc18dbddc500.camel@phytec.de> (raw)
In-Reply-To: <CAFLszTinU=6v+3OyaH0pp8snYm9BgfVrU_+35x2Az7fVs_CfYw@mail.gmail.com>
Hi Simon,
thanks for the review and sorry for my delayed reply.
On Mon, 2025-04-14 at 05:32 -0600, Simon Glass wrote:
> Hi Martin,
>
> On Wed, 29 Jan 2025 at 07:25, Martin Schwan <m.schwan@phytec.de>
> wrote:
> >
> > Add a bootmeth driver which supports booting A/B system with RAUC
> > as
> > their update client.
> >
> > Signed-off-by: Martin Schwan <m.schwan@phytec.de>
> > ---
> > boot/Kconfig | 11 ++
> > boot/Makefile | 1 +
> > boot/bootmeth_rauc.c | 408
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 420 insertions(+)
>
> This looks fine to me. Just some nits below.
>
> But this bootmeth does need documentation in doc/develop/bootstd/ and
Have a look at the second patch in this series, which exactly adds this
documentation:
[PATCH 2/2] doc: Add description for bootmeth rauc
> also a test, e.g. in test/boot/
Thanks, I will look into it.
>
> >
> > diff --git a/boot/Kconfig b/boot/Kconfig
> > index
> > 20935a269c60dad53ce11ac7e58247dc23cf617e..966573d8eb7675d36f299c8c2
> > fa6509bac147cae 100644
> > --- a/boot/Kconfig
> > +++ b/boot/Kconfig
> > @@ -782,6 +782,17 @@ config EXPO
> > The expo can be presented in graphics form using a
> > vidconsole, or in
> > text form on a serial console.
> >
> > +config BOOTMETH_RAUC
> > + bool "Bootdev support for RAUC A/B systems"
> > + default y if BOOTSTD_FULL
> > + depends on CMDLINE
> > + select BOOTMETH_GLOBAL
> > + select HUSH_PARSER
> > + help
> > + Enables support for booting RAUC A/B systems from MMC
> > devices. This
> > + makes the bootdevs look for a 'boot.scr.uimg' or
> > 'boot.scr' in the
> > + respective boot partitions, describing how to boot the
> > distro.
> > +
> > config BOOTMETH_SANDBOX
> > def_bool y
> > depends on SANDBOX
> > diff --git a/boot/Makefile b/boot/Makefile
> > index
> > c2753de8163cf59b1abdc01149a856e2847960e6..427898449a8727747137ab1c7
> > c571c473148c8c3 100644
> > --- a/boot/Makefile
> > +++ b/boot/Makefile
> > @@ -32,6 +32,7 @@ obj-$(CONFIG_$(PHASE_)BOOTMETH_EXTLINUX_PXE) +=
> > bootmeth_pxe.o
> > obj-$(CONFIG_$(PHASE_)BOOTMETH_EFILOADER) += bootmeth_efi.o
> > obj-$(CONFIG_$(PHASE_)BOOTMETH_CROS) += bootm.o bootm_os.o
> > bootmeth_cros.o
> > obj-$(CONFIG_$(PHASE_)BOOTMETH_QFW) += bootmeth_qfw.o
> > +obj-$(CONFIG_$(PHASE_)BOOTMETH_RAUC) += bootmeth_rauc.o
> > obj-$(CONFIG_$(PHASE_)BOOTMETH_SANDBOX) += bootmeth_sandbox.o
> > obj-$(CONFIG_$(PHASE_)BOOTMETH_SCRIPT) += bootmeth_script.o
> > obj-$(CONFIG_$(PHASE_)CEDIT) += cedit.o
> > diff --git a/boot/bootmeth_rauc.c b/boot/bootmeth_rauc.c
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..fcb7e8df4556710b160349f1d
> > cc54cdda9b59c9d
> > --- /dev/null
> > +++ b/boot/bootmeth_rauc.c
> > @@ -0,0 +1,408 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Bootmethod for distro boot with RAUC
> > + *
> > + * Copyright 2025 PHYTEC Messtechnik GmbH
> > + * Written by Martin Schwan <m.schwan@phytec.de>
> > + */
> > +
> > +#define LOG_CATEGORY UCLASS_BOOTSTD
> > +#define LOG_DEBUG
> > +
> > +#include <asm/cache.h>
>
> That should go last.
Okay, will do so in a v2.
>
> > +#include <blk.h>
> > +#include <bootflow.h>
> > +#include <bootmeth.h>
> > +#include <bootstd.h>
> > +#include <dm.h>
> > +#include <env.h>
> > +#include <fs.h>
> > +#include <malloc.h>
> > +#include <mapmem.h>
> > +#include <string.h>
> > +
> > +static const char * const script_names[] = { "boot.scr.uimg",
> > "boot.scr", NULL };
> > +static const char * const rauc_files[] = { "/usr/bin/rauc",
> > "/etc/rauc/system.conf", NULL };
> > +
> > +/**
> > + * struct distro_rauc_slot - Slot information
> > + *
> > + * @name The slot name
> > + * @left The number of boot tries left
> > + * @boot_part The boot partition number on disk
> > + * @root_part The boot partition number on disk
> > + */
> > +struct distro_rauc_slot {
> > + char *name;
> > + ulong left;
> > + int boot_part;
> > + int root_part;
> > +};
> > +
> > +/**
> > + * struct distro_rauc_priv - Private data
> > + *
> > + * @slots All slots of the device in default order
> > + * @boot_order The current boot order string containing the active
> > slot names
> > + */
> > +struct distro_rauc_priv {
> > + struct distro_rauc_slot *slots;
> > + char *boot_order;
> > +};
> > +
> > +static int distro_rauc_check(struct udevice *dev, struct
> > bootflow_iter *iter)
> > +{
> > + /* This distro only works on whole MMC devices, as multiple
> > partitions
> > + * are needed for an A/B system. */
> > + if (bootflow_iter_check_mmc(iter))
> > + return log_msg_ret("mmc", -EOPNOTSUPP);
> > + if (iter->part != 0)
>
> if (iter->part)
Okay!
>
> > + return log_msg_ret("part", -EOPNOTSUPP);
> > +
> > + return 0;
> > +}
> > +
> > +static int distro_rauc_scan_boot_part(struct bootflow *bflow)
> > +{
> > + struct blk_desc *desc;
> > + struct distro_rauc_priv *priv;
> > + int exists;
> > + int i;
> > + int j;
> > + int ret;
> > +
> > + desc = dev_get_uclass_plat(bflow->blk);
> > + if (!desc)
> > + return log_msg_ret("desc", -ENOMEM);
>
> That can't happen as the driver would never be bound in this case,
> please remove.
Will do.
>
> > +
> > + priv = bflow->bootmeth_priv;
> > + if (!priv || !priv->slots || !priv->boot_order)
> > + return log_msg_ret("priv", -EINVAL);
> > +
> > + for (i = 0; priv->slots[i].name != NULL; i++) {
> > + exists = 0;
> > + for (j = 0; script_names[j] != NULL; j++) {
> > + ret = fs_set_blk_dev_with_part(desc, priv-
> > >slots[i].boot_part);
> > + if (ret)
> > + return log_msg_ret("blk", ret);
> > + exists |= fs_exists(script_names[j]);
> > + }
> > + if (!exists)
> > + return log_msg_ret("fs", -ENOENT);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int distro_rauc_scan_root_part(struct bootflow *bflow)
> > +{
> > + struct blk_desc *desc;
> > + struct distro_rauc_priv *priv;
> > + int exists;
> > + int i;
> > + int j;
> > + int ret;
> > +
> > + desc = dev_get_uclass_plat(bflow->blk);
> > + if (!desc)
> > + return log_msg_ret("desc", -ENOMEM);
> > +
> > + priv = bflow->bootmeth_priv;
> > + if (!priv || !priv->slots || !priv->boot_order)
> > + return log_msg_ret("priv", -EINVAL);
> > +
> > + for (i = 0; priv->slots[i].name != NULL; i++) {
> > + exists = 0;
>
> bool exists = false; ?
"exists" is set from fs_exists(), which returns an int and I did not
want to alter the type. Though in this case, that wouldn't do much
harm, as int is the same as bool.
Though I noticed, "exists" seems to be a leftover in the root scanning
part. I will remove it from there.
>
> > + for (j = 0; rauc_files[j] != NULL; j++) {
> > + ret = fs_set_blk_dev_with_part(desc,
> > + priv->slots[i].root_part);
> > + if (ret)
> > + return log_msg_ret("set", ret);
> > + if (!fs_exists(rauc_files[j]))
> > + return log_msg_ret("fs", -ENOENT);
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int distro_rauc_read_bootflow(struct udevice *dev, struct
> > bootflow *bflow)
> > +{
> > + struct distro_rauc_priv *priv;
> > + int ret;
> > + char *slot;
> > + int i;
> > + char *boot_order;
> > + char *partitions;
> > + const char **partitions_list;
> > + char *slots;
> > + const char **slots_list;
> > + char boot_left[32];
> > + char *parts;
> > + ulong default_tries;
> > +
> > + bflow->state = BOOTFLOWST_MEDIA;
> > +
> > + priv = malloc(sizeof(struct distro_rauc_priv));
> > + if (!priv)
> > + return log_msg_ret("buf", -ENOMEM);
> > +
>
> Please run checkpatch or patman to check this patch.
Will do.
>
> > +
> > + partitions = env_get("rauc_partitions");
> > + if (!partitions)
> > + return log_msg_ret("env", -ENOENT);
> > + partitions_list = str_to_list(partitions);
> > +
> > + slots = env_get("rauc_slots");
> > + if (!slots)
> > + return log_msg_ret("env", -ENOENT);
> > + slots_list = str_to_list(slots);
> > +
> > + boot_order = env_get("BOOT_ORDER");
>
> Environment variables should be in lower case.
Unfortunately, we are bound to the naming RAUC uses for these
environment variables, which is upper case. See
https://rauc.readthedocs.io/en/latest/integration.html#set-up-u-boot-boot-script-for-rauc
>
> > + if (!boot_order) {
> > + if (env_set("BOOT_ORDER", slots))
> > + return log_msg_ret("env", -EPERM);
> > + }
> > + priv->boot_order = strdup(boot_order);
> > +
> > + default_tries = env_get_ulong("rauc_slot_default_tries",
> > 10, 3);
> > + for (i = 0; slots_list[i] != NULL; i++) {
> > + sprintf(boot_left, "BOOT_%s_LEFT", slots_list[i]);
>
> same here
>
> > + if (!env_get(boot_left))
> > + env_set_ulong(boot_left, default_tries);
>
> check error
Will do.
>
> > + }
> > +
> > + for (i = 0; slots_list[i] != NULL && partitions_list[i] !=
> > NULL; i++);
> > + priv->slots = malloc((i + 1) * sizeof(struct
> > distro_rauc_slot));
>
> calloc?
Yes, that's better in this situation indeed and would simplify things.
>
> > + if (!priv->slots)
> > + return log_msg_ret("priv", -ENOMEM);
> > + for (i = 0; slots_list[i] != NULL && partitions_list[i] !=
> > NULL; i++) {
> > + priv->slots[i].name = strdup(slots_list[i]);
> > + sprintf(boot_left, "BOOT_%s_LEFT", slot);
> > + priv->slots[i].left = env_get_ulong(boot_left, 10,
> > default_tries);
> > + parts = strdup(partitions_list[i]);
> > + priv->slots[i].boot_part =
> > simple_strtoul(strsep(&parts, ","), NULL, 10);
> > + priv->slots[i].root_part =
> > simple_strtoul(strsep(&parts, ","), NULL, 10);
> > + free(parts);
> > + parts = NULL;
> > + }
> > + priv->slots[i].name = NULL;
> > + priv->slots[i].left = 0;
> > + priv->slots[i].boot_part = 0;
> > + priv->slots[i].root_part = 0;
> > +
> > + bflow->bootmeth_priv = priv;
> > + bflow->state = BOOTFLOWST_FS;
> > +
> > + ret = distro_rauc_scan_boot_part(bflow);
> > + if (ret < 0)
> > + goto free_priv;
> > + ret = distro_rauc_scan_root_part(bflow);
> > + if (ret < 0)
> > + goto free_priv;
> > +
> > + bflow->state = BOOTFLOWST_READY;
> > +
> > + return 0;
> > +
> > +free_priv:
> > + for (i = 0; priv->slots[i].name != NULL; i++)
> > + free(priv->slots[i].name);
> > + free(priv->slots);
> > + free(priv);
> > + bflow->bootmeth_priv = NULL;
>
> blank line here
Okay!
>
> > + return ret;
> > +}
> > +
> > +static int distro_rauc_read_file(struct udevice *dev, struct
> > bootflow *bflow,
> > + const char *file_path, ulong addr,
> > + enum bootflow_img_t type, ulong
> > *sizep)
> > +{
> > + /* Reading individual files is not supported since we only
> > operate on
> > + * whole MMC devices (because we require multiple
> > partitions)
> > + */
> > + return log_msg_ret("Unsupported", -ENOSYS);
> > +}
> > +
> > +static int distro_rauc_load_boot_script(struct bootflow *bflow,
> > const char *slot)
>
> please as a comment, as I'm not sure what a slot is
In short, this refers to RAUC's definition of a "slot". I will add a
comment in v2.
>
> > +{
> > + struct blk_desc *desc;
> > + struct distro_rauc_priv *priv;
> > + const char *prefix = "/";
> > + int ret;
> > + int i;
> > +
> > + if (!slot)
> > + return log_msg_ret("slot", -EINVAL);
> > +
> > + desc = dev_get_uclass_plat(bflow->blk);
> > + priv = bflow->bootmeth_priv;
> > +
> > + bflow->part = 0;
>
> Isn't it already 0 ?
Seems so. I will check again and remove it, if not needed.
>
> > + for (i = 0; priv->slots[i].name != NULL; i++) {
> > + if (strcmp(priv->slots[i].name, slot) == 0) {
> > + bflow->part = priv->slots[i].boot_part;
> > + break;
> > + }
> > + }
> > + if (!bflow->part)
> > + return log_msg_ret("part", -ENOENT);
> > +
> > + ret = bootmeth_setup_fs(bflow, desc);
> > + if (ret)
> > + return log_msg_ret("set", ret);
> > +
> > + for (i = 0; script_names[i] != NULL; i++) {
>
> Drop the '!= NULL' here and above
Will do.
>
> > + if (!bootmeth_try_file(bflow, desc, prefix,
> > script_names[i]))
> > + break;
> > + }
> > + if (bflow->state != BOOTFLOWST_FILE)
> > + return log_msg_ret("file", -ENOENT);
> > +
> > + bflow->subdir = strdup(prefix);
> > +
> > + ret = bootmeth_alloc_file(bflow, 0x10000,
> > ARCH_DMA_MINALIGN,
> > + (enum
> > bootflow_img_t)IH_TYPE_SCRIPT);
> > + if (ret)
> > + return log_msg_ret("read", ret);
> > +
> > + return 0;
> > +}
> > +
> > +static int decrement_slot_tries(const char *slot)
> > +{
> > + ulong tries;
> > + char boot_left[32];
> > +
> > + sprintf(boot_left, "BOOT_%s_LEFT", slot);
>
> lower case
>
> > + tries = env_get_ulong(boot_left, 10, ULONG_MAX);
> > + if (tries == ULONG_MAX)
> > + return log_msg_ret("env", -ENOENT);
> > + if (tries <= 0)
> > + return 0;
> > +
> > + return env_set_ulong(boot_left, tries - 1);
> > +}
> > +
> > +static int distro_rauc_boot(struct udevice *dev, struct bootflow
> > *bflow)
> > +{
> > + struct blk_desc *desc;
> > + struct distro_rauc_priv *priv;
> > + char *boot_order;
> > + const char **boot_order_list;
> > + const char *active_slot;
> > + char raucargs[32];
> > + ulong addr;
> > + int ret = 0;
> > + int i;
> > +
> > + desc = dev_get_uclass_plat(bflow->blk);
> > + if (desc->uclass_id != UCLASS_MMC)
> > + return log_msg_ret("blk", -EINVAL);
> > + priv = bflow->bootmeth_priv;
> > +
> > + /* RAUC variables */
> > + boot_order = env_get("BOOT_ORDER");
> > + if (!boot_order)
> > + return log_msg_ret("env", -ENOENT);
> > + boot_order_list = str_to_list(boot_order);
> > +
> > + /* Device info variables */
> > + ret = env_set("devtype", blk_get_devtype(bflow->blk));
> > + if (ret)
> > + return log_msg_ret("env", ret);
> > +
> > + ret = env_set_hex("devnum", desc->devnum);
> > + if (ret)
> > + return log_msg_ret("env", ret);
> > +
> > + /* Kernel command line arguments */
> > + active_slot = strdup(boot_order_list[0]);
> > + if (!active_slot)
> > + return log_msg_ret("env", -ENOENT);
> > + sprintf(raucargs, "rauc.slot=%s", active_slot);
> > + ret = env_set("raucargs", raucargs);
> > + if (ret)
> > + return log_msg_ret("env", ret);
> > +
> > + /* TODO: If active_slot is in boot_order, but has 0 tries
> > left, remove
>
> /*
> * TODO: ...
Okay!
Thanks again!
Regards,
Martin
>
> > + * it from boot_order. Then, set active_slot to other, if
> > available.
> > + */
> > +
> > + /* Partition info variables */
> > + for (i = 0; priv->slots[i].name != NULL; i++) {
> > + if (strcmp(priv->slots[i].name, active_slot) == 0)
> > {
> > + ret = env_set_hex("mmcroot", priv-
> > >slots[i].root_part);
> > + if (ret)
> > + return log_msg_ret("env", ret);
> > + break;
> > + }
> > + }
> > +
> > + /* Load distro boot script */
> > + ret = distro_rauc_load_boot_script(bflow, active_slot);
> > + if (ret)
> > + return log_msg_ret("load", ret);
> > + ret = env_set_hex("distro_bootpart", bflow->part);
> > + if (ret)
> > + return log_msg_ret("env", ret);
> > +
> > + ret = decrement_slot_tries(active_slot);
> > + if (ret)
> > + return log_msg_ret("env", ret);
> > + ret = env_save();
> > + if (ret)
> > + return log_msg_ret("env", ret);
> > +
> > + log_debug("devtype: %s\n", env_get("devtype"));
> > + log_debug("devnum: %s\n", env_get("devnum"));
> > + log_debug("distro_bootpart: %s\n",
> > env_get("distro_bootpart"));
> > + log_debug("mmcroot: %s\n", env_get("mmcroot"));
> > + log_debug("raucargs: %s\n", env_get("raucargs"));
> > + log_debug("rauc_slots: %s\n", env_get("rauc_slots"));
> > + log_debug("rauc_partitions: %s\n",
> > env_get("rauc_partitions"));
> > + log_debug("rauc_slot_default_tries: %s\n",
> > env_get("rauc_slot_default_tries"));
> > + log_debug("BOOT_ORDER: %s\n", env_get("BOOT_ORDER"));
> > + for (i = 0; priv->slots[i].name != NULL; i++) {
> > + log_debug("BOOT_%s_LEFT: %ld\n", priv-
> > >slots[i].name,
> > + priv->slots[i].left);
> > + }
> > +
> > + /* Run distro boot script */
> > + addr = map_to_sysmem(bflow->buf);
> > + ret = cmd_source_script(addr, NULL, NULL);
> > + if (ret)
> > + return log_msg_ret("boot", ret);
> > +
> > + return 0;
> > +}
> > +
> > +static int distro_rauc_bootmeth_bind(struct udevice *dev)
> > +{
> > + struct bootmeth_uc_plat *plat = dev_get_uclass_plat(dev);
> > +
> > + plat->desc = "RAUC distro boot from MMC";
> > + plat->flags = BOOTMETHF_ANY_PART;
> > +
> > + return 0;
> > +}
> > +
> > +static struct bootmeth_ops distro_rauc_bootmeth_ops = {
> > + .check = distro_rauc_check,
> > + .read_bootflow = distro_rauc_read_bootflow,
> > + .read_file = distro_rauc_read_file,
> > + .boot = distro_rauc_boot,
> > +};
> > +
> > +static const struct udevice_id distro_rauc_bootmeth_ids[] = {
> > + { .compatible = "u-boot,distro-rauc" },
> > + { }
> > +};
> > +
> > +U_BOOT_DRIVER(bootmeth_rauc) = {
> > + .name = "bootmeth_rauc",
> > + .id = UCLASS_BOOTMETH,
> > + .of_match = distro_rauc_bootmeth_ids,
> > + .ops = &distro_rauc_bootmeth_ops,
> > + .bind = distro_rauc_bootmeth_bind,
> > +};
> >
> > --
> > 2.48.1
> >
>
> Regards,
> SImon
next prev parent reply other threads:[~2025-04-24 12:44 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-29 14:25 [PATCH 0/2] bootstd: New bootmeth for RAUC A/B systems Martin Schwan
2025-01-29 14:25 ` [PATCH 1/2] bootstd: Add implementation for bootmeth rauc Martin Schwan
2025-01-29 16:03 ` Tom Rini
2025-01-30 9:48 ` Martin Schwan
2025-01-30 14:51 ` Tom Rini
2025-04-14 5:49 ` Wadim Egorov
2025-04-14 11:32 ` Simon Glass
2025-04-24 12:43 ` Martin Schwan [this message]
2025-04-24 18:31 ` Simon Glass
2025-04-24 18:49 ` Tom Rini
2025-04-25 14:35 ` Simon Glass
2025-04-25 14:37 ` Tom Rini
2025-04-25 15:03 ` Simon Glass
2025-04-25 16:19 ` Tom Rini
2025-04-28 8:25 ` Martin Schwan
2025-04-28 15:39 ` Tom Rini
2025-04-29 14:19 ` Simon Glass
2025-04-28 8:35 ` Alexander Dahl
2025-05-06 7:05 ` [Upstream] " Wadim Egorov
2025-01-29 14:25 ` [PATCH 2/2] doc: Add description " Martin Schwan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4aef8b9ffd41d4e36550806f4688fc18dbddc500.camel@phytec.de \
--to=m.schwan@phytec.de \
--cc=sjg@chromium.org \
--cc=trini@konsulko.com \
--cc=u-boot@lists.denx.de \
--cc=upstream@lists.phytec.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.