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

Sam Protsenko semen.protsenko at linaro.org
Fri May 10 14:23:26 UTC 2019 

Hi Eugeniu,

On Thu, May 9, 2019 at 4:41 AM Eugeniu Rosca <roscaeugeniu at gmail.com> wrote:
>
> 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.
>

It's just a "proof-of-concept" code. It was sent by engineer from my
team some time ago, to our internal mailing list. Of course, it's not
mean to be accepted as is, it's just a "food for thought". Now that I
checked both "bcb" and "rb_reason" implementation, I'm inclined to
think that it's better to add missing functionality to "bcb" command,
and drop "rb_reason".

> 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?
>

It will be either Igor Opaniuk or me.

> 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.
>

Correct. After reading all the code I came to the same conclusion.

> 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
>

Agreed on suggested order.

> 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.
>

Yes. Can you please send v2 for "bcb" command, accounting for next:
  1. I've already sent android_bl_msg.h as a separate patch, no need
to re-send it
  2. Fix pending comments on "bcb" patch review
  3. Add referencing partitions by names (not only by number)
    3.1 It should be reflected in "help bcb" as well

Or, optionally, (3) can be done in subsequent patch, in order to
accelerate the process. Let's try and do that ASAP so we can unblock
Igor w.r.t. A/B patches. Also, I don't have much time left on this
task...

Once "bcb" patch is merged

> >     => 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.
>

I like "bcb" command more. Let's go ahead and merge it (as I mentioned
above). Then we can think how we can use/extend it for the further
implementation of "reboot reason" feature (probably the way you
mentioned in your pseudo-code section).

> > 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.

More information about the U-Boot mailing list