linux-kernel - Re: [PATCH v6 2/4] crypto: starfive

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <65fde5d5-6d86-c94e-9985-231e81641386@starfivetech.com>
Date:   Thu, 27 Apr 2023 13:06:49 +0800
From:   Jia Jie Ho <jiajie.ho@...rfivetech.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
CC:     <davem@...emloft.net>, <devicetree@...r.kernel.org>,
        <herbert@...dor.apana.org.au>, <kernel@...il.dk>,
        <krzysztof.kozlowski+dt@...aro.org>,
        <linux-crypto@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-riscv@...ts.infradead.org>, <palmer@...belt.com>,
        <robh+dt@...nel.org>
Subject: Re: [PATCH v6 2/4] crypto: starfive - Add crypto engine support

On 26/4/2023 3:56 pm, Christophe JAILLET wrote:
> Le 26/04/2023 à 08:58, Jia Jie Ho a écrit :
>> Adding device probe and DMA init for StarFive cryptographic module.
>>
>> Co-developed-by: Huan Feng <huan.feng-bONrM45KWFOXmMXjJBpWqg@...lic.gmane.org>
>> Signed-off-by: Huan Feng <huan.feng-bONrM45KWFOXmMXjJBpWqg@...lic.gmane.org>
>> Signed-off-by: Jia Jie Ho <jiajie.ho-bONrM45KWFOXmMXjJBpWqg@...lic.gmane.org>
>> ---
>>   MAINTAINERS                           |   7 +
>>   drivers/crypto/Kconfig                |   1 +
>>   drivers/crypto/Makefile               |   1 +
>>   drivers/crypto/starfive/Kconfig       |  17 +++
>>   drivers/crypto/starfive/Makefile      |   4 +
>>   drivers/crypto/starfive/jh7110-cryp.c | 199 ++++++++++++++++++++++++++
>>   drivers/crypto/starfive/jh7110-cryp.h |  63 ++++++++
>>   7 files changed, 292 insertions(+)
>>   create mode 100644 drivers/crypto/starfive/Kconfig
>>   create mode 100644 drivers/crypto/starfive/Makefile
>>   create mode 100644 drivers/crypto/starfive/jh7110-cryp.c
>>   create mode 100644 drivers/crypto/starfive/jh7110-cryp.h
>>

[...]

>> +
>> +static int starfive_dma_init(struct starfive_cryp_dev *cryp)
>> +{
>> +    dma_cap_mask_t mask;
>> +
>> +    cryp->tx = NULL;
>> +    cryp->rx = NULL;
> 
> Harmless, but 'crypt' is kzalloc()'ed, so these fields are already NULL.
>

Hi Christophe,
I'll remove these in the next version.
 
>> +
>> +    dma_cap_zero(mask);
>> +    dma_cap_set(DMA_SLAVE, mask);
>> +
>> +    cryp->tx = dma_request_chan(cryp->dev, "tx");
>> +    if (IS_ERR(cryp->tx))
>> +        return dev_err_probe(cryp->dev, PTR_ERR(cryp->tx),
>> +                     "Error requesting tx dma channel.\n");
>> +
>> +    cryp->rx = dma_request_chan(cryp->dev, "rx");
>> +    if (IS_ERR(cryp->rx)) {
>> +        dma_release_channel(cryp->tx);
>> +        return dev_err_probe(cryp->dev, PTR_ERR(cryp->rx),
>> +                     "Error requesting rx dma channel.\n");
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void starfive_dma_cleanup(struct starfive_cryp_dev *cryp)
>> +{
>> +    dma_release_channel(cryp->tx);
>> +    dma_release_channel(cryp->rx);
>> +}
>> +
>> +static int starfive_cryp_probe(struct platform_device *pdev)
>> +{

[...]

>> +
>> +    ret = crypto_engine_start(cryp->engine);
>> +    if (ret)
>> +        goto err_engine_start;
>> +
>> +    return 0;
>> +
>> +err_engine_start:
>> +    crypto_engine_exit(cryp->engine);
>> +err_engine:
>> +    starfive_dma_cleanup(cryp);
>> +err_dma_init:
>> +    spin_lock(&dev_list.lock);
>> +    list_del(&cryp->list);
>> +    spin_unlock(&dev_list.lock);
> 
> I think that there should be:
>     clk_disable_unprepare(cryp->hclk);
>     clk_disable_unprepare(cryp->ahb);
>     reset_control_assert(cryp->rst);
> 
> as in the remove function.
> 

Will add these in next version.

>> +
>> +    return ret;
>> +}
>> +
>> +static int starfive_cryp_remove(struct platform_device *pdev)
>> +{
>> +    struct starfive_cryp_dev *cryp = platform_get_drvdata(pdev);
>> +
>> +    if (!cryp)
>> +        return -ENODEV;
> 
> I don't think that this can happen.
> 

I'll update this too along with your other comments.
Thanks for taking time reviewing this patch series.

Best regards,
Jia Jie