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: """ // 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. 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! > >> 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]
