| Message ID | 20190612214945.32311-1-semen.protsenko@linaro.org |
|---|---|
| State | Accepted |
| Commit | 97a0c6ff577d57f162abc696c4efc962981229b1 |
| Delegated to: | Lukasz Majewski |
| Headers | show |
| Series | [U-Boot] fastboot: Fix slot names reported by getvar | expand |
Hi Sam, On Thu, Jun 13, 2019 at 12:49:45AM +0300, Sam Protsenko wrote: > In commit [1] fastboot tool was changed w.r.t. new A/B specification [2], > and now we should report slot names in "a" format instead of "_a". > Latter is now considered legacy and we shouldn't rely on that anymore. This looks like a change which advantages the users who are always on the tip/HEAD of all relevant components (fastboot and U-Boot), but that rarely happens in the industry. Suppliers and hardening vendors often deliver obsoleted versions because they can't keep up with upstream development. Can you please document the behavior of 'fastboot flash' (and anything else relying on 'fastboot getvar (current-slot|slot-suffixes) in below scenarios: A. fastboot >= [1] && U-Boot + this patch B. fastboot >= [1] && U-Boot - this patch C. fastboot < [1] && U-Boot + this patch D. fastboot < [1] && U-Boot - this patch Would it be possible to keep U-Boot backward-compatible, such that regardless of the scenario enumerated above, 'fastboot flash' will always succeed? > [1] https://android.googlesource.com/platform/system/core/+/8091947847d5e5130b09d2ac0a4bdc900f3b77c5 > [2] https://source.android.com/devices/tech/ota/ab/ab_implement#partitions
Hi Eugeniu, On Thu, Jun 13, 2019 at 12:31 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > Hi Sam, > > On Thu, Jun 13, 2019 at 12:49:45AM +0300, Sam Protsenko wrote: > > In commit [1] fastboot tool was changed w.r.t. new A/B specification [2], > > and now we should report slot names in "a" format instead of "_a". > > Latter is now considered legacy and we shouldn't rely on that anymore. > > This looks like a change which advantages the users who are always on > the tip/HEAD of all relevant components (fastboot and U-Boot), but that > rarely happens in the industry. Suppliers and hardening vendors often > deliver obsoleted versions because they can't keep up with upstream > development. Can you please document the behavior of 'fastboot flash' > (and anything else relying on > 'fastboot getvar (current-slot|slot-suffixes) in below scenarios: > > A. fastboot >= [1] && U-Boot + this patch > B. fastboot >= [1] && U-Boot - this patch > C. fastboot < [1] && U-Boot + this patch > D. fastboot < [1] && U-Boot - this patch > > Would it be possible to keep U-Boot backward-compatible, such that > regardless of the scenario enumerated above, 'fastboot flash' will > always succeed? > I'm afraid in this particular case we weren't given any choice, and we won't be able to provide backward-compatibility for older Android releases. After this commit: [3] https://android.googlesource.com/platform/system/core/+/42b18a518bac85c3eea14206f6cbafbd1e98a31f they dropped support for "_a" format completely (in fastboot tool). So: * if user runs new fastboot tool ( >= [3]), then the only way to make "fastboot flash" work is to return slot in "a" format from bootloader * if user runs old fastboot tool (< [1]), then the only way to make "fastboot flash" work is to return slot in "_a" format from bootloader Good news is that user can basically downgrade or upgrade fastboot tool, to be in sync with U-Boot version in use. Bad news, we need to decide which Android version to break in U-Boot/master. I suggest we track AOSP/master in U-Boot/master. Please let me know if you agree, or maybe there is some better way I'm missing. Thanks! > > [1] https://android.googlesource.com/platform/system/core/+/8091947847d5e5130b09d2ac0a4bdc900f3b77c5 > > [2] https://source.android.com/devices/tech/ota/ab/ab_implement#partitions > > -- > Best Regards, > Eugeniu.
Hi Sam, Thanks for the detailed answer. Some comments below. On Thu, Jun 13, 2019 at 04:59:40PM +0300, Sam Protsenko wrote: > Hi Eugeniu, > > On Thu, Jun 13, 2019 at 12:31 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > > > Hi Sam, > > > > On Thu, Jun 13, 2019 at 12:49:45AM +0300, Sam Protsenko wrote: > > > In commit [1] fastboot tool was changed w.r.t. new A/B specification [2], > > > and now we should report slot names in "a" format instead of "_a". > > > Latter is now considered legacy and we shouldn't rely on that anymore. > > > > This looks like a change which advantages the users who are always on > > the tip/HEAD of all relevant components (fastboot and U-Boot), but that > > rarely happens in the industry. Suppliers and hardening vendors often > > deliver obsoleted versions because they can't keep up with upstream > > development. Can you please document the behavior of 'fastboot flash' > > (and anything else relying on > > 'fastboot getvar (current-slot|slot-suffixes) in below scenarios: > > > > A. fastboot >= [1] && U-Boot + this patch > > B. fastboot >= [1] && U-Boot - this patch > > C. fastboot < [1] && U-Boot + this patch > > D. fastboot < [1] && U-Boot - this patch > > > > Would it be possible to keep U-Boot backward-compatible, such that > > regardless of the scenario enumerated above, 'fastboot flash' will > > always succeed? > > > > I'm afraid in this particular case we weren't given any choice, and we > won't be able to provide backward-compatibility for older Android > releases. After this commit: > > [3] https://android.googlesource.com/platform/system/core/+/42b18a518bac85c3eea14206f6cbafbd1e98a31f Thumbs up for pointing out this specific commit. > > they dropped support for "_a" format completely (in fastboot tool). So: > * if user runs new fastboot tool ( >= [3]), then the only way to > make "fastboot flash" work is to return slot in "a" format from > bootloader > * if user runs old fastboot tool (< [1]), then the only way to make > "fastboot flash" work is to return slot in "_a" format from bootloader This interface change seems to carry no semantic value, so it's unfortunate to see such kind of non-backward-compatible update contributed by the maintainers of the user-space tool. > Good news is that user can basically downgrade or upgrade fastboot > tool, to be in sync with U-Boot version in use. Bad news, we need to > decide which Android version to break in U-Boot/master. > > I suggest we track AOSP/master in U-Boot/master. Please let me know if > you agree, or maybe there is some better way I'm missing. I can't come up with a better proposal. Reflecting on fastboot in general, I think there are many more questions which lack a straightforward answer: - How accurately the fastboot protocol [4] reflects the AOSP user-space implementation (and viceversa)? - Some basic git grepping reveals that the fastboot protocol [4] was once upgraded from v0.3 to v0.4, but that's pretty much it. It definitely doesn't cover the evolution of fastboot usage/parameters. For example, none of the AB/slot-related parameters (e.g. "slot-suffixes", "current-slot") has ever been documented in [4]. - Since the protocol and the actual development are out of sync, staying aligned to AOSP means asserting about the fastboot interface updates by reviewing the AOSP patches (which is time consuming and cannot be easily automated). - OTOH, it is also not clear to which degree U-Boot/master is aligned to AOSP/master even after integrating this patch. As example, commit [3] mentioned by you dropped the support for 'slot-suffixes' altogether while the option is still being processed in U-Boot. In a nutshell, I think we have no other choice but to apply this fix (still not clear if w/ or w/o keeping the support for "slot-suffixes"). I think it's also a must to warn the users that this patch assumes/depends on AOSP fastboot version/commit [3] or higher. > > > [1] https://android.googlesource.com/platform/system/core/+/8091947847d5e5130b09d2ac0a4bdc900f3b77c5 > > > [2] https://source.android.com/devices/tech/ota/ab/ab_implement#partitions [4] https://android.googlesource.com/platform/system/core/+/master/fastboot/README.md
Hi Eugeniu, On Thu, Jun 13, 2019 at 8:57 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > Hi Sam, > > Thanks for the detailed answer. Some comments below. > > On Thu, Jun 13, 2019 at 04:59:40PM +0300, Sam Protsenko wrote: > > Hi Eugeniu, > > > > On Thu, Jun 13, 2019 at 12:31 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > > > > > Hi Sam, > > > > > > On Thu, Jun 13, 2019 at 12:49:45AM +0300, Sam Protsenko wrote: > > > > In commit [1] fastboot tool was changed w.r.t. new A/B specification [2], > > > > and now we should report slot names in "a" format instead of "_a". > > > > Latter is now considered legacy and we shouldn't rely on that anymore. > > > > > > This looks like a change which advantages the users who are always on > > > the tip/HEAD of all relevant components (fastboot and U-Boot), but that > > > rarely happens in the industry. Suppliers and hardening vendors often > > > deliver obsoleted versions because they can't keep up with upstream > > > development. Can you please document the behavior of 'fastboot flash' > > > (and anything else relying on > > > 'fastboot getvar (current-slot|slot-suffixes) in below scenarios: > > > > > > A. fastboot >= [1] && U-Boot + this patch > > > B. fastboot >= [1] && U-Boot - this patch > > > C. fastboot < [1] && U-Boot + this patch > > > D. fastboot < [1] && U-Boot - this patch > > > > > > Would it be possible to keep U-Boot backward-compatible, such that > > > regardless of the scenario enumerated above, 'fastboot flash' will > > > always succeed? > > > > > > > I'm afraid in this particular case we weren't given any choice, and we > > won't be able to provide backward-compatibility for older Android > > releases. After this commit: > > > > [3] https://android.googlesource.com/platform/system/core/+/42b18a518bac85c3eea14206f6cbafbd1e98a31f > > Thumbs up for pointing out this specific commit. > > > > > they dropped support for "_a" format completely (in fastboot tool). So: > > * if user runs new fastboot tool ( >= [3]), then the only way to > > make "fastboot flash" work is to return slot in "a" format from > > bootloader > > * if user runs old fastboot tool (< [1]), then the only way to make > > "fastboot flash" work is to return slot in "_a" format from bootloader > > This interface change seems to carry no semantic value, so it's > unfortunate to see such kind of non-backward-compatible update > contributed by the maintainers of the user-space tool. > Yeah, I was pretty frustrated too :) Anyway, let's try to handle this in U-Boot as good as we can. > > Good news is that user can basically downgrade or upgrade fastboot > > tool, to be in sync with U-Boot version in use. Bad news, we need to > > decide which Android version to break in U-Boot/master. > > > > I suggest we track AOSP/master in U-Boot/master. Please let me know if > > you agree, or maybe there is some better way I'm missing. > > I can't come up with a better proposal. > Well, theoretically we *can* provide some config option like AB_NEW/AB_OLD, or check similar variable in U-Boot shell, to figure out which format to use ("_a" or "a"). But right now A/B is not implemented in upstream U-Boot anyway, so we probably don't have much users for that feature yet. So let's implement it for the latest AOSP/master, and if the need arises, we can handle old format support later. The crucial thing is, this patch fixes "fastboot flash" in AOSP/master, when user has slotted partitions. > Reflecting on fastboot in general, I think there are many more > questions which lack a straightforward answer: > - How accurately the fastboot protocol [4] reflects the AOSP > user-space implementation (and viceversa)? > - Some basic git grepping reveals that the fastboot protocol [4] > was once upgraded from v0.3 to v0.4, but that's pretty much it. It > definitely doesn't cover the evolution of fastboot usage/parameters. > For example, none of the AB/slot-related parameters (e.g. > "slot-suffixes", "current-slot") has ever been documented in [4]. > - Since the protocol and the actual development are out of sync, > staying aligned to AOSP means asserting about the fastboot > interface updates by reviewing the AOSP patches (which is time > consuming and cannot be easily automated). > - OTOH, it is also not clear to which degree U-Boot/master is aligned > to AOSP/master even after integrating this patch. As example, > commit [3] mentioned by you dropped the support for 'slot-suffixes' > altogether while the option is still being processed in U-Boot. > Agree. Someone should check AOSP patches and provide corresponding support for the latest fastboot tool feature. I will probably look into that later, as we want this for our platforms to be implemented. I'm gonna send v2 soon, addressing your comments (will remove slot-suffixes variable and improve commit message). Thanks for your input! > In a nutshell, I think we have no other choice but to apply this fix > (still not clear if w/ or w/o keeping the support for "slot-suffixes"). > I think it's also a must to warn the users that this patch > assumes/depends on AOSP fastboot version/commit [3] or higher. > > > > > [1] https://android.googlesource.com/platform/system/core/+/8091947847d5e5130b09d2ac0a4bdc900f3b77c5 > > > > [2] https://source.android.com/devices/tech/ota/ab/ab_implement#partitions > > [4] https://android.googlesource.com/platform/system/core/+/master/fastboot/README.md > > -- > Best Regards, > Eugeniu.
Please drop this patch in favor of:
[PATCH v2 0/2] fastboot: Support new A/B slot format
Thanks!
On Fri, Jun 14, 2019 at 5:37 PM Sam Protsenko
<semen.protsenko@linaro.org> wrote:
>
> Hi Eugeniu,
>
> On Thu, Jun 13, 2019 at 8:57 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> >
> > Hi Sam,
> >
> > Thanks for the detailed answer. Some comments below.
> >
> > On Thu, Jun 13, 2019 at 04:59:40PM +0300, Sam Protsenko wrote:
> > > Hi Eugeniu,
> > >
> > > On Thu, Jun 13, 2019 at 12:31 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> > > >
> > > > Hi Sam,
> > > >
> > > > On Thu, Jun 13, 2019 at 12:49:45AM +0300, Sam Protsenko wrote:
> > > > > In commit [1] fastboot tool was changed w.r.t. new A/B specification [2],
> > > > > and now we should report slot names in "a" format instead of "_a".
> > > > > Latter is now considered legacy and we shouldn't rely on that anymore.
> > > >
> > > > This looks like a change which advantages the users who are always on
> > > > the tip/HEAD of all relevant components (fastboot and U-Boot), but that
> > > > rarely happens in the industry. Suppliers and hardening vendors often
> > > > deliver obsoleted versions because they can't keep up with upstream
> > > > development. Can you please document the behavior of 'fastboot flash'
> > > > (and anything else relying on
> > > > 'fastboot getvar (current-slot|slot-suffixes) in below scenarios:
> > > >
> > > > A. fastboot >= [1] && U-Boot + this patch
> > > > B. fastboot >= [1] && U-Boot - this patch
> > > > C. fastboot < [1] && U-Boot + this patch
> > > > D. fastboot < [1] && U-Boot - this patch
> > > >
> > > > Would it be possible to keep U-Boot backward-compatible, such that
> > > > regardless of the scenario enumerated above, 'fastboot flash' will
> > > > always succeed?
> > > >
> > >
> > > I'm afraid in this particular case we weren't given any choice, and we
> > > won't be able to provide backward-compatibility for older Android
> > > releases. After this commit:
> > >
> > > [3] https://android.googlesource.com/platform/system/core/+/42b18a518bac85c3eea14206f6cbafbd1e98a31f
> >
> > Thumbs up for pointing out this specific commit.
> >
> > >
> > > they dropped support for "_a" format completely (in fastboot tool). So:
> > > * if user runs new fastboot tool ( >= [3]), then the only way to
> > > make "fastboot flash" work is to return slot in "a" format from
> > > bootloader
> > > * if user runs old fastboot tool (< [1]), then the only way to make
> > > "fastboot flash" work is to return slot in "_a" format from bootloader
> >
> > This interface change seems to carry no semantic value, so it's
> > unfortunate to see such kind of non-backward-compatible update
> > contributed by the maintainers of the user-space tool.
> >
>
> Yeah, I was pretty frustrated too :) Anyway, let's try to handle this
> in U-Boot as good as we can.
>
> > > Good news is that user can basically downgrade or upgrade fastboot
> > > tool, to be in sync with U-Boot version in use. Bad news, we need to
> > > decide which Android version to break in U-Boot/master.
> > >
> > > I suggest we track AOSP/master in U-Boot/master. Please let me know if
> > > you agree, or maybe there is some better way I'm missing.
> >
> > I can't come up with a better proposal.
> >
>
> Well, theoretically we *can* provide some config option like
> AB_NEW/AB_OLD, or check similar variable in U-Boot shell, to figure
> out which format to use ("_a" or "a"). But right now A/B is not
> implemented in upstream U-Boot anyway, so we probably don't have much
> users for that feature yet. So let's implement it for the latest
> AOSP/master, and if the need arises, we can handle old format support
> later. The crucial thing is, this patch fixes "fastboot flash" in
> AOSP/master, when user has slotted partitions.
>
> > Reflecting on fastboot in general, I think there are many more
> > questions which lack a straightforward answer:
> > - How accurately the fastboot protocol [4] reflects the AOSP
> > user-space implementation (and viceversa)?
> > - Some basic git grepping reveals that the fastboot protocol [4]
> > was once upgraded from v0.3 to v0.4, but that's pretty much it. It
> > definitely doesn't cover the evolution of fastboot usage/parameters.
> > For example, none of the AB/slot-related parameters (e.g.
> > "slot-suffixes", "current-slot") has ever been documented in [4].
> > - Since the protocol and the actual development are out of sync,
> > staying aligned to AOSP means asserting about the fastboot
> > interface updates by reviewing the AOSP patches (which is time
> > consuming and cannot be easily automated).
> > - OTOH, it is also not clear to which degree U-Boot/master is aligned
> > to AOSP/master even after integrating this patch. As example,
> > commit [3] mentioned by you dropped the support for 'slot-suffixes'
> > altogether while the option is still being processed in U-Boot.
> >
>
> Agree. Someone should check AOSP patches and provide corresponding
> support for the latest fastboot tool feature. I will probably look
> into that later, as we want this for our platforms to be implemented.
>
> I'm gonna send v2 soon, addressing your comments (will remove
> slot-suffixes variable and improve commit message). Thanks for your
> input!
>
>
> > In a nutshell, I think we have no other choice but to apply this fix
> > (still not clear if w/ or w/o keeping the support for "slot-suffixes").
> > I think it's also a must to warn the users that this patch
> > assumes/depends on AOSP fastboot version/commit [3] or higher.
> >
> > > > > [1] https://android.googlesource.com/platform/system/core/+/8091947847d5e5130b09d2ac0a4bdc900f3b77c5
> > > > > [2] https://source.android.com/devices/tech/ota/ab/ab_implement#partitions
> >
> > [4] https://android.googlesource.com/platform/system/core/+/master/fastboot/README.md
> >
> > --
> > Best Regards,
> > Eugeniu.
diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c index 563bda0088..2f7fd23523 100644 --- a/drivers/fastboot/fb_getvar.c +++ b/drivers/fastboot/fb_getvar.c @@ -168,13 +168,13 @@ static void getvar_platform(char *var_parameter, char *response) static void getvar_current_slot(char *var_parameter, char *response) { - /* A/B not implemented, for now always return _a */ - fastboot_okay("_a", response); + /* A/B not implemented, for now always return "a" */ + fastboot_okay("a", response); } static void getvar_slot_suffixes(char *var_parameter, char *response) { - fastboot_okay("_a,_b", response); + fastboot_okay("a,b", response); } static void getvar_has_slot(char *part_name, char *response)
In commit [1] fastboot tool was changed w.r.t. new A/B specification [2], and now we should report slot names in "a" format instead of "_a". Latter is now considered legacy and we shouldn't rely on that anymore. Due to this one can observe next error with recent fastboot tool: $ fastboot flash boot boot.img Sending 'boot__a' (11301 KB) OKAY [ 0.451s] Writing 'boot__a' FAILED (remote: 'cannot find partition') fastboot: error: Command failed Let's use new slot format in order to fix double underscores "__" and to be in sync with AOSP master. [1] https://android.googlesource.com/platform/system/core/+/8091947847d5e5130b09d2ac0a4bdc900f3b77c5 [2] https://source.android.com/devices/tech/ota/ab/ab_implement#partitions Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- drivers/fastboot/fb_getvar.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)