[U-Boot] [PATCH v2 4/7] cmd: Add 'ab_select' command

[Igor Opaniuk]

Hello Ruslan,

On Wed, 12 Dec 2018 at 19:12, Ruslan Trofymenko
<ruslan.trofymenko at linaro.org> wrote:
>
> For A/B system update support the Android boot process requires to send
> 'androidboot.slot_suffix' parameter as a command line argument. This
> patch implementes 'ab_select' command which allows us to obtain current
> slot by processing the A/B metadata.
>
> The patch was extracted from commit [1] with one modification: the
> separator for specifying the name of metadata partition was changed
> from ';' to '#', because ';' is used for commands separation.

minor: so why not to add Alistair's Singed-off-by tag also?
>
> [1] https://android-review.googlesource.com/c/platform/external/u-boot/+/729880/2
>
> Signed-off-by: Ruslan Trofymenko <ruslan.trofymenko at linaro.org>
> Reviewed-by: Alistair Strachan <astrachan at google.com>
> Reviewed-by: Sam Protsenko <semen.protsenko at linaro.org>
> ---
> Changes in v2:
>   * 'android_ab_select' command is renamed to 'ab_select' command
>   * command is moved to the separate 'Android support commands' menu
>
>  cmd/Kconfig     | 15 +++++++++++++++
>  cmd/Makefile    |  1 +
>  cmd/ab_select.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 68 insertions(+)
>  create mode 100644 cmd/ab_select.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index ea1a325..ed60e1e 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1123,6 +1123,21 @@ config CMD_SETEXPR
>
>  endmenu
>
> +menu "Android support commands"
as a suggestion: use existing menu "Boot commands" ?

> +
> +config CMD_AB_SELECT
> +       bool "ab_select"
> +       default n
> +       depends on ANDROID_AB
> +       help
> +         On Android devices with more than one boot slot (multiple copies of
> +         the kernel and system images) this provides a command to select which
> +         slot should be used to boot from and register the boot attempt. This
> +         is used by the new A/B update model where one slot is updated in the
> +         background while running from the other slot.
> +
> +endmenu
> +
>  if NET
>
>  menuconfig CMD_NET
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 4998643..1d345c1 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -12,6 +12,7 @@ obj-y += version.o
>
>  # command
>  obj-$(CONFIG_CMD_AES) += aes.o
> +obj-$(CONFIG_CMD_AB_SELECT) += ab_select.o
>  obj-$(CONFIG_CMD_ADC) += adc.o
>  obj-$(CONFIG_CMD_ARMFLASH) += armflash.o
>  obj-y += blk_common.o
> diff --git a/cmd/ab_select.c b/cmd/ab_select.c
> new file mode 100644
> index 0000000..2a9e524
> --- /dev/null
> +++ b/cmd/ab_select.c
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: BSD-2-Clause
> +/*
> + * Copyright (C) 2017 The Android Open Source Project
> + */
> +
> +#include <android_ab.h>
> +#include <command.h>
> +
> +static int do_ab_select(cmd_tbl_t *cmdtp, int flag, int argc,
> +                       char * const argv[])
> +{
> +       int ret;
> +       struct blk_desc *dev_desc;
> +       disk_partition_t part_info;
> +       char slot[2];
> +
> +       if (argc != 4)
> +               return CMD_RET_USAGE;
> +
> +       /* Lookup the "misc" partition from argv[2] and argv[3] */
> +       if (part_get_info_by_dev_and_name_or_num(argv[2], argv[3],
> +                                                &dev_desc, &part_info) < 0) {
> +               return CMD_RET_FAILURE;
> +       }

minor: probably checkpatch will argue and ask to remove the check for
{} on single line if.

> +
> +       ret = ab_select_slot(dev_desc, &part_info);
> +       if (ret < 0) {
> +               printf("Android boot failed, error %d.\n", ret);
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       /* Android standard slot names are 'a', 'b', ... */
> +       slot[0] = ANDROID_BOOT_SLOT_NAME(ret);
> +       slot[1] = '\0';
> +       env_set(argv[1], slot);
> +       printf("ANDROID: Booting slot: %s\n", slot);
minor: too much stuff in a row, better to split filling of slot
buffer, environment variable setting and return with blank lines.
> +       return CMD_RET_SUCCESS;
> +}
> +
> +U_BOOT_CMD(ab_select, 4, 0, do_ab_select,
> +          "Select the slot used to boot from and register the boot attempt.",
> +          "<slot_var_name> <interface> <dev[:part|#part_name]>\n"
> +          "    - Load the slot metadata from the partition 'part' on\n"
> +          "      device type 'interface' instance 'dev' and store the active\n"
> +          "      slot in the 'slot_var_name' variable. This also updates the\n"
> +          "      Android slot metadata with a boot attempt, which can cause\n"
> +          "      successive calls to this function to return a different result\n"
> +          "      if the returned slot runs out of boot attempts.\n"
> +          "    - If 'part_name' is passed, preceded with a # instead of :, the\n"
> +          "      partition name whose label is 'part_name' will be looked up in\n"
> +          "      the partition table. This is commonly the \"misc\" partition.\n"
> +);
> --
> 2.7.4
>

With/without my minor comments addressed:
Reviewed-by: Igor Opaniuk <igor.opaniuk at linaro.org>

-- 
Regards,
Igor Opaniuk