Hi Colin, On Thu, Apr 09, 2026 at 15:34, "Pinnell McAllister, Colin" <[email protected]> wrote:
> Hi Mattijs, > > Thank you for the response! This wasn't a critical change, so no worries > about the delay. > > Outlook is kind of terrible for replying inline...I should have used my gmail > account for this patch. My responses to you message are below. I agree that Outlook is not very good here. I can't seem to be able to apply the patch: $ b4 shazam -s -l --check [email protected] Looking up [email protected] Checking for newer revisions Grabbing search results from lore.kernel.org Analyzing 3 messages in the thread Looking for additional code-review trailers on lore.kernel.org Analyzing 0 code-review messages Checking attestation on all messages, may take a moment... Retrieving patchwork CI status, may take a moment... --- ✓ [PATCH] android_ab: fix slot selection + Link: https://patch.msgid.link/[email protected] + Signed-off-by: Mattijs Korpershoek <[email protected]> ● checkpatch.pl: 196: ERROR: code indent should use tabs where possible ● checkpatch.pl: 196: WARNING: please, no spaces at the start of a line ● checkpatch.pl: 197: ERROR: code indent should use tabs where possible ● checkpatch.pl: 197: WARNING: please, no spaces at the start of a line ● checkpatch.pl: 201: ERROR: code indent should use tabs where possible ● checkpatch.pl: 201: WARNING: please, no spaces at the start of a line ● checkpatch.pl: 203: ERROR: code indent should use tabs where possible ● checkpatch.pl: 203: WARNING: please, no spaces at the start of a line ● checkpatch.pl: 204: ERROR: code indent should use tabs where possible ● checkpatch.pl: 204: WARNING: please, no spaces at the start of a line --- ✓ Signed: DKIM/garmin.com --- Total patches: 1 Can you resend it? More information below. > >> Please use ./scripts/get_maintainer.pl -f boot/android_ab.c next time you >> send a patch. > > ack > >> Can you please give me more references to the boot selection rules? >> Is this just the code comment that's mentioned (boot/android_ab.c:276) ? > > Yes, that is correct. I do not have any other references to cite. > >> It seems to be using libboot_control as a static library. >> libboot_control tells us that a slot is considered unbootable when: >> tries_remaining != 0; > > I also see that tries_remaining is set to 1 on successful boot: > https://cs.android.com/android/platform/superproject/+/android-latest-release:hardware/interfaces/boot/1.1/default/boot_control/libboot_control.cpp;l=260?q=libboot_control > > This is what we also currently do when marking a boot as successful. However, > it is unclear if this is a requirement in the spec and what the behavior in > u-boot should be if successful_boot == 1 and tries_remaining == 0. > >> What happens if the tries counter is zero and we attempt to boot with >> dec_tries=true? In that case, we will decrement tries_remaining which is >> an uint8_t (see: android_bootloader_message.h). > > As I read the code, dec_tries only decrements tries_remaining if > successful_boot == 0. So in the case where dec_tries == true, tries_remaining > == 0, and successful_boot == 1, the decrement would be skipped, which aligns > with the behavior for a case where tries_remaining > 0. > > I'm not too picky with a solution here. A valid alternative solution would be > to simply update the comment at android_ab.c:276. I do think I prefer > aligning the code to match the comment as it makes more sense to be able to > boot a slot when successful_boot == 1, regardless of tries_remaining. > However, if that goes against the spec, simply fixing the comment makes more > sense. > > Please let me know what you think and what solution is preferred. I've looked at another implementation of A/B slot booting in Qualcomm's bootloader (EDK2): https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/blob/uefi.lnx.5.0.r25-rel/QcomModulePkg/Library/BootLib/PartitionTableUpdate.c?ref_type=heads#L1846 We can see in FindBootableSlot() that it's possible to boot a slot with successful_boot == 1 and tries_remaining == 0 Therefore, I believe your patch is correct. Again, sorry for the long delays. I'll do my best to be a bit more response once you have resend this patch in an applicable format (with proper whitespacing and such) Consider tooling such as b4 to help you with sending: https://b4.docs.kernel.org/en/latest/contributor/overview.html https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1 Thanks! Mattijs > > Best, > Colin > > ________________________________________ > From: Mattijs Korpershoek <[email protected]> > Sent: Thursday, April 9, 2026 05:21 > To: Pinnell McAllister, Colin <[email protected]>; > [email protected] <[email protected]> > Cc: Pinnell McAllister, Colin <[email protected]> > Subject: Re: [PATCH] android_ab: fix slot selection > > Hi Colin, > > Thank you for the patch. > Sorry for the late review. For some reason I have not been cc'ed so I > missed this. > > Please use ./scripts/get_maintainer.pl -f boot/android_ab.c next time > you send a patch. > See: > https://urldefense.com/v3/__https://docs.u-boot.org/en/latest/CONTRIBUTE.html*contributions__;Iw!!EJc4YC3iFmQ!Tlm_0_94q_aNcwZ072h7OjCsbyXAaN6bx5DgRMsDkFUXhf8ofhAJ1slRLYnu1EpkNnuaZHqRBDVgEpNOTGCclGX-d_1n$ > > On Fri, Mar 13, 2026 at 15:14, Colin Pinnell McAllister > <[email protected]> wrote: > >> The boot selection rules state that a slot is bootable if it is not >> corrupted and either has tries remaining or has already booted >> successfully. However, slots that have tries_remaining == 0 and >> successful_boot == 1 will be disregarded when picking the slot to >> attempt. > > Can you please give me more references to the boot selection rules? > Is this just the code comment that's mentioned (boot/android_ab.c:276) ? > > I've checked: > * > https://urldefense.com/v3/__https://source.android.com/docs/core/ota/ab__;!!EJc4YC3iFmQ!Tlm_0_94q_aNcwZ072h7OjCsbyXAaN6bx5DgRMsDkFUXhf8ofhAJ1slRLYnu1EpkNnuaZHqRBDVgEpNOTGCclOK45V17$ > * > https://urldefense.com/v3/__https://source.android.com/docs/core/ota/ab/ab_implement__;!!EJc4YC3iFmQ!Tlm_0_94q_aNcwZ072h7OjCsbyXAaN6bx5DgRMsDkFUXhf8ofhAJ1slRLYnu1EpkNnuaZHqRBDVgEpNOTGCclDWZu3M8$ > and it's unclear to me from that. > > Then, I've looked a bit at the default bootcontrol AIDL hal at: > https://urldefense.com/v3/__https://android.googlesource.com/platform/hardware/interfaces/*/refs/heads/main/boot/aidl/default/Android.bp__;Kw!!EJc4YC3iFmQ!Tlm_0_94q_aNcwZ072h7OjCsbyXAaN6bx5DgRMsDkFUXhf8ofhAJ1slRLYnu1EpkNnuaZHqRBDVgEpNOTGCclARJQAuu$ > > It seems to be using libboot_control as a static library. > libboot_control tells us that a slot is considered unbootable when: > tries_remaining != 0; > > See: > https://urldefense.com/v3/__https://cs.android.com/android/platform/superproject/*/android-latest-release:hardware/interfaces/boot/1.1/default/boot_control/libboot_control.cpp;l=332?q=libboot_control__;Kw!!EJc4YC3iFmQ!Tlm_0_94q_aNcwZ072h7OjCsbyXAaN6bx5DgRMsDkFUXhf8ofhAJ1slRLYnu1EpkNnuaZHqRBDVgEpNOTGCclOsUzpyr$ > >> >> Update the selection logic so slots marked successful remain eligible >> even when their tries counter is zero. Also include the successful_boot >> state in the unbootable-slot debug output. > > What happens if the tries counter is zero and we attempt to boot with > dec_tries=true? In that case, we will decrement tries_remaining which is > an uint8_t (see: android_bootloader_message.h). > >> >> Signed-off-by: Colin Pinnell McAllister <[email protected]> >> --- >> >> An alternative would be to keep the existing behavior and update the >> comment in boot/android_ab.c:276 to remove the successful_boot >> condition. I do prefer updating the logic to match the documented rules. >> Currently, the userspace AB code will need to increment the tries >> counter for a slot that has booted successfully but has no tries left. >> >> boot/android_ab.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/boot/android_ab.c b/boot/android_ab.c >> index 13e82dbcb7f..93e74f81d4b 100644 >> --- a/boot/android_ab.c >> +++ b/boot/android_ab.c >> @@ -289,11 +289,14 @@ int ab_select_slot(struct blk_desc *dev_desc, struct >> disk_partition *part_info, >> slot = -1; >> for (i = 0; i < abc->nb_slot; ++i) { >> if (abc->slot_info[i].verity_corrupted || >> - !abc->slot_info[i].tries_remaining) { >> + (!abc->slot_info[i].tries_remaining && >> + !abc->slot_info[i].successful_boot)) { >> log_debug("ANDROID: unbootable slot %d tries: %d, ", >> i, abc->slot_info[i].tries_remaining); >> - log_debug("corrupt: %d\n", >> + log_debug("corrupt: %d, ", >> abc->slot_info[i].verity_corrupted); >> + log_debug("successful: %d\n", >> + abc->slot_info[i].successful_boot); >> continue; >> } >> log_debug("ANDROID: bootable slot %d pri: %d, tries: %d, ", >> -- >> 2.53.0 >> >> >> ________________________________ >> >> CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use >> of the intended recipient(s) and contain information that may be Garmin >> confidential and/or Garmin legally privileged. If you have received this >> email in error, please notify the sender by reply email and delete the >> message. Any disclosure, copying, distribution or use of this communication >> (including attachments) by someone other than the intended recipient is >> prohibited. Thank you. >> >> ________________________________ >> >> CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use >> of the intended recipient(s) and contain information that may be Garmin >> confidential and/or Garmin legally privileged. If you have received this >> email in error, please notify the sender by reply email and delete the >> message. Any disclosure, copying, distribution or use of this communication >> (including attachments) by someone other than the intended recipient is >> prohibited. Thank you. > > ________________________________ > > CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use > of the intended recipient(s) and contain information that may be Garmin > confidential and/or Garmin legally privileged. If you have received this > email in error, please notify the sender by reply email and delete the > message. Any disclosure, copying, distribution or use of this communication > (including attachments) by someone other than the intended recipient is > prohibited. Thank you.
