From: Krzysztof Kozlowski Subject: Re: [PATCH] crypto: s5p-sss: Add HASH support for Exynos Date: Fri, 15 Sep 2017 10:02:40 +0200 Message-ID: References: <51b3b386-22c0-7663-6f54-d166ebebd332@partner.samsung.com> <77dbef9a-a648-8380-ca08-e6874b4d27fb@partner.samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: linux-crypto@vger.kernel.org, Herbert Xu , Vladimir Zapolskiy , linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org, Bartlomiej Zolnierkiewicz To: Kamil Konieczny Return-path: Received: from mail.kernel.org ([198.145.29.99]:34150 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750787AbdIOICm (ORCPT ); Fri, 15 Sep 2017 04:02:42 -0400 In-Reply-To: Sender: linux-crypto-owner@vger.kernel.org List-ID: On Fri, Sep 15, 2017 at 8:10 AM, Krzysztof Kozlowski wrote: > On Wed, Sep 13, 2017 at 4:24 PM, Kamil Konieczny > wrote: >> Hi Krzysztof, >> >> On 13.09.2017 15:18, Krzysztof Kozlowski wrote: >>> On Wed, Sep 13, 2017 at 2:44 PM, Kamil Konieczny >>> wrote: >>>> Add support for MD5, SHA1, SHA256 hash algorithms for Exynos HW. >>>> It uses the crypto framework asynchronous hash api. >>>> It is based on omap-sham.c driver. >>>> S5P has some HW differencies and is not implemented. >>>> >>>> Modifications in s5p-sss: >>>> >>>> - Add hash supporting structures and functions. >>>> >>>> - Modify irq handler to handle both aes and hash signals. >>>> >>>> - Disable HASH in probe if Exynos PRNG is enabled. >>>> >>>> - Add new copyright line and new author. >>>> >>>> - Tested on Odroid-U3 with Exynos 4412 CPU, kernel 4.13-rc6 >>>> with crypto run-time self test testmgr >>>> and with tcrypt module with: modprobe tcrypt sec=1 mode=N >>>> where N=402, 403, 404 (MD5, SHA1, SHA256). >>>> >>>> Modifications in drivers/crypto/Kconfig: >>>> >>>> - Select sw algorithms MD5, SHA1 and SHA256 in S5P >>>> as they are nedded for fallback. >>>> >>>> Signed-off-by: Kamil Konieczny >>>> --- >>>> drivers/crypto/Kconfig | 6 + >>>> drivers/crypto/s5p-sss.c | 2062 +++++++++++++++++++++++++++++++++++++++++++--- >>>> 2 files changed, 1939 insertions(+), 129 deletions(-) >>>> >>> >>> Nice work, thanks! >>> >>> You need to split the patch, it is just too huge making it very >>> difficult to review. Please split it per logically sensible >>> improvements. >> >> Can you suggest how to break it up ?> >> It is now big update, added working functionallity in one piece, >> but I agree it can be hard to read >> as git did some strange things, >> like delete few aes functions (and mixing this delete with '+' lines) >> and then adding the same aes without any change. > > You know the changes you want to do, you know the new architecture so > usually it is easier for you to figure out the split. > But few ideas: > 1. For the problem of functions being deleted-moved without any > changes, you can try to reorder new code so diff will not show these > hunks. Indeed a lot diff here looks like weird matching of diff/patch. > This definitely has to be fixed because I cannot figure out the real > changes around existing functions (e.g. s5p_set_indata_start(), > s5p_aes_crypt_start()). > 2. You add debugging code - FLOW_LOG - this is one candidate to split entirely. > 3. Cleanups go to separate patch. > > Probably working on (1) above will bring the most benefit. BTW, using some stable git release (not latest master) might help as well... BR, Krzysztof