* Re: [PATCH] dmaengine: rcar-dmac: Reject zero-length slave DMA requests
2019-06-24 12:38 [PATCH] dmaengine: rcar-dmac: Reject zero-length slave DMA requests Geert Uytterhoeven
@ 2019-06-24 12:49 ` Wolfram Sang
2019-06-25 4:38 ` Vinod Koul
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2019-06-24 12:49 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Vinod Koul, Dan Williams, Eugeniu Rosca, Greg Kroah-Hartman,
Jiri Slaby, Yoshihiro Shimoda, Wolfram Sang, linux-serial,
linux-renesas-soc, dmaengine
[-- Attachment #1: Type: text/plain, Size: 229 bytes --]
> 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?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] dmaengine: rcar-dmac: Reject zero-length slave DMA requests
2019-06-24 12:38 [PATCH] dmaengine: rcar-dmac: Reject zero-length slave DMA requests Geert Uytterhoeven
2019-06-24 12:49 ` Wolfram Sang
@ 2019-06-25 4:38 ` Vinod Koul
2019-06-25 4:40 ` Vinod Koul
2019-06-26 18:14 ` Eugeniu Rosca
3 siblings, 0 replies; 8+ messages in thread
From: Vinod Koul @ 2019-06-25 4:38 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Dan Williams, Eugeniu Rosca, Greg Kroah-Hartman, Jiri Slaby,
Yoshihiro Shimoda, Wolfram Sang, linux-serial, linux-renesas-soc,
dmaengine
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
--
~Vinod
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] dmaengine: rcar-dmac: Reject zero-length slave DMA requests
2019-06-24 12:38 [PATCH] dmaengine: rcar-dmac: Reject zero-length slave DMA requests Geert Uytterhoeven
2019-06-24 12:49 ` Wolfram Sang
2019-06-25 4:38 ` Vinod Koul
@ 2019-06-25 4:40 ` Vinod Koul
2019-06-26 18:14 ` Eugeniu Rosca
3 siblings, 0 replies; 8+ messages in thread
From: Vinod Koul @ 2019-06-25 4:40 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Dan Williams, Eugeniu Rosca, Greg Kroah-Hartman, Jiri Slaby,
Yoshihiro Shimoda, Wolfram Sang, linux-serial, linux-renesas-soc,
dmaengine
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
--
~Vinod
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] dmaengine: rcar-dmac: Reject zero-length slave DMA requests
2019-06-24 12:38 [PATCH] dmaengine: rcar-dmac: Reject zero-length slave DMA requests Geert Uytterhoeven
` (2 preceding siblings ...)
2019-06-25 4:40 ` Vinod Koul
@ 2019-06-26 18:14 ` Eugeniu Rosca
2019-06-28 12:10 ` Geert Uytterhoeven
3 siblings, 1 reply; 8+ messages in thread
From: Eugeniu Rosca @ 2019-06-26 18:14 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Vinod Koul, Dan Williams, Greg Kroah-Hartman, Jiri Slaby,
Yoshihiro Shimoda, Wolfram Sang, linux-serial, linux-renesas-soc,
dmaengine, Eugeniu Rosca, Eugeniu Rosca
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/
--
Best regards,
Eugeniu.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] dmaengine: rcar-dmac: Reject zero-length slave DMA requests
2019-06-26 18:14 ` Eugeniu Rosca
@ 2019-06-28 12:10 ` Geert Uytterhoeven
2019-06-28 12:57 ` Wolfram Sang
2019-07-03 15:34 ` Eugeniu Rosca
0 siblings, 2 replies; 8+ messages in thread
From: Geert Uytterhoeven @ 2019-06-28 12:10 UTC (permalink / raw)
To: Eugeniu Rosca
Cc: Geert Uytterhoeven, Vinod Koul, Dan Williams, Greg Kroah-Hartman,
Jiri Slaby, Yoshihiro Shimoda, Wolfram Sang,
open list:SERIAL DRIVERS, Linux-Renesas, dmaengine,
Eugeniu Rosca
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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] dmaengine: rcar-dmac: Reject zero-length slave DMA requests
2019-06-28 12:10 ` Geert Uytterhoeven
@ 2019-06-28 12:57 ` Wolfram Sang
2019-07-03 15:34 ` Eugeniu Rosca
1 sibling, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2019-06-28 12:57 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Eugeniu Rosca, Geert Uytterhoeven, Vinod Koul, Dan Williams,
Greg Kroah-Hartman, Jiri Slaby, Yoshihiro Shimoda, Wolfram Sang,
open list:SERIAL DRIVERS, Linux-Renesas, dmaengine,
Eugeniu Rosca
[-- Attachment #1: Type: text/plain, Size: 701 bytes --]
> > [..]
> > > - 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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] dmaengine: rcar-dmac: Reject zero-length slave DMA requests
2019-06-28 12:10 ` Geert Uytterhoeven
2019-06-28 12:57 ` Wolfram Sang
@ 2019-07-03 15:34 ` Eugeniu Rosca
1 sibling, 0 replies; 8+ messages in thread
From: Eugeniu Rosca @ 2019-07-03 15:34 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Eugeniu Rosca, Geert Uytterhoeven, Vinod Koul, Dan Williams,
Greg Kroah-Hartman, Jiri Slaby, Yoshihiro Shimoda, Wolfram Sang,
open list:SERIAL DRIVERS, Linux-Renesas, dmaengine,
Eugeniu Rosca
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")
--
Best Regards,
Eugeniu.
^ permalink raw reply [flat|nested] 8+ messages in thread