[PATCH 06/18] image: Drop IMAGE_ENABLE_SHA1

[Simon Glass]

Hi Alex,

On Wed, 19 May 2021 at 20:41, Alex G. <mr.nuke.me at gmail.com> wrote:
>
>
>
> On 5/19/21 4:55 PM, Simon Glass wrote:
> > Hi Alex,
> >
> > On Wed, 19 May 2021 at 11:44, Alex G <mr.nuke.me at gmail.com> wrote:
> >>
> >>
> >>
> >> On 5/19/21 11:36 AM, Simon Glass wrote:
> >>> Hi Alexandru,
> >>>
> >>> On Mon, 17 May 2021 at 10:38, Alexandru Gagniuc <mr.nuke.me at gmail.com> wrote:
> >>>>
> >>>> From: Simon Glass <sjg at 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 <sjg at chromium.org>
> >>>> Reviewed-by: Alexandru Gagniuc <mr.nuke.me at gmail.com>
> >>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me at 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.
> >
> > 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.

Regards,
Simon