[U-Boot] [PATCH 1/2] include: android_bl_msg.h: Initial import

[Eugeniu Rosca <roscaeugeniu@gmail.com>]

Hi Sam,

Thanks for the amazing effort to put the recent BCB/AB-related
advancements together and make them work. I really look forward to
seeing this part of mainline. Still, I have some concerns/questions
and hope to get your feedback.

First, the ("Implement Reboot reason support") series included in your
"misc-reboot-reason" branch looks to be brand new, i.e. it hasn't been
previously submitted for community review. I have comments in that area.

Second, some review comments of mine posted in various patches of
https://patchwork.ozlabs.org/cover/1044152/ ("[U-Boot,v3,0/7] android:
implement A/B boot process") still haven't been addressed. Who is going
to handle that work?

Third, yes, there is more overlap than I initially expected. It is
mostly between ("Add 'bcb' command to read/modify/write Android BCB")
and ("Implement Reboot reason support"). The resolution is not as
straightforward as one might assume. It is both a conflict of code
and a conflict of perspective on how Android bootloader flow
should be implemented in U-Boot. More on that later.

Fourth, some words on commit order and split. I think the most natural
commit order is the one reflecting the development of features in AOSP,
i.e. I would expect getting the reboot reason to come before the A/B
support, just b/c BCB structure with its "command" field existed in
AOSP for ages (since the inception of "recovery" repository in 2008)
while A/B support came _much_ (at least 7 years) later. WRT commit
split, one comment is that we would potentially like to import getting
reboot reason w/o importing A/B support. Surprisingly, this is not
possible if the bootloader message header is glued to commit
("common: Implement A/B metadata"). So, the best order to me is:
 - add android_bl_msg.h in a standalone commit
 - add bcb command
 - add getting reboot reason (if needed at all, more on it later)
 - add A/B support
 - update platform support

More comments below.

On Wed, May 08, 2019 at 08:25:15PM +0300, Sam Protsenko wrote:
> Hi Eugeniu,
> 
> I created GitHub repo with all related patches applied on top of most
> recent mainline U-Boot master: [1]. It contains next patches (on
> "misc-reboot-reason" branch):
> 
> 1. Series from Ruslan Trofymenko: [PATCH v3 0/7] android: implement
> A/B boot process:
>   * cmd: part: Add 'number' sub-command
>   * disk: part: Extend API to get partition info
>   * common: Implement A/B metadata
>   * cmd: Add 'ab_select' command
>   * test/py: Add base test case for A/B updates
>   * doc: android: Add simple guide for A/B updates
>   * env: am57xx: Implement A/B boot process
> 2. Series from Ruslan Trofymenko: [U-Boot][PATCH 0/4] Implement Reboot
> reason support:
>   * common: Implement Reboot reason flow
>   * cmd: Add 'rb_reason' command
>   * env: am57xx: Add Reboot reason support
>   * configs: am57xx_evm: Enable Reboot reason support
>   * Rename android_bl_msg.h -> android_bl_msg2.h (my patch to make
> this series apply along with next patches from Eugeniu)
> 3. Series from Eugeniu Rosca: [PATCH 0/2] Add 'bcb' command to
> read/modify/write Android BCB:
>   * include: android_bl_msg.h: Initial import
>   * cmd: Add 'bcb' command to read/modify/write BCB fields
> 4. My local patches to enable it all on X15 board, make it work
> correctly, and use original android_bl_msg.h (as close as possible to
> AOSP file):
>   * configs: am57xx: Enable BCB command
>   * environment: ti: Fix USB controller number
>   * android: reboot reason: Use original android_bl_msg.h
>   * android: ab: Use original android_bl_msg.h
>   * android: Remove android_bl_msg2.h
>   * android: bcb: Use original android_bl_msg.h API
> 
> With all those patches applied, I'm able to do next (after "adb reboot
> bootloader" command):
> 1. Use "bcb" command by Eugeniu:
>     => bcb load 1 4

"bcb load 1 misc" should be also supported, in case it helps.

>     => bcb dump command
> 2. Use "rb_reason" command by Ruslan:
>     => rb_reason mmc 1:4
>     => rb_reason mmc 1:misc

Well, here we have two different perspectives.
Below should be a 'bcb' equivalent (mostly pseudo code, not tested):

if bcb load 1 misc; then
    # Valid BCB found
    if bcb test command = bootonce-bootloader; then
        bcb clear command; bcb store;
        # do the equivalent of $fastbootcmd
    else if bcb test command = boot-recovery; then
        bcb clear command; bcb store;
        # do the equivalent of $recoverycmd
    else
        # boot Android OS normally
    fi
else
    # Corrupted/non-existent BCB
    # Boot non-Android OS?
fi

Here I see some room for discussion, since we have two approaches to
getting the reboot reason and act accordingly. I'll point out some
pros (+) and cons (-) in each case (IMHO):

rb_reason:
+ compact when used (one-liner)
- does much more than just reading the boot reason:
   - clears the 'command' field in BCB
   - runs $fastbootcmd/$recoverycmd, presumably populated beforehand
 => the above means that:
   - command name does not reflect its function, i.e. the name is
     misleading
   - U-Boot gets sprinkled by and its flow becomes dependent on a
     number of prerequisite environment variables (fastbootcmd/
     recoverycmd). Boot flow becomes more complex and harder to
     comprehend/follow/debug. It's dispersed across several components
     communicating via environment variables (not at all centralized)
- If we need to read any other BCB fields (status, recovery, stage) as
  part of Android boot flow management, we will need to either spawn
  new U-Boot commands or further obfuscate rb_reason.

In comparison, bcb:
- less compact when used (multi-line)
+ brings more transparency via sub-commands
+ frees the need for fastbootcmd/recoverycmd, i.e. centralizes
  Android boot flow management in the U-Boot environment. This is easier
  to read and debug.
+ can be used to take action based on the contents of other BCB fields

The above is my subjective view and I am open for different opinions.

> 3. U-Boot automatically gets into fastboot mode when "adb reboot
> bootloader" command was issued
> 
> Now we need to figure out how to do next (w.r.t. repo [1]):
>   1. How to merge your "bcb" command and Ruslan's "rb_reason" command;
> I can see they both are working with "misc" BCB. So maybe it's good
> idea to merge them into one command.
>   2. How to handle android_bl_msg.h: it's a dependency for all 3 patch
> series (A/B, "reboot reason" command, BCB command). Maybe we should
> rework it and send as a separate patch, mentioning why it's needed,
> and after it's applied, we can re-send our patch series without that
> file included.
> 
> Please let me know what do you think.

I guess I touched most of your comments.
I look forward for your feedback!

> 
> Thanks!

Likewise!

> 
> [1] https://github.com/joe-skb7/u-boot-misc/commits/misc-reboot-reason
> 
> > --
> > Best Regards,
> > Eugeniu.