| Message ID | 20220830093439.951960-1-koba.ko@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | crypto: ccp - Release dma channels before dmaengine unrgister | expand |
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);
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;
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);
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);
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);
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(-)