Re: [PATCH 04/12] cmd: tlv_eeprom: convert functions used by command to api functions

[Josua Mayer <josua@solid-run.com>]

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
>