RE: [PATCH v8 5/5] crypto: aspeed: add HACE crypto driver

[Neal Liu]

> 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