Re: [PATCH v2 5/5] crypto: aspeed: add HACE crypto driver

[Christophe JAILLET]

Le 06/06/2022 à 08:49, Neal Liu a écrit :
> 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>
> ---

[...]

> +static int aspeed_sk_transfer_sg(struct aspeed_hace_dev *hace_dev)
> +{
> +	struct aspeed_engine_crypto *crypto_engine = &hace_dev->crypto_engine;
> +	struct device *dev = hace_dev->dev;
> +	struct aspeed_cipher_reqctx *rctx;
> +	struct skcipher_request *req;
> +
> +	CIPHER_DBG(hace_dev, "\n");
> +
> +	req = skcipher_request_cast(crypto_engine->areq);
> +	rctx = skcipher_request_ctx(req);
> +
> +	if (req->src == req->dst) {
> +		dma_unmap_sg(dev, req->src, rctx->src_nents, DMA_BIDIRECTIONAL);
> +

Unneeded empty line.

> +	} else {
> +		dma_unmap_sg(dev, req->src, rctx->src_nents, DMA_TO_DEVICE);
> +		dma_unmap_sg(dev, req->dst, rctx->dst_nents, DMA_FROM_DEVICE);
> +	}
> +
> +	return aspeed_sk_complete(hace_dev, 0);
> +}
> +

[...]

> +static int aspeed_sk_start_sg(struct aspeed_hace_dev *hace_dev)
> +{
> +	struct aspeed_engine_crypto *crypto_engine = &hace_dev->crypto_engine;
> +	struct aspeed_sg_list *src_list, *dst_list;
> +	dma_addr_t src_dma_addr, dst_dma_addr;
> +	struct aspeed_cipher_reqctx *rctx;
> +	struct skcipher_request *req;
> +	struct scatterlist *s;
> +	int src_sg_len;
> +	int dst_sg_len;
> +	int total, i;
> +	int rc;
> +
> +	CIPHER_DBG(hace_dev, "\n");
> +
> +	req = skcipher_request_cast(crypto_engine->areq);
> +	rctx = skcipher_request_ctx(req);
> +
> +	rctx->enc_cmd |= HACE_CMD_DES_SG_CTRL | HACE_CMD_SRC_SG_CTRL |
> +			 HACE_CMD_AES_KEY_HW_EXP | HACE_CMD_MBUS_REQ_SYNC_EN;
> +
> +	/* BIDIRECTIONAL */
> +	if (req->dst == req->src) {
> +		src_sg_len = dma_map_sg(hace_dev->dev, req->src,
> +					rctx->src_nents, DMA_BIDIRECTIONAL);
> +		dst_sg_len = src_sg_len;
> +		if (!src_sg_len) {
> +			dev_warn(hace_dev->dev, "dma_map_sg() src error\n");
> +			return -EINVAL;
> +		}
> +
> +	} else {
> +		src_sg_len = dma_map_sg(hace_dev->dev, req->src,
> +					rctx->src_nents, DMA_TO_DEVICE);
> +		if (!src_sg_len) {
> +			dev_warn(hace_dev->dev, "dma_map_sg() src error\n");
> +			return -EINVAL;
> +		}
> +
> +		dst_sg_len = dma_map_sg(hace_dev->dev, req->dst,
> +					rctx->dst_nents, DMA_FROM_DEVICE);
> +		if (!dst_sg_len) {
> +			dev_warn(hace_dev->dev, "dma_map_sg() dst error\n");
> +			rc = -EINVAL;
> +			goto free_req_src;

Should we realy call dma_unmap_sg() if dma_map_sg() fails?

> +		}
> +	}
> +
> +	src_list = (struct aspeed_sg_list *)crypto_engine->cipher_addr;
> +	src_dma_addr = crypto_engine->cipher_dma_addr;
> +	total = req->cryptlen;
> +
> +	for_each_sg(req->src, s, src_sg_len, i) {
> +		src_list[i].phy_addr = sg_dma_address(s);
> +
> +		/* last sg list */
> +		if (sg_dma_len(s) >= total) {
> +			src_list[i].len = total;
> +			src_list[i].len |= BIT(31);
> +			total = 0;
> +			break;
> +		}
> +
> +		src_list[i].len = sg_dma_len(s);
> +		total -= src_list[i].len;
> +	}
> +
> +	if (total != 0)
> +		return -EINVAL;

goto free_req_src; ?

> +
> +	if (req->dst == req->src) {
> +		dst_list = src_list;
> +		dst_dma_addr = src_dma_addr;
> +
> +	} else {
> +		dst_list = (struct aspeed_sg_list *)crypto_engine->dst_sg_addr;
> +		dst_dma_addr = crypto_engine->dst_sg_dma_addr;
> +		total = req->cryptlen;
> +
> +		for_each_sg(req->dst, s, dst_sg_len, i) {
> +			dst_list[i].phy_addr = sg_dma_address(s);
> +
> +			/* last sg list */
> +			if (sg_dma_len(s) >= total) {
> +				dst_list[i].len = total;
> +				dst_list[i].len |= BIT(31);
> +				total = 0;
> +				break;
> +			}
> +
> +			dst_list[i].len = sg_dma_len(s);
> +			total -= dst_list[i].len;
> +		}
> +
> +		dst_list[dst_sg_len].phy_addr = 0;
> +		dst_list[dst_sg_len].len = 0;
> +	}
> +
> +	if (total != 0)
> +		return -EINVAL;
> +
> +	crypto_engine->resume = aspeed_sk_transfer_sg;
> +
> +	/* Dummy read for barriers */
> +	readl(src_list);
> +	readl(dst_list);
> +
> +	/* Trigger engines */
> +	ast_hace_write(hace_dev, src_dma_addr, ASPEED_HACE_SRC);
> +	ast_hace_write(hace_dev, dst_dma_addr, ASPEED_HACE_DEST);
> +	ast_hace_write(hace_dev, req->cryptlen, ASPEED_HACE_DATA_LEN);
> +	ast_hace_write(hace_dev, rctx->enc_cmd, ASPEED_HACE_CMD);
> +
> +	return -EINPROGRESS;
> +
> +free_req_src:
> +	dma_unmap_sg(hace_dev->dev, req->src, rctx->src_nents, DMA_TO_DEVICE);
> +
> +	return rc;
> +}
> +

[...]

> +static int aspeed_aes_setkey(struct crypto_skcipher *cipher, const u8 *key,
> +			     unsigned int keylen)
> +{
> +	struct aspeed_cipher_ctx *ctx = crypto_skcipher_ctx(cipher);
> +	struct aspeed_hace_dev *hace_dev = ctx->hace_dev;
> +	struct crypto_aes_ctx gen_aes_key;
> +
> +	CIPHER_DBG(hace_dev, "keylen: %d bits\n", (keylen * 8));
> +
> +	if (keylen != AES_KEYSIZE_128 && keylen != AES_KEYSIZE_192 &&
> +	    keylen != AES_KEYSIZE_256)
> +		return -EINVAL;
> +
> +	if (ctx->hace_dev->version == AST2500_VERSION) {
> +		aes_expandkey(&gen_aes_key, key, keylen);
> +		memcpy(ctx->key, gen_aes_key.key_enc, AES_MAX_KEYLENGTH);
> +

Unneeded empty line

> +	} else {
> +		memcpy(ctx->key, key, keylen);
> +	}
> +
> +	ctx->key_len = keylen;
> +
> +	return 0;
> +}
> +

[...]

> +	crypto_engine->cipher_ctx =
> +		dma_alloc_coherent(&pdev->dev,
> +				   PAGE_SIZE,
> +				   &crypto_engine->cipher_ctx_dma,
> +				   GFP_KERNEL);
> +	if (!crypto_engine->cipher_ctx) {
> +		dev_err(&pdev->dev, "Failed to allocate cipher ctx dma\n");
> +		rc = -ENOMEM;
> +		goto free_hash_src;
> +	}
> +
> +	crypto_engine->cipher_addr =
> +		dma_alloc_coherent(&pdev->dev,
> +				   ASPEED_CRYPTO_SRC_DMA_BUF_LEN,
> +				   &crypto_engine->cipher_dma_addr,
> +				   GFP_KERNEL);
> +	if (!crypto_engine->cipher_addr) {
> +		dev_err(&pdev->dev, "Failed to allocate cipher addr dma\n");
> +		rc = -ENOMEM;
> +		goto free_cipher_ctx;
> +	}
> +
> +	if (hace_dev->version == AST2600_VERSION) {
> +		crypto_engine->dst_sg_addr =
> +			dma_alloc_coherent(&pdev->dev,
> +					   ASPEED_CRYPTO_DST_DMA_BUF_LEN,
> +					   &crypto_engine->dst_sg_dma_addr,
> +					   GFP_KERNEL);
> +		if (!crypto_engine->dst_sg_addr) {
> +			dev_err(&pdev->dev, "Failed to allocate dst_sg dma\n");
> +			rc = -ENOMEM;
> +			goto free_cipher_addr;
> +		}
> +	}
> +
>   	rc = aspeed_hace_register(hace_dev);
>   	if (rc) {
>   		dev_err(&pdev->dev, "Failed to register algs, rc:0x%x\n", rc);

I guess that the new dma_alloc_coherent() just a few lines above should 
also be undone in error hanfling path if aspeed_hace_register() fails?

> @@ -179,6 +282,18 @@ static int aspeed_hace_probe(struct platform_device *pdev)
>   
>   	return 0;
>   
> +free_cipher_addr:
> +	dma_free_coherent(&pdev->dev, ASPEED_CRYPTO_SRC_DMA_BUF_LEN,
> +			  crypto_engine->cipher_addr,
> +			  crypto_engine->cipher_dma_addr);
> +free_cipher_ctx:
> +	dma_free_coherent(&pdev->dev, PAGE_SIZE,
> +			  crypto_engine->cipher_ctx,
> +			  crypto_engine->cipher_ctx_dma);
> +free_hash_src:
> +	dma_free_coherent(&pdev->dev, ASPEED_HASH_SRC_DMA_BUF_LEN,
> +			  hash_engine->ahash_src_addr,
> +			  hash_engine->ahash_src_dma_addr);
>   end:
>   	clk_disable_unprepare(hace_dev->clk);
>   	return rc;