Le 21/06/2022 à 08:37, Neal Liu a écrit :
Hash and Crypto Engine (HACE) is designed to accelerate the throughput of hash data digest, encryption, and decryption. Basically, HACE can be divided into two independently engines - Hash Engine and Crypto Engine. This patch aims to add HACE hash engine driver for hash accelerator. Signed-off-by: Neal Liu <neal_liu-SAlXDmAnmOAqDJ6do+/SaQ@xxxxxxxxxxxxxxxx> Signed-off-by: Johnny Huang <johnny_huang-SAlXDmAnmOAqDJ6do+/SaQ@xxxxxxxxxxxxxxxx> ---
[...]
+++ b/drivers/crypto/aspeed/Kconfig @@ -0,0 +1,23 @@ +config CRYPTO_DEV_ASPEED + tristate "Support for Aspeed cryptographic engine driver" + depends on ARCH_ASPEED + help + Hash and Crypto Engine (HACE) is designed to accelerate the + throughput of hash data digest, encryption and decryption. + + Select y here to have support for the cryptographic driver + available on Aspeed SoC. + +config CRYPTO_DEV_ASPEED_HACE_HASH + bool "Enable ASPEED Hash & Crypto Engine (HACE) hash"
Nit: Sometimes you have ASPEED, sometimes you have Aspeed. (see a few lines above)
[...]
+static int aspeed_ahash_req_update(struct aspeed_hace_dev *hace_dev) +{ + struct aspeed_engine_hash *hash_engine = &hace_dev->hash_engine; + struct ahash_request *req = hash_engine->req; + struct aspeed_sham_reqctx *rctx = ahash_request_ctx(req); + aspeed_hace_fn_t resume; + + AHASH_DBG(hace_dev, "\n"); + + if (hace_dev->version == AST2600_VERSION) { + rctx->cmd |= HASH_CMD_HASH_SRC_SG_CTRL; + resume = aspeed_ahash_update_resume_sg; + + } else { + resume = aspeed_ahash_update_resume; + } + + hash_engine->dma_prepare(hace_dev);
Apparently dma_prepare() can fail. Should there be some error handling here?
+ + return aspeed_hace_ahash_trigger(hace_dev, resume); +} +
[...]
+static int aspeed_hace_probe(struct platform_device *pdev) +{ + const struct of_device_id *hace_dev_id; + struct aspeed_engine_hash *hash_engine; + struct aspeed_hace_dev *hace_dev; + struct resource *res; + int rc; + + hace_dev = devm_kzalloc(&pdev->dev, sizeof(struct aspeed_hace_dev), + GFP_KERNEL); + if (!hace_dev) + return -ENOMEM; + + hace_dev_id = of_match_device(aspeed_hace_of_matches, &pdev->dev); + if (!hace_dev_id) { + dev_err(&pdev->dev, "Failed to match hace dev id\n"); + return -EINVAL; + } + + hace_dev->dev = &pdev->dev; + hace_dev->version = (unsigned long)hace_dev_id->data; + hash_engine = &hace_dev->hash_engine; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + + platform_set_drvdata(pdev, hace_dev); + + /* Initialize crypto hardware engine structure for hash */ + hace_dev->crypt_engine_hash = crypto_engine_alloc_init(hace_dev->dev, + true);
This returns NULL on error and crypto_engine_start() will crash in such a case.
+ + rc = crypto_engine_start(hace_dev->crypt_engine_hash); + if (rc) + goto err_engine_hash_start; + + tasklet_init(&hash_engine->done_task, aspeed_hace_hash_done_task, + (unsigned long)hace_dev); + + hace_dev->regs = devm_ioremap_resource(&pdev->dev, res); + if (!hace_dev->regs) { + dev_err(&pdev->dev, "Failed to map resources\n"); + return -ENOMEM;
I think that all direct returns from here to the end of the function should be "goto err_engine_hash_start;".
+ } + + /* Get irq number and register it */ + hace_dev->irq = platform_get_irq(pdev, 0); + if (!hace_dev->irq) { + dev_err(&pdev->dev, "Failed to get interrupt\n"); + return -ENXIO; + } + + rc = devm_request_irq(&pdev->dev, hace_dev->irq, aspeed_hace_irq, 0, + dev_name(&pdev->dev), hace_dev); + if (rc) { + dev_err(&pdev->dev, "Failed to request interrupt\n"); + return rc; + } + + /* Get clk and enable it */ + hace_dev->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(hace_dev->clk)) { + dev_err(&pdev->dev, "Failed to get clk\n"); + return -ENODEV; + } + + rc = clk_prepare_enable(hace_dev->clk); + if (rc) { + dev_err(&pdev->dev, "Failed to enable clock 0x%x\n", rc); + return rc; + } + + /* Allocate DMA buffer for hash engine input used */ + hash_engine->ahash_src_addr = + dma_alloc_coherent(&pdev->dev, + ASPEED_HASH_SRC_DMA_BUF_LEN, + &hash_engine->ahash_src_dma_addr, + GFP_KERNEL);
Most of the resources are devm_'ed. Does it make sense to use dmam_alloc_coherent() here to simplify the .remove function?
+ if (!hash_engine->ahash_src_addr) { + dev_err(&pdev->dev, "Failed to allocate dma buffer\n"); + rc = -ENOMEM; + goto clk_exit; + } + + aspeed_hace_register(hace_dev); + + dev_info(&pdev->dev, "ASPEED Crypto Accelerator successfully registered\n"); +
Nit: Sometimes you have ASPEED, sometimes you have Aspeed.
+ return rc; + +clk_exit: + clk_disable_unprepare(hace_dev->clk); +err_engine_hash_start: + crypto_engine_exit(hace_dev->crypt_engine_hash); + + return rc; +} +
[...]
+MODULE_AUTHOR("Neal Liu <neal_liu-SAlXDmAnmOAqDJ6do+/SaQ@xxxxxxxxxxxxxxxx>"); +MODULE_DESCRIPTION("ASPEED HACE driver Crypto Accelerator");
Nit: Sometimes you have ASPEED, sometimes you have Aspeed.