[PATCH 04/12] cmd: tlv_eeprom: convert functions used by command to api functions
Stefan Roese
sr at denx.de
Thu May 5 07:09:22 CEST 2022
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 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
>>
Viele Grüße,
Stefan Roese
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de
More information about the U-Boot
mailing list