crypto: ccp - Release dma channels before dmaengine unrgister

crypto: ccp - Release dma channels before dmaengine unrgister

[Koba Ko]

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);
 
 	kmem_cache_destroy(ccp->dma_desc_cache);
 	kmem_cache_destroy(ccp->dma_cmd_cache);
-- 
2.25.1

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

[Tom Lendacky]

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?

Thanks,
Tom

>   
>   	kmem_cache_destroy(ccp->dma_desc_cache);
>   	kmem_cache_destroy(ccp->dma_cmd_cache);

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

[kernel test robot]

Hi Koba,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on herbert-cryptodev-2.6/master]
[also build test WARNING on herbert-crypto-2.6/master linus/master v6.0-rc3 next-20220830]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Koba-Ko/crypto-ccp-Release-dma-channels-before-dmaengine-unrgister/20220830-173803
base:   https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20220831/202208310843.cesiRP88-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/5aa373b58528f3f99c5a010e76728776f0240603
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Koba-Ko/crypto-ccp-Release-dma-channels-before-dmaengine-unrgister/20220830-173803
        git checkout 5aa373b58528f3f99c5a010e76728776f0240603
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/crypto/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/crypto/ccp/ccp-dmaengine.c: In function 'ccp_dmaengine_unregister':
>> drivers/crypto/ccp/ccp-dmaengine.c:770:22: warning: unused variable 'i' [-Wunused-variable]
     770 |         unsigned int i;
         |                      ^
>> drivers/crypto/ccp/ccp-dmaengine.c:769:26: warning: unused variable 'dma_chan' [-Wunused-variable]
     769 |         struct dma_chan *dma_chan;
         |                          ^~~~~~~~


vim +/i +770 drivers/crypto/ccp/ccp-dmaengine.c

   765	
   766	void ccp_dmaengine_unregister(struct ccp_device *ccp)
   767	{
   768		struct dma_device *dma_dev = &ccp->dma_dev;
 > 769		struct dma_chan *dma_chan;
 > 770		unsigned int i;

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[Koba Ko]

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.


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

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

[Tom Lendacky]

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);