LinuxLists.cc - [PATCH] dmaengine: xilinx: dpdma: request

2021-04-29 07:02:08

Subject: [PATCH] dmaengine: xilinx: dpdma: request_irq after initializing dma channels

From: Quanyang Wang <[email protected]>

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 <[email protected]>
---
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");
+ 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;
}

--
2.25.1

2021-04-29 13:06:17

by Dan Carpenter

[permalink] [raw]

Subject: [kbuild] Re: [PATCH] dmaengine: xilinx: dpdma: request_irq after initializing dma channels

Hi,

url: If you fix the Reported-by: kernel Reported-by: Dan
smatch warnings:
drivers/dma/xilinx/xilin
vim +/ret +1778
7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon b305dd098dae4f Rohit Visavalia 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon b305dd098dae4f Rohit Visavalia 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 50d6be0a1d25af Quanyang Wang 50d6be0a1d25af Quanyang Wang 50d6be0a1d25af Quanyang Wang 50d6be0a1d25af Quanyang Wang
ret = xdev->irq;

50d6be0a1d25af Quanyang Wang 50d6be0a1d25af Quanyang Wang 50d6be0a1d25af Quanyang Wang 50d6be0a1d25af Quanyang Wang 50d6be0a1d25af Quanyang Wang 50d6be0a1d25af Quanyang Wang 50d6be0a1d25af Quanyang Wang 50d6be0a1d25af Quanyang Wang 50d6be0a1d25af Quanyang Wang 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 1d220435cab3bf 1d220435cab3bf 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 50d6be0a1d25af Quanyang Wang 50d6be0a1d25af Quanyang Wang 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon 7cbb0c63de3fc2 Hyun Kwon
---
0-DAY CI Kernel https://github.com/0day-ci/linux/commits/quanyang-wang-windriver-com/dmaengine-xilinx-dpdma-request_irq-after-initializing-dma-channels/20210429-150050
//github.com/Xilinx/linux-xlnx">https://github.com/Xilinx/linux-xlnx master
g-m001-20210429 (attached as .config)
(Debian 9.3.0-22) 9.3.0
issue, kindly add following tag as appropriate
test robot <[email protected]>
Carpenter <[email protected]>
x_dpdma.c:1778 xilinx_dpdma_probe() warn: missing error code 'ret'
drivers/dma/xilinx/xilinx_dpdma.c
2020-07-17 1694 static int xilinx_dpdma_probe(struct platform_device *pdev)
2020-07-17 1695 {
2020-07-17 1696 struct xilinx_dpdma_device *xdev;
2020-07-17 1697 struct dma_device *ddev;
2020-07-17 1698 unsigned int i;
2020-07-17 1699 int ret;
2020-07-17 1700
2020-07-17 1701 xdev = devm_kzalloc(&pdev->dev, sizeof(*xdev), GFP_KERNEL);
2020-07-17 1702 if (!xdev)
2020-07-17 1703 return -ENOMEM;
2020-07-17 1704
2020-07-17 1705 xdev->dev = &pdev->dev;
2020-07-17 1706 xdev->ext_addr = sizeof(dma_addr_t) > 4;
2020-07-17 1707
2020-07-17 1708 INIT_LIST_HEAD(&xdev->common.channels);
2020-07-17 1709
2020-07-17 1710 platform_set_drvdata(pdev, xdev);
2020-07-17 1711
2020-07-17 1712 xdev->axi_clk = devm_clk_get(xdev->dev, "axi_clk");
2020-07-17 1713 if (IS_ERR(xdev->axi_clk))
2020-07-17 1714 return PTR_ERR(xdev->axi_clk);
2020-07-17 1715
2020-07-17 1716 xdev->reg = devm_platform_ioremap_resource(pdev, 0);
2020-07-17 1717 if (IS_ERR(xdev->reg))
2020-07-17 1718 return PTR_ERR(xdev->reg);
2020-07-17 1719
2020-07-17 1720 ddev = &xdev->common;
2020-07-17 1721 ddev->dev = &pdev->dev;
2020-07-17 1722
2020-07-17 1723 dma_cap_set(DMA_SLAVE, ddev->cap_mask);
2020-07-17 1724 dma_cap_set(DMA_PRIVATE, ddev->cap_mask);
2021-04-07 1725 dma_cap_set(DMA_CYCLIC, ddev->cap_mask);
2020-07-17 1726 dma_cap_set(DMA_INTERLEAVE, ddev->cap_mask);
2020-07-17 1727 dma_cap_set(DMA_REPEAT, ddev->cap_mask);
2020-07-17 1728 dma_cap_set(DMA_LOAD_EOT, ddev->cap_mask);
2020-07-17 1729 ddev->copy_align = fls(XILINX_DPDMA_ALIGN_BYTES - 1);
2020-07-17 1730
2020-07-17 1731 ddev->device_alloc_chan_resources = xilinx_dpdma_alloc_chan_resources;
2020-07-17 1732 ddev->device_free_chan_resources = xilinx_dpdma_free_chan_resources;
2021-04-07 1733 ddev->device_prep_dma_cyclic = xilinx_dpdma_prep_dma_cyclic;
2020-07-17 1734 ddev->device_prep_interleaved_dma = xilinx_dpdma_prep_interleaved_dma;
2020-07-17 1735 /* TODO: Can we achieve better granularity ? */
2020-07-17 1736 ddev->device_tx_status = dma_cookie_status;
2020-07-17 1737 ddev->device_issue_pending = xilinx_dpdma_issue_pending;
2020-07-17 1738 ddev->device_config = xilinx_dpdma_config;
2020-07-17 1739 ddev->device_pause = xilinx_dpdma_pause;
2020-07-17 1740 ddev->device_resume = xilinx_dpdma_resume;
2020-07-17 1741 ddev->device_terminate_all = xilinx_dpdma_terminate_all;
2020-07-17 1742 ddev->device_synchronize = xilinx_dpdma_synchronize;
2020-07-17 1743 ddev->src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED);
2020-07-17 1744 ddev->directions = BIT(DMA_MEM_TO_DEV);
2020-07-17 1745 ddev->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
2020-07-17 1746
2020-07-17 1747 for (i = 0; i < ARRAY_SIZE(xdev->chan); ++i) {
2020-07-17 1748 ret = xilinx_dpdma_chan_init(xdev, i);
2020-07-17 1749 if (ret < 0) {
2020-07-17 1750 dev_err(xdev->dev, "failed to initialize channel %u\n",
2020-07-17 1751 i);
2020-07-17 1752 goto error;
2020-07-17 1753 }
2020-07-17 1754 }
2020-07-17 1755
2020-07-17 1756 ret = clk_prepare_enable(xdev->axi_clk);
2020-07-17 1757 if (ret) {
2020-07-17 1758 dev_err(xdev->dev, "failed to enable the axi clock\n");
2020-07-17 1759 goto error;
2020-07-17 1760 }
2020-07-17 1761
2020-07-17 1762 ret = dma_async_device_register(ddev);
2020-07-17 1763 if (ret) {
2020-07-17 1764 dev_err(xdev->dev, "failed to register the dma device\n");
2020-07-17 1765 goto error_dma_async;
2020-07-17 1766 }
2020-07-17 1767
2020-07-17 1768 ret = of_dma_controller_register(xdev->dev->of_node,
2020-07-17 1769 of_dma_xilinx_xlate, ddev);
2020-07-17 1770 if (ret) {
2020-07-17 1771 dev_err(xdev->dev, "failed to register DMA to DT DMA helper\n");
2020-07-17 1772 goto error_of_dma;
2020-07-17 1773 }
2020-07-17 1774
2021-04-29 1775 xdev->irq = platform_get_irq(pdev, 0);
2021-04-29 1776 if (xdev->irq < 0) {
2021-04-29 1777 dev_err(xdev->dev, "failed to get platform irq\n");
2021-04-29 @1778 goto error_irq;
2021-04-29 1779 }
2021-04-29 1780
2021-04-29 1781 ret = request_irq(xdev->irq, xilinx_dpdma_irq_handler, IRQF_SHARED,
2021-04-29 1782 dev_name(xdev->dev), xdev);
2021-04-29 1783 if (ret) {
2021-04-29 1784 dev_err(xdev->dev, "failed to request IRQ\n");
2021-04-29 1785 goto error_irq;
2021-04-29 1786 }
2021-04-29 1787
2020-07-17 1788 xilinx_dpdma_enable_irq(xdev);
2020-07-17 1789
Laurent Pinchart 2020-08-12 1790 xilinx_dpdma_debugfs_init(xdev);
Laurent Pinchart 2020-08-12 1791
2020-07-17 1792 dev_info(&pdev->dev, "Xilinx DPDMA engine is probed\n");
2020-07-17 1793
2020-07-17 1794 return 0;
2020-07-17 1795
2021-04-29 1796 error_irq:
2021-04-29 1797 of_dma_controller_free(pdev->dev.of_node);
2020-07-17 1798 error_of_dma:
2020-07-17 1799 dma_async_device_unregister(ddev);
2020-07-17 1800 error_dma_async:
2020-07-17 1801 clk_disable_unprepare(xdev->axi_clk);
2020-07-17 1802 error:
2020-07-17 1803 for (i = 0; i < ARRAY_SIZE(xdev->chan); i++)
2020-07-17 1804 xilinx_dpdma_chan_remove(xdev->chan[i]);
2020-07-17 1805
2020-07-17 1806 return ret;
2020-07-17 1807 }
Test Service, Intel Corporation
01.org/hyperkitty/list/kbuild-all@lists.01.org">https://lists.01.org/hyperkitty/list/[email protected]
_______________________
list -- [email protected]
send an email to [email protected]

2021-04-30 00:40:52

by Laurent Pinchart

[permalink] [raw]

Subject: Re: [PATCH] dmaengine: xilinx: dpdma: request_irq after initializing dma channels

Hi Quanyang,

Thank you for the patch.

On Thu, Apr 29, 2021 at 02:58:21PM +0800, [email protected] wrote:
> From: Quanyang Wang <[email protected]>
>
> 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 <[email protected]>
> ---
> 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;
> }
>

--
Regards,

Laurent Pinchart

2021-04-30 01:48:29

by quanyang wang

[permalink] [raw]

Subject: Re: [PATCH] dmaengine: xilinx: dpdma: request_irq after initializing dma channels

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, [email protected] wrote:
>> From: Quanyang Wang <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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;
>> }
>>

2021-04-30 06:59:02

by quanyang wang

[permalink] [raw]

Subject: Re: [PATCH] dmaengine: xilinx: dpdma: request_irq after initializing dma channels

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, [email protected]
>>> wrote:
>>>> From: Quanyang Wang <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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
>