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

On Wed, Nov 6, 2024 at 4:02 AM Mattijs Korpershoek
<[email protected]> wrote:
>
> Hi Sam,
>
> On mar., nov. 05, 2024 at 18:58, Sam Protsenko <[email protected]> wrote:
>
> > On Tue, Nov 5, 2024 at 9:06 AM Mattijs Korpershoek
> > <[email protected]> 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.
>
> Those are valid concerns.
>
> Per my understanding, on recent Android versions the slot suffix is not
> read from BCB, but from the ro.boot.slot_suffix property:
>

That probably still leaves the possible combination of some devices
running new U-Boot versions (with this patch applied) together with
older Android versions. E.g. in case when U-Boot is updated but
Android isn't, may be especially relevant for some dev boards out
there.

> """
>   // Initialize the current_slot from the read-only property. If the property
>   // was not set (from either the command line or the device tree), we can later
>   // initialize it from the bootloader_control struct.

So even in recent Android versions it's being initialized from BCB in
case the property is not set.

>   std::string suffix_prop = android::base::GetProperty("ro.boot.slot_suffix", "");
>   if (suffix_prop.empty()) {
>     LOG(ERROR) << "Slot suffix property is not set";
>     return false;
>   }
>   current_slot_ = SlotSuffixToIndex(suffix_prop.c_str());
> """
>
> See:
> https://cs.android.com/android/platform/superproject/main/+/main:hardware/interfaces/boot/1.1/default/boot_control/libboot_control.cpp;l=185;drc=86b8f575059a1799c760ca7012f540a528d68a9d;bpv=1;bpt=1
>
> This has been the case since 2019.
>
> If we look at earlier implementations of libboot_control (which was in
> bootable/recovery)
> https://android-review.googlesource.com/c/platform/bootable/recovery/+/1191517
>
> So implementations before 2019 that do not have this patch:
> https://android-review.googlesource.com/c/platform/bootable/recovery/+/1111899
>
> Will get the slot suffix from the BCB (not from the commandline)
>
> For these older implementations, we will go through the following:
> BootControl::Init()
>   LoadBootloaderControl(device.c_str(), &boot_ctrl)
>   android::base::ReadFully(fd.get(), buffer, sizeof(bootloader_control)
>
> And struct bootloader_control has:
>
> struct bootloader_control {
>     // NUL terminated active slot suffix.
>     char slot_suffix[4];
>
> And if we look at how the BCB is initialized from userspace in:
> https://cs.android.com/android/platform/superproject/main/+/main:hardware/interfaces/boot/1.1/default/boot_control/libboot_control.cpp;l=120;drc=86b8f575059a1799c760ca7012f540a528d68a9d
>
> We can see that we copy _a, not a (for example, if slot == 0).
>
> So I think this is fine.
>
> If needed, I can dig more for behaviour on older devices (<2019), let me know!
>

My point is, if it's possible to introduce this change but keep the
same old behavior (across both U-Boot and AOSP), it's usually better
to do that that way. If I'm not missing anything and that's a valid
concern, maybe a separate patch can be developed on top of the merged
patch series handling that. Anyways, I don't have enough time to work
on this right now, so just pointing out what I noticed, if it's useful
for anybody. I'll let the maintainers decide :)

Thanks!

> >
> >> 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]