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 F1ABAC433EF for ; Tue, 3 May 2022 19:10:17 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 655DE83C7B; Tue, 3 May 2022 21:10:15 +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=pass (2048-bit key; unprotected) header.d=solid-run-com.20210112.gappssmtp.com header.i=@solid-run-com.20210112.gappssmtp.com header.b="n4Mhym1+"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 6A3A5802EF; Tue, 3 May 2022 21:10:12 +0200 (CEST) Received: from mail-wr1-x434.google.com (mail-wr1-x434.google.com [IPv6:2a00:1450:4864:20::434]) (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 6E477802EF for ; Tue, 3 May 2022 21:10:08 +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-wr1-x434.google.com with SMTP id j15so24657031wrb.2 for ; Tue, 03 May 2022 12:10:08 -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:content-transfer-encoding; bh=goG5wZJGQmCNx1Omo5PZ5akH6xOFU4Gs0hUUatS3ZDI=; b=n4Mhym1+t7yq6BO0sTjkP4OzkRt/LLzYsewyZsEddTO9WRawF2wF4i98JvV6rVW+vH Xa95iAsht8DSUGZ8fXjZRvlEBKM0Xu5nv1MNdc9Sl06UsECQHn1WiQG+LL6dNDkAWZFY D6M6SSOOStHzRs5fEfnbYTCtiV6vNN0cE2DUEDGit+BVNmV9u5+J4kRjIxFrx90nAAZo V/BYJV61BT0Pwjb2rFD4ku57aNSYoaqC5M9HKakciM9XNFCAjcnFq3mwVPRW30LQExF2 ECE4BPaSKwJPtz+HQNGv9SVscc/To98ZdtMcKULsCbPFtETB5ujv02DWzkXRAB+x4fMC 8Zlg== 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 :content-transfer-encoding; bh=goG5wZJGQmCNx1Omo5PZ5akH6xOFU4Gs0hUUatS3ZDI=; b=nhxfKT9kKDhf6E2lzMWigJRfLVe3y7wOKwj3R9MTAT+3duznM20KXfOxUezrjTx4Ch y+jq/JKwffVGdUNPzoF0/dQdIvrr/UIQDtxRXtZ7SL351TROyIqkTyBoEDX3zsHlPwjC R7X7izwOdJz9ryib/v/v9mJTiRsufG6jODh++Jy6abFrl0TjWxzIC4a7Qi2bgo+4EEfR iuH1vBtRRnS3A50sRew8BR+e0DOfJ+fodcwuhI1Wgpo3o0w7GylRxcwKw1v+0OPz9yqv LpQtAbohsrefO1pZoh3mPbbTF/ERhuEf51h+cqnrtRJ63krLO7nN6Ij4/g3o54X321WM FCxw== X-Gm-Message-State: AOAM532LkYfxSQTwD5xXENkAd4WmjpIvs7ffUEDkoEiOtFqVGgeaX3Im wOQ7d0QIYnoWDuKTVpRAUvpNQg== X-Google-Smtp-Source: ABdhPJyJO9bEzO7K0abZlj/AdVftgCVUaWv479rjwDR2AUsrXnOwGgwCXvP6XwQv2KgQsB09+TwTAQ== X-Received: by 2002:a05:6000:1569:b0:20c:4ed4:4ba8 with SMTP id 9-20020a056000156900b0020c4ed44ba8mr13487929wrz.270.1651605007916; Tue, 03 May 2022 12:10:07 -0700 (PDT) Received: from [10.195.0.230] ([194.169.217.136]) by smtp.gmail.com with ESMTPSA id q22-20020adfb196000000b0020c5253d8d2sm481970wra.30.2022.05.03.12.10.06 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 03 May 2022 12:10:07 -0700 (PDT) Message-ID: Date: Tue, 3 May 2022 22:09:38 +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> <81937bc9-a1d1-82e5-a7f6-e01780591bed@solid-run.com> <7b3d8658-64f4-f414-ca0e-21aed84108cc@denx.de> From: Josua Mayer In-Reply-To: <7b3d8658-64f4-f414-ca0e-21aed84108cc@denx.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 \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 >>>> --- >>>>   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 >