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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Sam,

On mer., nov. 06, 2024 at 21:17, Sam Protsenko <semen.protsenko@linaro.org> wrote:

> On Wed, Nov 6, 2024 at 4:02 AM Mattijs Korpershoek
> <mkorpershoek@baylibre.com> wrote:
>>
>> Hi Sam,
>>
>> On mar., nov. 05, 2024 at 18:58, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>>
>> > On Tue, Nov 5, 2024 at 9:06 AM Mattijs Korpershoek
>> > <mkorpershoek@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.
>>
>> 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.

Yes, you are right. Boards with Android older than 2019 and U-Boot v2025.01+

My experience is that most of the time, it's the bootloader that stays
out of date and the Android that gets updated. But that might not be the
general case.

>
>> """
>>   // 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.

Nope, the comment is wrong. If look at the lines below you can see that
the function exits early when the property is not set.

So in recent Android versions if the propery is not set, we just fail to initialize.

>
>>   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 :)

Yes, thanks a lot for noticing and taking the time to report your valid
concerns :)

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