Hi Sam, On mer., nov. 06, 2024 at 21:17, Sam Protsenko <semen.protsenko@...> wrote: On Wed, Nov 6, 2024 at 4:02?AM Mattijs Korpershoek
<mkorpershoek@...> wrote:
Hi Sam,
On mar., nov. 05, 2024 at 18:58, Sam Protsenko <semen.protsenko@...> wrote:
On Tue, Nov 5, 2024 at 9:06?AM Mattijs Korpershoek
<mkorpershoek@...> 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:
;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)
So implementations before 2019 that do not have this patch:
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:
;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]
|
