Hi Alex,
> From: Alex G. <mr.nuke...@gmail.com>
> Sent: Thursday, September 16, 2021 11:49 PM
>
> On 7/29/21 8:08 PM, Chia-Wei Wang wrote:
> > Add purely software-implmented drivers to support multiple hash
> > operations including CRC, MD5, and SHA family.
> >
> > This driver is based on the new hash uclass.
> >
> > Signed-off-by: Chia-Wei Wang <chiawei_w...@aspeedtech.com>
> > ---
> > drivers/crypto/hash/Kconfig | 11 ++
> > drivers/crypto/hash/Makefile | 1 +
> > drivers/crypto/hash/hash_sw.c | 301
> ++++++++++++++++++++++++++++++++++
> > 3 files changed, 313 insertions(+)
> > create mode 100644 drivers/crypto/hash/hash_sw.c
> >
> > diff --git a/drivers/crypto/hash/Kconfig b/drivers/crypto/hash/Kconfig
> > index e226144b9b..cd29a5c6a4 100644
> > --- a/drivers/crypto/hash/Kconfig
> > +++ b/drivers/crypto/hash/Kconfig
> > @@ -3,3 +3,14 @@ config DM_HASH
> > depends on DM
> > help
> > If you want to use driver model for Hash, say Y.
> > +
> > +config HASH_SOFTWARE
> > + bool "Enable driver for Hash in software"
> > + depends on DM_HASH
> > + depends on MD5
> > + depends on SHA1
> > + depends on SHA256
> > + depends on SHA512_ALGO
>
> I would have expected a U_BOOT_DRIVER() for each hash algo, rather than a
> U_BOOT_DRIVER() wich encompassess all possible algos. If I'm trying to use
> SHA256 in SPL, I might not have the room too add SHA1 and MD5, so I'd have
> issues using HASH_SOFTWARE, as designed.
I agree with the SPL size issue.
A future patches to move those CONFIG_SHAxxx/CONFIG_MD5 options into the
DM-based hash_sw.c could be an option.
Thus the balance between the hash algos support and the code size can be
managed.
>
> > diff --git a/drivers/crypto/hash/hash_sw.c
> > b/drivers/crypto/hash/hash_sw.c new file mode 100644 index
> > 0000000000..fea9d12609
> > --- /dev/null
> > +++ b/drivers/crypto/hash/hash_sw.c
> > @@ -0,0 +1,301 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2021 ASPEED Technology Inc.
> > + * Author: ChiaWei Wang <chiawei_w...@aspeedtech.com> */ #include
> > +<config.h> #include <common.h> #include <dm.h> #include <log.h>
> > +#include <malloc.h> #include <watchdog.h> #include <u-boot/hash.h>
> > +#include <u-boot/crc.h> #include <u-boot/md5.h> #include
> > +<u-boot/sha1.h> #include <u-boot/sha256.h> #include <u-boot/sha512.h>
> > +
> > +/* CRC16-CCITT */
> > +static void hash_init_crc16_ccitt(void *ctx) {
> > + *((uint16_t *)ctx) = 0;
>
> Undefined behavior: Pointer aliased type-punning. I would suggest using
> memset(). Might not be necessarrym as expleined in the next comment.
>
> > +static void hash_update_crc16_ccitt(void *ctx, const void *ibuf,
> > +uint32_t ilen) static void hash_finish_crc16_ccitt(void *ctx, void
> > +*obuf)
> > +/* CRC32 */
> > +static void hash_init_crc32(void *ctx) static void
> > +hash_update_crc32(void *ctx, const void *ibuf, uint32_t ilen) static
> > +void hash_finish_crc32(void *ctx, void *obuf)
> > +/* SHA1 */
> > +static void hash_init_sha1(void *ctx)
> > +/* SHA256 */
> > +/* SHA384 */
> > +/* SHA512 */
>
> This logic already exists in common/hash.c for hash_Lookup_algo() and
> hash_progressive_algo().
Yes.
To prevent modifying the 'static' keyword in common/hash.c while reusing the
hash lib, the same logic appears in the DM-based hash_sw.c.
Chiawei
