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

[Josua Mayer]

\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 <jo...@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