Message ID | 20190620140001.12811-1-semen.protsenko@linaro.org |
---|---|
State | Superseded |
Delegated to: | Lukasz Majewski |
Headers | show |
Series | [U-Boot] fastboot: Remove "bootloader-version" variable | expand |
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).
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@gmail.com > skype: igor.opanyuk > +380 (93) 836 40 67 > http://ua.linkedin.com/in/iopaniuk
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@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>
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!
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.
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/
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.
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); }
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(-)