Query on dmaengine: add support to dynamic register/unregister of channels patch

Query on dmaengine: add support to dynamic register/unregister of channels patch

[Radhey Shyam Pandey]

Hi Dave,

I have a query on below patch (commit-e81274cd6b52). It causes regression
in xilinx_dma driver and there is a reported kernel crash on rmmod.

commit e81274cd6b5264809384066e09a5253708822522
Author: Dave Jiang <dave.jiang@intel.com>
Date:   Tue Jan 21 16:43:53 2020 -0700

dmaengine: add support to dynamic register/unregister of channels
    
With the channel registration routines broken out, now add support code to
allow independent registering and unregistering of channels in a hotplug 
fashion.

Crash on the unloading xilinx_dma module is reproducible after e81274cd6b526
mainline commit is added in the 5.6 kernel.

[   42.142705] Internal error: Oops: 96000044 [#1] SMP
[   42.147566] Modules linked in: xilinx_dma(-) clk_xlnx_clock_wizard
uio_pdrv_genirq
[   42.155139] CPU: 1 PID: 2075 Comm: rmmod Not tainted
5.10.1-00026-g3a2e6dd7a05-dirty #192
[   42.163302] Hardware name: Enclustra XU5 SOM (DT)
[   42.167992] pstate: 40000005 (nZcv daif -PAN -UAO -TCO BTYPE=--)
[   42.173996] pc : xilinx_dma_chan_remove+0x74/0xa0 [xilinx_dma]
[   42.179815] lr : xilinx_dma_chan_remove+0x70/0xa0 [xilinx_dma]
[   42.185636] sp : ffffffc01112bca0
[   42.188935] x29: ffffffc01112bca0 x28: ffffff80402ea640


xilinx_dma_chan_remove+0x74/0xa0:
__list_del at ./include/linux/list.h:112 (inlined by)
 __list_del_entry at./include/linux/list.h:135 (inlined by)
list_del at ./include/linux/list.h:146 (inlined by)
xilinx_dma_chan_remove at drivers/dma/xilinx/xilinx_dma.c:2546

Looking into e81274cd6b526 commit - It deletes channel device_node entry.
Same channel device_node entry is also deleted in 
xilinx_dma_chan_remove as a result we see this crash.

@@ -993,12 +1007,22 @@ static
void __dma_async_device_channel_unregister(struct dma_device *device,
                  "%s called while %d clients hold a reference\n",
                  __func__, chan->client_count);
        mutex_lock(&dma_list_mutex);
+       list_del(&chan->device_node);
+       device->chancnt--;

I want to know some background for this change.  In dmaengine driver -
we are adding channel device node entry so deleting it in the exit 
the path should be done in dmaengine driver and not in _channel_unregister?

NOTE:  This issue is reported in https://www.spinics.net/lists/dmaengine/msg24923.html

Thanks,
Radhey

Re: Query on dmaengine: add support to dynamic register/unregister of channels patch

[Dave Jiang]

On 1/18/2021 12:54 AM, Radhey Shyam Pandey wrote:
> Hi Dave,
>
> I have a query on below patch (commit-e81274cd6b52). It causes regression
> in xilinx_dma driver and there is a reported kernel crash on rmmod.
>
> commit e81274cd6b5264809384066e09a5253708822522
> Author: Dave Jiang <dave.jiang@intel.com>
> Date:   Tue Jan 21 16:43:53 2020 -0700
>
> dmaengine: add support to dynamic register/unregister of channels
>      
> With the channel registration routines broken out, now add support code to
> allow independent registering and unregistering of channels in a hotplug
> fashion.
>
> Crash on the unloading xilinx_dma module is reproducible after e81274cd6b526
> mainline commit is added in the 5.6 kernel.
>
> [   42.142705] Internal error: Oops: 96000044 [#1] SMP
> [   42.147566] Modules linked in: xilinx_dma(-) clk_xlnx_clock_wizard
> uio_pdrv_genirq
> [   42.155139] CPU: 1 PID: 2075 Comm: rmmod Not tainted
> 5.10.1-00026-g3a2e6dd7a05-dirty #192
> [   42.163302] Hardware name: Enclustra XU5 SOM (DT)
> [   42.167992] pstate: 40000005 (nZcv daif -PAN -UAO -TCO BTYPE=--)
> [   42.173996] pc : xilinx_dma_chan_remove+0x74/0xa0 [xilinx_dma]
> [   42.179815] lr : xilinx_dma_chan_remove+0x70/0xa0 [xilinx_dma]
> [   42.185636] sp : ffffffc01112bca0
> [   42.188935] x29: ffffffc01112bca0 x28: ffffff80402ea640
>
>
> xilinx_dma_chan_remove+0x74/0xa0:
> __list_del at ./include/linux/list.h:112 (inlined by)
>   __list_del_entry at./include/linux/list.h:135 (inlined by)
> list_del at ./include/linux/list.h:146 (inlined by)
> xilinx_dma_chan_remove at drivers/dma/xilinx/xilinx_dma.c:2546
>
> Looking into e81274cd6b526 commit - It deletes channel device_node entry.
> Same channel device_node entry is also deleted in
> xilinx_dma_chan_remove as a result we see this crash.
>
> @@ -993,12 +1007,22 @@ static
> void __dma_async_device_channel_unregister(struct dma_device *device,
>                    "%s called while %d clients hold a reference\n",
>                    __func__, chan->client_count);
>          mutex_lock(&dma_list_mutex);
> +       list_del(&chan->device_node);
> +       device->chancnt--;
>
> I want to know some background for this change.  In dmaengine driver -
> we are adding channel device node entry so deleting it in the exit
> the path should be done in dmaengine driver and not in _channel_unregister?

Yes you are correct. It seems we need to remove that and push the 
management to the driver as that seems to be the way. The chancnt will 
continue to be managed by dmaengine. I'll create a fix.


>
> NOTE:  This issue is reported in https://www.spinics.net/lists/dmaengine/msg24923.html
>
> Thanks,
> Radhey