| Message ID | 20220502141838.15912-5-josua@solid-run.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | split tlv_eeprom command into a separate library | expand |
On 02.05.22 16:18, Josua Mayer wrote: > - prog_eeprom: write_tlvinfo_tlv_eeprom > - update_crc: tlvinfo_update_crc > - is_valid_tlv: is_valid_tlvinfo_entry > - is_checksum_valid: tlvinfo_check_crc So while creating a new API it makes sense to prepend the function names identical IMHO to not "pollute" the namespace. Something like - tlv_is_valid_entry - tlv_check_crc ... Just examples, you get the idea. Thanks, Stefan > Signed-off-by: Josua Mayer <josua@solid-run.com> > --- > cmd/tlv_eeprom.c | 56 +++++++++++++++---------------------------- > include/tlv_eeprom.h | 57 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 76 insertions(+), 37 deletions(-) > > diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c > index 00c5b5f840..1b4f2537f6 100644 > --- a/cmd/tlv_eeprom.c > +++ b/cmd/tlv_eeprom.c > @@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR; > #define MAX_TLV_DEVICES 2 > > /* File scope function prototypes */ > -static bool is_checksum_valid(u8 *eeprom); > static int read_eeprom(int devnum, u8 *eeprom); > static void show_eeprom(int devnum, u8 *eeprom); > static void decode_tlv(struct tlvinfo_tlv *tlv); > -static void update_crc(u8 *eeprom); > -static int prog_eeprom(int devnum, u8 *eeprom); > -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index); > static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code); > static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval); > static int set_mac(char *buf, const char *string); > @@ -58,18 +54,6 @@ static inline bool is_digit(char c) > return (c >= '0' && c <= '9'); > } > > -/** > - * is_valid_tlv > - * > - * Perform basic sanity checks on a TLV field. The TLV is pointed to > - * by the parameter provided. > - * 1. The type code is not reserved (0x00 or 0xFF) > - */ > -static inline bool is_valid_tlv(struct tlvinfo_tlv *tlv) > -{ > - return((tlv->type != 0x00) && (tlv->type != 0xFF)); > -} > - > /** > * is_hex > * > @@ -83,14 +67,12 @@ static inline u8 is_hex(char p) > } > > /** > - * is_checksum_valid > - * > * Validate the checksum in the provided TlvInfo EEPROM data. First, > * verify that the TlvInfo header is valid, then make sure the last > * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data > * and compare it to the value stored in the EEPROM CRC-32 TLV. > */ > -static bool is_checksum_valid(u8 *eeprom) > +bool tlvinfo_check_crc(u8 *eeprom) > { > struct tlvinfo_header *eeprom_hdr = to_header(eeprom); > struct tlvinfo_tlv *eeprom_crc; > @@ -137,11 +119,11 @@ static int read_eeprom(int devnum, u8 *eeprom) > > // If the contents are invalid, start over with default contents > if (!is_valid_tlvinfo_header(eeprom_hdr) || > - !is_checksum_valid(eeprom)) { > + !tlvinfo_check_crc(eeprom)) { > strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); > eeprom_hdr->version = TLV_INFO_VERSION; > eeprom_hdr->totallen = cpu_to_be16(0); > - update_crc(eeprom); > + tlvinfo_update_crc(eeprom); > } > > #ifdef DEBUG > @@ -183,7 +165,7 @@ static void show_eeprom(int devnum, u8 *eeprom) > tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); > while (curr_tlv < tlv_end) { > eeprom_tlv = to_entry(&eeprom[curr_tlv]); > - if (!is_valid_tlv(eeprom_tlv)) { > + if (!is_valid_tlvinfo_entry(eeprom_tlv)) { > printf("Invalid TLV field starting at EEPROM offset %d\n", > curr_tlv); > return; > @@ -193,7 +175,7 @@ static void show_eeprom(int devnum, u8 *eeprom) > } > > printf("Checksum is %s.\n", > - is_checksum_valid(eeprom) ? "valid" : "invalid"); > + tlvinfo_check_crc(eeprom) ? "valid" : "invalid"); > > #ifdef DEBUG > printf("EEPROM dump: (0x%x bytes)", TLV_INFO_MAX_LEN); > @@ -340,13 +322,13 @@ static void decode_tlv(struct tlvinfo_tlv *tlv) > } > > /** > - * update_crc > + * tlvinfo_update_crc > * > * This function updates the CRC-32 TLV. If there is no CRC-32 TLV, then > * one is added. This function should be called after each update to the > * EEPROM structure, to make sure the CRC is always correct. > */ > -static void update_crc(u8 *eeprom) > +void tlvinfo_update_crc(u8 *eeprom) > { > struct tlvinfo_header *eeprom_hdr = to_header(eeprom); > struct tlvinfo_tlv *eeprom_crc; > @@ -376,20 +358,20 @@ static void update_crc(u8 *eeprom) > } > > /** > - * prog_eeprom > + * write_tlvinfo_tlv_eeprom > * > - * Write the EEPROM data from CPU memory to the hardware. > + * Write the TLV data from CPU memory to the hardware. > */ > -static int prog_eeprom(int devnum, u8 *eeprom) > +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev) > { > int ret = 0; > struct tlvinfo_header *eeprom_hdr = to_header(eeprom); > int eeprom_len; > > - update_crc(eeprom); > + tlvinfo_update_crc(eeprom); > > eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); > - ret = write_tlv_eeprom(eeprom, eeprom_len, devnum); > + ret = write_tlv_eeprom(eeprom, eeprom_len, dev); > if (ret) { > printf("Programming failed.\n"); > return -1; > @@ -462,13 +444,13 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) > if (argc == 2) { > switch (cmd) { > case 'w': /* write */ > - prog_eeprom(current_dev, eeprom); > + write_tlvinfo_tlv_eeprom(eeprom, current_dev); > break; > case 'e': /* erase */ > strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); > eeprom_hdr->version = TLV_INFO_VERSION; > eeprom_hdr->totallen = cpu_to_be16(0); > - update_crc(eeprom); > + tlvinfo_update_crc(eeprom); > printf("EEPROM data in memory reset.\n"); > break; > case 'l': /* list */ > @@ -549,7 +531,7 @@ U_BOOT_CMD(tlv_eeprom, 4, 1, do_tlv_eeprom, > * An offset from the beginning of the EEPROM is returned in the > * eeprom_index parameter if the TLV is found. > */ > -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) > +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) > { > struct tlvinfo_header *eeprom_hdr = to_header(eeprom); > struct tlvinfo_tlv *eeprom_tlv; > @@ -561,7 +543,7 @@ static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) > eeprom_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); > while (*eeprom_index < eeprom_end) { > eeprom_tlv = to_entry(&eeprom[*eeprom_index]); > - if (!is_valid_tlv(eeprom_tlv)) > + if (!is_valid_tlvinfo_entry(eeprom_tlv)) > return false; > if (eeprom_tlv->type == tcode) > return true; > @@ -594,7 +576,7 @@ static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code) > eeprom_hdr->totallen = > cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - > tlength); > - update_crc(eeprom); > + tlvinfo_update_crc(eeprom); > return true; > } > return false; > @@ -695,7 +677,7 @@ static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval) > // Update the total length and calculate (add) a new CRC-32 TLV > eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) + > ENT_SIZE + new_tlv_len); > - update_crc(eeprom); > + tlvinfo_update_crc(eeprom); > > return true; > } > @@ -986,7 +968,7 @@ int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, > be16_to_cpu(tlv_hdr->totallen), dev_num); > if (ret < 0) > return ret; > - if (!is_checksum_valid(eeprom)) > + if (!tlvinfo_check_crc(eeprom)) > return -EINVAL; > > *hdr = tlv_hdr; > diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h > index fd45e5f6eb..30626a1067 100644 > --- a/include/tlv_eeprom.h > +++ b/include/tlv_eeprom.h > @@ -111,6 +111,51 @@ int write_tlv_eeprom(void *eeprom, int len, int dev); > int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, > struct tlvinfo_tlv **first_entry, int dev); > > +/** > + * Write TLV data to the EEPROM. > + * > + * - Only writes length of actual tlv data > + * - updates checksum > + * > + * @eeprom: Pointer to buffer to hold the binary data. Must point to a buffer > + * of size at least TLV_INFO_MAX_LEN. > + * @dev : EEPROM device to write > + * > + */ > +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev); > + > +/** > + * tlvinfo_find_tlv > + * > + * This function finds the TLV with the supplied code in the EERPOM. > + * An offset from the beginning of the EEPROM is returned in the > + * eeprom_index parameter if the TLV is found. > + */ > +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index); > + > +/** > + * tlvinfo_update_crc > + * > + * This function updates the CRC-32 TLV. If there is no CRC-32 TLV, then > + * one is added. This function should be called after each update to the > + * EEPROM structure, to make sure the CRC is always correct. > + * > + * @eeprom: Pointer to buffer to hold the binary data. Must point to a buffer > + * of size at least TLV_INFO_MAX_LEN. > + */ > +void tlvinfo_update_crc(u8 *eeprom); > + > +/** > + * Validate the checksum in the provided TlvInfo EEPROM data. First, > + * verify that the TlvInfo header is valid, then make sure the last > + * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data > + * and compare it to the value stored in the EEPROM CRC-32 TLV. > + * > + * @eeprom: Pointer to buffer to hold the binary data. Must point to a buffer > + * of size at least TLV_INFO_MAX_LEN. > + */ > +bool tlvinfo_check_crc(u8 *eeprom); > + > #else /* !CONFIG_IS_ENABLED(CMD_TLV_EEPROM) */ > > static inline int read_tlv_eeprom(void *eeprom, int offset, int len, int dev) > @@ -150,4 +195,16 @@ static inline bool is_valid_tlvinfo_header(struct tlvinfo_header *hdr) > (be16_to_cpu(hdr->totallen) <= TLV_TOTAL_LEN_MAX)); > } > > +/** > + * is_valid_tlv > + * > + * Perform basic sanity checks on a TLV field. The TLV is pointed to > + * by the parameter provided. > + * 1. The type code is not reserved (0x00 or 0xFF) > + */ > +static inline bool is_valid_tlvinfo_entry(struct tlvinfo_tlv *tlv) > +{ > + return((tlv->type != 0x00) && (tlv->type != 0xFF)); > +} > + > #endif /* __TLV_EEPROM_H_ */ Viele Grüße, Stefan Roese
Am 03.05.22 um 09:16 schrieb Stefan Roese: > On 02.05.22 16:18, Josua Mayer wrote: >> - prog_eeprom: write_tlvinfo_tlv_eeprom >> - update_crc: tlvinfo_update_crc >> - is_valid_tlv: is_valid_tlvinfo_entry >> - is_checksum_valid: tlvinfo_check_crc > > So while creating a new API it makes sense to prepend the function > names identical IMHO to not "pollute" the namespace. Something like > > - tlv_is_valid_entry > - tlv_check_crc > ... > > Just examples, you get the idea. Yes. The hard part in this particular implementation is that the naming is not consistent. The most sense I could make is that prefix tlvinfo indicates all tlv data, i.e. working with the whole structure, while tlvinfo_tlv indicates working with one data entry. Further write, read and is_ are currently prefixed in the header, but for previously static functions in the C file it was put in the middle ... I found it quite difficult to prepare for splitting off a library in a way that preserves history, i.e. diffs should still be readable for spotting mistakes. I was considering to at the very end do a mass-rename and come up with better naming, something like tlv_{set,get}_{blob,string,mac} tlv_find_entry tlv_{read,write}_eeprom But this is pending a refactoring and extension of the tlv parsing code in board/solidrun/common/tlv_data.*, to figure out what is required or useful. > > Thanks, > Stefan > >> Signed-off-by: Josua Mayer <josua@solid-run.com> >> --- >> cmd/tlv_eeprom.c | 56 +++++++++++++++---------------------------- >> include/tlv_eeprom.h | 57 ++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 76 insertions(+), 37 deletions(-) >> >> diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c >> index 00c5b5f840..1b4f2537f6 100644 >> --- a/cmd/tlv_eeprom.c >> +++ b/cmd/tlv_eeprom.c >> @@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR; >> #define MAX_TLV_DEVICES 2 >> /* File scope function prototypes */ >> -static bool is_checksum_valid(u8 *eeprom); >> static int read_eeprom(int devnum, u8 *eeprom); >> static void show_eeprom(int devnum, u8 *eeprom); >> static void decode_tlv(struct tlvinfo_tlv *tlv); >> -static void update_crc(u8 *eeprom); >> -static int prog_eeprom(int devnum, u8 *eeprom); >> -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index); >> static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code); >> static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval); >> static int set_mac(char *buf, const char *string); >> @@ -58,18 +54,6 @@ static inline bool is_digit(char c) >> return (c >= '0' && c <= '9'); >> } >> -/** >> - * is_valid_tlv >> - * >> - * Perform basic sanity checks on a TLV field. The TLV is pointed to >> - * by the parameter provided. >> - * 1. The type code is not reserved (0x00 or 0xFF) >> - */ >> -static inline bool is_valid_tlv(struct tlvinfo_tlv *tlv) >> -{ >> - return((tlv->type != 0x00) && (tlv->type != 0xFF)); >> -} >> - >> /** >> * is_hex >> * >> @@ -83,14 +67,12 @@ static inline u8 is_hex(char p) >> } >> /** >> - * is_checksum_valid >> - * >> * Validate the checksum in the provided TlvInfo EEPROM data. First, >> * verify that the TlvInfo header is valid, then make sure the last >> * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data >> * and compare it to the value stored in the EEPROM CRC-32 TLV. >> */ >> -static bool is_checksum_valid(u8 *eeprom) >> +bool tlvinfo_check_crc(u8 *eeprom) >> { >> struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >> struct tlvinfo_tlv *eeprom_crc; >> @@ -137,11 +119,11 @@ static int read_eeprom(int devnum, u8 *eeprom) >> // If the contents are invalid, start over with default contents >> if (!is_valid_tlvinfo_header(eeprom_hdr) || >> - !is_checksum_valid(eeprom)) { >> + !tlvinfo_check_crc(eeprom)) { >> strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); >> eeprom_hdr->version = TLV_INFO_VERSION; >> eeprom_hdr->totallen = cpu_to_be16(0); >> - update_crc(eeprom); >> + tlvinfo_update_crc(eeprom); >> } >> #ifdef DEBUG >> @@ -183,7 +165,7 @@ static void show_eeprom(int devnum, u8 *eeprom) >> tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); >> while (curr_tlv < tlv_end) { >> eeprom_tlv = to_entry(&eeprom[curr_tlv]); >> - if (!is_valid_tlv(eeprom_tlv)) { >> + if (!is_valid_tlvinfo_entry(eeprom_tlv)) { >> printf("Invalid TLV field starting at EEPROM offset %d\n", >> curr_tlv); >> return; >> @@ -193,7 +175,7 @@ static void show_eeprom(int devnum, u8 *eeprom) >> } >> printf("Checksum is %s.\n", >> - is_checksum_valid(eeprom) ? "valid" : "invalid"); >> + tlvinfo_check_crc(eeprom) ? "valid" : "invalid"); >> #ifdef DEBUG >> printf("EEPROM dump: (0x%x bytes)", TLV_INFO_MAX_LEN); >> @@ -340,13 +322,13 @@ static void decode_tlv(struct tlvinfo_tlv *tlv) >> } >> /** >> - * update_crc >> + * tlvinfo_update_crc >> * >> * This function updates the CRC-32 TLV. If there is no CRC-32 >> TLV, then >> * one is added. This function should be called after each update >> to the >> * EEPROM structure, to make sure the CRC is always correct. >> */ >> -static void update_crc(u8 *eeprom) >> +void tlvinfo_update_crc(u8 *eeprom) >> { >> struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >> struct tlvinfo_tlv *eeprom_crc; >> @@ -376,20 +358,20 @@ static void update_crc(u8 *eeprom) >> } >> /** >> - * prog_eeprom >> + * write_tlvinfo_tlv_eeprom >> * >> - * Write the EEPROM data from CPU memory to the hardware. >> + * Write the TLV data from CPU memory to the hardware. >> */ >> -static int prog_eeprom(int devnum, u8 *eeprom) >> +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev) >> { >> int ret = 0; >> struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >> int eeprom_len; >> - update_crc(eeprom); >> + tlvinfo_update_crc(eeprom); >> eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); >> - ret = write_tlv_eeprom(eeprom, eeprom_len, devnum); >> + ret = write_tlv_eeprom(eeprom, eeprom_len, dev); >> if (ret) { >> printf("Programming failed.\n"); >> return -1; >> @@ -462,13 +444,13 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int >> flag, int argc, char *const argv[]) >> if (argc == 2) { >> switch (cmd) { >> case 'w': /* write */ >> - prog_eeprom(current_dev, eeprom); >> + write_tlvinfo_tlv_eeprom(eeprom, current_dev); >> break; >> case 'e': /* erase */ >> strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); >> eeprom_hdr->version = TLV_INFO_VERSION; >> eeprom_hdr->totallen = cpu_to_be16(0); >> - update_crc(eeprom); >> + tlvinfo_update_crc(eeprom); >> printf("EEPROM data in memory reset.\n"); >> break; >> case 'l': /* list */ >> @@ -549,7 +531,7 @@ U_BOOT_CMD(tlv_eeprom, 4, 1, do_tlv_eeprom, >> * An offset from the beginning of the EEPROM is returned in the >> * eeprom_index parameter if the TLV is found. >> */ >> -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) >> +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) >> { >> struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >> struct tlvinfo_tlv *eeprom_tlv; >> @@ -561,7 +543,7 @@ static bool tlvinfo_find_tlv(u8 *eeprom, u8 >> tcode, int *eeprom_index) >> eeprom_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); >> while (*eeprom_index < eeprom_end) { >> eeprom_tlv = to_entry(&eeprom[*eeprom_index]); >> - if (!is_valid_tlv(eeprom_tlv)) >> + if (!is_valid_tlvinfo_entry(eeprom_tlv)) >> return false; >> if (eeprom_tlv->type == tcode) >> return true; >> @@ -594,7 +576,7 @@ static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code) >> eeprom_hdr->totallen = >> cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - >> tlength); >> - update_crc(eeprom); >> + tlvinfo_update_crc(eeprom); >> return true; >> } >> return false; >> @@ -695,7 +677,7 @@ static bool tlvinfo_add_tlv(u8 *eeprom, int >> tcode, char *strval) >> // Update the total length and calculate (add) a new CRC-32 TLV >> eeprom_hdr->totallen = >> cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) + >> ENT_SIZE + new_tlv_len); >> - update_crc(eeprom); >> + tlvinfo_update_crc(eeprom); >> return true; >> } >> @@ -986,7 +968,7 @@ int read_tlvinfo_tlv_eeprom(void *eeprom, struct >> tlvinfo_header **hdr, >> be16_to_cpu(tlv_hdr->totallen), dev_num); >> if (ret < 0) >> return ret; >> - if (!is_checksum_valid(eeprom)) >> + if (!tlvinfo_check_crc(eeprom)) >> return -EINVAL; >> *hdr = tlv_hdr; >> diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h >> index fd45e5f6eb..30626a1067 100644 >> --- a/include/tlv_eeprom.h >> +++ b/include/tlv_eeprom.h >> @@ -111,6 +111,51 @@ int write_tlv_eeprom(void *eeprom, int len, int >> dev); >> int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, >> struct tlvinfo_tlv **first_entry, int dev); >> +/** >> + * Write TLV data to the EEPROM. >> + * >> + * - Only writes length of actual tlv data >> + * - updates checksum >> + * >> + * @eeprom: Pointer to buffer to hold the binary data. Must point to >> a buffer >> + * of size at least TLV_INFO_MAX_LEN. >> + * @dev : EEPROM device to write >> + * >> + */ >> +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev); >> + >> +/** >> + * tlvinfo_find_tlv >> + * >> + * This function finds the TLV with the supplied code in the EERPOM. >> + * An offset from the beginning of the EEPROM is returned in the >> + * eeprom_index parameter if the TLV is found. >> + */ >> +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index); >> + >> +/** >> + * tlvinfo_update_crc >> + * >> + * This function updates the CRC-32 TLV. If there is no CRC-32 TLV, >> then >> + * one is added. This function should be called after each update >> to the >> + * EEPROM structure, to make sure the CRC is always correct. >> + * >> + * @eeprom: Pointer to buffer to hold the binary data. Must point to >> a buffer >> + * of size at least TLV_INFO_MAX_LEN. >> + */ >> +void tlvinfo_update_crc(u8 *eeprom); >> + >> +/** >> + * Validate the checksum in the provided TlvInfo EEPROM data. First, >> + * verify that the TlvInfo header is valid, then make sure the last >> + * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data >> + * and compare it to the value stored in the EEPROM CRC-32 TLV. >> + * >> + * @eeprom: Pointer to buffer to hold the binary data. Must point to >> a buffer >> + * of size at least TLV_INFO_MAX_LEN. >> + */ >> +bool tlvinfo_check_crc(u8 *eeprom); >> + >> #else /* !CONFIG_IS_ENABLED(CMD_TLV_EEPROM) */ >> static inline int read_tlv_eeprom(void *eeprom, int offset, int >> len, int dev) >> @@ -150,4 +195,16 @@ static inline bool >> is_valid_tlvinfo_header(struct tlvinfo_header *hdr) >> (be16_to_cpu(hdr->totallen) <= TLV_TOTAL_LEN_MAX)); >> } >> +/** >> + * is_valid_tlv >> + * >> + * Perform basic sanity checks on a TLV field. The TLV is pointed to >> + * by the parameter provided. >> + * 1. The type code is not reserved (0x00 or 0xFF) >> + */ >> +static inline bool is_valid_tlvinfo_entry(struct tlvinfo_tlv *tlv) >> +{ >> + return((tlv->type != 0x00) && (tlv->type != 0xFF)); >> +} >> + >> #endif /* __TLV_EEPROM_H_ */ > > Viele Grüße, > Stefan Roese >
Hi Josua, On 03.05.22 09:17, Josua Mayer wrote: > Am 03.05.22 um 09:16 schrieb Stefan Roese: >> On 02.05.22 16:18, Josua Mayer wrote: >>> - prog_eeprom: write_tlvinfo_tlv_eeprom >>> - update_crc: tlvinfo_update_crc >>> - is_valid_tlv: is_valid_tlvinfo_entry >>> - is_checksum_valid: tlvinfo_check_crc >> >> So while creating a new API it makes sense to prepend the function >> names identical IMHO to not "pollute" the namespace. Something like >> >> - tlv_is_valid_entry >> - tlv_check_crc >> ... >> >> Just examples, you get the idea. > Yes. The hard part in this particular implementation is that the naming > is not consistent. > > The most sense I could make is that prefix tlvinfo indicates all tlv > data, i.e. working with the whole structure, while tlvinfo_tlv indicates > working with one data entry. Further write, read and is_ are currently > prefixed in the header, but for previously static functions in the C > file it was put in the middle ... > > I found it quite difficult to prepare for splitting off a library in a > way that preserves history, i.e. diffs should still be readable for > spotting mistakes. Yes, a decent history would be welcome. But still, when going global here with a new API this should be consistant. > I was considering to at the very end do a mass-rename and come up with > better naming, something like > tlv_{set,get}_{blob,string,mac} > tlv_find_entry > tlv_{read,write}_eeprom > > But this is pending a refactoring and extension of the tlv parsing code > in board/solidrun/common/tlv_data.*, to figure out what is required or > useful. So your plan is to this: a) Get this patchset included b) Use it in board specific code, e.g. solidrun c) Do the mass-rename Is this correct? If yes, why is it better to do the renaming at the end? Thanks, Stefan >> >> Thanks, >> Stefan >> >>> Signed-off-by: Josua Mayer <josua@solid-run.com> >>> --- >>> cmd/tlv_eeprom.c | 56 +++++++++++++++---------------------------- >>> include/tlv_eeprom.h | 57 ++++++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 76 insertions(+), 37 deletions(-) >>> >>> diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c >>> index 00c5b5f840..1b4f2537f6 100644 >>> --- a/cmd/tlv_eeprom.c >>> +++ b/cmd/tlv_eeprom.c >>> @@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR; >>> #define MAX_TLV_DEVICES 2 >>> /* File scope function prototypes */ >>> -static bool is_checksum_valid(u8 *eeprom); >>> static int read_eeprom(int devnum, u8 *eeprom); >>> static void show_eeprom(int devnum, u8 *eeprom); >>> static void decode_tlv(struct tlvinfo_tlv *tlv); >>> -static void update_crc(u8 *eeprom); >>> -static int prog_eeprom(int devnum, u8 *eeprom); >>> -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index); >>> static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code); >>> static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval); >>> static int set_mac(char *buf, const char *string); >>> @@ -58,18 +54,6 @@ static inline bool is_digit(char c) >>> return (c >= '0' && c <= '9'); >>> } >>> -/** >>> - * is_valid_tlv >>> - * >>> - * Perform basic sanity checks on a TLV field. The TLV is pointed to >>> - * by the parameter provided. >>> - * 1. The type code is not reserved (0x00 or 0xFF) >>> - */ >>> -static inline bool is_valid_tlv(struct tlvinfo_tlv *tlv) >>> -{ >>> - return((tlv->type != 0x00) && (tlv->type != 0xFF)); >>> -} >>> - >>> /** >>> * is_hex >>> * >>> @@ -83,14 +67,12 @@ static inline u8 is_hex(char p) >>> } >>> /** >>> - * is_checksum_valid >>> - * >>> * Validate the checksum in the provided TlvInfo EEPROM data. First, >>> * verify that the TlvInfo header is valid, then make sure the last >>> * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data >>> * and compare it to the value stored in the EEPROM CRC-32 TLV. >>> */ >>> -static bool is_checksum_valid(u8 *eeprom) >>> +bool tlvinfo_check_crc(u8 *eeprom) >>> { >>> struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >>> struct tlvinfo_tlv *eeprom_crc; >>> @@ -137,11 +119,11 @@ static int read_eeprom(int devnum, u8 *eeprom) >>> // If the contents are invalid, start over with default contents >>> if (!is_valid_tlvinfo_header(eeprom_hdr) || >>> - !is_checksum_valid(eeprom)) { >>> + !tlvinfo_check_crc(eeprom)) { >>> strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); >>> eeprom_hdr->version = TLV_INFO_VERSION; >>> eeprom_hdr->totallen = cpu_to_be16(0); >>> - update_crc(eeprom); >>> + tlvinfo_update_crc(eeprom); >>> } >>> #ifdef DEBUG >>> @@ -183,7 +165,7 @@ static void show_eeprom(int devnum, u8 *eeprom) >>> tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); >>> while (curr_tlv < tlv_end) { >>> eeprom_tlv = to_entry(&eeprom[curr_tlv]); >>> - if (!is_valid_tlv(eeprom_tlv)) { >>> + if (!is_valid_tlvinfo_entry(eeprom_tlv)) { >>> printf("Invalid TLV field starting at EEPROM offset %d\n", >>> curr_tlv); >>> return; >>> @@ -193,7 +175,7 @@ static void show_eeprom(int devnum, u8 *eeprom) >>> } >>> printf("Checksum is %s.\n", >>> - is_checksum_valid(eeprom) ? "valid" : "invalid"); >>> + tlvinfo_check_crc(eeprom) ? "valid" : "invalid"); >>> #ifdef DEBUG >>> printf("EEPROM dump: (0x%x bytes)", TLV_INFO_MAX_LEN); >>> @@ -340,13 +322,13 @@ static void decode_tlv(struct tlvinfo_tlv *tlv) >>> } >>> /** >>> - * update_crc >>> + * tlvinfo_update_crc >>> * >>> * This function updates the CRC-32 TLV. If there is no CRC-32 >>> TLV, then >>> * one is added. This function should be called after each update >>> to the >>> * EEPROM structure, to make sure the CRC is always correct. >>> */ >>> -static void update_crc(u8 *eeprom) >>> +void tlvinfo_update_crc(u8 *eeprom) >>> { >>> struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >>> struct tlvinfo_tlv *eeprom_crc; >>> @@ -376,20 +358,20 @@ static void update_crc(u8 *eeprom) >>> } >>> /** >>> - * prog_eeprom >>> + * write_tlvinfo_tlv_eeprom >>> * >>> - * Write the EEPROM data from CPU memory to the hardware. >>> + * Write the TLV data from CPU memory to the hardware. >>> */ >>> -static int prog_eeprom(int devnum, u8 *eeprom) >>> +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev) >>> { >>> int ret = 0; >>> struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >>> int eeprom_len; >>> - update_crc(eeprom); >>> + tlvinfo_update_crc(eeprom); >>> eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); >>> - ret = write_tlv_eeprom(eeprom, eeprom_len, devnum); >>> + ret = write_tlv_eeprom(eeprom, eeprom_len, dev); >>> if (ret) { >>> printf("Programming failed.\n"); >>> return -1; >>> @@ -462,13 +444,13 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int >>> flag, int argc, char *const argv[]) >>> if (argc == 2) { >>> switch (cmd) { >>> case 'w': /* write */ >>> - prog_eeprom(current_dev, eeprom); >>> + write_tlvinfo_tlv_eeprom(eeprom, current_dev); >>> break; >>> case 'e': /* erase */ >>> strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); >>> eeprom_hdr->version = TLV_INFO_VERSION; >>> eeprom_hdr->totallen = cpu_to_be16(0); >>> - update_crc(eeprom); >>> + tlvinfo_update_crc(eeprom); >>> printf("EEPROM data in memory reset.\n"); >>> break; >>> case 'l': /* list */ >>> @@ -549,7 +531,7 @@ U_BOOT_CMD(tlv_eeprom, 4, 1, do_tlv_eeprom, >>> * An offset from the beginning of the EEPROM is returned in the >>> * eeprom_index parameter if the TLV is found. >>> */ >>> -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) >>> +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) >>> { >>> struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >>> struct tlvinfo_tlv *eeprom_tlv; >>> @@ -561,7 +543,7 @@ static bool tlvinfo_find_tlv(u8 *eeprom, u8 >>> tcode, int *eeprom_index) >>> eeprom_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); >>> while (*eeprom_index < eeprom_end) { >>> eeprom_tlv = to_entry(&eeprom[*eeprom_index]); >>> - if (!is_valid_tlv(eeprom_tlv)) >>> + if (!is_valid_tlvinfo_entry(eeprom_tlv)) >>> return false; >>> if (eeprom_tlv->type == tcode) >>> return true; >>> @@ -594,7 +576,7 @@ static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code) >>> eeprom_hdr->totallen = >>> cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - >>> tlength); >>> - update_crc(eeprom); >>> + tlvinfo_update_crc(eeprom); >>> return true; >>> } >>> return false; >>> @@ -695,7 +677,7 @@ static bool tlvinfo_add_tlv(u8 *eeprom, int >>> tcode, char *strval) >>> // Update the total length and calculate (add) a new CRC-32 TLV >>> eeprom_hdr->totallen = >>> cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) + >>> ENT_SIZE + new_tlv_len); >>> - update_crc(eeprom); >>> + tlvinfo_update_crc(eeprom); >>> return true; >>> } >>> @@ -986,7 +968,7 @@ int read_tlvinfo_tlv_eeprom(void *eeprom, struct >>> tlvinfo_header **hdr, >>> be16_to_cpu(tlv_hdr->totallen), dev_num); >>> if (ret < 0) >>> return ret; >>> - if (!is_checksum_valid(eeprom)) >>> + if (!tlvinfo_check_crc(eeprom)) >>> return -EINVAL; >>> *hdr = tlv_hdr; >>> diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h >>> index fd45e5f6eb..30626a1067 100644 >>> --- a/include/tlv_eeprom.h >>> +++ b/include/tlv_eeprom.h >>> @@ -111,6 +111,51 @@ int write_tlv_eeprom(void *eeprom, int len, int >>> dev); >>> int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, >>> struct tlvinfo_tlv **first_entry, int dev); >>> +/** >>> + * Write TLV data to the EEPROM. >>> + * >>> + * - Only writes length of actual tlv data >>> + * - updates checksum >>> + * >>> + * @eeprom: Pointer to buffer to hold the binary data. Must point to >>> a buffer >>> + * of size at least TLV_INFO_MAX_LEN. >>> + * @dev : EEPROM device to write >>> + * >>> + */ >>> +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev); >>> + >>> +/** >>> + * tlvinfo_find_tlv >>> + * >>> + * This function finds the TLV with the supplied code in the EERPOM. >>> + * An offset from the beginning of the EEPROM is returned in the >>> + * eeprom_index parameter if the TLV is found. >>> + */ >>> +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index); >>> + >>> +/** >>> + * tlvinfo_update_crc >>> + * >>> + * This function updates the CRC-32 TLV. If there is no CRC-32 TLV, >>> then >>> + * one is added. This function should be called after each update >>> to the >>> + * EEPROM structure, to make sure the CRC is always correct. >>> + * >>> + * @eeprom: Pointer to buffer to hold the binary data. Must point to >>> a buffer >>> + * of size at least TLV_INFO_MAX_LEN. >>> + */ >>> +void tlvinfo_update_crc(u8 *eeprom); >>> + >>> +/** >>> + * Validate the checksum in the provided TlvInfo EEPROM data. First, >>> + * verify that the TlvInfo header is valid, then make sure the last >>> + * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data >>> + * and compare it to the value stored in the EEPROM CRC-32 TLV. >>> + * >>> + * @eeprom: Pointer to buffer to hold the binary data. Must point to >>> a buffer >>> + * of size at least TLV_INFO_MAX_LEN. >>> + */ >>> +bool tlvinfo_check_crc(u8 *eeprom); >>> + >>> #else /* !CONFIG_IS_ENABLED(CMD_TLV_EEPROM) */ >>> static inline int read_tlv_eeprom(void *eeprom, int offset, int >>> len, int dev) >>> @@ -150,4 +195,16 @@ static inline bool >>> is_valid_tlvinfo_header(struct tlvinfo_header *hdr) >>> (be16_to_cpu(hdr->totallen) <= TLV_TOTAL_LEN_MAX)); >>> } >>> +/** >>> + * is_valid_tlv >>> + * >>> + * Perform basic sanity checks on a TLV field. The TLV is pointed to >>> + * by the parameter provided. >>> + * 1. The type code is not reserved (0x00 or 0xFF) >>> + */ >>> +static inline bool is_valid_tlvinfo_entry(struct tlvinfo_tlv *tlv) >>> +{ >>> + return((tlv->type != 0x00) && (tlv->type != 0xFF)); >>> +} >>> + >>> #endif /* __TLV_EEPROM_H_ */ >> >> Viele Grüße, >> Stefan Roese >> Viele Grüße, Stefan Roese
\o/ Am 03.05.22 um 13:54 schrieb Stefan Roese: > Hi Josua, > > On 03.05.22 09:17, Josua Mayer wrote: >> Am 03.05.22 um 09:16 schrieb Stefan Roese: >>> On 02.05.22 16:18, Josua Mayer wrote: >>>> - prog_eeprom: write_tlvinfo_tlv_eeprom >>>> - update_crc: tlvinfo_update_crc >>>> - is_valid_tlv: is_valid_tlvinfo_entry >>>> - is_checksum_valid: tlvinfo_check_crc >>> >>> So while creating a new API it makes sense to prepend the function >>> names identical IMHO to not "pollute" the namespace. Something like >>> >>> - tlv_is_valid_entry >>> - tlv_check_crc >>> ... >>> >>> Just examples, you get the idea. >> Yes. The hard part in this particular implementation is that the >> naming is not consistent. >> >> The most sense I could make is that prefix tlvinfo indicates all tlv >> data, i.e. working with the whole structure, while tlvinfo_tlv >> indicates working with one data entry. Further write, read and is_ >> are currently prefixed in the header, but for previously static >> functions in the C file it was put in the middle ... >> >> I found it quite difficult to prepare for splitting off a library in >> a way that preserves history, i.e. diffs should still be readable for >> spotting mistakes. > > Yes, a decent history would be welcome. But still, when going global > here with a new API this should be consistant. My view more like - patches 1-10 are not really new API, in that I am only changing what is necessary to allow splitting the code. >> I was considering to at the very end do a mass-rename and come up >> with better naming, something like >> tlv_{set,get}_{blob,string,mac} >> tlv_find_entry >> tlv_{read,write}_eeprom >> >> But this is pending a refactoring and extension of the tlv parsing >> code in board/solidrun/common/tlv_data.*, to figure out what is >> required or useful. > > So your plan is to this: > a) Get this patchset included > b) Use it in board specific code, e.g. solidrun > c) Do the mass-rename > > Is this correct? This is close. I would say 1) get the split merged 2) rebase board-specific code 2a) figure out what kinds of set/get/add/remove functions are useful 3) redesign api - there are inconsistencies with the order of function arguments - read/write functions imo should use the tlv header to determine size, not a function argument - at least one of the tlv data types can appear multiple times, existing code does not support this 4) submit any renames and extensions to the tlv library 5) submit board-specific use of tlv eeprom data > If yes, why is it better to do the renaming at the end? It is very difficult to work on a patch-set that touches the same code before and after moving it to a different file, which I found myself doing a lot while prototyping this. So if now I went ahead and figured out proper names and purposes for all functions that I think should be the tlv api, then I have to divide that into those parts that are renames or refactoring of existing functionality, and those parts that are strictly new - putting the former before splitting off the library, and the latter to after. I am not sure I can manage this level of complexity. - Josua Mayer > > Thanks, > Stefan > >>> >>> Thanks, >>> Stefan >>> >>>> Signed-off-by: Josua Mayer <josua@solid-run.com> >>>> --- >>>> cmd/tlv_eeprom.c | 56 >>>> +++++++++++++++---------------------------- >>>> include/tlv_eeprom.h | 57 >>>> ++++++++++++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 76 insertions(+), 37 deletions(-) >>>> >>>> diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c >>>> index 00c5b5f840..1b4f2537f6 100644 >>>> --- a/cmd/tlv_eeprom.c >>>> +++ b/cmd/tlv_eeprom.c >>>> @@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR; >>>> #define MAX_TLV_DEVICES 2 >>>> /* File scope function prototypes */ >>>> -static bool is_checksum_valid(u8 *eeprom); >>>> static int read_eeprom(int devnum, u8 *eeprom); >>>> static void show_eeprom(int devnum, u8 *eeprom); >>>> static void decode_tlv(struct tlvinfo_tlv *tlv); >>>> -static void update_crc(u8 *eeprom); >>>> -static int prog_eeprom(int devnum, u8 *eeprom); >>>> -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int >>>> *eeprom_index); >>>> static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code); >>>> static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval); >>>> static int set_mac(char *buf, const char *string); >>>> @@ -58,18 +54,6 @@ static inline bool is_digit(char c) >>>> return (c >= '0' && c <= '9'); >>>> } >>>> -/** >>>> - * is_valid_tlv >>>> - * >>>> - * Perform basic sanity checks on a TLV field. The TLV is pointed to >>>> - * by the parameter provided. >>>> - * 1. The type code is not reserved (0x00 or 0xFF) >>>> - */ >>>> -static inline bool is_valid_tlv(struct tlvinfo_tlv *tlv) >>>> -{ >>>> - return((tlv->type != 0x00) && (tlv->type != 0xFF)); >>>> -} >>>> - >>>> /** >>>> * is_hex >>>> * >>>> @@ -83,14 +67,12 @@ static inline u8 is_hex(char p) >>>> } >>>> /** >>>> - * is_checksum_valid >>>> - * >>>> * Validate the checksum in the provided TlvInfo EEPROM data. >>>> First, >>>> * verify that the TlvInfo header is valid, then make sure the last >>>> * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data >>>> * and compare it to the value stored in the EEPROM CRC-32 TLV. >>>> */ >>>> -static bool is_checksum_valid(u8 *eeprom) >>>> +bool tlvinfo_check_crc(u8 *eeprom) >>>> { >>>> struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >>>> struct tlvinfo_tlv *eeprom_crc; >>>> @@ -137,11 +119,11 @@ static int read_eeprom(int devnum, u8 *eeprom) >>>> // If the contents are invalid, start over with default >>>> contents >>>> if (!is_valid_tlvinfo_header(eeprom_hdr) || >>>> - !is_checksum_valid(eeprom)) { >>>> + !tlvinfo_check_crc(eeprom)) { >>>> strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); >>>> eeprom_hdr->version = TLV_INFO_VERSION; >>>> eeprom_hdr->totallen = cpu_to_be16(0); >>>> - update_crc(eeprom); >>>> + tlvinfo_update_crc(eeprom); >>>> } >>>> #ifdef DEBUG >>>> @@ -183,7 +165,7 @@ static void show_eeprom(int devnum, u8 *eeprom) >>>> tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); >>>> while (curr_tlv < tlv_end) { >>>> eeprom_tlv = to_entry(&eeprom[curr_tlv]); >>>> - if (!is_valid_tlv(eeprom_tlv)) { >>>> + if (!is_valid_tlvinfo_entry(eeprom_tlv)) { >>>> printf("Invalid TLV field starting at EEPROM offset >>>> %d\n", >>>> curr_tlv); >>>> return; >>>> @@ -193,7 +175,7 @@ static void show_eeprom(int devnum, u8 *eeprom) >>>> } >>>> printf("Checksum is %s.\n", >>>> - is_checksum_valid(eeprom) ? "valid" : "invalid"); >>>> + tlvinfo_check_crc(eeprom) ? "valid" : "invalid"); >>>> #ifdef DEBUG >>>> printf("EEPROM dump: (0x%x bytes)", TLV_INFO_MAX_LEN); >>>> @@ -340,13 +322,13 @@ static void decode_tlv(struct tlvinfo_tlv *tlv) >>>> } >>>> /** >>>> - * update_crc >>>> + * tlvinfo_update_crc >>>> * >>>> * This function updates the CRC-32 TLV. If there is no CRC-32 >>>> TLV, then >>>> * one is added. This function should be called after each >>>> update to the >>>> * EEPROM structure, to make sure the CRC is always correct. >>>> */ >>>> -static void update_crc(u8 *eeprom) >>>> +void tlvinfo_update_crc(u8 *eeprom) >>>> { >>>> struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >>>> struct tlvinfo_tlv *eeprom_crc; >>>> @@ -376,20 +358,20 @@ static void update_crc(u8 *eeprom) >>>> } >>>> /** >>>> - * prog_eeprom >>>> + * write_tlvinfo_tlv_eeprom >>>> * >>>> - * Write the EEPROM data from CPU memory to the hardware. >>>> + * Write the TLV data from CPU memory to the hardware. >>>> */ >>>> -static int prog_eeprom(int devnum, u8 *eeprom) >>>> +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev) >>>> { >>>> int ret = 0; >>>> struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >>>> int eeprom_len; >>>> - update_crc(eeprom); >>>> + tlvinfo_update_crc(eeprom); >>>> eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); >>>> - ret = write_tlv_eeprom(eeprom, eeprom_len, devnum); >>>> + ret = write_tlv_eeprom(eeprom, eeprom_len, dev); >>>> if (ret) { >>>> printf("Programming failed.\n"); >>>> return -1; >>>> @@ -462,13 +444,13 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int >>>> flag, int argc, char *const argv[]) >>>> if (argc == 2) { >>>> switch (cmd) { >>>> case 'w': /* write */ >>>> - prog_eeprom(current_dev, eeprom); >>>> + write_tlvinfo_tlv_eeprom(eeprom, current_dev); >>>> break; >>>> case 'e': /* erase */ >>>> strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); >>>> eeprom_hdr->version = TLV_INFO_VERSION; >>>> eeprom_hdr->totallen = cpu_to_be16(0); >>>> - update_crc(eeprom); >>>> + tlvinfo_update_crc(eeprom); >>>> printf("EEPROM data in memory reset.\n"); >>>> break; >>>> case 'l': /* list */ >>>> @@ -549,7 +531,7 @@ U_BOOT_CMD(tlv_eeprom, 4, 1, do_tlv_eeprom, >>>> * An offset from the beginning of the EEPROM is returned in the >>>> * eeprom_index parameter if the TLV is found. >>>> */ >>>> -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) >>>> +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) >>>> { >>>> struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >>>> struct tlvinfo_tlv *eeprom_tlv; >>>> @@ -561,7 +543,7 @@ static bool tlvinfo_find_tlv(u8 *eeprom, u8 >>>> tcode, int *eeprom_index) >>>> eeprom_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); >>>> while (*eeprom_index < eeprom_end) { >>>> eeprom_tlv = to_entry(&eeprom[*eeprom_index]); >>>> - if (!is_valid_tlv(eeprom_tlv)) >>>> + if (!is_valid_tlvinfo_entry(eeprom_tlv)) >>>> return false; >>>> if (eeprom_tlv->type == tcode) >>>> return true; >>>> @@ -594,7 +576,7 @@ static bool tlvinfo_delete_tlv(u8 *eeprom, u8 >>>> code) >>>> eeprom_hdr->totallen = >>>> cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - >>>> tlength); >>>> - update_crc(eeprom); >>>> + tlvinfo_update_crc(eeprom); >>>> return true; >>>> } >>>> return false; >>>> @@ -695,7 +677,7 @@ static bool tlvinfo_add_tlv(u8 *eeprom, int >>>> tcode, char *strval) >>>> // Update the total length and calculate (add) a new CRC-32 TLV >>>> eeprom_hdr->totallen = >>>> cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) + >>>> ENT_SIZE + new_tlv_len); >>>> - update_crc(eeprom); >>>> + tlvinfo_update_crc(eeprom); >>>> return true; >>>> } >>>> @@ -986,7 +968,7 @@ int read_tlvinfo_tlv_eeprom(void *eeprom, >>>> struct tlvinfo_header **hdr, >>>> be16_to_cpu(tlv_hdr->totallen), dev_num); >>>> if (ret < 0) >>>> return ret; >>>> - if (!is_checksum_valid(eeprom)) >>>> + if (!tlvinfo_check_crc(eeprom)) >>>> return -EINVAL; >>>> *hdr = tlv_hdr; >>>> diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h >>>> index fd45e5f6eb..30626a1067 100644 >>>> --- a/include/tlv_eeprom.h >>>> +++ b/include/tlv_eeprom.h >>>> @@ -111,6 +111,51 @@ int write_tlv_eeprom(void *eeprom, int len, >>>> int dev); >>>> int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header >>>> **hdr, >>>> struct tlvinfo_tlv **first_entry, int dev); >>>> +/** >>>> + * Write TLV data to the EEPROM. >>>> + * >>>> + * - Only writes length of actual tlv data >>>> + * - updates checksum >>>> + * >>>> + * @eeprom: Pointer to buffer to hold the binary data. Must point >>>> to a buffer >>>> + * of size at least TLV_INFO_MAX_LEN. >>>> + * @dev : EEPROM device to write >>>> + * >>>> + */ >>>> +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev); >>>> + >>>> +/** >>>> + * tlvinfo_find_tlv >>>> + * >>>> + * This function finds the TLV with the supplied code in the EERPOM. >>>> + * An offset from the beginning of the EEPROM is returned in the >>>> + * eeprom_index parameter if the TLV is found. >>>> + */ >>>> +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index); >>>> + >>>> +/** >>>> + * tlvinfo_update_crc >>>> + * >>>> + * This function updates the CRC-32 TLV. If there is no CRC-32 >>>> TLV, then >>>> + * one is added. This function should be called after each update >>>> to the >>>> + * EEPROM structure, to make sure the CRC is always correct. >>>> + * >>>> + * @eeprom: Pointer to buffer to hold the binary data. Must point >>>> to a buffer >>>> + * of size at least TLV_INFO_MAX_LEN. >>>> + */ >>>> +void tlvinfo_update_crc(u8 *eeprom); >>>> + >>>> +/** >>>> + * Validate the checksum in the provided TlvInfo EEPROM data. First, >>>> + * verify that the TlvInfo header is valid, then make sure the last >>>> + * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data >>>> + * and compare it to the value stored in the EEPROM CRC-32 TLV. >>>> + * >>>> + * @eeprom: Pointer to buffer to hold the binary data. Must point >>>> to a buffer >>>> + * of size at least TLV_INFO_MAX_LEN. >>>> + */ >>>> +bool tlvinfo_check_crc(u8 *eeprom); >>>> + >>>> #else /* !CONFIG_IS_ENABLED(CMD_TLV_EEPROM) */ >>>> static inline int read_tlv_eeprom(void *eeprom, int offset, int >>>> len, int dev) >>>> @@ -150,4 +195,16 @@ static inline bool >>>> is_valid_tlvinfo_header(struct tlvinfo_header *hdr) >>>> (be16_to_cpu(hdr->totallen) <= TLV_TOTAL_LEN_MAX)); >>>> } >>>> +/** >>>> + * is_valid_tlv >>>> + * >>>> + * Perform basic sanity checks on a TLV field. The TLV is pointed to >>>> + * by the parameter provided. >>>> + * 1. The type code is not reserved (0x00 or 0xFF) >>>> + */ >>>> +static inline bool is_valid_tlvinfo_entry(struct tlvinfo_tlv *tlv) >>>> +{ >>>> + return((tlv->type != 0x00) && (tlv->type != 0xFF)); >>>> +} >>>> + >>>> #endif /* __TLV_EEPROM_H_ */ >>> >>> Viele Grüße, >>> Stefan Roese >>> > > Viele Grüße, > Stefan Roese >
Hi Josua, On 03.05.22 21:09, Josua Mayer wrote: > \o/ > > Am 03.05.22 um 13:54 schrieb Stefan Roese: >> Hi Josua, >> >> On 03.05.22 09:17, Josua Mayer wrote: >>> Am 03.05.22 um 09:16 schrieb Stefan Roese: >>>> On 02.05.22 16:18, Josua Mayer wrote: >>>>> - prog_eeprom: write_tlvinfo_tlv_eeprom >>>>> - update_crc: tlvinfo_update_crc >>>>> - is_valid_tlv: is_valid_tlvinfo_entry >>>>> - is_checksum_valid: tlvinfo_check_crc >>>> >>>> So while creating a new API it makes sense to prepend the function >>>> names identical IMHO to not "pollute" the namespace. Something like >>>> >>>> - tlv_is_valid_entry >>>> - tlv_check_crc >>>> ... >>>> >>>> Just examples, you get the idea. >>> Yes. The hard part in this particular implementation is that the >>> naming is not consistent. >>> >>> The most sense I could make is that prefix tlvinfo indicates all tlv >>> data, i.e. working with the whole structure, while tlvinfo_tlv >>> indicates working with one data entry. Further write, read and is_ >>> are currently prefixed in the header, but for previously static >>> functions in the C file it was put in the middle ... >>> >>> I found it quite difficult to prepare for splitting off a library in >>> a way that preserves history, i.e. diffs should still be readable for >>> spotting mistakes. >> >> Yes, a decent history would be welcome. But still, when going global >> here with a new API this should be consistant. > My view more like - patches 1-10 are not really new API, in that I am > only changing what is necessary to allow splitting the code. >>> I was considering to at the very end do a mass-rename and come up >>> with better naming, something like >>> tlv_{set,get}_{blob,string,mac} >>> tlv_find_entry >>> tlv_{read,write}_eeprom >>> >>> But this is pending a refactoring and extension of the tlv parsing >>> code in board/solidrun/common/tlv_data.*, to figure out what is >>> required or useful. >> >> So your plan is to this: >> a) Get this patchset included >> b) Use it in board specific code, e.g. solidrun >> c) Do the mass-rename >> >> Is this correct? > This is close. I would say > 1) get the split merged > 2) rebase board-specific code > 2a) figure out what kinds of set/get/add/remove functions are useful > 3) redesign api > - there are inconsistencies with the order of function arguments > - read/write functions imo should use the tlv header to determine > size, not a function argument > - at least one of the tlv data types can appear multiple times, > existing code does not support this > 4) submit any renames and extensions to the tlv library > 5) submit board-specific use of tlv eeprom data >> If yes, why is it better to do the renaming at the end? > It is very difficult to work on a patch-set that touches the same code > before and after moving it to a different file, which I found myself > doing a lot while prototyping this. > So if now I went ahead and figured out proper names and purposes for all > functions that I think should be the tlv api, then I have to divide that > into those parts that are renames or refactoring of existing > functionality, and those parts that are strictly new - putting the > former before splitting off the library, and the latter to after. > > I am not sure I can manage this level of complexity. To cut it short, if you plan to rename the API to some consitant naming in the end, then I am okay with moving forward without a renaming at this early stage. Thanks, Stefan > > - Josua Mayer > >> >> Thanks, >> Stefan >> >>>> >>>> Thanks, >>>> Stefan >>>> >>>>> Signed-off-by: Josua Mayer <josua@solid-run.com> >>>>> --- >>>>> cmd/tlv_eeprom.c | 56 >>>>> +++++++++++++++---------------------------- >>>>> include/tlv_eeprom.h | 57 >>>>> ++++++++++++++++++++++++++++++++++++++++++++ >>>>> 2 files changed, 76 insertions(+), 37 deletions(-) >>>>> >>>>> diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c >>>>> index 00c5b5f840..1b4f2537f6 100644 >>>>> --- a/cmd/tlv_eeprom.c >>>>> +++ b/cmd/tlv_eeprom.c >>>>> @@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR; >>>>> #define MAX_TLV_DEVICES 2 >>>>> /* File scope function prototypes */ >>>>> -static bool is_checksum_valid(u8 *eeprom); >>>>> static int read_eeprom(int devnum, u8 *eeprom); >>>>> static void show_eeprom(int devnum, u8 *eeprom); >>>>> static void decode_tlv(struct tlvinfo_tlv *tlv); >>>>> -static void update_crc(u8 *eeprom); >>>>> -static int prog_eeprom(int devnum, u8 *eeprom); >>>>> -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int >>>>> *eeprom_index); >>>>> static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code); >>>>> static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval); >>>>> static int set_mac(char *buf, const char *string); >>>>> @@ -58,18 +54,6 @@ static inline bool is_digit(char c) >>>>> return (c >= '0' && c <= '9'); >>>>> } >>>>> -/** >>>>> - * is_valid_tlv >>>>> - * >>>>> - * Perform basic sanity checks on a TLV field. The TLV is pointed to >>>>> - * by the parameter provided. >>>>> - * 1. The type code is not reserved (0x00 or 0xFF) >>>>> - */ >>>>> -static inline bool is_valid_tlv(struct tlvinfo_tlv *tlv) >>>>> -{ >>>>> - return((tlv->type != 0x00) && (tlv->type != 0xFF)); >>>>> -} >>>>> - >>>>> /** >>>>> * is_hex >>>>> * >>>>> @@ -83,14 +67,12 @@ static inline u8 is_hex(char p) >>>>> } >>>>> /** >>>>> - * is_checksum_valid >>>>> - * >>>>> * Validate the checksum in the provided TlvInfo EEPROM data. >>>>> First, >>>>> * verify that the TlvInfo header is valid, then make sure the last >>>>> * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data >>>>> * and compare it to the value stored in the EEPROM CRC-32 TLV. >>>>> */ >>>>> -static bool is_checksum_valid(u8 *eeprom) >>>>> +bool tlvinfo_check_crc(u8 *eeprom) >>>>> { >>>>> struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >>>>> struct tlvinfo_tlv *eeprom_crc; >>>>> @@ -137,11 +119,11 @@ static int read_eeprom(int devnum, u8 *eeprom) >>>>> // If the contents are invalid, start over with default >>>>> contents >>>>> if (!is_valid_tlvinfo_header(eeprom_hdr) || >>>>> - !is_checksum_valid(eeprom)) { >>>>> + !tlvinfo_check_crc(eeprom)) { >>>>> strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); >>>>> eeprom_hdr->version = TLV_INFO_VERSION; >>>>> eeprom_hdr->totallen = cpu_to_be16(0); >>>>> - update_crc(eeprom); >>>>> + tlvinfo_update_crc(eeprom); >>>>> } >>>>> #ifdef DEBUG >>>>> @@ -183,7 +165,7 @@ static void show_eeprom(int devnum, u8 *eeprom) >>>>> tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); >>>>> while (curr_tlv < tlv_end) { >>>>> eeprom_tlv = to_entry(&eeprom[curr_tlv]); >>>>> - if (!is_valid_tlv(eeprom_tlv)) { >>>>> + if (!is_valid_tlvinfo_entry(eeprom_tlv)) { >>>>> printf("Invalid TLV field starting at EEPROM offset >>>>> %d\n", >>>>> curr_tlv); >>>>> return; >>>>> @@ -193,7 +175,7 @@ static void show_eeprom(int devnum, u8 *eeprom) >>>>> } >>>>> printf("Checksum is %s.\n", >>>>> - is_checksum_valid(eeprom) ? "valid" : "invalid"); >>>>> + tlvinfo_check_crc(eeprom) ? "valid" : "invalid"); >>>>> #ifdef DEBUG >>>>> printf("EEPROM dump: (0x%x bytes)", TLV_INFO_MAX_LEN); >>>>> @@ -340,13 +322,13 @@ static void decode_tlv(struct tlvinfo_tlv *tlv) >>>>> } >>>>> /** >>>>> - * update_crc >>>>> + * tlvinfo_update_crc >>>>> * >>>>> * This function updates the CRC-32 TLV. If there is no CRC-32 >>>>> TLV, then >>>>> * one is added. This function should be called after each >>>>> update to the >>>>> * EEPROM structure, to make sure the CRC is always correct. >>>>> */ >>>>> -static void update_crc(u8 *eeprom) >>>>> +void tlvinfo_update_crc(u8 *eeprom) >>>>> { >>>>> struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >>>>> struct tlvinfo_tlv *eeprom_crc; >>>>> @@ -376,20 +358,20 @@ static void update_crc(u8 *eeprom) >>>>> } >>>>> /** >>>>> - * prog_eeprom >>>>> + * write_tlvinfo_tlv_eeprom >>>>> * >>>>> - * Write the EEPROM data from CPU memory to the hardware. >>>>> + * Write the TLV data from CPU memory to the hardware. >>>>> */ >>>>> -static int prog_eeprom(int devnum, u8 *eeprom) >>>>> +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev) >>>>> { >>>>> int ret = 0; >>>>> struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >>>>> int eeprom_len; >>>>> - update_crc(eeprom); >>>>> + tlvinfo_update_crc(eeprom); >>>>> eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); >>>>> - ret = write_tlv_eeprom(eeprom, eeprom_len, devnum); >>>>> + ret = write_tlv_eeprom(eeprom, eeprom_len, dev); >>>>> if (ret) { >>>>> printf("Programming failed.\n"); >>>>> return -1; >>>>> @@ -462,13 +444,13 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int >>>>> flag, int argc, char *const argv[]) >>>>> if (argc == 2) { >>>>> switch (cmd) { >>>>> case 'w': /* write */ >>>>> - prog_eeprom(current_dev, eeprom); >>>>> + write_tlvinfo_tlv_eeprom(eeprom, current_dev); >>>>> break; >>>>> case 'e': /* erase */ >>>>> strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); >>>>> eeprom_hdr->version = TLV_INFO_VERSION; >>>>> eeprom_hdr->totallen = cpu_to_be16(0); >>>>> - update_crc(eeprom); >>>>> + tlvinfo_update_crc(eeprom); >>>>> printf("EEPROM data in memory reset.\n"); >>>>> break; >>>>> case 'l': /* list */ >>>>> @@ -549,7 +531,7 @@ U_BOOT_CMD(tlv_eeprom, 4, 1, do_tlv_eeprom, >>>>> * An offset from the beginning of the EEPROM is returned in the >>>>> * eeprom_index parameter if the TLV is found. >>>>> */ >>>>> -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) >>>>> +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) >>>>> { >>>>> struct tlvinfo_header *eeprom_hdr = to_header(eeprom); >>>>> struct tlvinfo_tlv *eeprom_tlv; >>>>> @@ -561,7 +543,7 @@ static bool tlvinfo_find_tlv(u8 *eeprom, u8 >>>>> tcode, int *eeprom_index) >>>>> eeprom_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); >>>>> while (*eeprom_index < eeprom_end) { >>>>> eeprom_tlv = to_entry(&eeprom[*eeprom_index]); >>>>> - if (!is_valid_tlv(eeprom_tlv)) >>>>> + if (!is_valid_tlvinfo_entry(eeprom_tlv)) >>>>> return false; >>>>> if (eeprom_tlv->type == tcode) >>>>> return true; >>>>> @@ -594,7 +576,7 @@ static bool tlvinfo_delete_tlv(u8 *eeprom, u8 >>>>> code) >>>>> eeprom_hdr->totallen = >>>>> cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - >>>>> tlength); >>>>> - update_crc(eeprom); >>>>> + tlvinfo_update_crc(eeprom); >>>>> return true; >>>>> } >>>>> return false; >>>>> @@ -695,7 +677,7 @@ static bool tlvinfo_add_tlv(u8 *eeprom, int >>>>> tcode, char *strval) >>>>> // Update the total length and calculate (add) a new CRC-32 TLV >>>>> eeprom_hdr->totallen = >>>>> cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) + >>>>> ENT_SIZE + new_tlv_len); >>>>> - update_crc(eeprom); >>>>> + tlvinfo_update_crc(eeprom); >>>>> return true; >>>>> } >>>>> @@ -986,7 +968,7 @@ int read_tlvinfo_tlv_eeprom(void *eeprom, >>>>> struct tlvinfo_header **hdr, >>>>> be16_to_cpu(tlv_hdr->totallen), dev_num); >>>>> if (ret < 0) >>>>> return ret; >>>>> - if (!is_checksum_valid(eeprom)) >>>>> + if (!tlvinfo_check_crc(eeprom)) >>>>> return -EINVAL; >>>>> *hdr = tlv_hdr; >>>>> diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h >>>>> index fd45e5f6eb..30626a1067 100644 >>>>> --- a/include/tlv_eeprom.h >>>>> +++ b/include/tlv_eeprom.h >>>>> @@ -111,6 +111,51 @@ int write_tlv_eeprom(void *eeprom, int len, >>>>> int dev); >>>>> int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header >>>>> **hdr, >>>>> struct tlvinfo_tlv **first_entry, int dev); >>>>> +/** >>>>> + * Write TLV data to the EEPROM. >>>>> + * >>>>> + * - Only writes length of actual tlv data >>>>> + * - updates checksum >>>>> + * >>>>> + * @eeprom: Pointer to buffer to hold the binary data. Must point >>>>> to a buffer >>>>> + * of size at least TLV_INFO_MAX_LEN. >>>>> + * @dev : EEPROM device to write >>>>> + * >>>>> + */ >>>>> +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev); >>>>> + >>>>> +/** >>>>> + * tlvinfo_find_tlv >>>>> + * >>>>> + * This function finds the TLV with the supplied code in the EERPOM. >>>>> + * An offset from the beginning of the EEPROM is returned in the >>>>> + * eeprom_index parameter if the TLV is found. >>>>> + */ >>>>> +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index); >>>>> + >>>>> +/** >>>>> + * tlvinfo_update_crc >>>>> + * >>>>> + * This function updates the CRC-32 TLV. If there is no CRC-32 >>>>> TLV, then >>>>> + * one is added. This function should be called after each update >>>>> to the >>>>> + * EEPROM structure, to make sure the CRC is always correct. >>>>> + * >>>>> + * @eeprom: Pointer to buffer to hold the binary data. Must point >>>>> to a buffer >>>>> + * of size at least TLV_INFO_MAX_LEN. >>>>> + */ >>>>> +void tlvinfo_update_crc(u8 *eeprom); >>>>> + >>>>> +/** >>>>> + * Validate the checksum in the provided TlvInfo EEPROM data. First, >>>>> + * verify that the TlvInfo header is valid, then make sure the last >>>>> + * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data >>>>> + * and compare it to the value stored in the EEPROM CRC-32 TLV. >>>>> + * >>>>> + * @eeprom: Pointer to buffer to hold the binary data. Must point >>>>> to a buffer >>>>> + * of size at least TLV_INFO_MAX_LEN. >>>>> + */ >>>>> +bool tlvinfo_check_crc(u8 *eeprom); >>>>> + >>>>> #else /* !CONFIG_IS_ENABLED(CMD_TLV_EEPROM) */ >>>>> static inline int read_tlv_eeprom(void *eeprom, int offset, int >>>>> len, int dev) >>>>> @@ -150,4 +195,16 @@ static inline bool >>>>> is_valid_tlvinfo_header(struct tlvinfo_header *hdr) >>>>> (be16_to_cpu(hdr->totallen) <= TLV_TOTAL_LEN_MAX)); >>>>> } >>>>> +/** >>>>> + * is_valid_tlv >>>>> + * >>>>> + * Perform basic sanity checks on a TLV field. The TLV is pointed to >>>>> + * by the parameter provided. >>>>> + * 1. The type code is not reserved (0x00 or 0xFF) >>>>> + */ >>>>> +static inline bool is_valid_tlvinfo_entry(struct tlvinfo_tlv *tlv) >>>>> +{ >>>>> + return((tlv->type != 0x00) && (tlv->type != 0xFF)); >>>>> +} >>>>> + >>>>> #endif /* __TLV_EEPROM_H_ */ >>>> >>>> Viele Grüße, >>>> Stefan Roese >>>> >> >> Viele Grüße, >> Stefan Roese >> Viele Grüße, Stefan Roese
diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index 00c5b5f840..1b4f2537f6 100644 --- a/cmd/tlv_eeprom.c +++ b/cmd/tlv_eeprom.c @@ -28,13 +28,9 @@ DECLARE_GLOBAL_DATA_PTR; #define MAX_TLV_DEVICES 2 /* File scope function prototypes */ -static bool is_checksum_valid(u8 *eeprom); static int read_eeprom(int devnum, u8 *eeprom); static void show_eeprom(int devnum, u8 *eeprom); static void decode_tlv(struct tlvinfo_tlv *tlv); -static void update_crc(u8 *eeprom); -static int prog_eeprom(int devnum, u8 *eeprom); -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index); static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code); static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval); static int set_mac(char *buf, const char *string); @@ -58,18 +54,6 @@ static inline bool is_digit(char c) return (c >= '0' && c <= '9'); } -/** - * is_valid_tlv - * - * Perform basic sanity checks on a TLV field. The TLV is pointed to - * by the parameter provided. - * 1. The type code is not reserved (0x00 or 0xFF) - */ -static inline bool is_valid_tlv(struct tlvinfo_tlv *tlv) -{ - return((tlv->type != 0x00) && (tlv->type != 0xFF)); -} - /** * is_hex * @@ -83,14 +67,12 @@ static inline u8 is_hex(char p) } /** - * is_checksum_valid - * * Validate the checksum in the provided TlvInfo EEPROM data. First, * verify that the TlvInfo header is valid, then make sure the last * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data * and compare it to the value stored in the EEPROM CRC-32 TLV. */ -static bool is_checksum_valid(u8 *eeprom) +bool tlvinfo_check_crc(u8 *eeprom) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_crc; @@ -137,11 +119,11 @@ static int read_eeprom(int devnum, u8 *eeprom) // If the contents are invalid, start over with default contents if (!is_valid_tlvinfo_header(eeprom_hdr) || - !is_checksum_valid(eeprom)) { + !tlvinfo_check_crc(eeprom)) { strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); eeprom_hdr->version = TLV_INFO_VERSION; eeprom_hdr->totallen = cpu_to_be16(0); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); } #ifdef DEBUG @@ -183,7 +165,7 @@ static void show_eeprom(int devnum, u8 *eeprom) tlv_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); while (curr_tlv < tlv_end) { eeprom_tlv = to_entry(&eeprom[curr_tlv]); - if (!is_valid_tlv(eeprom_tlv)) { + if (!is_valid_tlvinfo_entry(eeprom_tlv)) { printf("Invalid TLV field starting at EEPROM offset %d\n", curr_tlv); return; @@ -193,7 +175,7 @@ static void show_eeprom(int devnum, u8 *eeprom) } printf("Checksum is %s.\n", - is_checksum_valid(eeprom) ? "valid" : "invalid"); + tlvinfo_check_crc(eeprom) ? "valid" : "invalid"); #ifdef DEBUG printf("EEPROM dump: (0x%x bytes)", TLV_INFO_MAX_LEN); @@ -340,13 +322,13 @@ static void decode_tlv(struct tlvinfo_tlv *tlv) } /** - * update_crc + * tlvinfo_update_crc * * This function updates the CRC-32 TLV. If there is no CRC-32 TLV, then * one is added. This function should be called after each update to the * EEPROM structure, to make sure the CRC is always correct. */ -static void update_crc(u8 *eeprom) +void tlvinfo_update_crc(u8 *eeprom) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_crc; @@ -376,20 +358,20 @@ static void update_crc(u8 *eeprom) } /** - * prog_eeprom + * write_tlvinfo_tlv_eeprom * - * Write the EEPROM data from CPU memory to the hardware. + * Write the TLV data from CPU memory to the hardware. */ -static int prog_eeprom(int devnum, u8 *eeprom) +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev) { int ret = 0; struct tlvinfo_header *eeprom_hdr = to_header(eeprom); int eeprom_len; - update_crc(eeprom); + tlvinfo_update_crc(eeprom); eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); - ret = write_tlv_eeprom(eeprom, eeprom_len, devnum); + ret = write_tlv_eeprom(eeprom, eeprom_len, dev); if (ret) { printf("Programming failed.\n"); return -1; @@ -462,13 +444,13 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) if (argc == 2) { switch (cmd) { case 'w': /* write */ - prog_eeprom(current_dev, eeprom); + write_tlvinfo_tlv_eeprom(eeprom, current_dev); break; case 'e': /* erase */ strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING); eeprom_hdr->version = TLV_INFO_VERSION; eeprom_hdr->totallen = cpu_to_be16(0); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); printf("EEPROM data in memory reset.\n"); break; case 'l': /* list */ @@ -549,7 +531,7 @@ U_BOOT_CMD(tlv_eeprom, 4, 1, do_tlv_eeprom, * An offset from the beginning of the EEPROM is returned in the * eeprom_index parameter if the TLV is found. */ -static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) { struct tlvinfo_header *eeprom_hdr = to_header(eeprom); struct tlvinfo_tlv *eeprom_tlv; @@ -561,7 +543,7 @@ static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index) eeprom_end = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen); while (*eeprom_index < eeprom_end) { eeprom_tlv = to_entry(&eeprom[*eeprom_index]); - if (!is_valid_tlv(eeprom_tlv)) + if (!is_valid_tlvinfo_entry(eeprom_tlv)) return false; if (eeprom_tlv->type == tcode) return true; @@ -594,7 +576,7 @@ static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code) eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) - tlength); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); return true; } return false; @@ -695,7 +677,7 @@ static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval) // Update the total length and calculate (add) a new CRC-32 TLV eeprom_hdr->totallen = cpu_to_be16(be16_to_cpu(eeprom_hdr->totallen) + ENT_SIZE + new_tlv_len); - update_crc(eeprom); + tlvinfo_update_crc(eeprom); return true; } @@ -986,7 +968,7 @@ int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, be16_to_cpu(tlv_hdr->totallen), dev_num); if (ret < 0) return ret; - if (!is_checksum_valid(eeprom)) + if (!tlvinfo_check_crc(eeprom)) return -EINVAL; *hdr = tlv_hdr; diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h index fd45e5f6eb..30626a1067 100644 --- a/include/tlv_eeprom.h +++ b/include/tlv_eeprom.h @@ -111,6 +111,51 @@ int write_tlv_eeprom(void *eeprom, int len, int dev); int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr, struct tlvinfo_tlv **first_entry, int dev); +/** + * Write TLV data to the EEPROM. + * + * - Only writes length of actual tlv data + * - updates checksum + * + * @eeprom: Pointer to buffer to hold the binary data. Must point to a buffer + * of size at least TLV_INFO_MAX_LEN. + * @dev : EEPROM device to write + * + */ +int write_tlvinfo_tlv_eeprom(void *eeprom, int dev); + +/** + * tlvinfo_find_tlv + * + * This function finds the TLV with the supplied code in the EERPOM. + * An offset from the beginning of the EEPROM is returned in the + * eeprom_index parameter if the TLV is found. + */ +bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index); + +/** + * tlvinfo_update_crc + * + * This function updates the CRC-32 TLV. If there is no CRC-32 TLV, then + * one is added. This function should be called after each update to the + * EEPROM structure, to make sure the CRC is always correct. + * + * @eeprom: Pointer to buffer to hold the binary data. Must point to a buffer + * of size at least TLV_INFO_MAX_LEN. + */ +void tlvinfo_update_crc(u8 *eeprom); + +/** + * Validate the checksum in the provided TlvInfo EEPROM data. First, + * verify that the TlvInfo header is valid, then make sure the last + * TLV is a CRC-32 TLV. Then calculate the CRC over the EEPROM data + * and compare it to the value stored in the EEPROM CRC-32 TLV. + * + * @eeprom: Pointer to buffer to hold the binary data. Must point to a buffer + * of size at least TLV_INFO_MAX_LEN. + */ +bool tlvinfo_check_crc(u8 *eeprom); + #else /* !CONFIG_IS_ENABLED(CMD_TLV_EEPROM) */ static inline int read_tlv_eeprom(void *eeprom, int offset, int len, int dev) @@ -150,4 +195,16 @@ static inline bool is_valid_tlvinfo_header(struct tlvinfo_header *hdr) (be16_to_cpu(hdr->totallen) <= TLV_TOTAL_LEN_MAX)); } +/** + * is_valid_tlv + * + * Perform basic sanity checks on a TLV field. The TLV is pointed to + * by the parameter provided. + * 1. The type code is not reserved (0x00 or 0xFF) + */ +static inline bool is_valid_tlvinfo_entry(struct tlvinfo_tlv *tlv) +{ + return((tlv->type != 0x00) && (tlv->type != 0xFF)); +} + #endif /* __TLV_EEPROM_H_ */
- prog_eeprom: write_tlvinfo_tlv_eeprom - update_crc: tlvinfo_update_crc - is_valid_tlv: is_valid_tlvinfo_entry - is_checksum_valid: tlvinfo_check_crc Signed-off-by: Josua Mayer <josua@solid-run.com> --- cmd/tlv_eeprom.c | 56 +++++++++++++++---------------------------- include/tlv_eeprom.h | 57 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 37 deletions(-)