Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <65fde5d5-6d86-c94e-9985-231e81641386@starfivetech.com>
Date:   Thu, 27 Apr 2023 13:06:49 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
Subject: Re: [PATCH v6 2/4] crypto: starfive - Add crypto engine support
To:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>
CC:     <davem@davemloft.net>, <devicetree@vger.kernel.org>,
        <herbert@gondor.apana.org.au>, <kernel@esmil.dk>,
        <krzysztof.kozlowski+dt@linaro.org>,
        <linux-crypto@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-riscv@lists.infradead.org>, <palmer@dabbelt.com>,
        <robh+dt@kernel.org>
References: <20230426065848.842221-1-jiajie.ho@starfivetech.com>
 <20230426065848.842221-3-jiajie.ho@starfivetech.com>
 <c3faff36-3698-a137-f007-ff8bca6a2e88@wanadoo.fr>
Content-Language: en-US
From:   Jia Jie Ho <jiajie.ho@starfivetech.com>
In-Reply-To: <c3faff36-3698-a137-f007-ff8bca6a2e88@wanadoo.fr>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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@public.gmane.org>
>> Signed-off-by: Huan Feng <huan.feng-bONrM45KWFOXmMXjJBpWqg@public.gmane.org>
>> Signed-off-by: Jia Jie Ho <jiajie.ho-bONrM45KWFOXmMXjJBpWqg@public.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