Re: [PATCH] crypto: s5p-sss: Add HASH support for Exynos

[Krzysztof Kozlowski]

On Fri, Sep 15, 2017 at 8:10 AM, Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
> On Wed, Sep 13, 2017 at 4:24 PM, Kamil Konieczny
> <k.konieczny@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
>>>> ---
>>>>  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