lkml.org 
[lkml]   [2017]   [Sep]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] crypto: s5p-sss: Add HASH support for Exynos
On Fri, Sep 15, 2017 at 8:10 AM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Wed, Sep 13, 2017 at 4:24 PM, Kamil Konieczny
> <k.konieczny@partner.samsung.com> wrote:
>> Hi Krzysztof,
>>
>> On 13.09.2017 15:18, Krzysztof Kozlowski wrote:
>>> On Wed, Sep 13, 2017 at 2:44 PM, Kamil Konieczny
>>> <k.konieczny@partner.samsung.com> 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 <k.konieczny@partner.samsung.com>
>>>> ---
>>>> 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

\
 
 \ /
  Last update: 2017-09-15 10:03    [W:0.046 / U:1.612 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site