From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex G. Date: Thu, 20 May 2021 19:07:19 -0500 Subject: [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1 In-Reply-To: References: <20210517163840.839097-1-mr.nuke.me@gmail.com> <20210517163840.839097-7-mr.nuke.me@gmail.com> <9b8e856a-d5e7-0565-1365-f600df61be4d@gmail.com> <4810e21e-004f-ef3d-b833-467a68afa5bf@gmail.com> <9b8e8725-9538-56c9-ee9e-8c4aea34b273@gmail.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 5/20/21 6:17 PM, Simon Glass wrote: > Hi Alex, > > On Thu, 20 May 2021 at 17:13, Alex G. wrote: >> >> >> >> On 5/20/21 12:52 PM, Simon Glass wrote: >>> Hi Alex, >>> >>> On Wed, 19 May 2021 at 20:41, Alex G. wrote: >>>> >>>> >>>> >>>> On 5/19/21 4:55 PM, Simon Glass wrote: >>>>> Hi Alex, >>>>> >>>>> On Wed, 19 May 2021 at 11:44, Alex G wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 5/19/21 11:36 AM, Simon Glass wrote: >>>>>>> Hi Alexandru, >>>>>>> >>>>>>> On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc wrote: >>>>>>>> >>>>>>>> From: Simon Glass >>>>>>>> >>>>>>>> We already have a host Kconfig for SHA1. Use CONFIG_IS_ENABLED(SHA1) >>>>>>>> directly in the code shared with the host build, so we can drop the >>>>>>>> unnecessary indirection. >>>>>>>> >>>>>>>> Signed-off-by: Simon Glass >>>>>>>> Reviewed-by: Alexandru Gagniuc >>>>>>>> Signed-off-by: Alexandru Gagniuc >>>>>>>> --- >>>>>>>> common/image-fit.c | 2 +- >>>>>>>> include/image.h | 8 -------- >>>>>>>> 2 files changed, 1 insertion(+), 9 deletions(-) >>>>>>>> >>>>>>>> diff --git a/common/image-fit.c b/common/image-fit.c >>>>>>>> index e614643fe3..24e92a8e92 100644 >>>>>>>> --- a/common/image-fit.c >>>>>>>> +++ b/common/image-fit.c >>>>>>>> @@ -1218,7 +1218,7 @@ int calculate_hash(const void *data, int data_len, const char *algo, >>>>>>>> CHUNKSZ_CRC32); >>>>>>>> *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); >>>>>>>> *value_len = 4; >>>>>>>> - } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) { >>>>>>>> + } else if (CONFIG_IS_ENABLED(SHA1) && strcmp(algo, "sha1") == 0) { >>>>>>> >>>>>>> This can only work if the my host Kconfig patch comes first. Otherwise >>>>>>> this code will just be skipped on the host. >>>>>> >>>>>> I was scratching my head too as to why this works in practice, but not >>>>>> in theory. There is a #define CONFIG_SHA1 in image.h. >>>>>> >>>>>> Although not a perfect fix, we go from two ways to enable SHA1 ("#define >>>>>> IMAGE_ENABLE_SHA1", and "#define CONFIG_SHA1"), to just one. That's why >>>>>> I think this change is an improvement, and part of this series. >>>>> >>>>> No, we really should not do that...everything needs to be in Kconfig. >>>> >>>> I agree for target code. But, as a long term solution, let's look at how >>>> we can get hash algos in linker lists, like we're proposing to do for >>>> crytpo algos. Or I could just drop this change in v2. >>> >>> Would it not be easier to have a host Kconfig for these? You seem to >>> be going to extreme lengths to avoid it, but it seems like the >>> simplest solution, easy to understand, no effect on code size and >>> scalable to the future. >> >> It's easy for the short term in terms if the goal is to get something >> merged. It just hides more fundamental issues with the code. For >> ecample, why is there hash_calculate() and clacultae_hash() > > It is just a naming issue, isn't it? They are quite different functions. Because one resets the watchdog after every CHUNK bytes and the other doesn't? >> >> I was under the impression that we were agreed on the combination of >> patches. I won't try to defend your patch from yourself. I'll drop the >> hash changes from v2 if it helps get things moving along. > > I'm OK with this as a short-term fix to get this series through. But I > think we are going to end up with a Kconfig solution at some point. > What do you think? I think it's possible and reasonable to have common code without host Kconfig. coreboot did it. Alex