Re: [PATCH 2/4] dm: hash: Add new UCLASS_HASH support

["Alex G." <mr.nuke.me@gmail.com>]

Hi,

On 7/29/21 8:08 PM, Chia-Wei Wang wrote:
> Add UCLASS_HASH for hash driver development. Thus the
> hash drivers (SW or HW-accelerated) can be developed
> in the DM-based fashion.

Software hashing implementations are shared tightly with host tools. 
With DM, there's no opportunity for code sharing with host tools. The 
design question that I have is "do we want to DM hashes, or do we want 
to DM hardware accelerators for hashes?"

I did some parallel work expose remaining hash algos via 
hash_lookup_algo() and hash_progressive_lookup_algo().

> Signed-off-by: Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> ---
>   drivers/crypto/Kconfig            |   2 +
>   drivers/crypto/Makefile           |   1 +
>   drivers/crypto/hash/Kconfig       |   5 ++
>   drivers/crypto/hash/Makefile      |   5 ++
>   drivers/crypto/hash/hash-uclass.c | 121 ++++++++++++++++++++++++++++++
>   include/dm/uclass-id.h            |   1 +
>   include/u-boot/hash.h             |  61 +++++++++++++++
>   7 files changed, 196 insertions(+)
>   create mode 100644 drivers/crypto/hash/Kconfig
>   create mode 100644 drivers/crypto/hash/Makefile
>   create mode 100644 drivers/crypto/hash/hash-uclass.c
>   create mode 100644 include/u-boot/hash.h
> 
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 1ea116be75..0082177c21 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -1,5 +1,7 @@
>   menu "Hardware crypto devices"
>   
> +source drivers/crypto/hash/Kconfig
> +
Hashes are useful outside of cryptographic functions, so it seems odd to 
merge them in crypto. For example, CRC32 is not a hash useful in crypto, 
but otherwise widely used in u-boot.

[snip]
> diff --git a/drivers/crypto/hash/hash-uclass.c b/drivers/crypto/hash/hash-uclass.c
> new file mode 100644
> index 0000000000..446eb9e56a
> --- /dev/null
> +++ b/drivers/crypto/hash/hash-uclass.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2021 ASPEED Technology Inc.
> + * Author: ChiaWei Wang <chiawei_wang@aspeedtech.com>
> + */
> +
> +#define LOG_CATEGORY UCLASS_HASH
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <asm/global_data.h>
> +#include <u-boot/hash.h>
> +#include <errno.h>
> +#include <fdtdec.h>
> +#include <malloc.h>
> +#include <asm/io.h>
> +#include <linux/list.h>
> +
> +struct hash_info {
> +	char *name;
> +	uint32_t digest_size;
> +};
> +
> +static const struct hash_info hash_info[HASH_ALGO_NUM] = {
> +	[HASH_ALGO_CRC16_CCITT] = { "crc16-ccitt", 2 },
> +	[HASH_ALGO_CRC32] = { "crc32", 4 },
> +	[HASH_ALGO_MD5] = { "md5", 16 },
> +	[HASH_ALGO_SHA1] = { "sha1", 20 },
> +	[HASH_ALGO_SHA256] = { "sha256", 32 },
> +	[HASH_ALGO_SHA384] = { "sha384", 48 },
> +	[HASH_ALGO_SHA512] = { "sha512", 64},
> +};

It seems a step backwards to have to enum {} our hash algos, since we 
already identify them by their strings (e.g. "sha256"). and then 
associated ops structure. The

> +
> +enum HASH_ALGO hash_algo_lookup_by_name(const char *name)

     string -> hash_lookup_algo() -> ops struct

Is the current way to do things. hash_algo_lookup_by_name() does the 
roundabout through an enum. That doesn't make sense to me.


Alex