Re: [PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1

[Alex G]

On 5/19/21 11:36 AM, Simon Glass wrote:
Hi Alexandru,

On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc <mr.nuke...@gmail.com> wrote:

From: Simon Glass <s...@chromium.org>

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 <s...@chromium.org>
Reviewed-by: Alexandru Gagniuc <mr.nuke...@gmail.com>
Signed-off-by: Alexandru Gagniuc <mr.nuke...@gmail.com>
---
  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.

Alex

                 sha1_csum_wd((unsigned char *)data, data_len,
                              (unsigned char *)value, CHUNKSZ_SHA1);
                 *value_len = 20;
diff --git a/include/image.h b/include/image.h
index 887a3270bd..8c718adba0 100644
--- a/include/image.h
+++ b/include/image.h
@@ -68,13 +68,9 @@ struct fdt_region;
  #  ifdef CONFIG_SPL_MD5
  #   define IMAGE_ENABLE_MD5    1
  #  endif
-#  ifdef CONFIG_SPL_FIT_SHA1
-#   define IMAGE_ENABLE_SHA1   1
-#  endif
  # else
  #  define IMAGE_ENABLE_CRC32   1
  #  define IMAGE_ENABLE_MD5     1
-#  define IMAGE_ENABLE_SHA1    1
  # endif

  #ifndef IMAGE_ENABLE_CRC32
@@ -85,10 +81,6 @@ struct fdt_region;
  #define IMAGE_ENABLE_MD5       0
  #endif

-#ifndef IMAGE_ENABLE_SHA1
-#define IMAGE_ENABLE_SHA1      0
-#endif
-
  #if defined(CONFIG_FIT_SHA256) || \
         defined(CONFIG_SPL_FIT_SHA256)
  #define IMAGE_ENABLE_SHA256    1
--
2.31.1


Regards,
Simon