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

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

[Sam Protsenko]

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

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

[Igor Opaniuk]

Hi Sam,

On Thu, Jun 20, 2019 at 5:00 PM Sam Protsenko
<semen.protsenko@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@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);

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

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

[Sam Protsenko]

Hi Igor,

On Thu, Jun 20, 2019 at 5:55 PM Igor Opaniuk <igor.opaniuk@gmail.com> wrote:
>
> Hi Sam,
>
> On Thu, Jun 20, 2019 at 5:00 PM Sam Protsenko
> <semen.protsenko@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@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

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

[Igor Opaniuk]

On Thu, Jun 20, 2019 at 7:08 PM Sam Protsenko
<semen.protsenko@linaro.org> wrote:
>
> Hi Igor,
>
> On Thu, Jun 20, 2019 at 5:55 PM Igor Opaniuk <igor.opaniuk@gmail.com> wrote:
> >
> > Hi Sam,
> >
> > On Thu, Jun 20, 2019 at 5:00 PM Sam Protsenko
> > <semen.protsenko@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@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@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

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

[Eugeniu Rosca]

Hi Sam,

On Thu, Jun 20, 2019 at 05:00:01PM +0300, Sam Protsenko 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.
> 

FWIW, this could carry a Fixes line?
Fixes: 3aab70afc531d1 ("usb/gadget: add the fastboot gadget")

I strongly believe there are ongoing attempts/projects to build
relationships/dependencies between commits, which would help in the
context of bug prediction. Code Maat [2] comes to mind, but there could
be better examples.

> [1] https://android.googlesource.com/platform/system/core/+/master/fastboot/README.md#client-variables
[2] https://github.com/adamtornhill/code-maat 

In case AOSP gives it a second thought and drops or renames the
"is-userspace" option or the Client Variables section (both unlikely,
but possible), then the link will point to the wrong contents. This
happened to me in the past, but I am lazy to look for some records.

[..]

> 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

I am afraid this may introduce confusion, specifically that v2014.04+
(more precisely v2014.04-207-g3aab70afc531) U-Boot responded properly
to 'fastboot getvar version-bootloader'. Please, feel free to ignore.

[..]

> @@ -37,12 +37,9 @@ static const struct {
>  	{
>  		.variable = "version",
>  		.dispatch = getvar_version
> -	}, {
> -		.variable = "bootloader-version",
> -		.dispatch = getvar_bootloader_version
>  	}, {

On one hand, I agree with Igor that certain users might be hurt by
this change. On the other hand, it sounds good not to spend time and
effort maintaining non-AOSP options, added to U-Boot by accident.

Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>

Thanks!

-- 
Best Regards,
Eugeniu.

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

[Sam Protsenko]

Hi Eugeniu,

On Wed, Jun 26, 2019 at 1:12 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
>
> Hi Sam,
>
> On Thu, Jun 20, 2019 at 05:00:01PM +0300, Sam Protsenko 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.
> >
>
> FWIW, this could carry a Fixes line?
> Fixes: 3aab70afc531d1 ("usb/gadget: add the fastboot gadget")
>
> I strongly believe there are ongoing attempts/projects to build
> relationships/dependencies between commits, which would help in the
> context of bug prediction. Code Maat [2] comes to mind, but there could
> be better examples.
>
> > [1] https://android.googlesource.com/platform/system/core/+/master/fastboot/README.md#client-variables
> [2] https://github.com/adamtornhill/code-maat
>
> In case AOSP gives it a second thought and drops or renames the
> "is-userspace" option or the Client Variables section (both unlikely,
> but possible), then the link will point to the wrong contents. This
> happened to me in the past, but I am lazy to look for some records.
>
> [..]
>
> > 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
>
> I am afraid this may introduce confusion, specifically that v2014.04+
> (more precisely v2014.04-207-g3aab70afc531) U-Boot responded properly
> to 'fastboot getvar version-bootloader'. Please, feel free to ignore.
>
> [..]
>
> > @@ -37,12 +37,9 @@ static const struct {
> >       {
> >               .variable = "version",
> >               .dispatch = getvar_version
> > -     }, {
> > -             .variable = "bootloader-version",
> > -             .dispatch = getvar_bootloader_version
> >       }, {
>
> On one hand, I agree with Igor that certain users might be hurt by
> this change. On the other hand, it sounds good not to spend time and
> effort maintaining non-AOSP options, added to U-Boot by accident.
>

All comments are addressed in v2. Thanks!

> Reviewed-by: Eugeniu Rosca <erosca@de.adit-jv.com>
>
> Thanks!
>
> --
> Best Regards,
> Eugeniu.

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

[Eugeniu Rosca]

Hi Sam,

On Thu, Jun 20, 2019 at 05:00:01PM +0300, Sam Protsenko 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@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

This patch seems to depend on the BCB series [1].
The file renames which took place there are:

 --------8<--------
 doc/{README.avb2 => android/avb2.txt}
 doc/{README.android-fastboot => android/fastboot.txt}
 --------8<--------

The "README" wording is gone. Apparently Simon was fine with that.
Do you prefer leaving the README in place in the filename or is this
a typo? Sorry for any confusion contributed from my side and thanks
in advance for feedback.

[1] https://patchwork.ozlabs.org/patch/1104245/

-- 
Best Regards,
Eugeniu.

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

[Eugeniu Rosca]

Hi Sam,

Last comment was posted in the wrong thread.

On Thu, Jul 04, 2019 at 05:31:45PM +0200, Eugeniu Rosca wrote:
> > --- a/doc/README.android-fastboot
> > +++ b/doc/README.android-fastboot
> 
> This patch seems to depend on the BCB series [1].

This observation is valid for the v2, found here:
https://patchwork.ozlabs.org/patch/1126954/

[..]

> Do you prefer leaving the README in place in the filename or is this
> a typo? Sorry for any confusion contributed from my side and thanks
> in advance for feedback.

Please, ignore this feedback. It does not apply to v2.

-- 
Best Regards,
Eugeniu.