| Message ID | 2028294275.2280404.1344620768178.JavaMail.root@advansee.com |
|---|---|
| State | Rejected |
| Delegated to: | Wolfgang Denk |
| Headers | show |
On Friday 10 August 2012 13:46:08 Benoît Thébaudeau wrote:
> The board revision can be a useful env var, like its serial number.
unless i missed something, there is no standard "rev" variable today, which
means this change can easily break anyone who happens to already be using a
variable named "rev".
i also don't see value here in hardcoding another variable that:
- no one is setting
- is way too generic (rev of *what* ? cpu ? board ? u-boot ?
something else ?)
- adds nothing on top of the existing "serial#"
so NAK from me
-mike
Hi Mike, On Saturday 11 August 2012 19:48:33 Mike Frysinger wrote: > On Friday 10 August 2012 13:46:08 Benoît Thébaudeau wrote: > > The board revision can be a useful env var, like its serial number. > > unless i missed something, there is no standard "rev" variable today, > which > means this change can easily break anyone who happens to already be > using a > variable named "rev". I have searched such a usage in the tree, but did not find any, so this should not break anything. > i also don't see value here in hardcoding another variable that: > - no one is setting Well, I am setting it for my local boards that are not yet ready for upstream, so I thought that it could also be useful for others. > - is way too generic (rev of *what* ? cpu ? board ? u-boot ? > something else ?) It could be renamed to hwrev, board_rev or whatever you like. This is not really an issue. Its purpose is the board hardware revision. The CPU revision can often be read from the CPU and is printed upon startup. U-Boot's revision already has the ver env var and the version command. On the contrary, the board revision can not always be determined by analyzing the hardware (OTP, fuses, EEPROM, GPIOs, etc.), so it can be useful to have an official env var to store it in the backed up env, exactly like for the serial# env var that can not always be stored in some dedicated hardware location. > - adds nothing on top of the existing "serial#" This is all the contrary. If you think that a rev variable would be useless, then you can also remove the serial# variable. And if you mean that a rev variable would duplicate the serial# information, this is wrong: The serial# variable gives a UID for a board, while the rev variable would give the hardware revision of that board. These are completely different things, and the board revision can not always be easily derived from its serial number. > so NAK from me > -mike Best regards, Benoît
Dear =?utf-8?Q?Beno=C3=AEt_Th=C3=A9baudeau?=,
In message <2028294275.2280404.1344620768178.JavaMail.root@advansee.com> you wrote:
> The board revision can be a useful env var, like its serial number.
It can be useful, but it appears not many boards needed it so far,
so I suggest we leave as is; boards that want such a thing can
implement it easily in their board specific code.
Best regards,
Wolfgang Denk
Dear =?utf-8?Q?Beno=C3=AEt_Th=C3=A9baudeau?=, In message <1666183421.2304101.1344712292461.JavaMail.root@advansee.com> you wrote: > > I have searched such a usage in the tree, but did not find any, so this should > not break anything. You cannot expect to see the real, production environments in the mainline source tree. > It could be renamed to hwrev, board_rev or whatever you like. This is not really > an issue. Its purpose is the board hardware revision. The CPU revision can often > be read from the CPU and is printed upon startup. U-Boot's revision already has > the ver env var and the version command. On the contrary, the board revision can > not always be determined by analyzing the hardware (OTP, fuses, EEPROM, GPIOs, > etc.), so it can be useful to have an official env var to store it in the backed > up env, exactly like for the serial# env var that can not always be stored in > some dedicated hardware location. As mentioned before, I don't see need for such a thing in general. Any such use is highly board specific, and vendors use different ways to address this. I don't intend to apply this patch, sorry. Best regards, Wolfgang Denk
Dear Wolfgang Denk, > > I have searched such a usage in the tree, but did not find any, so > > this should > > not break anything. > > You cannot expect to see the real, production environments in the > mainline source tree. Right, but the same applied to serial# and ethaddr when they were added, except if U-Boot deployment was not large enough at that time to worry you. > > It could be renamed to hwrev, board_rev or whatever you like. This > > is not really > > an issue. Its purpose is the board hardware revision. The CPU > > revision can often > > be read from the CPU and is printed upon startup. U-Boot's revision > > already has > > the ver env var and the version command. On the contrary, the board > > revision can > > not always be determined by analyzing the hardware (OTP, fuses, > > EEPROM, GPIOs, > > etc.), so it can be useful to have an official env var to store it > > in the backed > > up env, exactly like for the serial# env var that can not always be > > stored in > > some dedicated hardware location. > > As mentioned before, I don't see need for such a thing in general. > Any such use is highly board specific, and vendors use different ways > to address this. The same applies to serial# again. > I don't intend to apply this patch, sorry. OK. Best regards, Benoît
Dear Wolfgang Denk, > > > I have searched such a usage in the tree, but did not find any, > > > so > > > this should > > > not break anything. > > > > You cannot expect to see the real, production environments in the > > mainline source tree. > > Right, but the same applied to serial# and ethaddr when they were > added, except > if U-Boot deployment was not large enough at that time to worry you. > > > > It could be renamed to hwrev, board_rev or whatever you like. > > > This > > > is not really > > > an issue. Its purpose is the board hardware revision. The CPU > > > revision can often > > > be read from the CPU and is printed upon startup. U-Boot's > > > revision > > > already has > > > the ver env var and the version command. On the contrary, the > > > board > > > revision can > > > not always be determined by analyzing the hardware (OTP, fuses, > > > EEPROM, GPIOs, > > > etc.), so it can be useful to have an official env var to store > > > it > > > in the backed > > > up env, exactly like for the serial# env var that can not always > > > be > > > stored in > > > some dedicated hardware location. > > > > As mentioned before, I don't see need for such a thing in general. > > Any such use is highly board specific, and vendors use different > > ways > > to address this. > > The same applies to serial# again. > > > I don't intend to apply this patch, sorry. > > OK. Anyway, when you will have implemented read-only and volatile flags for env vars, this patch will no longer be needed. But with the current code, there is no way board-specific code can create a board revision env var and make it read-only, except with this patch. Best regards, Benoît
On Sunday 12 August 2012 10:02:48 Benoît Thébaudeau wrote: > Dear Wolfgang Denk, > > > I have searched such a usage in the tree, but did not find any, so > > > this should > > > not break anything. > > > > You cannot expect to see the real, production environments in the > > mainline source tree. > > Right, but the same applied to serial# and ethaddr when they were added, > except if U-Boot deployment was not large enough at that time to worry > you. which makes all the difference in the world. those two variables were set up this way before 2002 (at least, that's according to the git history, and that's when the source code was first imported, so i can't easily check just how far back it goes). as the project grows up, policies evolve. -mike
Dear Mike Frysinger, > > > > I have searched such a usage in the tree, but did not find any, > > > > so > > > > this should > > > > not break anything. > > > > > > You cannot expect to see the real, production environments in the > > > mainline source tree. > > > > Right, but the same applied to serial# and ethaddr when they were > > added, > > except if U-Boot deployment was not large enough at that time to > > worry > > you. > > which makes all the difference in the world. those two variables > were set up > this way before 2002 (at least, that's according to the git history, > and > that's when the source code was first imported, so i can't easily > check just > how far back it goes). as the project grows up, policies evolve. > -mike OK. Actually, the only reason for which I need this patch is to make a variable read-only, and the only reason for which you reject it is because you fear that it breaks something. So we could add a config like CONFIG_BOARD_REV_RO_VARIABLE to enable the code in my patch. But I think you won't like that either because you will find it too specific. What about adding a config like CONFIG_READONLY_VARS that would be an array initializer containing the names of the board-specific variables to make read-only? _do_env_set() and fw_env_write() would use it besides the hard-coded serial# and the like. That would give something like: #define CONFIG_READONLY_VARS {"my_ro_var1", "my_ro_var2", "board_rev"} That would be a very simple solution to make everyone happy before Wolfgang implements a more sophisticated solution with read-only and volatile flags. What do you think? Best regards, Benoît
Dear =?utf-8?Q?Beno=C3=AEt_Th=C3=A9baudeau?=, In message <1642597694.2324115.1344780168514.JavaMail.root@advansee.com> you wrote: > > > You cannot expect to see the real, production environments in the > > mainline source tree. > > Right, but the same applied to serial# and ethaddr when they were added, except > if U-Boot deployment was not large enough at that time to worry you. Did you check when these were added? This was long before the project even was called U-Boot... IIRC they were there right from the beginning when I implemented the first version of the environment... Best regards, Wolfgang Denk
Dear =?utf-8?Q?Beno=C3=AEt_Th=C3=A9baudeau?=, In message <1323086777.2324156.1344780597072.JavaMail.root@advansee.com> you wrote: > > Anyway, when you will have implemented read-only and volatile flags for env > vars, this patch will no longer be needed. But with the current code, there is > no way board-specific code can create a board revision env var and make it > read-only, except with this patch. Correct, so far we don't support custom read-only variables. But if we add these, then in a generic way, and definitely not for a single, specific variable. This is a lesson we learned form experience. Best regards, Wolfgang Denk
Dear =?utf-8?Q?Beno=C3=AEt_Th=C3=A9baudeau?=, In message <383502175.2331918.1344791463023.JavaMail.root@advansee.com> you wrote: > > OK. Actually, the only reason for which I need this patch is to make a variable > read-only, and the only reason for which you reject it is because you fear that > it breaks something. > > So we could add a config like CONFIG_BOARD_REV_RO_VARIABLE to enable the code in > my patch. But I think you won't like that either because you will find it too > specific. No, this may solve your problem, but will never scale for any real life use. > What about adding a config like CONFIG_READONLY_VARS that would be an array > initializer containing the names of the board-specific variables to make > read-only? _do_env_set() and fw_env_write() would use it besides the hard-coded > serial# and the like. That would give something like: > #define CONFIG_READONLY_VARS {"my_ro_var1", "my_ro_var2", "board_rev"} > That would be a very simple solution to make everyone happy before Wolfgang > implements a more sophisticated solution with read-only and volatile flags. What > do you think? Please feel free to add this to your local code. Best regards, Wolfgang Denk
On 08/12/2012 11:06 PM, Wolfgang Denk wrote: > Dear =?utf-8?Q?Beno=C3=AEt_Th=C3=A9baudeau?=, > > In message <1323086777.2324156.1344780597072.JavaMail.root@advansee.com> you wrote: >> Anyway, when you will have implemented read-only and volatile flags for env >> vars, this patch will no longer be needed. But with the current code, there is >> no way board-specific code can create a board revision env var and make it >> read-only, except with this patch. > Correct, so far we don't support custom read-only variables. But if > we add these, then in a generic way, and definitely not for a single, > specific variable. This is a lesson we learned form experience. > I didn't follow this thread, but would it be an option to create a command for these kind of things, instead of read-only variables? e.g. some like if get revision out; then; echo $out; else; echo revision not supported fi; Just a though, regards, Jeroen
Dear Wolfgang Denk, > > OK. Actually, the only reason for which I need this patch is to > > make a variable > > read-only, and the only reason for which you reject it is because > > you fear that > > it breaks something. > > > > So we could add a config like CONFIG_BOARD_REV_RO_VARIABLE to > > enable the code in > > my patch. But I think you won't like that either because you will > > find it too > > specific. > > No, this may solve your problem, but will never scale for any real > life use. OK. > > What about adding a config like CONFIG_READONLY_VARS that would be > > an array > > initializer containing the names of the board-specific variables to > > make > > read-only? _do_env_set() and fw_env_write() would use it besides > > the hard-coded > > serial# and the like. That would give something like: > > #define CONFIG_READONLY_VARS {"my_ro_var1", "my_ro_var2", > > "board_rev"} > > That would be a very simple solution to make everyone happy before > > Wolfgang > > implements a more sophisticated solution with read-only and > > volatile flags. What > > do you think? > > Please feel free to add this to your local code. By "local", do you mean that this new suggestion would still not be generic enough for you to be interested in it for a patch? Best regards, Benoît
On Sunday 12 August 2012 13:11:03 Benoît Thébaudeau wrote: > OK. Actually, the only reason for which I need this patch is to make a > variable read-only, and the only reason for which you reject it is because > you fear that it breaks something. and because it bloats the codebase for 0 gain for the vast majority of boards (every one currently in the tree). -mike
Dear =?utf-8?Q?Beno=C3=AEt_Th=C3=A9baudeau?=, In message <1012612599.2335651.1344807326593.JavaMail.root@advansee.com> you wrote: > > > Please feel free to add this to your local code. > > By "local", do you mean that this new suggestion would still not be generic > enough for you to be interested in it for a patch? Correct. Best regards, Wolfgang Denk
diff --git u-boot-4d3c95f.orig/README u-boot-4d3c95f/README index 369ea9c..2ea48cf 100644 --- u-boot-4d3c95f.orig/README +++ u-boot-4d3c95f/README @@ -2073,13 +2073,13 @@ The following options need to be configured: - Vendor Parameter Protection: U-Boot considers the values of the environment - variables "serial#" (Board Serial Number) and - "ethaddr" (Ethernet Address) to be parameters that - are set once by the board vendor / manufacturer, and - protects these variables from casual modification by - the user. Once set, these variables are read-only, - and write or delete attempts are rejected. You can - change this behaviour: + variables "serial#" (Board Serial Number), "rev" + (Board Revision) and "ethaddr" (Ethernet Address) + to be parameters that are set once by the board + vendor / manufacturer, and protects these variables + from casual modification by the user. Once set, + these variables are read-only, and write or delete + attempts are rejected. You can change this behaviour: If CONFIG_ENV_OVERWRITE is #defined in your config file, the write protection for vendor parameters is @@ -2090,8 +2090,8 @@ The following options need to be configured: _and_ CONFIG_OVERWRITE_ETHADDR_ONCE, a default Ethernet address is installed in the environment, which can be changed exactly ONCE by the user. [The - serial# is unaffected by this, i. e. it remains - read-only.] + serial# and rev are unaffected by this, i. e. they + remain read-only.] - Protected RAM: CONFIG_PRAM @@ -3968,10 +3968,11 @@ depending the information provided by your boot server: serverip - see above -There are two special Environment Variables: +There are three special Environment Variables: serial# - contains hardware identification information such as type string and/or serial number + rev - hardware revision ethaddr - Ethernet address These variables can be set only once (usually during manufacturing of diff --git u-boot-4d3c95f.orig/common/cmd_nvedit.c u-boot-4d3c95f/common/cmd_nvedit.c index 0f320cc..d16aeb6 100644 --- u-boot-4d3c95f.orig/common/cmd_nvedit.c +++ u-boot-4d3c95f/common/cmd_nvedit.c @@ -255,12 +255,13 @@ int _do_env_set(int flag, int argc, char * const argv[]) } /* - * Some variables like "ethaddr" and "serial#" can be set only - * once and cannot be deleted; also, "ver" is readonly. + * Some variables like "ethaddr", "serial#" and "rev" can be set only + * once and cannot be deleted. */ if (ep) { /* variable exists */ #ifndef CONFIG_ENV_OVERWRITE if (strcmp(name, "serial#") == 0 || + strcmp(name, "rev") == 0 || (strcmp(name, "ethaddr") == 0 #if defined(CONFIG_OVERWRITE_ETHADDR_ONCE) && defined(CONFIG_ETHADDR) && strcmp(ep->data, MK_STR(CONFIG_ETHADDR)) != 0 diff --git u-boot-4d3c95f.orig/tools/env/fw_env.c u-boot-4d3c95f/tools/env/fw_env.c index 1a2c227..b5aa3aa 100644 --- u-boot-4d3c95f.orig/tools/env/fw_env.c +++ u-boot-4d3c95f/tools/env/fw_env.c @@ -405,10 +405,11 @@ int fw_env_write(char *name, char *value) if (oldval) { #ifndef CONFIG_ENV_OVERWRITE /* - * Ethernet Address and serial# can be set only once + * Ethernet Address, serial# and rev can be set only once */ if ( (strcmp(name, "serial#") == 0) || + (strcmp (name, "rev") == 0) || ((strcmp(name, "ethaddr") == 0) #if defined(CONFIG_OVERWRITE_ETHADDR_ONCE) && defined(CONFIG_ETHADDR) && (strcmp(oldval, MK_STR(CONFIG_ETHADDR)) != 0) @@ -474,7 +475,7 @@ int fw_env_write(char *name, char *value) * Deletes or sets environment variables. Returns -1 and sets errno error codes: * 0 - OK * EINVAL - need at least 1 argument - * EROFS - certain variables ("ethaddr", "serial#") cannot be + * EROFS - certain variables ("ethaddr", "serial#", "rev") cannot be * modified or deleted * */
The board revision can be a useful env var, like its serial number. Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> Cc: Wolfgang Denk <wd@denx.de> --- {u-boot-4d3c95f.orig => u-boot-4d3c95f}/README | 21 ++++++++++---------- .../common/cmd_nvedit.c | 5 +++-- .../tools/env/fw_env.c | 5 +++-- 3 files changed, 17 insertions(+), 14 deletions(-)