> Hi Neal,
>
> Thanks for addressing v7 review comments, few more below.
>
> On 7/26/2022 4:34 AM, Neal Liu wrote:
> > Add HACE crypto driver to support symmetric-key encryption and
> > decryption with multiple modes of operation.
> >
> > Signed-off-by: Neal Liu <neal_liu@xxxxxxxxxxxxxx>
> > Signed-off-by: Johnny Huang <johnny_huang@xxxxxxxxxxxxxx>
> > ---
> > drivers/crypto/aspeed/Kconfig | 26 +
> > drivers/crypto/aspeed/Makefile | 7 +-
> > drivers/crypto/aspeed/aspeed-hace-crypto.c | 1121
> ++++++++++++++++++++
> > drivers/crypto/aspeed/aspeed-hace.c | 91 +-
> > drivers/crypto/aspeed/aspeed-hace.h | 112 ++
> > 5 files changed, 1354 insertions(+), 3 deletions(-)
> > create mode 100644 drivers/crypto/aspeed/aspeed-hace-crypto.c
> >
> > diff --git a/drivers/crypto/aspeed/Kconfig
> > b/drivers/crypto/aspeed/Kconfig index 059e627efef8..f19994915a5e
> > 100644
> > --- a/drivers/crypto/aspeed/Kconfig
> > +++ b/drivers/crypto/aspeed/Kconfig
> > @@ -30,3 +30,29 @@ config CRYPTO_DEV_ASPEED_HACE_HASH_DEBUG
> > to ask for those messages.
> > Avoid enabling this option for production build to
> > minimize driver timing.
> > +
> > +config CRYPTO_DEV_ASPEED_HACE_CRYPTO
> > + bool "Enable Aspeed Hash & Crypto Engine (HACE) crypto"
> > + depends on CRYPTO_DEV_ASPEED
> > + select CRYPTO_ENGINE
> > + select CRYPTO_AES
> > + select CRYPTO_DES
> > + select CRYPTO_ECB
> > + select CRYPTO_CBC
> > + select CRYPTO_CFB
> > + select CRYPTO_OFB
> > + select CRYPTO_CTR
> > + help
> > + Select here to enable Aspeed Hash & Crypto Engine (HACE)
> > + crypto driver.
> > + Supports AES/DES symmetric-key encryption and decryption
> > + with ECB/CBC/CFB/OFB/CTR options.
> > +
> > +config CRYPTO_DEV_ASPEED_HACE_CRYPTO_DEBUG
> > + bool "Enable HACE crypto debug messages"
> > + depends on CRYPTO_DEV_ASPEED_HACE_CRYPTO
> > + help
> > + Print HACE crypto debugging messages if you use this option
> > + to ask for those messages.
> > + Avoid enabling this option for production build to
> > + minimize driver timing.
>
> Why are separate options required for hash and crypto algorithms, if hace is
> only hw crypto on the SoCs?
>
> Looks like that's requiring unnecessary __weak register / unregister functions
> [see below].
>
> Couldn't just two options CONFIG_CRYPTO_DEV_ASPEED and
> CONFIG_CRYPTO_DEV_ASPEED_DEBUG be simpler to set for downstream
> defconfigs?
I would like to separate different algorithms by different options for more convenient for further use and debug.
We also have RSA engine named ACRY, and would upstream once hash & crypto being accepted.
So combined them into one option seems not a good choice for multiple hw crypto, do you agree?
> > diff --git a/drivers/crypto/aspeed/Makefile
> > b/drivers/crypto/aspeed/Makefile index 8bc8d4fed5a9..421e2ca9c53e
> > 100644
> > --- a/drivers/crypto/aspeed/Makefile
> > +++ b/drivers/crypto/aspeed/Makefile
> > @@ -1,6 +1,9 @@
> > obj-$(CONFIG_CRYPTO_DEV_ASPEED) += aspeed_crypto.o
> > -aspeed_crypto-objs := aspeed-hace.o \
> > - $(hace-hash-y)
> > +aspeed_crypto-objs := aspeed-hace.o \
> > + $(hace-hash-y) \
> > + $(hace-crypto-y)
> >
> > obj-$(CONFIG_CRYPTO_DEV_ASPEED_HACE_HASH) +=
> aspeed-hace-hash.o
> > hace-hash-$(CONFIG_CRYPTO_DEV_ASPEED_HACE_HASH) :=
> > aspeed-hace-hash.o
> > +obj-$(CONFIG_CRYPTO_DEV_ASPEED_HACE_CRYPTO) +=
> aspeed-hace-crypto.o
> > +hace-crypto-$(CONFIG_CRYPTO_DEV_ASPEED_HACE_CRYPTO) :=
> > +aspeed-hace-crypto.o
> > diff --git a/drivers/crypto/aspeed/aspeed-hace-crypto.c
> > b/drivers/crypto/aspeed/aspeed-hace-crypto.c
> > new file mode 100644
>
> [...]
>
> > +
> > +void aspeed_register_hace_crypto_algs(struct aspeed_hace_dev
> > +*hace_dev) {
> > + int rc, i;
> > +
> > + CIPHER_DBG(hace_dev, "\n");
> > +
> > + for (i = 0; i < ARRAY_SIZE(aspeed_crypto_algs); i++) {
> > + aspeed_crypto_algs[i].hace_dev = hace_dev;
> > + rc = crypto_register_skcipher(&aspeed_crypto_algs[i].alg.skcipher);
> > + if (rc) {
> > + CIPHER_DBG(hace_dev, "Failed to register %s\n",
> > + aspeed_crypto_algs[i].alg.skcipher.base.cra_name);
> > + }
> > + }
> > +
> > + if (hace_dev->version != AST2600_VERSION)
> > + return;
> > +
> > + for (i = 0; i < ARRAY_SIZE(aspeed_crypto_algs_g6); i++) {
> > + aspeed_crypto_algs_g6[i].hace_dev = hace_dev;
> > + rc =
> crypto_register_skcipher(&aspeed_crypto_algs_g6[i].alg.skcipher);
> > + if (rc) {
> > + CIPHER_DBG(hace_dev, "Failed to register %s\n",
> > + aspeed_crypto_algs_g6[i].alg.skcipher.base.cra_name);
> > + }
> > + }
> > +}
> > diff --git a/drivers/crypto/aspeed/aspeed-hace.c
> > b/drivers/crypto/aspeed/aspeed-hace.c
> > index 89b1585d72e2..efc0725ebf98 100644
> > --- a/drivers/crypto/aspeed/aspeed-hace.c
> > +++ b/drivers/crypto/aspeed/aspeed-hace.c
> > @@ -32,10 +32,22 @@ void __weak
> aspeed_unregister_hace_hash_algs(struct aspeed_hace_dev *hace_dev)
> > dev_warn(hace_dev->dev, "%s: Not supported yet\n", __func__);
> > }
> >
> > +/* Weak function for HACE crypto */
> > +void __weak aspeed_register_hace_crypto_algs(struct aspeed_hace_dev
> > +*hace_dev) {
> > + dev_warn(hace_dev->dev, "%s: Not supported yet\n", __func__); }
> > +
> > +void __weak aspeed_unregister_hace_crypto_algs(struct aspeed_hace_dev
> > +*hace_dev) {
> > + dev_warn(hace_dev->dev, "%s: Not supported yet\n", __func__); }
> > +
>
> aspeed_unregister_hace_crypto_algs() is not implemented in
> aspeed-hace-crypto.c, so those algorithms are not unregistered during unload.
>
> This was missed because of __weak function.
I missed this part, thanks for pointing out.
I'll add it in next patch.
> > /* HACE interrupt service routine */
> > static irqreturn_t aspeed_hace_irq(int irq, void *dev)
> > {
> > struct aspeed_hace_dev *hace_dev = (struct aspeed_hace_dev
> *)dev;
> > + struct aspeed_engine_crypto *crypto_engine =
> > +&hace_dev->crypto_engine;
> > struct aspeed_engine_hash *hash_engine =
> &hace_dev->hash_engine;
> > u32 sts;
> >
> > @@ -51,9 +63,24 @@ static irqreturn_t aspeed_hace_irq(int irq, void *dev)
> > dev_warn(hace_dev->dev, "HASH no active requests.\n");
> > }
> >
> > + if (sts & HACE_CRYPTO_ISR) {
> > + if (crypto_engine->flags & CRYPTO_FLAGS_BUSY)
> > + tasklet_schedule(&crypto_engine->done_task);
> > + else
> > + dev_warn(hace_dev->dev, "CRYPTO no active requests.\n");
> > + }
> > +
> > return IRQ_HANDLED;
> > }
> >
> > +static void aspeed_hace_crypto_done_task(unsigned long data) {
> > + struct aspeed_hace_dev *hace_dev = (struct aspeed_hace_dev *)data;
> > + struct aspeed_engine_crypto *crypto_engine =
> > +&hace_dev->crypto_engine;
> > +
> > + crypto_engine->resume(hace_dev);
> > +}
> > +
> > static void aspeed_hace_hash_done_task(unsigned long data)
> > {
> > struct aspeed_hace_dev *hace_dev = (struct aspeed_hace_dev
> *)data;
> > @@ -65,11 +92,13 @@ static void aspeed_hace_hash_done_task(unsigned
> long data)
> > static void aspeed_hace_register(struct aspeed_hace_dev *hace_dev)
> > {
> > aspeed_register_hace_hash_algs(hace_dev);
> > + aspeed_register_hace_crypto_algs(hace_dev);
> > }
> >
> > static void aspeed_hace_unregister(struct aspeed_hace_dev *hace_dev)
> > {
> > aspeed_unregister_hace_hash_algs(hace_dev);
> > + aspeed_unregister_hace_crypto_algs(hace_dev);
> > }
>
> Could just wrap these calls instead of weak functions.
>
> static void aspeed_hace_unregister(struct aspeed_hace_dev *hace_dev)
> { #ifdef CONFIG_CRYPTO_DEV_ASPEED_HACE_HASH
> aspeed_unregister_hace_hash_algs(hace_dev);
> #endif
> #ifdef CONFIG_CRYPTO_DEV_ASPEED_HACE_CRYPTO
> aspeed_unregister_hace_crypto_algs(hace_dev);
> #endif
> }
Okay, I'll revise it as you suggested.
> >
> > static const struct of_device_id aspeed_hace_of_matches[] = { @@
> > -80,6 +109,7 @@ static const struct of_device_id
> > aspeed_hace_of_matches[] = {
> >
> > static int aspeed_hace_probe(struct platform_device *pdev)
> > {
> > + struct aspeed_engine_crypto *crypto_engine;
> > const struct of_device_id *hace_dev_id;
> > struct aspeed_engine_hash *hash_engine;
> > struct aspeed_hace_dev *hace_dev;
> > @@ -100,6 +130,7 @@ static int aspeed_hace_probe(struct platform_device
> *pdev)
> > hace_dev->dev = &pdev->dev;
> > hace_dev->version = (unsigned long)hace_dev_id->data;
> > hash_engine = &hace_dev->hash_engine;
> > + crypto_engine = &hace_dev->crypto_engine;
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> Thanks,
> Dhananjay
