diff mbox series

dmaengine: xilinx: dpdma: request_irq after initializing dma channels

Message ID 20210429065821.3495234-1-quanyang.wang@windriver.com (mailing list archive)
State New
Headers show
Series dmaengine: xilinx: dpdma: request_irq after initializing dma channels | expand

Commit Message

quanyang.wang April 29, 2021, 6:58 a.m. UTC
From: Quanyang Wang <quanyang.wang@windriver.com>

In some scenarios (kdump), dpdma hardware irqs has been enabled when
calling request_irq in probe function, and then the dpdma irq handler
xilinx_dpdma_irq_handler is invoked to access xdev->chan[i]. But at
this moment xdev->chan[i] hasn't been initialized. So let's call the
request_irq after initializing dma channels to avoid kdump kernel
crash as below:

[    3.696128] Unable to handle kernel NULL pointer dereference at virtual address 000000000000012c
[    3.696710] xilinx-zynqmp-dpdma fd4c0000.dma-controller: Xilinx DPDMA engine is probed
[    3.704900] Mem abort info:
[    3.704902]   ESR = 0x96000005
[    3.704905]   EC = 0x25: DABT (current EL), IL = 32 bits
[    3.704907]   SET = 0, FnV = 0
[    3.704912]   EA = 0, S1PTW = 0
[    3.713800] ahci-ceva fd0c0000.ahci: supply ahci not found, using dummy regulator
[    3.715585] Data abort info:
[    3.715587]   ISV = 0, ISS = 0x00000005
[    3.715589]   CM = 0, WnR = 0
[    3.715592] [000000000000012c] user address but active_mm is swapper
[    3.715596] Internal error: Oops: 96000005 [#1] SMP
[    3.715599] Modules linked in:
[    3.715608] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.10.0-12170-g60894882155f-dirty #77
[    3.723937] Hardware name: ZynqMP ZCU102 Rev1.0 (DT)
[    3.723942] pstate: 80000085 (Nzcv daIf -PAN -UAO -TCO BTYPE=--)
[    3.723956] pc : xilinx_dpdma_irq_handler+0x418/0x560
[    3.793049] lr : xilinx_dpdma_irq_handler+0x3d8/0x560
[    3.798089] sp : ffffffc01186bdf0
[    3.801388] x29: ffffffc01186bdf0 x28: ffffffc011836f28
[    3.806692] x27: ffffff8023e0ac80 x26: 0000000000000080
[    3.811996] x25: 0000000008000408 x24: 0000000000000003
[    3.817300] x23: ffffffc01186be70 x22: ffffffc011291740
[    3.822604] x21: 0000000000000000 x20: 0000000008000408
[    3.827908] x19: 0000000000000000 x18: 0000000000000010
[    3.833212] x17: 0000000000000000 x16: 0000000000000000
[    3.838516] x15: 0000000000000000 x14: ffffffc011291740
[    3.843820] x13: ffffffc02eb4d000 x12: 0000000034d4d91d
[    3.849124] x11: 0000000000000040 x10: ffffffc0112d2d48
[    3.854428] x9 : ffffffc0112d2d40 x8 : ffffff8021c00268
[    3.859732] x7 : 0000000000000000 x6 : ffffffc011836000
[    3.865036] x5 : 0000000000000003 x4 : 0000000000000000
[    3.870340] x3 : 0000000000000001 x2 : 0000000000000000
[    3.875644] x1 : 0000000000000000 x0 : 000000000000012c
[    3.880948] Call trace:
[    3.883382]  xilinx_dpdma_irq_handler+0x418/0x560
[    3.888079]  __handle_irq_event_percpu+0x5c/0x178
[    3.892774]  handle_irq_event_percpu+0x34/0x98
[    3.897210]  handle_irq_event+0x44/0xb8
[    3.901030]  handle_fasteoi_irq+0xd0/0x190
[    3.905117]  generic_handle_irq+0x30/0x48
[    3.909111]  __handle_domain_irq+0x64/0xc0
[    3.913192]  gic_handle_irq+0x78/0xa0
[    3.916846]  el1_irq+0xc4/0x180
[    3.919982]  cpuidle_enter_state+0x134/0x2f8
[    3.924243]  cpuidle_enter+0x38/0x50
[    3.927810]  call_cpuidle+0x1c/0x40
[    3.931290]  do_idle+0x20c/0x270
[    3.934502]  cpu_startup_entry+0x28/0x58
[    3.938410]  rest_init+0xbc/0xcc
[    3.941631]  arch_call_rest_init+0x10/0x1c
[    3.945718]  start_kernel+0x51c/0x558

Fixes: 7cbb0c63de3f ("dmaengine: xilinx: dpdma: Add the Xilinx DisplayPort DMA engine driver")
Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
---
 drivers/dma/xilinx/xilinx_dpdma.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

Comments

Dan Carpenter April 29, 2021, 1:03 p.m. UTC | #1
Hi,

url:    https://github.com/0day-ci/linux/commits/quanyang-wang-windriver-com/dmaengine-xilinx-dpdma-request_irq-after-initializing-dma-channels/20210429-150050 
base:   https://github.com/Xilinx/linux-xlnx  master
config: x86_64-randconfig-m001-20210429 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/dma/xilinx/xilinx_dpdma.c:1778 xilinx_dpdma_probe() warn: missing error code 'ret'

vim +/ret +1778 drivers/dma/xilinx/xilinx_dpdma.c

7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1694  static int xilinx_dpdma_probe(struct platform_device *pdev)
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1695  {
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1696  	struct xilinx_dpdma_device *xdev;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1697  	struct dma_device *ddev;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1698  	unsigned int i;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1699  	int ret;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1700  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1701  	xdev = devm_kzalloc(&pdev->dev, sizeof(*xdev), GFP_KERNEL);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1702  	if (!xdev)
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1703  		return -ENOMEM;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1704  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1705  	xdev->dev = &pdev->dev;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1706  	xdev->ext_addr = sizeof(dma_addr_t) > 4;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1707  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1708  	INIT_LIST_HEAD(&xdev->common.channels);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1709  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1710  	platform_set_drvdata(pdev, xdev);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1711  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1712  	xdev->axi_clk = devm_clk_get(xdev->dev, "axi_clk");
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1713  	if (IS_ERR(xdev->axi_clk))
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1714  		return PTR_ERR(xdev->axi_clk);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1715  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1716  	xdev->reg = devm_platform_ioremap_resource(pdev, 0);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1717  	if (IS_ERR(xdev->reg))
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1718  		return PTR_ERR(xdev->reg);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1719  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1720  	ddev = &xdev->common;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1721  	ddev->dev = &pdev->dev;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1722  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1723  	dma_cap_set(DMA_SLAVE, ddev->cap_mask);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1724  	dma_cap_set(DMA_PRIVATE, ddev->cap_mask);
b305dd098dae4f Rohit Visavalia  2021-04-07  1725  	dma_cap_set(DMA_CYCLIC, ddev->cap_mask);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1726  	dma_cap_set(DMA_INTERLEAVE, ddev->cap_mask);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1727  	dma_cap_set(DMA_REPEAT, ddev->cap_mask);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1728  	dma_cap_set(DMA_LOAD_EOT, ddev->cap_mask);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1729  	ddev->copy_align = fls(XILINX_DPDMA_ALIGN_BYTES - 1);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1730  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1731  	ddev->device_alloc_chan_resources = xilinx_dpdma_alloc_chan_resources;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1732  	ddev->device_free_chan_resources = xilinx_dpdma_free_chan_resources;
b305dd098dae4f Rohit Visavalia  2021-04-07  1733  	ddev->device_prep_dma_cyclic = xilinx_dpdma_prep_dma_cyclic;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1734  	ddev->device_prep_interleaved_dma = xilinx_dpdma_prep_interleaved_dma;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1735  	/* TODO: Can we achieve better granularity ? */
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1736  	ddev->device_tx_status = dma_cookie_status;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1737  	ddev->device_issue_pending = xilinx_dpdma_issue_pending;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1738  	ddev->device_config = xilinx_dpdma_config;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1739  	ddev->device_pause = xilinx_dpdma_pause;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1740  	ddev->device_resume = xilinx_dpdma_resume;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1741  	ddev->device_terminate_all = xilinx_dpdma_terminate_all;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1742  	ddev->device_synchronize = xilinx_dpdma_synchronize;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1743  	ddev->src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1744  	ddev->directions = BIT(DMA_MEM_TO_DEV);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1745  	ddev->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1746  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1747  	for (i = 0; i < ARRAY_SIZE(xdev->chan); ++i) {
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1748  		ret = xilinx_dpdma_chan_init(xdev, i);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1749  		if (ret < 0) {
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1750  			dev_err(xdev->dev, "failed to initialize channel %u\n",
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1751  				i);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1752  			goto error;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1753  		}
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1754  	}
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1755  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1756  	ret = clk_prepare_enable(xdev->axi_clk);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1757  	if (ret) {
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1758  		dev_err(xdev->dev, "failed to enable the axi clock\n");
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1759  		goto error;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1760  	}
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1761  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1762  	ret = dma_async_device_register(ddev);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1763  	if (ret) {
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1764  		dev_err(xdev->dev, "failed to register the dma device\n");
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1765  		goto error_dma_async;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1766  	}
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1767  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1768  	ret = of_dma_controller_register(xdev->dev->of_node,
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1769  					 of_dma_xilinx_xlate, ddev);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1770  	if (ret) {
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1771  		dev_err(xdev->dev, "failed to register DMA to DT DMA helper\n");
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1772  		goto error_of_dma;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1773  	}
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1774  
50d6be0a1d25af Quanyang Wang    2021-04-29  1775  	xdev->irq = platform_get_irq(pdev, 0);
50d6be0a1d25af Quanyang Wang    2021-04-29  1776  	if (xdev->irq < 0) {
50d6be0a1d25af Quanyang Wang    2021-04-29  1777  		dev_err(xdev->dev, "failed to get platform irq\n");
50d6be0a1d25af Quanyang Wang    2021-04-29 @1778  		goto error_irq;

ret = xdev->irq;

50d6be0a1d25af Quanyang Wang    2021-04-29  1779  	}
50d6be0a1d25af Quanyang Wang    2021-04-29  1780  
50d6be0a1d25af Quanyang Wang    2021-04-29  1781  	ret = request_irq(xdev->irq, xilinx_dpdma_irq_handler, IRQF_SHARED,
50d6be0a1d25af Quanyang Wang    2021-04-29  1782  			  dev_name(xdev->dev), xdev);
50d6be0a1d25af Quanyang Wang    2021-04-29  1783  	if (ret) {
50d6be0a1d25af Quanyang Wang    2021-04-29  1784  		dev_err(xdev->dev, "failed to request IRQ\n");
50d6be0a1d25af Quanyang Wang    2021-04-29  1785  		goto error_irq;
50d6be0a1d25af Quanyang Wang    2021-04-29  1786  	}
50d6be0a1d25af Quanyang Wang    2021-04-29  1787  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1788  	xilinx_dpdma_enable_irq(xdev);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1789  
1d220435cab3bf Laurent Pinchart 2020-08-12  1790  	xilinx_dpdma_debugfs_init(xdev);
1d220435cab3bf Laurent Pinchart 2020-08-12  1791  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1792  	dev_info(&pdev->dev, "Xilinx DPDMA engine is probed\n");
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1793  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1794  	return 0;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1795  
50d6be0a1d25af Quanyang Wang    2021-04-29  1796  error_irq:
50d6be0a1d25af Quanyang Wang    2021-04-29  1797  	of_dma_controller_free(pdev->dev.of_node);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1798  error_of_dma:
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1799  	dma_async_device_unregister(ddev);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1800  error_dma_async:
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1801  	clk_disable_unprepare(xdev->axi_clk);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1802  error:
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1803  	for (i = 0; i < ARRAY_SIZE(xdev->chan); i++)
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1804  		xilinx_dpdma_chan_remove(xdev->chan[i]);
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1805  
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1806  	return ret;
7cbb0c63de3fc2 Hyun Kwon        2020-07-17  1807  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Laurent Pinchart April 30, 2021, 12:36 a.m. UTC | #2
Hi Quanyang,

Thank you for the patch.

On Thu, Apr 29, 2021 at 02:58:21PM +0800, quanyang.wang@windriver.com wrote:
> From: Quanyang Wang <quanyang.wang@windriver.com>
> 
> In some scenarios (kdump), dpdma hardware irqs has been enabled when
> calling request_irq in probe function, and then the dpdma irq handler
> xilinx_dpdma_irq_handler is invoked to access xdev->chan[i]. But at
> this moment xdev->chan[i] hasn't been initialized. So let's call the
> request_irq after initializing dma channels to avoid kdump kernel
> crash as below:
> 
> [    3.696128] Unable to handle kernel NULL pointer dereference at virtual address 000000000000012c
> [    3.696710] xilinx-zynqmp-dpdma fd4c0000.dma-controller: Xilinx DPDMA engine is probed
> [    3.704900] Mem abort info:
> [    3.704902]   ESR = 0x96000005
> [    3.704905]   EC = 0x25: DABT (current EL), IL = 32 bits
> [    3.704907]   SET = 0, FnV = 0
> [    3.704912]   EA = 0, S1PTW = 0
> [    3.713800] ahci-ceva fd0c0000.ahci: supply ahci not found, using dummy regulator
> [    3.715585] Data abort info:
> [    3.715587]   ISV = 0, ISS = 0x00000005
> [    3.715589]   CM = 0, WnR = 0
> [    3.715592] [000000000000012c] user address but active_mm is swapper
> [    3.715596] Internal error: Oops: 96000005 [#1] SMP
> [    3.715599] Modules linked in:
> [    3.715608] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.10.0-12170-g60894882155f-dirty #77
> [    3.723937] Hardware name: ZynqMP ZCU102 Rev1.0 (DT)
> [    3.723942] pstate: 80000085 (Nzcv daIf -PAN -UAO -TCO BTYPE=--)
> [    3.723956] pc : xilinx_dpdma_irq_handler+0x418/0x560
> [    3.793049] lr : xilinx_dpdma_irq_handler+0x3d8/0x560
> [    3.798089] sp : ffffffc01186bdf0
> [    3.801388] x29: ffffffc01186bdf0 x28: ffffffc011836f28
> [    3.806692] x27: ffffff8023e0ac80 x26: 0000000000000080
> [    3.811996] x25: 0000000008000408 x24: 0000000000000003
> [    3.817300] x23: ffffffc01186be70 x22: ffffffc011291740
> [    3.822604] x21: 0000000000000000 x20: 0000000008000408
> [    3.827908] x19: 0000000000000000 x18: 0000000000000010
> [    3.833212] x17: 0000000000000000 x16: 0000000000000000
> [    3.838516] x15: 0000000000000000 x14: ffffffc011291740
> [    3.843820] x13: ffffffc02eb4d000 x12: 0000000034d4d91d
> [    3.849124] x11: 0000000000000040 x10: ffffffc0112d2d48
> [    3.854428] x9 : ffffffc0112d2d40 x8 : ffffff8021c00268
> [    3.859732] x7 : 0000000000000000 x6 : ffffffc011836000
> [    3.865036] x5 : 0000000000000003 x4 : 0000000000000000
> [    3.870340] x3 : 0000000000000001 x2 : 0000000000000000
> [    3.875644] x1 : 0000000000000000 x0 : 000000000000012c
> [    3.880948] Call trace:
> [    3.883382]  xilinx_dpdma_irq_handler+0x418/0x560
> [    3.888079]  __handle_irq_event_percpu+0x5c/0x178
> [    3.892774]  handle_irq_event_percpu+0x34/0x98
> [    3.897210]  handle_irq_event+0x44/0xb8
> [    3.901030]  handle_fasteoi_irq+0xd0/0x190
> [    3.905117]  generic_handle_irq+0x30/0x48
> [    3.909111]  __handle_domain_irq+0x64/0xc0
> [    3.913192]  gic_handle_irq+0x78/0xa0
> [    3.916846]  el1_irq+0xc4/0x180
> [    3.919982]  cpuidle_enter_state+0x134/0x2f8
> [    3.924243]  cpuidle_enter+0x38/0x50
> [    3.927810]  call_cpuidle+0x1c/0x40
> [    3.931290]  do_idle+0x20c/0x270
> [    3.934502]  cpu_startup_entry+0x28/0x58
> [    3.938410]  rest_init+0xbc/0xcc
> [    3.941631]  arch_call_rest_init+0x10/0x1c
> [    3.945718]  start_kernel+0x51c/0x558
> 
> Fixes: 7cbb0c63de3f ("dmaengine: xilinx: dpdma: Add the Xilinx DisplayPort DMA engine driver")
> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
> ---
>  drivers/dma/xilinx/xilinx_dpdma.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c
> index 70b29bd079c9..0b599402c53f 100644
> --- a/drivers/dma/xilinx/xilinx_dpdma.c
> +++ b/drivers/dma/xilinx/xilinx_dpdma.c
> @@ -1622,19 +1622,6 @@ static int xilinx_dpdma_probe(struct platform_device *pdev)
>  	if (IS_ERR(xdev->reg))
>  		return PTR_ERR(xdev->reg);
>  
> -	xdev->irq = platform_get_irq(pdev, 0);
> -	if (xdev->irq < 0) {
> -		dev_err(xdev->dev, "failed to get platform irq\n");
> -		return xdev->irq;
> -	}
> -
> -	ret = request_irq(xdev->irq, xilinx_dpdma_irq_handler, IRQF_SHARED,
> -			  dev_name(xdev->dev), xdev);
> -	if (ret) {
> -		dev_err(xdev->dev, "failed to request IRQ\n");
> -		return ret;
> -	}
> -
>  	ddev = &xdev->common;
>  	ddev->dev = &pdev->dev;
>  
> @@ -1688,6 +1675,19 @@ static int xilinx_dpdma_probe(struct platform_device *pdev)
>  		goto error_of_dma;
>  	}
>  
> +	xdev->irq = platform_get_irq(pdev, 0);
> +	if (xdev->irq < 0) {
> +		dev_err(xdev->dev, "failed to get platform irq\n");

As reported by the kbuild bot, ret isn't initialized here.

> +		goto error_irq;
> +	}

This part could stay where it was, just after
devm_platform_ioremap_resource().

> +
> +	ret = request_irq(xdev->irq, xilinx_dpdma_irq_handler, IRQF_SHARED,
> +			  dev_name(xdev->dev), xdev);
> +	if (ret) {
> +		dev_err(xdev->dev, "failed to request IRQ\n");
> +		goto error_irq;
> +	}

Ideally we should reset the device before requesting the IRQ, to ensure
we start in a consistent state. There doesn't seem to be any global
reset unfortunately. Shouldn't we at least disable interrupts ? It may
be a bit pointless though as we reenable them right below, so I suppose
this would be OK.

> +
>  	xilinx_dpdma_enable_irq(xdev);
>  
>  	xilinx_dpdma_debugfs_init(xdev);
> @@ -1696,6 +1696,8 @@ static int xilinx_dpdma_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> +error_irq:
> +	of_dma_controller_free(pdev->dev.of_node);

Why not free_irq() ? This change isn't described in the commit message.

>  error_of_dma:
>  	dma_async_device_unregister(ddev);
>  error_dma_async:
> @@ -1704,8 +1706,6 @@ static int xilinx_dpdma_probe(struct platform_device *pdev)
>  	for (i = 0; i < ARRAY_SIZE(xdev->chan); i++)
>  		xilinx_dpdma_chan_remove(xdev->chan[i]);
>  
> -	free_irq(xdev->irq, xdev);
> -
>  	return ret;
>  }
>
quanyang.wang April 30, 2021, 1:45 a.m. UTC | #3
Hi Laurent,

On 4/30/21 8:36 AM, Laurent Pinchart wrote:
> Hi Quanyang,
>
> Thank you for the patch.
>
> On Thu, Apr 29, 2021 at 02:58:21PM +0800, quanyang.wang@windriver.com wrote:
>> From: Quanyang Wang <quanyang.wang@windriver.com>
>>
>> In some scenarios (kdump), dpdma hardware irqs has been enabled when
>> calling request_irq in probe function, and then the dpdma irq handler
>> xilinx_dpdma_irq_handler is invoked to access xdev->chan[i]. But at
>> this moment xdev->chan[i] hasn't been initialized. So let's call the
>> request_irq after initializing dma channels to avoid kdump kernel
>> crash as below:
>>
>> [    3.696128] Unable to handle kernel NULL pointer dereference at virtual address 000000000000012c
>> [    3.696710] xilinx-zynqmp-dpdma fd4c0000.dma-controller: Xilinx DPDMA engine is probed
>> [    3.704900] Mem abort info:
>> [    3.704902]   ESR = 0x96000005
>> [    3.704905]   EC = 0x25: DABT (current EL), IL = 32 bits
>> [    3.704907]   SET = 0, FnV = 0
>> [    3.704912]   EA = 0, S1PTW = 0
>> [    3.713800] ahci-ceva fd0c0000.ahci: supply ahci not found, using dummy regulator
>> [    3.715585] Data abort info:
>> [    3.715587]   ISV = 0, ISS = 0x00000005
>> [    3.715589]   CM = 0, WnR = 0
>> [    3.715592] [000000000000012c] user address but active_mm is swapper
>> [    3.715596] Internal error: Oops: 96000005 [#1] SMP
>> [    3.715599] Modules linked in:
>> [    3.715608] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.10.0-12170-g60894882155f-dirty #77
>> [    3.723937] Hardware name: ZynqMP ZCU102 Rev1.0 (DT)
>> [    3.723942] pstate: 80000085 (Nzcv daIf -PAN -UAO -TCO BTYPE=--)
>> [    3.723956] pc : xilinx_dpdma_irq_handler+0x418/0x560
>> [    3.793049] lr : xilinx_dpdma_irq_handler+0x3d8/0x560
>> [    3.798089] sp : ffffffc01186bdf0
>> [    3.801388] x29: ffffffc01186bdf0 x28: ffffffc011836f28
>> [    3.806692] x27: ffffff8023e0ac80 x26: 0000000000000080
>> [    3.811996] x25: 0000000008000408 x24: 0000000000000003
>> [    3.817300] x23: ffffffc01186be70 x22: ffffffc011291740
>> [    3.822604] x21: 0000000000000000 x20: 0000000008000408
>> [    3.827908] x19: 0000000000000000 x18: 0000000000000010
>> [    3.833212] x17: 0000000000000000 x16: 0000000000000000
>> [    3.838516] x15: 0000000000000000 x14: ffffffc011291740
>> [    3.843820] x13: ffffffc02eb4d000 x12: 0000000034d4d91d
>> [    3.849124] x11: 0000000000000040 x10: ffffffc0112d2d48
>> [    3.854428] x9 : ffffffc0112d2d40 x8 : ffffff8021c00268
>> [    3.859732] x7 : 0000000000000000 x6 : ffffffc011836000
>> [    3.865036] x5 : 0000000000000003 x4 : 0000000000000000
>> [    3.870340] x3 : 0000000000000001 x2 : 0000000000000000
>> [    3.875644] x1 : 0000000000000000 x0 : 000000000000012c
>> [    3.880948] Call trace:
>> [    3.883382]  xilinx_dpdma_irq_handler+0x418/0x560
>> [    3.888079]  __handle_irq_event_percpu+0x5c/0x178
>> [    3.892774]  handle_irq_event_percpu+0x34/0x98
>> [    3.897210]  handle_irq_event+0x44/0xb8
>> [    3.901030]  handle_fasteoi_irq+0xd0/0x190
>> [    3.905117]  generic_handle_irq+0x30/0x48
>> [    3.909111]  __handle_domain_irq+0x64/0xc0
>> [    3.913192]  gic_handle_irq+0x78/0xa0
>> [    3.916846]  el1_irq+0xc4/0x180
>> [    3.919982]  cpuidle_enter_state+0x134/0x2f8
>> [    3.924243]  cpuidle_enter+0x38/0x50
>> [    3.927810]  call_cpuidle+0x1c/0x40
>> [    3.931290]  do_idle+0x20c/0x270
>> [    3.934502]  cpu_startup_entry+0x28/0x58
>> [    3.938410]  rest_init+0xbc/0xcc
>> [    3.941631]  arch_call_rest_init+0x10/0x1c
>> [    3.945718]  start_kernel+0x51c/0x558
>>
>> Fixes: 7cbb0c63de3f ("dmaengine: xilinx: dpdma: Add the Xilinx DisplayPort DMA engine driver")
>> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
>> ---
>>   drivers/dma/xilinx/xilinx_dpdma.c | 30 +++++++++++++++---------------
>>   1 file changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c
>> index 70b29bd079c9..0b599402c53f 100644
>> --- a/drivers/dma/xilinx/xilinx_dpdma.c
>> +++ b/drivers/dma/xilinx/xilinx_dpdma.c
>> @@ -1622,19 +1622,6 @@ static int xilinx_dpdma_probe(struct platform_device *pdev)
>>   	if (IS_ERR(xdev->reg))
>>   		return PTR_ERR(xdev->reg);
>>   
>> -	xdev->irq = platform_get_irq(pdev, 0);
>> -	if (xdev->irq < 0) {
>> -		dev_err(xdev->dev, "failed to get platform irq\n");
>> -		return xdev->irq;
>> -	}
>> -
>> -	ret = request_irq(xdev->irq, xilinx_dpdma_irq_handler, IRQF_SHARED,
>> -			  dev_name(xdev->dev), xdev);
>> -	if (ret) {
>> -		dev_err(xdev->dev, "failed to request IRQ\n");
>> -		return ret;
>> -	}
>> -
>>   	ddev = &xdev->common;
>>   	ddev->dev = &pdev->dev;
>>   
>> @@ -1688,6 +1675,19 @@ static int xilinx_dpdma_probe(struct platform_device *pdev)
>>   		goto error_of_dma;
>>   	}
>>   
>> +	xdev->irq = platform_get_irq(pdev, 0);
>> +	if (xdev->irq < 0) {
>> +		dev_err(xdev->dev, "failed to get platform irq\n");
> As reported by the kbuild bot, ret isn't initialized here.
I will fix this in V2.
>
>> +		goto error_irq;
>> +	}
> This part could stay where it was, just after
> devm_platform_ioremap_resource().
OK,I will move it back.
>
>> +
>> +	ret = request_irq(xdev->irq, xilinx_dpdma_irq_handler, IRQF_SHARED,
>> +			  dev_name(xdev->dev), xdev);
>> +	if (ret) {
>> +		dev_err(xdev->dev, "failed to request IRQ\n");
>> +		goto error_irq;
>> +	}
> Ideally we should reset the device before requesting the IRQ, to ensure
> we start in a consistent state. There doesn't seem to be any global
> reset unfortunately. Shouldn't we at least disable interrupts ? It may
> be a bit pointless though as we reenable them right below, so I suppose
> this would be OK.

Yes, we should start in a consistent and clean state. I will disable 
interrupts

and clean up registers state in V2.

>
>> +
>>   	xilinx_dpdma_enable_irq(xdev);
>>   
>>   	xilinx_dpdma_debugfs_init(xdev);
>> @@ -1696,6 +1696,8 @@ static int xilinx_dpdma_probe(struct platform_device *pdev)
>>   
>>   	return 0;
>>   
>> +error_irq:
>> +	of_dma_controller_free(pdev->dev.of_node);
> Why not free_irq() ? This change isn't described in the commit message.

I will add it in V2.

Thanks,

Quanyang

>
>>   error_of_dma:
>>   	dma_async_device_unregister(ddev);
>>   error_dma_async:
>> @@ -1704,8 +1706,6 @@ static int xilinx_dpdma_probe(struct platform_device *pdev)
>>   	for (i = 0; i < ARRAY_SIZE(xdev->chan); i++)
>>   		xilinx_dpdma_chan_remove(xdev->chan[i]);
>>   
>> -	free_irq(xdev->irq, xdev);
>> -
>>   	return ret;
>>   }
>>
quanyang.wang April 30, 2021, 6:56 a.m. UTC | #4
Hi Michal,

On 4/30/21 2:51 PM, Michal Simek wrote:
> Hi,
>
> On 4/30/21 3:45 AM, quanyang.wang wrote:
>> Hi Laurent,
>>
>> On 4/30/21 8:36 AM, Laurent Pinchart wrote:
>>> Hi Quanyang,
>>>
>>> Thank you for the patch.
>>>
>>> On Thu, Apr 29, 2021 at 02:58:21PM +0800, quanyang.wang@windriver.com
>>> wrote:
>>>> From: Quanyang Wang <quanyang.wang@windriver.com>
>>>>
>>>> In some scenarios (kdump), dpdma hardware irqs has been enabled when
>>>> calling request_irq in probe function, and then the dpdma irq handler
>>>> xilinx_dpdma_irq_handler is invoked to access xdev->chan[i]. But at
>>>> this moment xdev->chan[i] hasn't been initialized. So let's call the
>>>> request_irq after initializing dma channels to avoid kdump kernel
>>>> crash as below:
>>>>
>>>> [    3.696128] Unable to handle kernel NULL pointer dereference at
>>>> virtual address 000000000000012c
>>>> [    3.696710] xilinx-zynqmp-dpdma fd4c0000.dma-controller: Xilinx
>>>> DPDMA engine is probed
>>>> [    3.704900] Mem abort info:
>>>> [    3.704902]   ESR = 0x96000005
>>>> [    3.704905]   EC = 0x25: DABT (current EL), IL = 32 bits
>>>> [    3.704907]   SET = 0, FnV = 0
>>>> [    3.704912]   EA = 0, S1PTW = 0
>>>> [    3.713800] ahci-ceva fd0c0000.ahci: supply ahci not found, using
>>>> dummy regulator
>>>> [    3.715585] Data abort info:
>>>> [    3.715587]   ISV = 0, ISS = 0x00000005
>>>> [    3.715589]   CM = 0, WnR = 0
>>>> [    3.715592] [000000000000012c] user address but active_mm is swapper
>>>> [    3.715596] Internal error: Oops: 96000005 [#1] SMP
>>>> [    3.715599] Modules linked in:
>>>> [    3.715608] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
>>>> 5.10.0-12170-g60894882155f-dirty #77
>>>> [    3.723937] Hardware name: ZynqMP ZCU102 Rev1.0 (DT)
>>>> [    3.723942] pstate: 80000085 (Nzcv daIf -PAN -UAO -TCO BTYPE=--)
>>>> [    3.723956] pc : xilinx_dpdma_irq_handler+0x418/0x560
>>>> [    3.793049] lr : xilinx_dpdma_irq_handler+0x3d8/0x560
>>>> [    3.798089] sp : ffffffc01186bdf0
>>>> [    3.801388] x29: ffffffc01186bdf0 x28: ffffffc011836f28
>>>> [    3.806692] x27: ffffff8023e0ac80 x26: 0000000000000080
>>>> [    3.811996] x25: 0000000008000408 x24: 0000000000000003
>>>> [    3.817300] x23: ffffffc01186be70 x22: ffffffc011291740
>>>> [    3.822604] x21: 0000000000000000 x20: 0000000008000408
>>>> [    3.827908] x19: 0000000000000000 x18: 0000000000000010
>>>> [    3.833212] x17: 0000000000000000 x16: 0000000000000000
>>>> [    3.838516] x15: 0000000000000000 x14: ffffffc011291740
>>>> [    3.843820] x13: ffffffc02eb4d000 x12: 0000000034d4d91d
>>>> [    3.849124] x11: 0000000000000040 x10: ffffffc0112d2d48
>>>> [    3.854428] x9 : ffffffc0112d2d40 x8 : ffffff8021c00268
>>>> [    3.859732] x7 : 0000000000000000 x6 : ffffffc011836000
>>>> [    3.865036] x5 : 0000000000000003 x4 : 0000000000000000
>>>> [    3.870340] x3 : 0000000000000001 x2 : 0000000000000000
>>>> [    3.875644] x1 : 0000000000000000 x0 : 000000000000012c
>>>> [    3.880948] Call trace:
>>>> [    3.883382]  xilinx_dpdma_irq_handler+0x418/0x560
>>>> [    3.888079]  __handle_irq_event_percpu+0x5c/0x178
>>>> [    3.892774]  handle_irq_event_percpu+0x34/0x98
>>>> [    3.897210]  handle_irq_event+0x44/0xb8
>>>> [    3.901030]  handle_fasteoi_irq+0xd0/0x190
>>>> [    3.905117]  generic_handle_irq+0x30/0x48
>>>> [    3.909111]  __handle_domain_irq+0x64/0xc0
>>>> [    3.913192]  gic_handle_irq+0x78/0xa0
>>>> [    3.916846]  el1_irq+0xc4/0x180
>>>> [    3.919982]  cpuidle_enter_state+0x134/0x2f8
>>>> [    3.924243]  cpuidle_enter+0x38/0x50
>>>> [    3.927810]  call_cpuidle+0x1c/0x40
>>>> [    3.931290]  do_idle+0x20c/0x270
>>>> [    3.934502]  cpu_startup_entry+0x28/0x58
>>>> [    3.938410]  rest_init+0xbc/0xcc
>>>> [    3.941631]  arch_call_rest_init+0x10/0x1c
>>>> [    3.945718]  start_kernel+0x51c/0x558
>>>>
>>>> Fixes: 7cbb0c63de3f ("dmaengine: xilinx: dpdma: Add the Xilinx
>>>> DisplayPort DMA engine driver")
>>>> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
>>>> ---
>>>>    drivers/dma/xilinx/xilinx_dpdma.c | 30 +++++++++++++++---------------
>>>>    1 file changed, 15 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/dma/xilinx/xilinx_dpdma.c
>>>> b/drivers/dma/xilinx/xilinx_dpdma.c
>>>> index 70b29bd079c9..0b599402c53f 100644
>>>> --- a/drivers/dma/xilinx/xilinx_dpdma.c
>>>> +++ b/drivers/dma/xilinx/xilinx_dpdma.c
>>>> @@ -1622,19 +1622,6 @@ static int xilinx_dpdma_probe(struct
>>>> platform_device *pdev)
>>>>        if (IS_ERR(xdev->reg))
>>>>            return PTR_ERR(xdev->reg);
>>>>    -    xdev->irq = platform_get_irq(pdev, 0);
>>>> -    if (xdev->irq < 0) {
>>>> -        dev_err(xdev->dev, "failed to get platform irq\n");
>>>> -        return xdev->irq;
>>>> -    }
>>>> -
>>>> -    ret = request_irq(xdev->irq, xilinx_dpdma_irq_handler, IRQF_SHARED,
>>>> -              dev_name(xdev->dev), xdev);
>>>> -    if (ret) {
>>>> -        dev_err(xdev->dev, "failed to request IRQ\n");
>>>> -        return ret;
>>>> -    }
>>>> -
>>>>        ddev = &xdev->common;
>>>>        ddev->dev = &pdev->dev;
>>>>    @@ -1688,6 +1675,19 @@ static int xilinx_dpdma_probe(struct
>>>> platform_device *pdev)
>>>>            goto error_of_dma;
>>>>        }
>>>>    +    xdev->irq = platform_get_irq(pdev, 0);
>>>> +    if (xdev->irq < 0) {
>>>> +        dev_err(xdev->dev, "failed to get platform irq\n");
>>> As reported by the kbuild bot, ret isn't initialized here.
>> I will fix this in V2.
>>>> +        goto error_irq;
>>>> +    }
>>> This part could stay where it was, just after
>>> devm_platform_ioremap_resource().
>> OK,I will move it back.
>>>> +
>>>> +    ret = request_irq(xdev->irq, xilinx_dpdma_irq_handler, IRQF_SHARED,
>>>> +              dev_name(xdev->dev), xdev);
>>>> +    if (ret) {
>>>> +        dev_err(xdev->dev, "failed to request IRQ\n");
>>>> +        goto error_irq;
>>>> +    }
>>> Ideally we should reset the device before requesting the IRQ, to ensure
>>> we start in a consistent state. There doesn't seem to be any global
>>> reset unfortunately. Shouldn't we at least disable interrupts ? It may
>>> be a bit pointless though as we reenable them right below, so I suppose
>>> this would be OK.
>> Yes, we should start in a consistent and clean state. I will disable
>> interrupts
>>
>> and clean up registers state in V2.
>>
>>>> +
>>>>        xilinx_dpdma_enable_irq(xdev);
>>>>          xilinx_dpdma_debugfs_init(xdev);
>>>> @@ -1696,6 +1696,8 @@ static int xilinx_dpdma_probe(struct
>>>> platform_device *pdev)
>>>>          return 0;
>>>>    +error_irq:
>>>> +    of_dma_controller_free(pdev->dev.of_node);
>>> Why not free_irq() ? This change isn't described in the commit message.
>> I will add it in V2.
> When I was checking it that request_irq was called last. It means it
> wasn't necessary to free irq in error path. But double check it.

Thank you very much for your reminder.

Yes, free_irq is not necessary, and I have abandoned all modifications 
of V1 and

make a new V2 patch.

Thanks,

Quanyang

>
> M
>
diff mbox series

Patch

diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c
index 70b29bd079c9..0b599402c53f 100644
--- a/drivers/dma/xilinx/xilinx_dpdma.c
+++ b/drivers/dma/xilinx/xilinx_dpdma.c
@@ -1622,19 +1622,6 @@  static int xilinx_dpdma_probe(struct platform_device *pdev)
 	if (IS_ERR(xdev->reg))
 		return PTR_ERR(xdev->reg);
 
-	xdev->irq = platform_get_irq(pdev, 0);
-	if (xdev->irq < 0) {
-		dev_err(xdev->dev, "failed to get platform irq\n");
-		return xdev->irq;
-	}
-
-	ret = request_irq(xdev->irq, xilinx_dpdma_irq_handler, IRQF_SHARED,
-			  dev_name(xdev->dev), xdev);
-	if (ret) {
-		dev_err(xdev->dev, "failed to request IRQ\n");
-		return ret;
-	}
-
 	ddev = &xdev->common;
 	ddev->dev = &pdev->dev;
 
@@ -1688,6 +1675,19 @@  static int xilinx_dpdma_probe(struct platform_device *pdev)
 		goto error_of_dma;
 	}
 
+	xdev->irq = platform_get_irq(pdev, 0);
+	if (xdev->irq < 0) {
+		dev_err(xdev->dev, "failed to get platform irq\n");
+		goto error_irq;
+	}
+
+	ret = request_irq(xdev->irq, xilinx_dpdma_irq_handler, IRQF_SHARED,
+			  dev_name(xdev->dev), xdev);
+	if (ret) {
+		dev_err(xdev->dev, "failed to request IRQ\n");
+		goto error_irq;
+	}
+
 	xilinx_dpdma_enable_irq(xdev);
 
 	xilinx_dpdma_debugfs_init(xdev);
@@ -1696,6 +1696,8 @@  static int xilinx_dpdma_probe(struct platform_device *pdev)
 
 	return 0;
 
+error_irq:
+	of_dma_controller_free(pdev->dev.of_node);
 error_of_dma:
 	dma_async_device_unregister(ddev);
 error_dma_async:
@@ -1704,8 +1706,6 @@  static int xilinx_dpdma_probe(struct platform_device *pdev)
 	for (i = 0; i < ARRAY_SIZE(xdev->chan); i++)
 		xilinx_dpdma_chan_remove(xdev->chan[i]);
 
-	free_irq(xdev->irq, xdev);
-
 	return ret;
 }