On Wed, May 2, 2018 at 1:14 AM Alex Kiernan <alex.kier...@gmail.com> wrote:
> On Wed, May 2, 2018 at 7:34 AM Jocelyn Bohr <b...@google.com> wrote:
>
> > Hi Alex,
>
> > I think this approach looks really good so far, and will make maintaining
> both
> > implementations easier going forward. I haven't looked at every change
> here
> > yet, but when going through the patches should I add a "Reviewed-by:"
> line if
> > the change looks good to me?
>
>
> Yes please!
>
Great, I believe I have reviewed most of the relevant patchsets so far. I
am planning to continue reviewing on the next set of patches, since it
sounds like there will be several changes.
> > Thanks,
> > Jocelyn
>
> > On Mon, Apr 30, 2018 at 1:33 AM Alex Kiernan <alex.kier...@gmail.com>
> wrote:
>
>
> >> This series merges the fastboot UDP support from AOSP into mainline
> >> U-Boot.
>
> >> The underlying implementations of most commands are now merged between
> >> both code paths ('oem format' from the USB side is the only one
> remaining).
>
> >> Other changes to command handling so that UDP follows the existing USB
> >> behaviour:
>
> >> - 'boot' now follows the USB code and does 'fastboot
> CONFIG_FASTBOOT_BUF_ADDR'.
> >> I've added 'fastbootcmd' which if set overrides the boot command and
> >> allows the existing UDP behaviour to be preserved.
>
>
> > The UDP behavior more correct based on the specification.
> > "The previously downloaded data is a boot.img and should be booted
> according to
> > the normal procedure for a boot.img."
>
> > https://android.googlesource.com/platform/system/core/+/master/fastboot/
>
> > I'm not sure who we will break if we change the USB code...
>
> Making sure we don't break (or at least trying my hardest to) existing
> upstream code was my aim in this. If the right thing is to change it, then
> do that when the android bootflow gets merged?
> > also we probably
> > want to make sure image verification happens with "fastboot boot", making
> sure
> > "fastboot boot" doesn't allow booting an unverified kernel on locked
> devices. I
> > think we can add that flow later though, upstream U-boot doesn't support
> > lock/unlock commands anyway.
>
>
> I think running a general command here (via 'fastbootcmd') should give
> enough options to implement that policy. For my use case fastboot is
> actually just a convenient technology and I don't need the Android image
> format or bootflow... so I'm getting my signature verification out of a
> signed FIT image, and use something like:
>
> fastbootcmd='setenv verify yes; bootm 0x81000800#conf@0'
>
SGTM, I like this approach too beacuse it doesn't break upstream code.
Thanks for providing the additional context of your use case! I was
assuming you were booting Android...that explains some other questions I
had, like why reboot-bootloader was made optional.
>
> >> - 'continue' in UDP now exits the fastboot server rather than executing
> >> 'run bootcmd'
> >> - 'reboot-bootloader' no longer writes 'reboot-bootloader' to
> >> CONFIG_FASTBOOT_BUF_ADDR as its marker for the subsequent boot. The
> code
> >> which is in AOSP common/android_bootloader.c expects this marker, but
> >> we have prior art in the USB code using the weak function
> >> fb_set_reboot_flag
> >> - 'getvar' in the UDP path now supports fetching 'fastboot.' prefixed
> >> variables (inherited from the USB path)
> >> - 'getvar' in the USB path inherits all the variables from the UDP path
>
> >> Remaining issues:
>
> >> - whilst I've merged NAND support into the UDP code path, I've no way
> >> of testing it. My expectation is it'll work, but will need
> timed_send_info
> >> working into the NAND path to avoid timeouts on the network side.
>
>
> > I don't know of any devices that use NAND and the fastboot UDP code, so
> > I'm inclined to say this is okay.
>
>
> I think so... it's not broken today because there's no UDP and NAND code in
> upstream, if someone needs that then the mechanics are all there to wire it
> in (and I've no way of testing it).
>
> >> - I still need to fix timed_send_info handling when going through the
> USB
> >> path.
> >> - the protocol part of the fastboot UDP implementation is separated out
> >> and would I expect form the basis of a consolidated implementation,
> but
> >> I'm inclined to address that as a clearly separate patch so it can be
> >> tested in isolation (I have no USB hardware I can try this on).
>
>
> > Should this set of patches be tested on USB hardware too? There are some
> > changes here to the USB implementation.
>
>
> My aim thus far was to keep them sufficiently minimal that you get decide
> on correctness by inspection, which isn't to say I've actually got it
> right, but I'm hopeful. That said you can't beat a real test!
>
> --
> Alex Kiernan
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
