| Message ID | 20190624123818.20919-1-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | dmaengine: rcar-dmac: Reject zero-length slave DMA requests | expand |
> Although requesting a zero-length DMA request is a driver bug, rejecting > it early eases debugging. True. I wonder, though, if we then should turn the message into a WARN, so the call stack will point to the faulty driver?
On 24-06-19, 14:38, Geert Uytterhoeven wrote: > While the .device_prep_slave_sg() callback rejects empty scatterlists, > it still accepts single-entry scatterlists with a zero-length segment. > These may happen if a driver calls dmaengine_prep_slave_single() with a > zero len parameter. The corresponding DMA request will never complete, > leading to messages like: > > rcar-dmac e7300000.dma-controller: Channel Address Error happen > > and DMA timeouts. > > Although requesting a zero-length DMA request is a driver bug, rejecting > it early eases debugging. Note that the .device_prep_dma_memcpy() > callback already rejects requests to copy zero bytes. Applied, thanks
On 24-06-19, 14:38, Geert Uytterhoeven wrote: > While the .device_prep_slave_sg() callback rejects empty scatterlists, > it still accepts single-entry scatterlists with a zero-length segment. > These may happen if a driver calls dmaengine_prep_slave_single() with a > zero len parameter. The corresponding DMA request will never complete, > leading to messages like: > > rcar-dmac e7300000.dma-controller: Channel Address Error happen > > and DMA timeouts. > > Although requesting a zero-length DMA request is a driver bug, rejecting > it early eases debugging. Note that the .device_prep_dma_memcpy() > callback already rejects requests to copy zero bytes. Applied, thanks
Hi All, On Mon, Jun 24, 2019 at 02:38:18PM +0200, Geert Uytterhoeven wrote: [..] > - if (rchan->mid_rid < 0 || !sg_len) { > + if (rchan->mid_rid < 0 || !sg_len || !sg_dma_len(sgl)) { > dev_warn(chan->device->dev, > "%s: bad parameter: len=%d, id=%d\n", > __func__, sg_len, rchan->mid_rid); Just wanted to share the WARN output proposed by Wolfram in https://patchwork.kernel.org/patch/11012991/#22721733 in case the issue discussed in [1] is reproduced with this patch: [ 2.065337] ------------[ cut here ]------------ [ 2.065346] rcar_dmac_prep_slave_sg: <here-comes-the-warning-message> [ 2.065394] WARNING: CPU: 2 PID: 252 at drivers/dma/sh/rcar-dmac.c:1169 rcar_dmac_prep_slave_sg+0x50/0xc4 [ 2.065397] Modules linked in: [ 2.065407] CPU: 2 PID: 252 Comm: kworker/2:1 Not tainted 5.2.0-rc6-00016-g2bfb85ba1481-dirty #26 [ 2.065410] Hardware name: Renesas H3ULCB Kingfisher board based on r8a7795 ES2.0+ (DT) [ 2.065420] Workqueue: events sci_dma_tx_work_fn [ 2.065425] pstate: 40000005 (nZcv daif -PAN -UAO) [ 2.065430] pc : rcar_dmac_prep_slave_sg+0x50/0xc4 [ 2.065434] lr : rcar_dmac_prep_slave_sg+0x50/0xc4 [ 2.065436] sp : ffff0000112ebd00 [ 2.065438] x29: ffff0000112ebd00 x28: 0000000000000000 [ 2.065443] x27: ffff8006fa367138 x26: ffff000010c5bce8 [ 2.065447] x25: 0000000738b1d000 x24: 0000000000000000 [ 2.065451] x23: ffff000010b76e00 x22: ffff000010a18000 [ 2.065455] x21: 0000000000000001 x20: ffff8006f9b5a080 [ 2.065459] x19: ffff0000107adc86 x18: 0000000000000000 [ 2.065462] x17: 0000000000000000 x16: 0000000000000000 [ 2.065466] x15: 0000000000000000 x14: 0000000000000000 [ 2.065469] x13: 0000000000040000 x12: ffff000010a35000 [ 2.065473] x11: ffff000010b12981 x10: 0000000000000040 [ 2.065477] x9 : 000000000000013e x8 : ffff000010b1b73b [ 2.065481] x7 : 0000000000000000 x6 : 0000000000000001 [ 2.065484] x5 : ffff8006ff72f7c0 x4 : 0000000000000001 [ 2.065488] x3 : 0000000000000007 x2 : 0000000000000007 [ 2.065491] x1 : 878c73041cedc400 x0 : 0000000000000000 [ 2.065495] Call trace: [ 2.065500] rcar_dmac_prep_slave_sg+0x50/0xc4 [ 2.065504] sci_dma_tx_work_fn+0xd8/0x1d4 [ 2.065511] process_one_work+0x1dc/0x394 [ 2.065515] worker_thread+0x21c/0x308 [ 2.065520] kthread+0x118/0x128 [ 2.065527] ret_from_fork+0x10/0x18 [ 2.065530] ---[ end trace 75fc17d9000f1224 ]--- At first glance, it seems to give more details compared to: rcar-dmac e7300000.dma-controller: rcar_dmac_prep_slave_sg: bad parameter: len=1, id=19 [1] https://patchwork.kernel.org/cover/11012981/
Hi Eugeniu, On Wed, Jun 26, 2019 at 8:15 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: > On Mon, Jun 24, 2019 at 02:38:18PM +0200, Geert Uytterhoeven wrote: > [..] > > - if (rchan->mid_rid < 0 || !sg_len) { > > + if (rchan->mid_rid < 0 || !sg_len || !sg_dma_len(sgl)) { > > dev_warn(chan->device->dev, > > "%s: bad parameter: len=%d, id=%d\n", > > __func__, sg_len, rchan->mid_rid); > > Just wanted to share the WARN output proposed by Wolfram in > https://patchwork.kernel.org/patch/11012991/#22721733 > in case the issue discussed in [1] is reproduced with this patch: I'm not such a big fan of WARN()... > [ 2.065337] ------------[ cut here ]------------ > [ 2.065346] rcar_dmac_prep_slave_sg: <here-comes-the-warning-message> > [ 2.065394] WARNING: CPU: 2 PID: 252 at drivers/dma/sh/rcar-dmac.c:1169 rcar_dmac_prep_slave_sg+0x50/0xc4 > [ 2.065397] Modules linked in: > [ 2.065407] CPU: 2 PID: 252 Comm: kworker/2:1 Not tainted 5.2.0-rc6-00016-g2bfb85ba1481-dirty #26 > [ 2.065410] Hardware name: Renesas H3ULCB Kingfisher board based on r8a7795 ES2.0+ (DT) > [ 2.065420] Workqueue: events sci_dma_tx_work_fn > [ 2.065425] pstate: 40000005 (nZcv daif -PAN -UAO) > [ 2.065430] pc : rcar_dmac_prep_slave_sg+0x50/0xc4 > [ 2.065434] lr : rcar_dmac_prep_slave_sg+0x50/0xc4 > [ 2.065436] sp : ffff0000112ebd00 > [ 2.065438] x29: ffff0000112ebd00 x28: 0000000000000000 > [ 2.065443] x27: ffff8006fa367138 x26: ffff000010c5bce8 > [ 2.065447] x25: 0000000738b1d000 x24: 0000000000000000 > [ 2.065451] x23: ffff000010b76e00 x22: ffff000010a18000 > [ 2.065455] x21: 0000000000000001 x20: ffff8006f9b5a080 > [ 2.065459] x19: ffff0000107adc86 x18: 0000000000000000 > [ 2.065462] x17: 0000000000000000 x16: 0000000000000000 > [ 2.065466] x15: 0000000000000000 x14: 0000000000000000 > [ 2.065469] x13: 0000000000040000 x12: ffff000010a35000 > [ 2.065473] x11: ffff000010b12981 x10: 0000000000000040 > [ 2.065477] x9 : 000000000000013e x8 : ffff000010b1b73b > [ 2.065481] x7 : 0000000000000000 x6 : 0000000000000001 > [ 2.065484] x5 : ffff8006ff72f7c0 x4 : 0000000000000001 > [ 2.065488] x3 : 0000000000000007 x2 : 0000000000000007 > [ 2.065491] x1 : 878c73041cedc400 x0 : 0000000000000000 > [ 2.065495] Call trace: > [ 2.065500] rcar_dmac_prep_slave_sg+0x50/0xc4 > [ 2.065504] sci_dma_tx_work_fn+0xd8/0x1d4 > [ 2.065511] process_one_work+0x1dc/0x394 > [ 2.065515] worker_thread+0x21c/0x308 > [ 2.065520] kthread+0x118/0x128 > [ 2.065527] ret_from_fork+0x10/0x18 > [ 2.065530] ---[ end trace 75fc17d9000f1224 ]--- > > At first glance, it seems to give more details compared to: > rcar-dmac e7300000.dma-controller: rcar_dmac_prep_slave_sg: bad parameter: len=1, id=19 Which would be followed by sh-sci e6e88000.serial: Failed preparing Tx DMA descriptor pointing to the sh-sci driver, right? The id=19 points to channel 0x13, i.e. SCIF2, according to arch/arm64/boot/dts/renesas/r8a7795.dtsi. > [1] https://patchwork.kernel.org/cover/11012981/ Gr{oetje,eeting}s, Geert
> > [..] > > > - if (rchan->mid_rid < 0 || !sg_len) { > > > + if (rchan->mid_rid < 0 || !sg_len || !sg_dma_len(sgl)) { > > > dev_warn(chan->device->dev, > > > "%s: bad parameter: len=%d, id=%d\n", > > > __func__, sg_len, rchan->mid_rid); > > > > Just wanted to share the WARN output proposed by Wolfram in > > https://patchwork.kernel.org/patch/11012991/#22721733 > > in case the issue discussed in [1] is reproduced with this patch: > > I'm not such a big fan of WARN()... Well, if 'id' points indirectly to the driver, then I agree here that WARN might not be needed. Not as obvious, but probably good enough.
Hi Geert, On Fri, Jun 28, 2019 at 02:10:01PM +0200, Geert Uytterhoeven wrote: > On Wed, Jun 26, 2019 at 8:15 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote: [..] > I'm not such a big fan of WARN()... [..] > > rcar-dmac e7300000.dma-controller: rcar_dmac_prep_slave_sg: bad parameter: len=1, id=19 > > Which would be followed by > > sh-sci e6e88000.serial: Failed preparing Tx DMA descriptor > > pointing to the sh-sci driver, right? > > The id=19 points to channel 0x13, i.e. SCIF2, according to > arch/arm64/boot/dts/renesas/r8a7795.dtsi. Thank you for the detailed rationale. Much appreciated. FTR, the patch landed in vkoul/slave-dma.git, as commit https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/slave-dma.git/commit/?h=next&id=78efb76ab4dfb8f ("dmaengine: rcar-dmac: Reject zero-length slave DMA requests")
diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c index 67df54ac329400b7..9c41a4e425759fcc 100644 --- a/drivers/dma/sh/rcar-dmac.c +++ b/drivers/dma/sh/rcar-dmac.c @@ -1165,7 +1165,7 @@ rcar_dmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, struct rcar_dmac_chan *rchan = to_rcar_dmac_chan(chan); /* Someone calling slave DMA on a generic channel? */ - if (rchan->mid_rid < 0 || !sg_len) { + if (rchan->mid_rid < 0 || !sg_len || !sg_dma_len(sgl)) { dev_warn(chan->device->dev, "%s: bad parameter: len=%d, id=%d\n", __func__, sg_len, rchan->mid_rid);
While the .device_prep_slave_sg() callback rejects empty scatterlists, it still accepts single-entry scatterlists with a zero-length segment. These may happen if a driver calls dmaengine_prep_slave_single() with a zero len parameter. The corresponding DMA request will never complete, leading to messages like: rcar-dmac e7300000.dma-controller: Channel Address Error happen and DMA timeouts. Although requesting a zero-length DMA request is a driver bug, rejecting it early eases debugging. Note that the .device_prep_dma_memcpy() callback already rejects requests to copy zero bytes. Reported-by: Eugeniu Rosca <erosca@de.adit-jv.com> Analyzed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- See "[PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues" (https://lore.kernel.org/linux-renesas-soc/20190624123540.20629-1-geert+renesas@glider.be/) for the driver fix. drivers/dma/sh/rcar-dmac.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)