| Message ID | 20210429065821.3495234-1-quanyang.wang@windriver.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dmaengine: xilinx: dpdma: request_irq after initializing dma channels | expand |
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
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; > } >
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; >> } >>
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. M
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 --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; }