[U-Boot] [PATCH] fastboot: Remove "bootloader-version" variable

[Igor Opaniuk]

On Thu, Jun 20, 2019 at 7:08 PM Sam Protsenko
<semen.protsenko at linaro.org> wrote:
>
> Hi Igor,
>
> On Thu, Jun 20, 2019 at 5:55 PM Igor Opaniuk <igor.opaniuk at gmail.com> wrote:
> >
> > Hi Sam,
> >
> > On Thu, Jun 20, 2019 at 5:00 PM Sam Protsenko
> > <semen.protsenko at linaro.org> wrote:
> > >
> > > As per [1], there is no such fastboot variable as "bootloader-version".
> > > Only "version-bootloader" is supported. Let's reflect this and not
> > > confuse users further.
> > >
> > > [1] https://android.googlesource.com/platform/system/core/+/master/fastboot/README.md#client-variables
> > >
> > > Signed-off-by: Sam Protsenko <semen.protsenko at linaro.org>
> > > ---
> > >  doc/README.android-fastboot  | 4 ++--
> > >  drivers/fastboot/fb_getvar.c | 9 +++------
> > >  2 files changed, 5 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/doc/README.android-fastboot b/doc/README.android-fastboot
> > > index 431191c473..ce852a4fd1 100644
> > > --- a/doc/README.android-fastboot
> > > +++ b/doc/README.android-fastboot
> > > @@ -169,8 +169,8 @@ On the client side you can fetch the bootloader version for instance:
> > >
> > >  ::
> > >
> > > -   $ fastboot getvar bootloader-version
> > > -   bootloader-version: U-Boot 2014.04-00005-gd24cabc
> > > +   $ fastboot getvar version-bootloader
> > > +   version-bootloader: U-Boot 2014.04-00005-gd24cabc
> > >     finished. total time: 0.000s
> > >
> > >  or initiate a reboot:
> > > diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
> > > index fd0823b2bf..ebe5c8a104 100644
> > > --- a/drivers/fastboot/fb_getvar.c
> > > +++ b/drivers/fastboot/fb_getvar.c
> > > @@ -12,7 +12,7 @@
> > >  #include <version.h>
> > >
> > >  static void getvar_version(char *var_parameter, char *response);
> > > -static void getvar_bootloader_version(char *var_parameter, char *response);
> > > +static void getvar_version_bootloader(char *var_parameter, char *response);
> > >  static void getvar_downloadsize(char *var_parameter, char *response);
> > >  static void getvar_serialno(char *var_parameter, char *response);
> > >  static void getvar_version_baseband(char *var_parameter, char *response);
> > > @@ -37,12 +37,9 @@ static const struct {
> > >         {
> > >                 .variable = "version",
> > >                 .dispatch = getvar_version
> > > -       }, {
> > > -               .variable = "bootloader-version",
> > > -               .dispatch = getvar_bootloader_version
> > >         }, {
> > >                 .variable = "version-bootloader",
> > > -               .dispatch = getvar_bootloader_version
> > > +               .dispatch = getvar_version_bootloader
> > >         }, {
> > >                 .variable = "downloadsize",
> > >                 .dispatch = getvar_downloadsize
> > > @@ -131,7 +128,7 @@ static void getvar_version(char *var_parameter, char *response)
> > >         fastboot_okay(FASTBOOT_VERSION, response);
> > >  }
> > >
> > > -static void getvar_bootloader_version(char *var_parameter, char *response)
> > > +static void getvar_version_bootloader(char *var_parameter, char *response)
> > >  {
> > >         fastboot_okay(U_BOOT_VERSION, response);
> > >  }
> > > --
> > > 2.20.1
> > >
> >
> > My two cents here,
> >
> > Based on the commit messages from "git log --grep=bootloader-version"
> > people prefer to use "bootloader-version" instead of
> > "version-bootloader", and totally removing it will probably affect
> > usual workflow with fastboot (probably someone will be suprised that
> > "bootloader-version" doesn't work anymore), including some CI automate
> > testing etc (if there is any);
> >
>
> We need to decide what has more value in this particular case:
>   1. Keeping protocol clean, correct and up-to-date
>   2. Supporting all erroneous choices we've done before
>
> If we follow golden rule of kernel, (2) is proffered. But I don't
> think in this particular case a lot of harm will be done. So from my
> POV (1) is preferred. Otherwise we can clutter the protocol, causing
> some confusion.
>
> You can check fastboot project in AOSP [1]
>
>     $ git log -S'bootloader-version' -- fastboot/
>
> No occurrences, ever. AOSP is unaware we have 'bootloader-version'
> variable in U-Boot, but AOSP defines 'version-bootloader' variable,
> for the same stuff. I would really prefer we avoid using some weird
> undocumented stuff, and I think in this particular case the
> cleanliness of protocol overrules golden rule of kernel development,
> as it seems relatively easy to fix that command (matter of one sed
> execution).
>
> [1] https://android.googlesource.com/platform/system/core/+/master/
>
> > I think I does make sense to involve all of them to this discussion
> > also (already added to CC).
> >
> > --
> > Best regards - Freundliche Grüsse - Meilleures salutations
> >
> > Igor Opaniuk
> >
> > mailto: igor.opaniuk at gmail.com
> > skype: igor.opanyuk
> > +380 (93) 836 40 67
> > http://ua.linkedin.com/in/iopaniuk

It's was just about making people aware about these changes :). Anyway:
Reviewed-by: Igor Opaniuk <igor.opaniuk at toradex.com>

-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk at gmail.com
skype: igor.opanyuk
+380 (93) 836 40 67
http://ua.linkedin.com/in/iopaniuk