[PATCH v5 6/6] common: android_ab: fix slot suffix for abc block

[Sam Protsenko]

On Tue, Nov 5, 2024 at 9:06 AM Mattijs Korpershoek
<mkorpershoek at baylibre.com> wrote:
>
> Hi Sam,
>

Hey Mattijs,

[snip]

> >> @@ -328,7 +328,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
> >>                  * or the device tree.
> >>                  */
> >>                 memset(slot_suffix, 0, sizeof(slot_suffix));
> >> -               slot_suffix[0] = BOOT_SLOT_NAME(slot);
> >> +               slot_suffix[0] = '_';
> >> +               slot_suffix[1] = BOOT_SLOT_NAME(slot);
> >
> > AFAIU, this changes the behavior of two above functions, and
> > consequently of "bcb ab_select" command? If so, just to double check:
> > were all users of those reworked correspondingly? I can see next
> > occurrences (there may be more):
> >
> > $ grep -sIrHn '"_' boot/bootmeth_android.c
>
> I thought the same when first reviewing the patch.
> Looking a bit closer...
>
> >
> > boot/bootmeth_android.c:74:    sprintf(partname, BOOT_PART_NAME "_%s",
> > priv->slot);
> > boot/bootmeth_android.c:111:    sprintf(partname,
> > VENDOR_BOOT_PART_NAME "_%s", priv->slot);
> > boot/bootmeth_android.c:156:    sprintf(slot_suffix, "_%s", priv->slot);
> > boot/bootmeth_android.c:397:    sprintf(slot_suffix, "_%s", priv->slot);
>
> ... We can see that ab_select_slot() returns an integer
> That integer is used later on to initialize priv->slot:
>
> """
>         priv->slot[0] = BOOT_SLOT_NAME(ret);
>         priv->slot[1] = '\0';
> """
>
> The change from Dmitry only changes what we **write** to the BCB (into
> the misc partition), not what is returned by ab_select_slot().
>

Sure. Just wanted to double check that the behavior is not changed in
any related parts, as the commit message doesn't mention that. Btw,
BCB is an interface between the bootloader and AOSP, so if this patch
changes what's being written into BCB, does it affect AOSP part of it
somehow? Especially for already existing devices with particular BCB
data, in case U-Boot gets updated there.

> ab_select_slot() still returns an integer which needs to be converted
> via the BOOT_SLOT_NAME() macro.
>
> >
> > $ grep -sIrHn 'slot_suffix _' include/configs/
> > include/configs/ti_omap5_common.h:107:    "setenv slot_suffix _${slot_name};"
> > include/configs/meson64_android.h:65:        "setenv slot_suffix
> > _${current_slot}; " \
>
> Same goes for these 2 examples, we use:
> The "bcb ab_select current_slot" command to store the slot into the
> "current_slot" environment variable.
> Looking at cmd/bcb.c we can see:
>
> """
>         ret = ab_select_slot(dev_desc, &part_info, dec_tries);
>         if (ret < 0) {
>                 printf("Android boot failed, error %d.\n", ret);
>                 return CMD_RET_FAILURE;
>         }
>
>         /* Android standard slot names are 'a', 'b', ... */
>         slot[0] = BOOT_SLOT_NAME(ret);
>         slot[1] = '\0';
>         env_set(argv[1], slot);
>         printf("ANDROID: Booting slot: %s\n", slot);
> """
>
> So I think this is fine.
>

[snip]