[PATCH 04/12] cmd: tlv_eeprom: convert functions used by command to api functions
Josua Mayer
josua at solid-run.com
Tue May 3 21:09:38 CEST 2022
\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 at 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
>
More information about the U-Boot
mailing list