[PATCH 04/12] cmd: tlv_eeprom: convert functions used by command to api functions
Stefan Roese
sr at denx.de
Tue May 3 12:54:55 CEST 2022
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 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
--
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