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

Hi Sam,

Thank you for the review.

On lun., nov. 04, 2024 at 17:06, Sam Protsenko <[email protected]> wrote:

> On Thu, Oct 17, 2024 at 9:12 AM Dmitry Rokosov
> <[email protected]> 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 <[email protected]>
>> Reviewed-by: Simon Glass <[email protected]>
>> Tested-by: Guillaume La Roque <[email protected]>
>> Signed-off-by: Dmitry Rokosov <[email protected]>
>
> 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
>>