Re: crypto: ccp - Release dma channels before dmaengine unrgister

[Tom Lendacky <thomas.lendacky@amd.com>]

On 8/31/22 00:39, Koba Ko wrote:
> On Tue, Aug 30, 2022 at 11:32 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 8/30/22 04:34, Koba Ko wrote:
>>> A warning is shown during shutdown,
>>>
>>> __dma_async_device_channel_unregister called while 2 clients hold a reference
>>> WARNING: CPU: 15 PID: 1 at drivers/dma/dmaengine.c:1110
>>> __dma_async_device_channel_unregister
>>>
>>> Call dma_release_channel for occupied channles
>>> before dma_async_device_unregister.
>>>
>>> Fixes: 4cbe9bc34ed0 ("crypto: ccp - ccp_dmaengine_unregister release dma channels")
>>> Signed-off-by: Koba Ko <koba.ko@canonical.com>
>>> ---
>>>    drivers/crypto/ccp/ccp-dmaengine.c | 8 +++++++-
>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/crypto/ccp/ccp-dmaengine.c b/drivers/crypto/ccp/ccp-dmaengine.c
>>> index 7d4b4ad1db1f3..ba3eb1ac3b55d 100644
>>> --- a/drivers/crypto/ccp/ccp-dmaengine.c
>>> +++ b/drivers/crypto/ccp/ccp-dmaengine.c
>>> @@ -641,6 +641,10 @@ static void ccp_dma_release(struct ccp_device *ccp)
>>>        for (i = 0; i < ccp->cmd_q_count; i++) {
>>>                chan = ccp->ccp_dma_chan + i;
>>>                dma_chan = &chan->dma_chan;
>>> +
>>> +             if (dma_chan->client_count)
>>> +                     dma_release_channel(dma_chan);
>>> +
>>>                tasklet_kill(&chan->cleanup_tasklet);
>>>                list_del_rcu(&dma_chan->device_node);
>>>        }
>>> @@ -762,12 +766,14 @@ int ccp_dmaengine_register(struct ccp_device *ccp)
>>>    void ccp_dmaengine_unregister(struct ccp_device *ccp)
>>>    {
>>>        struct dma_device *dma_dev = &ccp->dma_dev;
>>> +     struct dma_chan *dma_chan;
>>> +     unsigned int i;
>>>
>>>        if (!dmaengine)
>>>                return;
>>>
>>> -     dma_async_device_unregister(dma_dev);
>>>        ccp_dma_release(ccp);
>>> +     dma_async_device_unregister(dma_dev);
>>
>> Seems the right way to get rid of the warn, but can there still be a race
>> where the channel is grabbed again after ccp_dma_release() and before
>> dma_async_device_unregister() is called?
> 
> After tracing the ccp driver,
> dma_device is held by the ccp device.
> the only function called ccp_dmaengine_register is ccp5_init/ccp_init,
> ccp_dma_release is called in ccp_dmaengine_unregister/ccp_dmaengine_register,
> so there's no race condition in current codes.

I'm referring to some other piece of software claiming the channel, like 
the raid driver or the AMD NTB driver. This just seems racy to me, but it 
does address the WARNing.

Clean up the kernel test robot complaints and submit a v2.

Thanks,
Tom

> 
> 
>>
>> Thanks,
>> Tom
>>
>>>
>>>        kmem_cache_destroy(ccp->dma_desc_cache);
>>>        kmem_cache_destroy(ccp->dma_cmd_cache);