From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 9DCEDC43334 for ; Mon, 6 Jun 2022 19:44:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=midhwshEHlIn9gXgNvjFbrfhdhXxI5LiEZ/Ba+TUTgM=; b=SfZRXw1b4jEv9o S7IHjHu/DSvPVA3Gb/p0pj93+SYhrxD5Jyt1wGumD9jZiywEt3Uzn9TJEJLAAQDZ6/nsnxsFGLUym k/9q59jaE831JTozHP0+SG76QIxddI8SqLVz4mKUFSgZkdkue4JTKhOvCswoXSTxhSJmTf6kxAh7j KSj2rX2TNUJWu5fTIzqFDkLfAhB1bySnAm7KT3FWRMEX8JBKRuc+X6gZ3+Qu+ptdDKR9MTHeCLeu1 mdaqZWg+67LkwlknPS9q/CtqowifjmbhBK9MIITvUoBDhBmMEiv/OqpKyk+lJzcgvWNiNxqiT2ye5 8VpsuOiRQCGPV0X+lrSw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nyIdT-002Vup-Pl; Mon, 06 Jun 2022 19:43:35 +0000 Received: from mail-wm1-x32c.google.com ([2a00:1450:4864:20::32c]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nyIdQ-002VuD-FL for linux-arm-kernel@lists.infradead.org; Mon, 06 Jun 2022 19:43:34 +0000 Received: by mail-wm1-x32c.google.com with SMTP id 67-20020a1c1946000000b00397382b44f4so8427779wmz.2 for ; Mon, 06 Jun 2022 12:43:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=5A0Tk1TtWUpBiRRuio2LxMIdLJAOAXj5CmUrpNuf59M=; b=nG50Y+kZ0OsrnMz4iw7KEi8zqJIpYAYQKD0xA8K2beHGzBBOBQWPIK8tdvR8a5N8TW WjsL1tm2Z+ERfMEMSNpnhRPS8qKLdOUbWIx0AQ2qh7p+jjBrQGJmPD2zW1Zo6JVU3QHq /4jCD656ZpkozBnuY1ZJ0/xesYY2d6QSxlURLCqxfk7AxAqjdAi4aQfKK5KsQWotUfPh sq413oCLmVlIGjBEApMp98q96rxIqttDaa4Ta22uYSSMgZJ+eSSQz8ckhOwJvEgi4+2m Ur4taGenPP669FVSyrlTw/e6Ae9zqVF/95HH4ZuaIK74avrSGyMV89RtXG5rR5OS2vnJ /yqw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=5A0Tk1TtWUpBiRRuio2LxMIdLJAOAXj5CmUrpNuf59M=; b=e8eSeObp4veXHRr5IDXFeESouVE2XeAQBRYx8u8Iniz5m0XNc0ESJk61BgbRQMicGJ 9WYw2y5ABLPQhEC/lTQ5wRDXH9OYKxAhIqoCRZVR1EJaQ4yB7sAs7Yu0zu4ftA1BMiBX ZwF6BqhD1ZbVgxMExNVnI8yO3gDnZh87rFlBbdoUrR9LkzhitK7a2dh1oKxI/FnyH4IT nDWtRkVxYjTO/uuV9N66lKF//vDkjDC5IlTCsHC9KhLzaBSxM6grAV5513eyJFM5ILJH V748CCpiAd8gQGood6iDGYLZdLQOZ2Ck7gAWcJNvKw+pU1oKI1U+GHBeqrEgNQDxWkbL 1PJQ== X-Gm-Message-State: AOAM533TCwh1iqQPIjYtyPPjT01xilV8woNp9u2yLjtw9nRWzjUMa2ED GNu18089c8fcCNVYn/48kbY= X-Google-Smtp-Source: ABdhPJwaGLLzWURo5J4qJcPtntGNMvWmblFxfMvb3jlJ8k4IMCCsZCC6GyTpWDgzVnTMo40K2EGHKQ== X-Received: by 2002:a05:600c:2105:b0:39c:37d0:6f5e with SMTP id u5-20020a05600c210500b0039c37d06f5emr21147503wml.44.1654544610604; Mon, 06 Jun 2022 12:43:30 -0700 (PDT) Received: from Red ([2a01:cb1d:3d5:a100:264b:feff:fe03:2806]) by smtp.googlemail.com with ESMTPSA id h13-20020a05600c2cad00b00397623ff335sm19982775wmc.10.2022.06.06.12.43.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Jun 2022 12:43:30 -0700 (PDT) Date: Mon, 6 Jun 2022 21:43:25 +0200 From: Corentin Labbe To: Neal Liu Cc: Herbert Xu , "David S . Miller" , Rob Herring , Krzysztof Kozlowski , Joel Stanley , Andrew Jeffery , Johnny Huang , linux-aspeed@lists.ozlabs.org, linux-crypto@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, BMC-SW@aspeedtech.com Subject: Re: [PATCH v2 1/5] crypto: aspeed: Add HACE hash driver Message-ID: References: <20220606064935.1458903-1-neal_liu@aspeedtech.com> <20220606064935.1458903-2-neal_liu@aspeedtech.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220606064935.1458903-2-neal_liu@aspeedtech.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220606_124332_576731_92A7B880 X-CRM114-Status: GOOD ( 32.31 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Le Mon, Jun 06, 2022 at 02:49:31PM +0800, Neal Liu a =E9crit : > 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 > Signed-off-by: Johnny Huang > --- > MAINTAINERS | 7 + > drivers/crypto/Kconfig | 1 + > drivers/crypto/Makefile | 1 + > drivers/crypto/aspeed/Kconfig | 22 + > drivers/crypto/aspeed/Makefile | 6 + > drivers/crypto/aspeed/aspeed-hace-hash.c | 1409 ++++++++++++++++++++++ > drivers/crypto/aspeed/aspeed-hace.c | 221 ++++ > drivers/crypto/aspeed/aspeed-hace.h | 182 +++ > 8 files changed, 1849 insertions(+) > create mode 100644 drivers/crypto/aspeed/Kconfig > create mode 100644 drivers/crypto/aspeed/Makefile > create mode 100644 drivers/crypto/aspeed/aspeed-hace-hash.c > create mode 100644 drivers/crypto/aspeed/aspeed-hace.c > create mode 100644 drivers/crypto/aspeed/aspeed-hace.h Hello please see my comments below. [...] > diff --git a/drivers/crypto/aspeed/aspeed-hace-hash.c b/drivers/crypto/as= peed/aspeed-hace-hash.c > new file mode 100644 > index 000000000000..9b003e12b2c8 > --- /dev/null > +++ b/drivers/crypto/aspeed/aspeed-hace-hash.c > @@ -0,0 +1,1409 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (c) 2021 Aspeed Technology Inc. > + */ > + > +#include "aspeed-hace.h" > + > +//#define ASPEED_AHASH_DEBUG Please remove this [...] > + > +#ifdef ASPEED_AHASH_DEBUG > +#define AHASH_DBG(h, fmt, ...) \ > + dev_info((h)->dev, "%s() " fmt, __func__, ##__VA_ARGS__) > +#else > +#define AHASH_DBG(h, fmt, ...) \ > + ((void)(h)) > +#endif Why not using dev_dbg() ? [...] > + sg_len =3D dma_map_sg(hace_dev->dev, rctx->src_sg, rctx->src_nents, > + DMA_TO_DEVICE); > + if (!sg_len) { > + dev_warn(hace_dev->dev, "dma_map_sg() src error\n"); > + rc =3D -ENOMEM; > + goto end; > + } This test does handle the possible negative return of dma_map_sg() [...] > + memset(bctx->ipad + keylen, 0, bs - keylen); > + memcpy(bctx->opad, bctx->ipad, bs); > + > + for (i =3D 0; i < bs; i++) { > + bctx->ipad[i] ^=3D 0x36; > + bctx->opad[i] ^=3D 0x5c; Please use HMAC_OPAD_VALUE and HMAC_IPAD_VALUE from include/crypto/hmac.h [...] > +int aspeed_register_hace_hash_algs(struct aspeed_hace_dev *hace_dev) > +{ > + int rc, i; > + > + AHASH_DBG(hace_dev, "\n"); > + > + for (i =3D 0; i < ARRAY_SIZE(aspeed_ahash_algs); i++) { > + aspeed_ahash_algs[i].hace_dev =3D hace_dev; > + rc =3D crypto_register_ahash(&aspeed_ahash_algs[i].alg.ahash); > + if (rc) > + return rc; > + } If any hash fail to register, the function exits but you will still unregis= ter all hashes (even ones not registered) on aspeed_unregister_hace_hash_al= gs(). [...] > +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 =3D devm_kzalloc(&pdev->dev, sizeof(struct aspeed_hace_dev), > + GFP_KERNEL); > + if (!hace_dev) > + return -ENOMEM; > + > + hace_dev_id =3D 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 =3D &pdev->dev; > + hace_dev->version =3D (unsigned long)hace_dev_id->data; > + hash_engine =3D &hace_dev->hash_engine; > + > + res =3D platform_get_resource(pdev, IORESOURCE_MEM, 0); > + > + platform_set_drvdata(pdev, hace_dev); > + > + spin_lock_init(&hash_engine->lock); > + tasklet_init(&hash_engine->done_task, aspeed_hace_hash_done_task, > + (unsigned long)hace_dev); > + tasklet_init(&hash_engine->queue_task, aspeed_hace_hash_queue_task, > + (unsigned long)hace_dev); > + crypto_init_queue(&hash_engine->queue, ASPEED_HASH_QUEUE_LENGTH); > + > + hace_dev->regs =3D devm_ioremap_resource(&pdev->dev, res); > + if (!hace_dev->regs) { > + dev_err(&pdev->dev, "Failed to map resources\n"); > + return -ENOMEM; > + } > + > + hace_dev->irq =3D platform_get_irq(pdev, 0); > + if (!hace_dev->irq) { > + dev_err(&pdev->dev, "Failed to get interrupt\n"); > + return -ENXIO; > + } > + > + rc =3D 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; > + } > + > + hace_dev->clk =3D devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(hace_dev->clk)) { > + dev_err(&pdev->dev, "Failed to get clk\n"); > + return -ENODEV; > + } > + > + rc =3D clk_prepare_enable(hace_dev->clk); > + if (rc) { > + dev_err(&pdev->dev, "Failed to enable clock 0x%x\n", rc); > + return rc; > + } > + > + hash_engine->ahash_src_addr =3D > + dma_alloc_coherent(&pdev->dev, > + ASPEED_HASH_SRC_DMA_BUF_LEN, > + &hash_engine->ahash_src_dma_addr, > + GFP_KERNEL); > + if (!hash_engine->ahash_src_addr) { > + dev_err(&pdev->dev, "Failed to allocate dma buffer\n"); > + rc =3D -ENOMEM; > + goto end; > + } > + > + rc =3D aspeed_hace_register(hace_dev); > + if (rc) { > + dev_err(&pdev->dev, "Failed to register algs, rc:0x%x\n", rc); > + rc =3D 0; > + } So you print an error but you keep the driver loaded even if no hashes (or = a limited number of them) are registered, for which interest ? [...] > +struct aspeed_sham_ctx { > + struct aspeed_hace_dev *hace_dev; > + unsigned long flags; /* hmac flag */ > + > + /* fallback stuff */ > + struct aspeed_sha_hmac_ctx base[0]; I am not sure to understand the [0] purpose, and the comment said something= about fallback, but your driver dont do any real fallback. [...] > +struct aspeed_hace_dev { > + void __iomem *regs; > + void __iomem *sec_regs; sec_regs is unused. > + struct device *dev; > + int irq; > + struct clk *clk; > + unsigned long version; > + struct aspeed_engine_hash hash_engine; > +}; > + > +struct aspeed_hace_alg { > + struct aspeed_hace_dev *hace_dev; > + union { > + struct skcipher_alg skcipher; > + struct aead_alg aead; > + struct ahash_alg ahash; > + struct kpp_alg kpp; > + struct akcipher_alg akcipher; Your patch dont do any kpp or akcipher (and aead/skcipher also). Please drop this. Regards _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel