Hi Sam,
Thank you for the review.
On lun., nov. 04, 2024 at 17:06, Sam Protsenko semen.protsenko@linaro.org wrote:
On Thu, Oct 17, 2024 at 9:12 AM Dmitry Rokosov ddrokosov@salutedevices.com wrote:
To align with the official Android BCB (Bootloader Control Block) specifications, it's important to note that the slot_suffix should start with an underscore symbol.
For a comprehensive understanding of the expected slot_suffix format in userspace, please refer to the provided reference [1].
Links: [1] - https://source.android.com/docs/core/architecture/bootloader/updating#slots
Based-on: https://android-review.googlesource.com/c/platform/external/u-boot/+/1446439 Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com Reviewed-by: Simon Glass sjg@chromium.org Tested-by: Guillaume La Roque glaroque@baylibre.com Signed-off-by: Dmitry Rokosov ddrokosov@salutedevices.com
Would be nice to add "Fixes:" tag here, pointing to the corresponding commit where the issue was introduced (see kernel docs for details). It could be quite useful for possible stable branches and other purposes, I'd recommend to add that tag for all fixes if you have more in this series.
I agree. Unfortunately, this series has already been merged here:
https://lore.kernel.org/r/all/20241025175409.GB4959@bill-the-cat/
boot/android_ab.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/boot/android_ab.c b/boot/android_ab.c index c93e51541019d0fe793303c4b3d5286df061906f..a287eac04fe88ad08bdcf1b1b1d6e564d503d800 100644 --- a/boot/android_ab.c +++ b/boot/android_ab.c @@ -52,7 +52,7 @@ static int ab_control_default(struct bootloader_control *abc) if (!abc) return -EFAULT;
memcpy(abc->slot_suffix, "a\0\0\0", 4);
memcpy(abc->slot_suffix, "_a\0\0", 4); abc->magic = BOOT_CTRL_MAGIC; abc->version = BOOT_CTRL_VERSION; abc->nb_slot = NUM_SLOTS;@@ -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().
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.
if (memcmp(abc->slot_suffix, slot_suffix, sizeof(slot_suffix))) { memcpy(abc->slot_suffix, slot_suffix,-- 2.43.0