| Message ID | 02d1e3f265123ff0296b1c38b227f6d30393ee77.1304508448.git.holger.brunck@keymile.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Holger Brunck, In message <02d1e3f265123ff0296b1c38b227f6d30393ee77.1304508448.git.holger.brunck@keymile.com> you wrote: > From: Thomas Herzmann <thomas.herzmann@keymile.com> > > BoardId and HWKey are used to identify the HW class of a given board. > The correct values are stored in the inventory eeprom. During creation > time of a boot package the boardId and HWkey for the SW is stored in > the default environment and burned into the flash. During boottime > the values in the inventory and in the environment are compared to > avoid starting of a SW which is not authorized for this board. > > Some bootpackages are allowed to run on a set of different boardId > hwKey. In this case the environment variable boardIdListHex was added > to the default environment. In this case the command iterates over the > pair values and compares them with the values read from the inventory > eeprom. > > The syntax of such a boardIdListHex value is e.g.: 158_1 159_1 159_2 > > Signed-off-by: Thomas Herzmann <thomas.herzmann@keymile.com> > Signed-off-by: Holger Brunck <holger.brunck@keymile.com> > Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com> > Acked-by: Heiko Schocher <hs@denx.de> > cc: Wolfgang Denk <wd@denx.de> > cc: Detlev Zundel <dzu@denx.de> > --- > Changes for v2: > - split up first large patch series to three independent smaller > patch series > - give the cmd a more precise name > - rework the patch with inputs from W.Denk: > - adapt and enhance commit msg > - comment the code > - add error handling This patch has checkpatch warnings. Please fix. ... > + if (!envbid || !envhwkey) { > + /* > + * BoardId/HWkey not available in the environment, so try the > + * environment variable for BoardId/HWkey list > + */ > + char *bidhwklist = getenv("boardIdListHex"); > + if (bidhwklist) { Please insert a blank line after declarations. Best regards, Wolfgang Denk
Hi, On 05/05/2011 12:11 AM, Wolfgang Denk wrote: > Dear Holger Brunck, > > In message <02d1e3f265123ff0296b1c38b227f6d30393ee77.1304508448.git.holger.brunck@keymile.com> you wrote: >> From: Thomas Herzmann <thomas.herzmann@keymile.com> >> >> BoardId and HWKey are used to identify the HW class of a given board. >> The correct values are stored in the inventory eeprom. During creation >> time of a boot package the boardId and HWkey for the SW is stored in >> the default environment and burned into the flash. During boottime >> the values in the inventory and in the environment are compared to >> avoid starting of a SW which is not authorized for this board. >> >> Some bootpackages are allowed to run on a set of different boardId >> hwKey. In this case the environment variable boardIdListHex was added >> to the default environment. In this case the command iterates over the >> pair values and compares them with the values read from the inventory >> eeprom. >> >> The syntax of such a boardIdListHex value is e.g.: 158_1 159_1 159_2 >> >> Signed-off-by: Thomas Herzmann <thomas.herzmann@keymile.com> >> Signed-off-by: Holger Brunck <holger.brunck@keymile.com> >> Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com> >> Acked-by: Heiko Schocher <hs@denx.de> >> cc: Wolfgang Denk <wd@denx.de> >> cc: Detlev Zundel <dzu@denx.de> >> --- >> Changes for v2: >> - split up first large patch series to three independent smaller >> patch series >> - give the cmd a more precise name >> - rework the patch with inputs from W.Denk: >> - adapt and enhance commit msg >> - comment the code >> - add error handling > > This patch has checkpatch warnings. Please fix. > Ok the one warning that we exceed 80 characters per line is fixed, sorry for that. But there are two warnings remaining: WARNING: consider using strict_strtoul in preference to simple_strtoul #137: FILE: board/keymile/common/common.c:813: + bid = simple_strtoul(rest, &endp, 16); WARNING: consider using strict_strtoul in preference to simple_strtoul #141: FILE: board/keymile/common/common.c:817: + hwkey = simple_strtoul(rest, &endp, 16); I know that we use strict_strtoul in the same patch some lines above, but at this point we need *endp and we know that we got a non numeric character at the end. So using simple_strtoul at this point is exactly what we want here. Is it ok to ignore this warnings and add a comment above the codeline why we use simple_stroul? > ... >> + if (!envbid || !envhwkey) { >> + /* >> + * BoardId/HWkey not available in the environment, so try the >> + * environment variable for BoardId/HWkey list >> + */ >> + char *bidhwklist = getenv("boardIdListHex"); >> + if (bidhwklist) { > > Please insert a blank line after declarations. > > Ok fixed. Best regards Holger Brunck
Dear Holger Brunck, In message <4DC2AB4A.608@keymile.com> you wrote: > > > This patch has checkpatch warnings. Please fix. > > Ok the one warning that we exceed 80 characters per line is fixed, sorry for > that. But there are two warnings remaining: > WARNING: consider using strict_strtoul in preference to simple_strtoul > #137: FILE: board/keymile/common/common.c:813: > + bid = simple_strtoul(rest, &endp, 16); > > WARNING: consider using strict_strtoul in preference to simple_strtoul > #141: FILE: board/keymile/common/common.c:817: > + hwkey = simple_strtoul(rest, &endp, 16); > > I know that we use strict_strtoul in the same patch some lines above, but at > this point we need *endp and we know that we got a non numeric character at the > end. So using simple_strtoul at this point is exactly what we want here. Well, mixing both strict_strtoul() and simple_strtoul() [without any comment] in the same patch was what attracted my attention in the first place. > Is it ok to ignore this warnings and add a comment above the codeline why we use > simple_stroul? Indeed this needs a comment. Best regards, Wolfgang Denk
diff --git a/board/keymile/common/common.c b/board/keymile/common/common.c index 68e6b9f..602ebc8 100644 --- a/board/keymile/common/common.c +++ b/board/keymile/common/common.c @@ -32,6 +32,7 @@ #include <net.h> #include <netdev.h> #include <asm/io.h> +#include <linux/ctype.h> #if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_OF_LIBFDT) #include <libfdt.h> @@ -719,3 +720,141 @@ static int do_setboardid(cmd_tbl_t *cmdtp, int flag, int argc, U_BOOT_CMD(km_setboardid, 1, 0, do_setboardid, "setboardid", "read out bid and " "hwkey from IVM and set in environment"); + +/* + * command km_checkbidhwk + * if "boardid" and "hwkey" are not already set in the environment, do: + * if a "boardIdListHex" exists in the environment: + * - read ivm data for boardid and hwkey + * - compare each entry of the boardIdListHex with the + * IVM data: + * if match: + * set environment variables boardid, boardId, + * hwkey, hwKey to the found values + * both (boardid and boardId) are set because + * they might be used differently in the + * application and in the init scripts (?) + * return 0 in case of match, 1 if not match or error + */ +int do_checkboardidhwk(cmd_tbl_t *cmdtp, int flag, int argc, + char *const argv[]) +{ + unsigned long ivmbid = 0, ivmhwkey = 0; + unsigned long envbid = 0, envhwkey = 0; + char *p; + int verbose = argc > 1 && *argv[1] == 'v'; + int rc = 1; /* default: no match */ + + /* + * first read out the real inventory values, these values are + * already stored in the local hush variables + */ + p = get_local_var("IVM_BoardId"); + if (p == NULL) { + printf("can't get the IVM_Boardid\n"); + return rc; + } + strict_strtoul(p, 16, &ivmbid); + + p = get_local_var("IVM_HWKey"); + if (p == NULL) { + printf("can't get the IVM_HWKey\n"); + return rc; + } + strict_strtoul(p, 16, &ivmhwkey); + + if (!ivmbid || !ivmhwkey) { + printf("Error: IVM_BoardId and/or IVM_HWKey not set!\n"); + return rc; + } + + /* now try to read values from environment if available */ + p = getenv("boardid"); + if (p != NULL) + strict_strtoul(p, 16, &envbid); + p = getenv("hwkey"); + if (p != NULL) + strict_strtoul(p, 16, &envhwkey); + + if (!envbid || !envhwkey) { + /* + * BoardId/HWkey not available in the environment, so try the + * environment variable for BoardId/HWkey list + */ + char *bidhwklist = getenv("boardIdListHex"); + if (bidhwklist) { + int found = 0; + char *rest = bidhwklist; + char *endp; + if (verbose) { + printf("IVM_BoardId: %ld, IVM_HWKey=%ld\n", + ivmbid, ivmhwkey); + printf("boardIdHwKeyList: %s\n", + bidhwklist); + } + while (!found) { + /* loop over each bid/hwkey pair in the list */ + unsigned long bid = 0; + unsigned long hwkey = 0; + while (*rest && !isxdigit(*rest)) + rest++; + bid = simple_strtoul(rest, &endp, 16); + /* BoardId/HWkey is separated with a "_" for one entry */ + if (*endp == '_') { + rest = endp + 1; + hwkey = simple_strtoul(rest, &endp, 16); + rest = endp; + while (*rest && !isxdigit(*rest)) + rest++; + } + if ((!bid) || (!hwkey)) { + /* end of list */ + break; + } + if (verbose) { + printf("trying bid=0x%lX, hwkey=%ld\n", + bid, hwkey); + } + /* + * Compare the values of the found entry in the + * list with the valid values which are stored + * in the inventory eeprom. If they are equal + * store the values in environment variables + * and save the environment. + * This can only happen once for the lifetime + * of a board, because once saved the function + * will never reach the while loop. + */ + if ((bid == ivmbid) && (hwkey == ivmhwkey)) { + char buf[10]; + + found = 1; + envbid = bid; + envhwkey = hwkey; + sprintf(buf, "%lx", bid); + setenv("boardid", buf); + sprintf(buf, "%lx", hwkey); + setenv("hwkey", buf); + saveenv(); + } + } /* end while( ! found ) */ + } + } + + /* compare now the values */ + if ((ivmbid == envbid) && (ivmhwkey == envhwkey)) { + printf("boardid=0x%3lX, hwkey=%ld\n", envbid, envhwkey); + rc = 0; + } else { + printf("Error: env bId=0x%3lX, hwKey=%ld\n", envbid, envhwkey); + printf(" IVM bId=0x%3lX, hwKey=%ld\n", ivmbid, ivmhwkey); + } + return rc; +} + +U_BOOT_CMD(km_checkbidhwk, 2, 0, do_checkboardidhwk, + "check boardid and hwkey", + "[v]\n - check environment parameter "\ + "\"boardIdListHex\" against stored boardid and hwkey "\ + "from the IVM\n v: verbose output" +); diff --git a/include/configs/keymile-common.h b/include/configs/keymile-common.h index bc8a896..da3711b 100644 --- a/include/configs/keymile-common.h +++ b/include/configs/keymile-common.h @@ -238,7 +238,6 @@ "release=" \ "setenv actual_bank ${initial_boot_bank} && " \ "setenv subbootcmds \"" \ - "checkboardidlist " \ "checkboardid " \ "ubiattach ubicopy " \ "cramfsloadfdt cramfsloadkernel " \ @@ -389,36 +388,9 @@ "default=" \ "setenv default 'run newenv; reset' && " \ "run release && saveenv; reset\0" \ - "checkboardidlist=" \ - "if test \"x${boardIdListHex}\" != \"x\"; then " \ - "IVMbidhwk=${IVM_BoardId}_${IVM_HWKey}; " \ - "found=0; " \ - "for bidhwk in \"${boardIdListHex}\"; do " \ - "echo trying $bidhwk ...; " \ - "if test \"x$bidhwk\" = \"x$IVMbidhwk\"; then " \ - "found=1; " \ - "echo match found for $bidhwk; " \ - "if test \"x$bidhwk\" != \"x${boardId}_${hwKey}\";then "\ - "setenv boardid ${IVM_BoardId}; " \ - "setenv boardId ${IVM_BoardId}; " \ - "setenv hwkey ${IVM_HWKey}; " \ - "setenv hwKey ${IVM_HWKey}; " \ - "echo \"boardId set to ${boardId}\"; " \ - "echo \"hwKey set to ${hwKey}\"; " \ - "saveenv; " \ - "fi; " \ - "fi; " \ - "done; " \ - "else " \ - "echo \"boardIdListHex not set, not checked\"; "\ - "found=1; " \ - "fi; " \ - "test \"$found\" = 1 \0" \ - "checkboardid=" \ - "test \"x${boardId}\" = \"x${IVM_BoardId}\" && " \ - "test \"x${hwKey}\" = \"x${IVM_HWKey}\"\0" \ "printbootargs=print bootargs\0" \ "rootfsfile="xstr(CONFIG_HOSTNAME) "/rootfsImage\0" \ + "checkboardid=km_checkbidhwk\0" \ "" #ifndef CONFIG_KM_DEF_ENV