From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 94B5FC433EF for ; Tue, 3 May 2022 07:18:04 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C4CEB83D71; Tue, 3 May 2022 09:18:01 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=solid-run.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=solid-run-com.20210112.gappssmtp.com header.i=@solid-run-com.20210112.gappssmtp.com header.b="QviCe8Xn"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id BDD7983D71; Tue, 3 May 2022 09:17:59 +0200 (CEST) Received: from mail-wm1-x32a.google.com (mail-wm1-x32a.google.com [IPv6:2a00:1450:4864:20::32a]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 5599983D07 for ; Tue, 3 May 2022 09:17:53 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=solid-run.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=josua@solid-run.com Received: by mail-wm1-x32a.google.com with SMTP id 129so9388112wmz.0 for ; Tue, 03 May 2022 00:17:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=solid-run-com.20210112.gappssmtp.com; s=20210112; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to; bh=/gbdflXq4GSZ7yO34QpivjW1oDu8wRyjN2bF67+Z/9Y=; b=QviCe8XnGB1lxQBiJVQiy6zLLTrUbnH3zAJL6nNgRO/cK+ZFXOyFFF79Zgwr5HrWvV rlKIVIfuxJQ10Cs1Nu7MJsO/EWYXA9DgPO7Jr7mI9ztReLJi0+EELC//Vt2smduAitq8 bsyzic8ydo1zVvOPz4Xv1aqsAZZ++F+mOPY088Kb6020PsF5+2D1N8uDmF7ob4vzQozi 5tXRA0iaZi7n/FcKABdEMblWnUF3VzTBe+2mmqL+OJzJ9S2B409unyjvvnD04dicuj41 0DvimRYLj6AIdwSnHR4zItX7gpG3jQKe6djaFQXiA3XaqNQIp7Tspv4G8aFOfOMJQEji weoQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to; bh=/gbdflXq4GSZ7yO34QpivjW1oDu8wRyjN2bF67+Z/9Y=; b=Fh4RVFTezjSHz1SUL6v3d2TzADzm2/n7lELGzO2Yvn4UFcfqNa73w/gFtyI4KVloCU jKwm2uyAMzHNz8ESchOzP7k+ucCrjBwgYkC3VyUjusINKygPIE4ytMQrFsLKt1gopxAE 7Bg6DsgcSXSpLKnlXrpXvElmGGzZUAf2rP+uqp+mSJjzzGmFtx74ol60Ij/oqOJPWzaK 28zITXjOvwur/hDNyGoiPEbs46Y5FPQ7TgS7xF4CmLpfwsHutRWUOLf5DJ5TiJrvanAR g1h3uO7JNSdVOTEegme4QeGcbCsOkQpvuYnZJ946gw4JyGhYrClqgfsVPYYrQu86U2Zm wAyA== X-Gm-Message-State: AOAM531OGe1kO3ATuCWc7wg5OQdLN9o4ovbr0DLa+Fiql7NRVVYTnEdp 0pzcDkXzKAUoJI88xCC4Tc1AdQ== X-Google-Smtp-Source: ABdhPJyY4J80v6rdodi8yOlNcuZFGFiuhwj6c1NkmbWfkDsai7mrpx+KAXDHe91IQOW4yYDrDUxbdQ== X-Received: by 2002:a1c:29c3:0:b0:350:9797:b38f with SMTP id p186-20020a1c29c3000000b003509797b38fmr2180325wmp.22.1651562272762; Tue, 03 May 2022 00:17:52 -0700 (PDT) Received: from [10.149.0.162] ([194.169.217.220]) by smtp.gmail.com with ESMTPSA id g11-20020adfa48b000000b0020c5253d8e3sm8417128wrb.47.2022.05.03.00.17.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 03 May 2022 00:17:52 -0700 (PDT) Message-ID: <81937bc9-a1d1-82e5-a7f6-e01780591bed@solid-run.com> Date: Tue, 3 May 2022 10:17:50 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [PATCH 04/12] cmd: tlv_eeprom: convert functions used by command to api functions Content-Language: en-US To: Stefan Roese , u-boot@lists.denx.de Cc: Sven Auhagen , Simon Glass References: <20220502141838.15912-1-josua@solid-run.com> <20220502141838.15912-5-josua@solid-run.com> <063890fa-b1d7-30fd-837c-4bcd6e8d2071@denx.de> From: Josua Mayer In-Reply-To: <063890fa-b1d7-30fd-837c-4bcd6e8d2071@denx.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.39 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean 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 >> --- >>   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 >