[PATCH 0/6] Zap SCIF2 DMA configuration in R-Car Gen3 DTS

[PATCH 0/6] Zap SCIF2 DMA configuration in R-Car Gen3 DTS

[Eugeniu Rosca]

This series is triggered by a regression on M3 targets caused by
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=703db5d1b175
("arm64: dts: renesas: r8a7796: Enable DMA for SCIF2"), when applied
on top of rcar-3.9.x Renesas official kernel.

This collection of patches attempts to consistently propagate the fix
across the existing R-Car3 DTS. Full story is placed into
commit ("Revert "arm64: dts: renesas: r8a7796: Enable DMA for SCIF2"").

While debugging drivers/tty/serial/sh-sci.c, a minor update avoiding
__ptrval__ in dev_dbg() is included here as well.

Tested using v5.1-rc7-131-gea9866793d1e on:
 - H3-ES2.0-ULCB
 - M3N-ES1.1-ULCB
 - M3-ES1.1-Salvator-XS

Eugeniu Rosca (6):
  serial: sh-sci: Reveal ptrval in dev_dbg
  Revert "arm64: dts: renesas: r8a7796: Enable DMA for SCIF2"
  arm64: dts: renesas: r8a7795: zap dma configuration in scif2
  Revert "arm64: dts: renesas: r8a77965: Enable DMA for SCIF2"
  Revert "arm64: dts: renesas: r8a77990: Enable DMA for SCIF2"
  Revert "arm64: dts: renesas: r8a77995: add DMA for SCIF2"

 arch/arm64/boot/dts/renesas/r8a7795.dtsi  | 3 ---
 arch/arm64/boot/dts/renesas/r8a7796.dtsi  | 3 ---
 arch/arm64/boot/dts/renesas/r8a77965.dtsi | 3 ---
 arch/arm64/boot/dts/renesas/r8a77990.dtsi | 4 +---
 arch/arm64/boot/dts/renesas/r8a77995.dtsi | 3 ---
 drivers/tty/serial/sh-sci.c               | 8 ++++----
 6 files changed, 5 insertions(+), 19 deletions(-)

-- 
2.21.0

[PATCH 1/6] serial: sh-sci: Reveal ptrval in dev_dbg

[Eugeniu Rosca]

Starting with v4.15-rc2 commit ad67b74d2469d9 ("printk: hash addresses
printed with %p"), enabling debug prints in sh-sci.c would generate
output like below confusing the users who try to sneak into the
internals of the driver:

sh-sci e6e88000.serial: sci_request_dma: TX: got channel (____ptrval____)
sh-sci e6e88000.serial: sci_request_dma: mapped 4096@(____ptrval____) to 0x00000006798bf000
sh-sci e6e88000.serial: sci_request_dma: RX: got channel (____ptrval____)
sh-sci e6e88000.serial: sci_dma_tx_work_fn: (____ptrval____): 0...2, cookie 2

There are two possible fixes for that:
 - get rid of '%p' prints if they don't reveal any useful information
 - s/%p/%px/, since it is unlikely we have any concerns leaking the
   pointer values when running a debug/non-production kernel

Go second route to preserve original debug output and minimize the diff.

Fixes: ad67b74d2469d9 ("printk: hash addresses printed with %p")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 drivers/tty/serial/sh-sci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 3cd139752d3f..82660f8e6d86 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1434,7 +1434,7 @@ static void sci_dma_tx_work_fn(struct work_struct *work)
 		goto switch_to_pio;
 	}
 
-	dev_dbg(port->dev, "%s: %p: %d...%d, cookie %d\n",
+	dev_dbg(port->dev, "%s: %px: %d...%d, cookie %d\n",
 		__func__, xmit->buf, xmit->tail, xmit->head, s->cookie_tx);
 
 	dma_async_issue_pending(chan);
@@ -1570,7 +1570,7 @@ static void sci_request_dma(struct uart_port *port)
 		return;
 
 	chan = sci_request_dma_chan(port, DMA_MEM_TO_DEV);
-	dev_dbg(port->dev, "%s: TX: got channel %p\n", __func__, chan);
+	dev_dbg(port->dev, "%s: TX: got channel %px\n", __func__, chan);
 	if (chan) {
 		/* UART circular tx buffer is an aligned page. */
 		s->tx_dma_addr = dma_map_single(chan->device->dev,
@@ -1581,7 +1581,7 @@ static void sci_request_dma(struct uart_port *port)
 			dev_warn(port->dev, "Failed mapping Tx DMA descriptor\n");
 			dma_release_channel(chan);
 		} else {
-			dev_dbg(port->dev, "%s: mapped %lu@%p to %pad\n",
+			dev_dbg(port->dev, "%s: mapped %lu@%px to %pad\n",
 				__func__, UART_XMIT_SIZE,
 				port->state->xmit.buf, &s->tx_dma_addr);
 
@@ -1591,7 +1591,7 @@ static void sci_request_dma(struct uart_port *port)
 	}
 
 	chan = sci_request_dma_chan(port, DMA_DEV_TO_MEM);
-	dev_dbg(port->dev, "%s: RX: got channel %p\n", __func__, chan);
+	dev_dbg(port->dev, "%s: RX: got channel %px\n", __func__, chan);
 	if (chan) {
 		unsigned int i;
 		dma_addr_t dma;
-- 
2.21.0

Re: [PATCH 1/6] serial: sh-sci: Reveal ptrval in dev_dbg

[Simon Horman]

On Sat, May 04, 2019 at 02:42:53AM +0200, Eugeniu Rosca wrote:
> Starting with v4.15-rc2 commit ad67b74d2469d9 ("printk: hash addresses
> printed with %p"), enabling debug prints in sh-sci.c would generate
> output like below confusing the users who try to sneak into the
> internals of the driver:
> 
> sh-sci e6e88000.serial: sci_request_dma: TX: got channel (____ptrval____)
> sh-sci e6e88000.serial: sci_request_dma: mapped 4096@(____ptrval____) to 0x00000006798bf000
> sh-sci e6e88000.serial: sci_request_dma: RX: got channel (____ptrval____)
> sh-sci e6e88000.serial: sci_dma_tx_work_fn: (____ptrval____): 0...2, cookie 2
> 
> There are two possible fixes for that:
>  - get rid of '%p' prints if they don't reveal any useful information
>  - s/%p/%px/, since it is unlikely we have any concerns leaking the
>    pointer values when running a debug/non-production kernel

I am concerned that this may expose information in circumstances
where it is undesirable. Is it generally accepted practice to
use %px in conjunction with dev_dbg() ?

...

Re: [PATCH 1/6] serial: sh-sci: Reveal ptrval in dev_dbg

[Eugeniu Rosca]

On Mon, May 06, 2019 at 03:47:05PM +0200, Simon Horman wrote:
> On Sat, May 04, 2019 at 02:42:53AM +0200, Eugeniu Rosca wrote:
> > Starting with v4.15-rc2 commit ad67b74d2469d9 ("printk: hash addresses
> > printed with %p"), enabling debug prints in sh-sci.c would generate
> > output like below confusing the users who try to sneak into the
> > internals of the driver:
> > 
> > sh-sci e6e88000.serial: sci_request_dma: TX: got channel (____ptrval____)
> > sh-sci e6e88000.serial: sci_request_dma: mapped 4096@(____ptrval____) to 0x00000006798bf000
> > sh-sci e6e88000.serial: sci_request_dma: RX: got channel (____ptrval____)
> > sh-sci e6e88000.serial: sci_dma_tx_work_fn: (____ptrval____): 0...2, cookie 2
> > 
> > There are two possible fixes for that:
> >  - get rid of '%p' prints if they don't reveal any useful information
> >  - s/%p/%px/, since it is unlikely we have any concerns leaking the
> >    pointer values when running a debug/non-production kernel
> 
> I am concerned that this may expose information in circumstances
> where it is undesirable. Is it generally accepted practice to
> use %px in conjunction with dev_dbg() ?
> 
> ...

Below commits performed a similar s/%p/%px/ update in debug context:

Authors (CC-ed)   Commit         Subject
----------------------------------------
Christophe Leroy  b18f0ae92b0a1d ("powerpc/prom: fix early DEBUG messages")
Helge Deller      3847dab7742186 ("parisc: Add alternative coding infrastructure")
Michael Neuling   51c3c62b58b357 ("powerpc: Avoid code patching freed init sections")
Kuninori Morimoto dabdbe3ae0cb9a ("ASoC: rsnd: don't use %p for dev_dbg()")
Philip Yang       fa7e65147e5dca ("drm/amdkfd: use %px to print user space address instead of %p")
Matthew Wilcox    68c1f08203f2b0 ("lib/list_debug.c: print unmangled addresses")
Borislav Petkov   0e6c16c652cada ("x86/alternative: Print unadorned pointers")
Darrick J. Wong   c96900435fa9fd ("xfs: use %px for data pointers when debugging")
Helge Deller      04903c06b4854d ("parisc: Show unhashed HPA of Dino chip")

To quote Matthew, with respect to any debug prints:
If an attacker can force this message to be printed, we've already lost.

In any case, I won't be affected much if the change is not accepted,
since it doesn't resolve any major issue on my end. Thanks!

-- 
Best Regards,
Eugeniu.

Re: [PATCH 1/6] serial: sh-sci: Reveal ptrval in dev_dbg

[Geert Uytterhoeven]

Hi Eugeniu,

On Mon, May 6, 2019 at 5:24 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> On Mon, May 06, 2019 at 03:47:05PM +0200, Simon Horman wrote:
> > On Sat, May 04, 2019 at 02:42:53AM +0200, Eugeniu Rosca wrote:
> > > Starting with v4.15-rc2 commit ad67b74d2469d9 ("printk: hash addresses
> > > printed with %p"), enabling debug prints in sh-sci.c would generate
> > > output like below confusing the users who try to sneak into the
> > > internals of the driver:
> > >
> > > sh-sci e6e88000.serial: sci_request_dma: TX: got channel (____ptrval____)
> > > sh-sci e6e88000.serial: sci_request_dma: mapped 4096@(____ptrval____) to 0x00000006798bf000
> > > sh-sci e6e88000.serial: sci_request_dma: RX: got channel (____ptrval____)
> > > sh-sci e6e88000.serial: sci_dma_tx_work_fn: (____ptrval____): 0...2, cookie 2
> > >
> > > There are two possible fixes for that:
> > >  - get rid of '%p' prints if they don't reveal any useful information
> > >  - s/%p/%px/, since it is unlikely we have any concerns leaking the
> > >    pointer values when running a debug/non-production kernel
> >
> > I am concerned that this may expose information in circumstances
> > where it is undesirable. Is it generally accepted practice to
> > use %px in conjunction with dev_dbg() ?
> >
> > ...
>
> Below commits performed a similar s/%p/%px/ update in debug context:
>
> Authors (CC-ed)   Commit         Subject
> ----------------------------------------
> Christophe Leroy  b18f0ae92b0a1d ("powerpc/prom: fix early DEBUG messages")
> Helge Deller      3847dab7742186 ("parisc: Add alternative coding infrastructure")
> Michael Neuling   51c3c62b58b357 ("powerpc: Avoid code patching freed init sections")
> Kuninori Morimoto dabdbe3ae0cb9a ("ASoC: rsnd: don't use %p for dev_dbg()")
> Philip Yang       fa7e65147e5dca ("drm/amdkfd: use %px to print user space address instead of %p")
> Matthew Wilcox    68c1f08203f2b0 ("lib/list_debug.c: print unmangled addresses")
> Borislav Petkov   0e6c16c652cada ("x86/alternative: Print unadorned pointers")
> Darrick J. Wong   c96900435fa9fd ("xfs: use %px for data pointers when debugging")
> Helge Deller      04903c06b4854d ("parisc: Show unhashed HPA of Dino chip")
>
> To quote Matthew, with respect to any debug prints:
> If an attacker can force this message to be printed, we've already lost.

I think the issue with using %px in debug code is that a distro may enable
CONFIG_DYNAMIC_DEBUG (it is enabled in several defconfigs), after which
an attacker just has to convince/trick the system into enabling debug for that
particular driver.

> In any case, I won't be affected much if the change is not accepted,
> since it doesn't resolve any major issue on my end. Thanks!

OK.

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

Re: [PATCH 1/6] serial: sh-sci: Reveal ptrval in dev_dbg

[Eugeniu Rosca]

On Mon, May 06, 2019 at 06:46:57PM +0200, Geert Uytterhoeven wrote:
> Hi Eugeniu,
> 
> On Mon, May 6, 2019 at 5:24 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> > On Mon, May 06, 2019 at 03:47:05PM +0200, Simon Horman wrote:
> > > On Sat, May 04, 2019 at 02:42:53AM +0200, Eugeniu Rosca wrote:
> > > > Starting with v4.15-rc2 commit ad67b74d2469d9 ("printk: hash addresses
> > > > printed with %p"), enabling debug prints in sh-sci.c would generate
> > > > output like below confusing the users who try to sneak into the
> > > > internals of the driver:
> > > >
> > > > sh-sci e6e88000.serial: sci_request_dma: TX: got channel (____ptrval____)
> > > > sh-sci e6e88000.serial: sci_request_dma: mapped 4096@(____ptrval____) to 0x00000006798bf000
> > > > sh-sci e6e88000.serial: sci_request_dma: RX: got channel (____ptrval____)
> > > > sh-sci e6e88000.serial: sci_dma_tx_work_fn: (____ptrval____): 0...2, cookie 2
> > > >
> > > > There are two possible fixes for that:
> > > >  - get rid of '%p' prints if they don't reveal any useful information
> > > >  - s/%p/%px/, since it is unlikely we have any concerns leaking the
> > > >    pointer values when running a debug/non-production kernel
> > >
> > > I am concerned that this may expose information in circumstances
> > > where it is undesirable. Is it generally accepted practice to
> > > use %px in conjunction with dev_dbg() ?
> > >
> > > ...
> >
> > Below commits performed a similar s/%p/%px/ update in debug context:
> >
> > Authors (CC-ed)   Commit         Subject
> > ----------------------------------------
> > Christophe Leroy  b18f0ae92b0a1d ("powerpc/prom: fix early DEBUG messages")
> > Helge Deller      3847dab7742186 ("parisc: Add alternative coding infrastructure")
> > Michael Neuling   51c3c62b58b357 ("powerpc: Avoid code patching freed init sections")
> > Kuninori Morimoto dabdbe3ae0cb9a ("ASoC: rsnd: don't use %p for dev_dbg()")
> > Philip Yang       fa7e65147e5dca ("drm/amdkfd: use %px to print user space address instead of %p")
> > Matthew Wilcox    68c1f08203f2b0 ("lib/list_debug.c: print unmangled addresses")
> > Borislav Petkov   0e6c16c652cada ("x86/alternative: Print unadorned pointers")
> > Darrick J. Wong   c96900435fa9fd ("xfs: use %px for data pointers when debugging")
> > Helge Deller      04903c06b4854d ("parisc: Show unhashed HPA of Dino chip")
> >
> > To quote Matthew, with respect to any debug prints:
> > If an attacker can force this message to be printed, we've already lost.
> 
> I think the issue with using %px in debug code is that a distro may enable
> CONFIG_DYNAMIC_DEBUG (it is enabled in several defconfigs), after which
> an attacker just has to convince/trick the system into enabling debug for that
> particular driver.

How about going the route of commit c96900435fa9fd ("xfs: use %px for
data pointers when debugging"), i.e. s/%p/"PTR_FMT"/ like below (this
would enable the expected debug output only on manually defining DEBUG
in the *.c file, while still keeping the output hashed on
DYNAMIC_DEBUG=y if DEBUG is undefined).

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 3cd139752d3f..69cd87c5ef0c 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -56,6 +56,12 @@
 #include <asm/sh_bios.h>
 #endif
 
+#ifdef DEBUG
+#define PTR_FMT "%px"
+#else
+#define PTR_FMT "%p"
+#endif
+
 #include "serial_mctrl_gpio.h"
 #include "sh-sci.h"
 
@@ -1434,7 +1440,7 @@ static void sci_dma_tx_work_fn(struct work_struct *work)
 		goto switch_to_pio;
 	}
 
-	dev_dbg(port->dev, "%s: %p: %d...%d, cookie %d\n",
+	dev_dbg(port->dev, "%s: "PTR_FMT": %d...%d, cookie %d\n",
 		__func__, xmit->buf, xmit->tail, xmit->head, s->cookie_tx);
 
 	dma_async_issue_pending(chan);

> 
> > In any case, I won't be affected much if the change is not accepted,
> > since it doesn't resolve any major issue on my end. Thanks!
> 
> OK.
> 
> 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

-- 
Best Regards,
Eugeniu.

[PATCH 2/6] Revert "arm64: dts: renesas: r8a7796: Enable DMA for SCIF2"

[Eugeniu Rosca]

This reverts commit 97f26702bc95b5c3a72671d5c6675e4d6ee0a2f4.

Here is the story behind this revert.

Mainline commit [0] landed in the stable tree as commit [1], from where
it reached us in the form of regular stable update. After that, Michael
started to report occasional (30-50%) freezes of serial console on
booting M3-ES1.1-Salvator-XS. Same happened on M3-ES1.1-Salvator-X.

Every time the issue occurs, the serial console outputs below [2]
before becoming totally unresponsive and printing nothing else:
  rcar-dmac e7300000.dma-controller: Channel Address Error

Git bisecting shows that the problem is contributed by commits [0-1].

While we can't be 100% certain (since we don't have the SCIF design docs
revealing its internal implementation detail) we think there is plenty
of evidence to assume that DMA is not supported on SCIF2, hence should
stay disabled on this specific channel:

 - Excerpt from Chapter 17. Direct Memory Access Controller for System
   (SYS-DMAC) of R19UH0105EJ0150 Rev.1.50:
   ---------8<---------
   [H3, H3-N, M3-W, V3M, V3H, D3, M3-N, E3]
   The following modules can issue on-chip peripheral module requests.
   [..] HSCIF0/1/2/3/4, [..] SCIF0/1/3/4/5,
   ---------8<---------

 - Excerpt from RENESAS_RCH3M3M3NE3_SCIF_UME_v2.00.pdf (Yocto v3.15.0):
   ---------8<---------
   DMA Transfer:
   - Support: SCIF0, SCIF1, SCIF3, SCIF4, SCIF5
   - Not support: SCIF2
   ---------8<---------

 - Disabled SCIF2 DMA in official Renesas v4.9/v4.14 kernels, e.g. see:
   https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=e79c418fda8c

Based on the issues generated by [0-1] (reproduced on H3, M3 and M3N)
and the doc statements presented above, we think it makes sense to
disable DMA on SCIF2 for most/all R-Car3 SoCs.

[0] v5.0-rc6 commit 97f26702bc95b5 ("arm64: dts: renesas: r8a7796: Enable DMA for SCIF2")
[1] v4.14.106 commit 703db5d1b1759f ("arm64: dts: renesas: r8a7796: Enable DMA for SCIF2")
[2] scif (DEBUG) and rcar-dmac logs:
    https://gist.github.com/erosca/132cce76a619724a9e4fa61d1db88c66

Fixes: 97f26702bc95b5 ("arm64: dts: renesas: r8a7796: Enable DMA for SCIF2")
Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 arch/arm64/boot/dts/renesas/r8a7796.dtsi | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
index cdf784899cf8..23de63f3d6c3 100644
--- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
@@ -1262,9 +1262,6 @@
 				 <&cpg CPG_CORE R8A7796_CLK_S3D1>,
 				 <&scif_clk>;
 			clock-names = "fck", "brg_int", "scif_clk";
-			dmas = <&dmac1 0x13>, <&dmac1 0x12>,
-			       <&dmac2 0x13>, <&dmac2 0x12>;
-			dma-names = "tx", "rx", "tx", "rx";
 			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
 			resets = <&cpg 310>;
 			status = "disabled";
-- 
2.21.0

Re: [PATCH 2/6] Revert "arm64: dts: renesas: r8a7796: Enable DMA for SCIF2"

[Geert Uytterhoeven]

Hi Eugeniu,

Thanks for your report!

On Sat, May 4, 2019 at 2:45 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> This reverts commit 97f26702bc95b5c3a72671d5c6675e4d6ee0a2f4.
>
> Here is the story behind this revert.
>
> Mainline commit [0] landed in the stable tree as commit [1], from where
> it reached us in the form of regular stable update. After that, Michael
> started to report occasional (30-50%) freezes of serial console on
> booting M3-ES1.1-Salvator-XS. Same happened on M3-ES1.1-Salvator-X.
>
> Every time the issue occurs, the serial console outputs below [2]
> before becoming totally unresponsive and printing nothing else:
>   rcar-dmac e7300000.dma-controller: Channel Address Error
>
> Git bisecting shows that the problem is contributed by commits [0-1].
>
> While we can't be 100% certain (since we don't have the SCIF design docs
> revealing its internal implementation detail) we think there is plenty
> of evidence to assume that DMA is not supported on SCIF2, hence should
> stay disabled on this specific channel:
>
>  - Excerpt from Chapter 17. Direct Memory Access Controller for System
>    (SYS-DMAC) of R19UH0105EJ0150 Rev.1.50:
>    ---------8<---------
>    [H3, H3-N, M3-W, V3M, V3H, D3, M3-N, E3]
>    The following modules can issue on-chip peripheral module requests.
>    [..] HSCIF0/1/2/3/4, [..] SCIF0/1/3/4/5,
>    ---------8<---------
>
>  - Excerpt from RENESAS_RCH3M3M3NE3_SCIF_UME_v2.00.pdf (Yocto v3.15.0):
>    ---------8<---------
>    DMA Transfer:
>    - Support: SCIF0, SCIF1, SCIF3, SCIF4, SCIF5
>    - Not support: SCIF2
>    ---------8<---------

>  - Disabled SCIF2 DMA in official Renesas v4.9/v4.14 kernels, e.g. see:
>    https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=e79c418fda8c

Table 17.5 ("Selecting On-Chip Peripheral Module Request Modes") of
"R-Car Series, 3rd Generation User’s Manual: Hardware" gained entries
for SCIF2 in Revision 1.50 of the document, but it seems 17.1.1
("Features") and Table 17.6 ("Data Length of DMA Transfer for Each of
the On-Chip Peripheral Modules") were forgotten to be updated.
The addition of the entry for SCIF2 is also mentioned in "Renesas
Technical Update  TN-RCT-S019A/E / R-Car M3-W Additional Explanation for
Direct Memory Access Controller for System (SYS-DMAC)".
Unfortunately both documents report wrong MID/RID values, due to a
hexadecimal vs. decimal mistake, which were corrected in the Feb 12
errata for Rev. 1.50.

So in my understanding, and according to my testing, DMA has always
worked for SCIF2 on (at least) R-Car H3 ES1.0/2.0, M3-W, and M3-N.
However, early firmware versions (before IPL and Secure Monitor
Rev1.0.6, released on Feb 25, 2016) prohibited the use of SYS-DMAC2,
cfr. commit eb21089c32054ecd ("arm64: dts: renesas: r8a7795: Add missing
SYS-DMAC2 dmas").

Perhaps some firmware versions may impose additional restrictions?

> Based on the issues generated by [0-1] (reproduced on H3, M3 and M3N)
> and the doc statements presented above, we think it makes sense to
> disable DMA on SCIF2 for most/all R-Car3 SoCs.
>
> [0] v5.0-rc6 commit 97f26702bc95b5 ("arm64: dts: renesas: r8a7796: Enable DMA for SCIF2")
> [1] v4.14.106 commit 703db5d1b1759f ("arm64: dts: renesas: r8a7796: Enable DMA for SCIF2")
> [2] scif (DEBUG) and rcar-dmac logs:
>     https://gist.github.com/erosca/132cce76a619724a9e4fa61d1db88c66

I have checked my kernel logs, and found a few instances of "Channel
Address Error".  In all cases, I had enabled/added extra debug prints in
the sh-sci driver, which may have had impact.
Last occurrence was in a kernel based on v4.18-rc2, which predates
several recent fixes for the sh-sci and rcar-dmac drivers.
Can the issue be reproduced on current mainline?

Thanks!

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

Re: [PATCH 2/6] Revert "arm64: dts: renesas: r8a7796: Enable DMA for SCIF2"

[Eugeniu Rosca]

Hi Geert,

On Mon, May 06, 2019 at 12:02:41PM +0200, Geert Uytterhoeven wrote:
> Hi Eugeniu,
> 
> Thanks for your report!

Thanks for your feedback.

> 
> On Sat, May 4, 2019 at 2:45 AM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> > This reverts commit 97f26702bc95b5c3a72671d5c6675e4d6ee0a2f4.
> >
> > Here is the story behind this revert.
> >
> > Mainline commit [0] landed in the stable tree as commit [1], from where
> > it reached us in the form of regular stable update. After that, Michael
> > started to report occasional (30-50%) freezes of serial console on
> > booting M3-ES1.1-Salvator-XS. Same happened on M3-ES1.1-Salvator-X.
> >
> > Every time the issue occurs, the serial console outputs below [2]
> > before becoming totally unresponsive and printing nothing else:
> >   rcar-dmac e7300000.dma-controller: Channel Address Error
> >
> > Git bisecting shows that the problem is contributed by commits [0-1].
> >
> > While we can't be 100% certain (since we don't have the SCIF design docs
> > revealing its internal implementation detail) we think there is plenty
> > of evidence to assume that DMA is not supported on SCIF2, hence should
> > stay disabled on this specific channel:
> >
> >  - Excerpt from Chapter 17. Direct Memory Access Controller for System
> >    (SYS-DMAC) of R19UH0105EJ0150 Rev.1.50:
> >    ---------8<---------
> >    [H3, H3-N, M3-W, V3M, V3H, D3, M3-N, E3]
> >    The following modules can issue on-chip peripheral module requests.
> >    [..] HSCIF0/1/2/3/4, [..] SCIF0/1/3/4/5,
> >    ---------8<---------
> >
> >  - Excerpt from RENESAS_RCH3M3M3NE3_SCIF_UME_v2.00.pdf (Yocto v3.15.0):
> >    ---------8<---------
> >    DMA Transfer:
> >    - Support: SCIF0, SCIF1, SCIF3, SCIF4, SCIF5
> >    - Not support: SCIF2
> >    ---------8<---------
> 
> >  - Disabled SCIF2 DMA in official Renesas v4.9/v4.14 kernels, e.g. see:
> >    https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=e79c418fda8c
> 
> Table 17.5 ("Selecting On-Chip Peripheral Module Request Modes") of
> "R-Car Series, 3rd Generation User’s Manual: Hardware" gained entries
> for SCIF2 in Revision 1.50 of the document, but it seems 17.1.1
> ("Features") and Table 17.6 ("Data Length of DMA Transfer for Each of
> the On-Chip Peripheral Modules") were forgotten to be updated.
> The addition of the entry for SCIF2 is also mentioned in "Renesas
> Technical Update  TN-RCT-S019A/E / R-Car M3-W Additional Explanation for
> Direct Memory Access Controller for System (SYS-DMAC)".
> Unfortunately both documents report wrong MID/RID values, due to a
> hexadecimal vs. decimal mistake, which were corrected in the Feb 12
> errata for Rev. 1.50.

I do observe now that the most recent Rev. 1.50 of "R-Car Series, 3rd
Generation User’s Manual: Hardware" does update _some_ of its internal
chapters/tables to reflect the support of DMA on SCIF2. These SCIF2
changes look to be also tracked in the "Revision History" companion doc:

Rev  | Date         | Page     | Summary
1.50 | Nov 30, 2018 | 17-86-87 | Table 17.5 Selecting On-Chip Peripheral
                                 Module Request Modes: DMA Transfer
                                 Request Source, changed. SCIF2
                                 reception and SCIF2 transmission, added
                    | 17-91    | Table 17.6 Data Length of DMA Transfer
                                 for Each of the On-Chip Peripheral
                                 Modules: SCIF2, added

As you have already stated, it looks like certain chapters like
"17.1.1 Features" didn't receive a proper update, generating confusion.
I will report this in parallel to Renesas Duesseldorf.

> 
> So in my understanding, and according to my testing, DMA has always
> worked for SCIF2 on (at least) R-Car H3 ES1.0/2.0, M3-W, and M3-N.

Well, my testing shows different results. Using M3-W-ES1.1-Salvator-XS,
I can reproduce the issue since v4.17 (also reproduced on v4.18, v4.19
and v5.1 with cherry picking 97f26702bc95b5 ("arm64: dts: renesas:
r8a7796: Enable DMA for SCIF2") where appropriate).

> However, early firmware versions (before IPL and Secure Monitor
> Rev1.0.6, released on Feb 25, 2016) prohibited the use of SYS-DMAC2,
> cfr. commit eb21089c32054ecd ("arm64: dts: renesas: r8a7795: Add missing
> SYS-DMAC2 dmas").

I use a very recent Rev2.0.2 of
https://github.com/renesas-rcar/arm-trusted-firmware .

> 
> Perhaps some firmware versions may impose additional restrictions?

I would have some suspicions about ATF if the issue was consistent.
Since it is not, I believe there is a race going on in the kernel.

> 
> > Based on the issues generated by [0-1] (reproduced on H3, M3 and M3N)
> > and the doc statements presented above, we think it makes sense to
> > disable DMA on SCIF2 for most/all R-Car3 SoCs.
> >
> > [0] v5.0-rc6 commit 97f26702bc95b5 ("arm64: dts: renesas: r8a7796: Enable DMA for SCIF2")
> > [1] v4.14.106 commit 703db5d1b1759f ("arm64: dts: renesas: r8a7796: Enable DMA for SCIF2")
> > [2] scif (DEBUG) and rcar-dmac logs:
> >     https://gist.github.com/erosca/132cce76a619724a9e4fa61d1db88c66
> 
> I have checked my kernel logs, and found a few instances of "Channel
> Address Error".  In all cases, I had enabled/added extra debug prints in
> the sh-sci driver, which may have had impact.
> Last occurrence was in a kernel based on v4.18-rc2, which predates
> several recent fixes for the sh-sci and rcar-dmac drivers.
> Can the issue be reproduced on current mainline?

With pure vanilla sources, arm64 defconfig and DTS (+97f26702bc95b5
where appropriate), the issue is seen on M3-W-ES1.1-Salvator-XS since
v4.17. Can you please confirm you are seeing it too?

Enabling DEBUG in drivers/dma/sh/rcar-dmac.c, I can notice that one of
the symptoms is a NULL dst_addr revealed by:

rcar-dmac e7300000.dma-controller: chan0: queue chunk (____ptrval____): 0@0xffff800639eb8090 -> 0x0000000000000000

In working scenarios, dst_addr is never zero. Does it give any hints?

> 
> Thanks!

Likewise!

> 
> 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

-- 
Best Regards,
Eugeniu.

Re: [PATCH] serial: sh-sci: disable DMA for uart_console

[Eugeniu Rosca]

Hi George,

I am able to reproduce the SCIF2 console freeze described in the
referenced patchwork link using M3-ES1.1-Salvator-XS and recent
v5.1-9573-gb970afcfcabd kernel.

I confirm the behavior is healed with this patch. Thanks!
Hope to see it accepted soon, since it fixes a super annoying
console breakage every fourth boot or so on lots of R-Car3 targets.

Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>

On Thu, May 09, 2019 at 10:43:30AM -0400, George G. Davis wrote:
> As noted in commit 84b40e3b57ee ("serial: 8250: omap: Disable DMA for
> console UART"), UART console lines use low-level PIO only access functions
> which will conflict with use of the line when DMA is enabled, e.g. when
> the console line is also used for systemd messages. So disable DMA
> support for UART console lines.
> 
> Fixes: https://patchwork.kernel.org/patch/10929511/
> Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
> Cc: Eugeniu Rosca <erosca@de.adit-jv.com>
> Signed-off-by: George G. Davis <george_davis@mentor.com>
> ---
>  drivers/tty/serial/sh-sci.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 3cd139752d3f..885b56b1d4e4 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1557,6 +1557,9 @@ static void sci_request_dma(struct uart_port *port)
>  
>  	dev_dbg(port->dev, "%s: port %d\n", __func__, port->line);
>  
> +	if (uart_console(port))
> +		return; /* Cannot use DMA on console */
> +
>  	if (!port->dev->of_node)
>  		return;
>  
> -- 
> 2.7.4
> 

-- 
Best Regards,
Eugeniu.

Re: [PATCH] serial: sh-sci: disable DMA for uart_console

[George G. Davis]

Hello Eugeniu,

On Fri, May 10, 2019 at 07:10:21PM +0200, Eugeniu Rosca wrote:
> Hi George,
> 
> I am able to reproduce the SCIF2 console freeze described in the
> referenced patchwork link using M3-ES1.1-Salvator-XS and recent
> v5.1-9573-gb970afcfcabd kernel.
> 
> I confirm the behavior is healed with this patch. Thanks!
> Hope to see it accepted soon, since it fixes a super annoying
> console breakage every fourth boot or so on lots of R-Car3 targets.
> 
> Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>

Thanks for testing.

Also note, for the record, that the problem is not limited to SCIF2, e.g. try
setting console=ttySC<n> wheren <n> is not SCIF2 on any other board which
includes support for other serial ports, e.g. r8a7795-salvator-x, and you will
observe the same problem on other SCIF ports too. It's just a concidence that
most boards use SCIF2 as the default serial console where the console hangs
(resolved by this patch) have been observed on multiple boards.

> 
> On Thu, May 09, 2019 at 10:43:30AM -0400, George G. Davis wrote:
> > As noted in commit 84b40e3b57ee ("serial: 8250: omap: Disable DMA for
> > console UART"), UART console lines use low-level PIO only access functions
> > which will conflict with use of the line when DMA is enabled, e.g. when
> > the console line is also used for systemd messages. So disable DMA
> > support for UART console lines.
> > 
> > Fixes: https://patchwork.kernel.org/patch/10929511/
> > Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
> > Cc: Eugeniu Rosca <erosca@de.adit-jv.com>
> > Signed-off-by: George G. Davis <george_davis@mentor.com>
> > ---
> >  drivers/tty/serial/sh-sci.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> > index 3cd139752d3f..885b56b1d4e4 100644
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -1557,6 +1557,9 @@ static void sci_request_dma(struct uart_port *port)
> >  
> >  	dev_dbg(port->dev, "%s: port %d\n", __func__, port->line);
> >  
> > +	if (uart_console(port))
> > +		return; /* Cannot use DMA on console */
> > +
> >  	if (!port->dev->of_node)
> >  		return;
> >  
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Best Regards,
> Eugeniu.

-- 
Regards,
George

Re: [PATCH] serial: sh-sci: disable DMA for uart_console

[Eugeniu Rosca]

Hi George,

On Fri, May 10, 2019 at 02:38:47PM -0400, George G. Davis wrote:
> Hello Eugeniu,
> 
> On Fri, May 10, 2019 at 07:10:21PM +0200, Eugeniu Rosca wrote:
> > Hi George,
> > 
> > I am able to reproduce the SCIF2 console freeze described in the
> > referenced patchwork link using M3-ES1.1-Salvator-XS and recent
> > v5.1-9573-gb970afcfcabd kernel.
> > 
> > I confirm the behavior is healed with this patch. Thanks!
> > Hope to see it accepted soon, since it fixes a super annoying
> > console breakage every fourth boot or so on lots of R-Car3 targets.
> > 
> > Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> 
> Thanks for testing.
> 
> Also note, for the record, that the problem is not limited to SCIF2, e.g. try
> setting console=ttySC<n> wheren <n> is not SCIF2 on any other board which
> includes support for other serial ports, e.g. r8a7795-salvator-x, and you will
> observe the same problem on other SCIF ports too. It's just a concidence that
> most boards use SCIF2 as the default serial console where the console hangs
> (resolved by this patch) have been observed on multiple boards.

Thanks for the additional level of detail.

FTR, trying to track the origin of the problem, it looks to me that the
issue was _unmasked_ by v4.16-rc1 commit be7e251d20e6c8 ("tty: serial:
sh-sci: Hide DMA config question") which turned on DMA on SCIF by
default.

I wonder if it'd be helpful to resend the patch w/o using --in-reply-to,
so that it appears as standalone entry in linux-renesas-soc patchwork.
Currently, assuming that the R-Car maintainers filter out any "Rejected"
patches (which is the default patchwork behavior), your patch would be
hidden from their eye.

-- 
Best Regards,
Eugeniu.

Re: [PATCH] serial: sh-sci: disable DMA for uart_console

[Geert Uytterhoeven]

Hi George,

On Thu, May 9, 2019 at 4:44 PM George G. Davis <ggdavisiv@gmail.com> wrote:
> As noted in commit 84b40e3b57ee ("serial: 8250: omap: Disable DMA for
> console UART"), UART console lines use low-level PIO only access functions
> which will conflict with use of the line when DMA is enabled, e.g. when
> the console line is also used for systemd messages. So disable DMA
> support for UART console lines.
>
> Fixes: https://patchwork.kernel.org/patch/10929511/
> Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
> Cc: Eugeniu Rosca <erosca@de.adit-jv.com>
> Signed-off-by: George G. Davis <george_davis@mentor.com>

I think this makes sense.  In addition to OMAP 8250, the same approach
is used in the Mediatek 8250 and iMX serial drivers.

Regardless, this is definitely better than removing the "dmas" properties
from DT, as DT describes hardware, not usage policies.

Anyone else with a comment?

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

Re: [PATCH] serial: sh-sci: disable DMA for uart_console

[Simon Horman]

On Mon, May 13, 2019 at 01:13:16PM +0200, Geert Uytterhoeven wrote:
> Hi George,
> 
> On Thu, May 9, 2019 at 4:44 PM George G. Davis <ggdavisiv@gmail.com> wrote:
> > As noted in commit 84b40e3b57ee ("serial: 8250: omap: Disable DMA for
> > console UART"), UART console lines use low-level PIO only access functions
> > which will conflict with use of the line when DMA is enabled, e.g. when
> > the console line is also used for systemd messages. So disable DMA
> > support for UART console lines.
> >
> > Fixes: https://patchwork.kernel.org/patch/10929511/
> > Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
> > Cc: Eugeniu Rosca <erosca@de.adit-jv.com>
> > Signed-off-by: George G. Davis <george_davis@mentor.com>
> 
> I think this makes sense.  In addition to OMAP 8250, the same approach
> is used in the Mediatek 8250 and iMX serial drivers.
> 
> Regardless, this is definitely better than removing the "dmas" properties
> from DT, as DT describes hardware, not usage policies.

+1

> Anyone else with a comment?

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Re: [PATCH] serial: sh-sci: disable DMA for uart_console

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]

On Thu, May 09, 2019 at 10:43:30AM -0400, George G. Davis wrote:
> As noted in commit 84b40e3b57ee ("serial: 8250: omap: Disable DMA for
> console UART"), UART console lines use low-level PIO only access functions
> which will conflict with use of the line when DMA is enabled, e.g. when
> the console line is also used for systemd messages. So disable DMA
> support for UART console lines.
> 
> Fixes: https://patchwork.kernel.org/patch/10929511/
> Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
> Cc: Eugeniu Rosca <erosca@de.adit-jv.com>
> Signed-off-by: George G. Davis <george_davis@mentor.com>
> ---
>  drivers/tty/serial/sh-sci.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 3cd139752d3f..885b56b1d4e4 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1557,6 +1557,9 @@ static void sci_request_dma(struct uart_port *port)
>  
>  	dev_dbg(port->dev, "%s: port %d\n", __func__, port->line);
>  
> +	if (uart_console(port))
> +		return; /* Cannot use DMA on console */

Minor nit: maybe the comment can be made more specific?

/*
 * DMA on console may interfere with Kernel log messages which use
 * plain putchar(). So, simply don't use it with a console.
 */

Other than that:

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Much better than dropping the properties, as Geert noted.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] serial: sh-sci: disable DMA for uart_console

[George G. Davis]

Hello Wolfram,

On Mon, May 13, 2019 at 03:51:14PM +0200, Wolfram Sang wrote:
> On Thu, May 09, 2019 at 10:43:30AM -0400, George G. Davis wrote:
> > As noted in commit 84b40e3b57ee ("serial: 8250: omap: Disable DMA for
> > console UART"), UART console lines use low-level PIO only access functions
> > which will conflict with use of the line when DMA is enabled, e.g. when
> > the console line is also used for systemd messages. So disable DMA
> > support for UART console lines.
> > 
> > Fixes: https://patchwork.kernel.org/patch/10929511/
> > Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
> > Cc: Eugeniu Rosca <erosca@de.adit-jv.com>
> > Signed-off-by: George G. Davis <george_davis@mentor.com>
> > ---
> >  drivers/tty/serial/sh-sci.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> > index 3cd139752d3f..885b56b1d4e4 100644
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -1557,6 +1557,9 @@ static void sci_request_dma(struct uart_port *port)
> >  
> >  	dev_dbg(port->dev, "%s: port %d\n", __func__, port->line);
> >  
> > +	if (uart_console(port))
> > +		return; /* Cannot use DMA on console */
> 
> Minor nit: maybe the comment can be made more specific?
> 
> /*
>  * DMA on console may interfere with Kernel log messages which use
>  * plain putchar(). So, simply don't use it with a console.
>  */

I'll submit v2 with the above recommended change.

Thanks!

> Other than that:
> 
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Much better than dropping the properties, as Geert noted.

-- 
Regards,
George

RE: [PATCH 2/6] Revert "arm64: dts: renesas: r8a7796: Enable DMA for SCIF2"

[Yoshihiro Shimoda]

Hi Eugeniu-san, Geert-san,

> From: Eugeniu Rosca, Sent: Tuesday, May 7, 2019 4:43 AM
<snip>
> > > [0] v5.0-rc6 commit 97f26702bc95b5 ("arm64: dts: renesas: r8a7796: Enable DMA for SCIF2")
> > > [1] v4.14.106 commit 703db5d1b1759f ("arm64: dts: renesas: r8a7796: Enable DMA for SCIF2")
> > > [2] scif (DEBUG) and rcar-dmac logs:
> > >     https://gist.github.com/erosca/132cce76a619724a9e4fa61d1db88c66
<snip>
> Enabling DEBUG in drivers/dma/sh/rcar-dmac.c, I can notice that one of
> the symptoms is a NULL dst_addr revealed by:
> 
> rcar-dmac e7300000.dma-controller: chan0: queue chunk (____ptrval____): 0@0xffff800639eb8090 -> 0x0000000000000000
> 
> In working scenarios, dst_addr is never zero. Does it give any hints?

Thank you for the report! It's very helpful to me.
I think we should fix the sh-sci driver at least.

According to the [2] log above,

[    4.379716] sh-sci e6e88000.serial: sci_dma_tx_work_fn: ffff800639b55000: 0...0, cookie 126

This "0...0" means the s->tx_dma_len on the sci_dma_tx_work_fn will be zero. And,

> rcar-dmac e7300000.dma-controller: chan0: queue chunk (____ptrval____): 0@0xffff800639eb8090 -> 0x0000000000000000

This means the chunk->dst_addr is not set to the "dst_addr" for SCIF because the len on rcar_dmac_chan_prep_sg is zero.
So, I'm thinking:
 - we have to fix the sh_sci driver to avoid "tx_dma_len = 0" transferring.

and 

 - also we have to fix the rcar-dmac driver to avoid this issue because the DMA Engine API
   guide doesn't prevent the len = 0.

Eugeniu-san, Geert-san, what do you think?

Best regards,
Yoshihiro Shimoda

>>
> > Thanks!
> 
> Likewise!
> 
> >
> > 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
> 
> --
> Best Regards,
> Eugeniu.

Re: [PATCH 2/6] Revert "arm64: dts: renesas: r8a7796: Enable DMA for SCIF2"

[Geert Uytterhoeven]

Hi Shimoda-san,

Thanks for your analysis!

On Mon, May 20, 2019 at 4:18 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Eugeniu Rosca, Sent: Tuesday, May 7, 2019 4:43 AM
> <snip>
> > > > [0] v5.0-rc6 commit 97f26702bc95b5 ("arm64: dts: renesas: r8a7796: Enable DMA for SCIF2")
> > > > [1] v4.14.106 commit 703db5d1b1759f ("arm64: dts: renesas: r8a7796: Enable DMA for SCIF2")
> > > > [2] scif (DEBUG) and rcar-dmac logs:
> > > >     https://gist.github.com/erosca/132cce76a619724a9e4fa61d1db88c66
> <snip>
> > Enabling DEBUG in drivers/dma/sh/rcar-dmac.c, I can notice that one of
> > the symptoms is a NULL dst_addr revealed by:
> >
> > rcar-dmac e7300000.dma-controller: chan0: queue chunk (____ptrval____): 0@0xffff800639eb8090 -> 0x0000000000000000
> >
> > In working scenarios, dst_addr is never zero. Does it give any hints?
>
> Thank you for the report! It's very helpful to me.
> I think we should fix the sh-sci driver at least.
>
> According to the [2] log above,
>
> [    4.379716] sh-sci e6e88000.serial: sci_dma_tx_work_fn: ffff800639b55000: 0...0, cookie 126
>
> This "0...0" means the s->tx_dma_len on the sci_dma_tx_work_fn will be zero. And,

How can this happen? schedule_work(&s->work_tx) is called only if
!uart_circ_empty(), and while holding the port lock? So the circular
buffer must be made empty in between the call to schedule_work() and the
work function sci_dma_tx_work_fn() being called.

I think this can happen if uart_flush_buffer() is called at the right
moment?

> > rcar-dmac e7300000.dma-controller: chan0: queue chunk (____ptrval____): 0@0xffff800639eb8090 -> 0x0000000000000000
>
> This means the chunk->dst_addr is not set to the "dst_addr" for SCIF because the len on rcar_dmac_chan_prep_sg is zero.
> So, I'm thinking:
>  - we have to fix the sh_sci driver to avoid "tx_dma_len = 0" transferring.

That sounds like just a simple check for !s->tx_dma_len in
sci_dma_tx_work_fn(), to return early, _and_ reset s->cookie_tx to
-EINVAL.

However, uart_flush_buffer() may still be called in between the check
and the calls to dmaengine_prep_slave_single() /
dma_sync_single_for_device(), clearing s->tx_dma_len again.
Unless something has changed recently, these two calls cannot be moved
inside the spinlock-protected section?
Using a cached value of s->tx_dma_len for the dmaengine calls might
work, though.

> and
>
>  - also we have to fix the rcar-dmac driver to avoid this issue because the DMA Engine API
>    guide doesn't prevent the len = 0.

I guess returning an error makes most sense?
Else we have to fix it deeper into the driver, where handling becomes
more complex.

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

RE: [PATCH 2/6] Revert "arm64: dts: renesas: r8a7796: Enable DMA for SCIF2"

[Yoshihiro Shimoda]

Hi Geert-san,

Thank you for your reply!

> From: Geert Uytterhoeven, Sent: Monday, May 20, 2019 4:38 PM
> 
> Hi Shimoda-san,
> 
> Thanks for your analysis!
> 
> On Mon, May 20, 2019 at 4:18 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > From: Eugeniu Rosca, Sent: Tuesday, May 7, 2019 4:43 AM
> > <snip>
> > > > > [0] v5.0-rc6 commit 97f26702bc95b5 ("arm64: dts: renesas: r8a7796: Enable DMA for SCIF2")
> > > > > [1] v4.14.106 commit 703db5d1b1759f ("arm64: dts: renesas: r8a7796: Enable DMA for SCIF2")
> > > > > [2] scif (DEBUG) and rcar-dmac logs:
> > > > >     https://gist.github.com/erosca/132cce76a619724a9e4fa61d1db88c66
> > <snip>
> > > Enabling DEBUG in drivers/dma/sh/rcar-dmac.c, I can notice that one of
> > > the symptoms is a NULL dst_addr revealed by:
> > >
> > > rcar-dmac e7300000.dma-controller: chan0: queue chunk (____ptrval____): 0@0xffff800639eb8090 -> 0x0000000000000000
> > >
> > > In working scenarios, dst_addr is never zero. Does it give any hints?
> >
> > Thank you for the report! It's very helpful to me.
> > I think we should fix the sh-sci driver at least.
> >
> > According to the [2] log above,
> >
> > [    4.379716] sh-sci e6e88000.serial: sci_dma_tx_work_fn: ffff800639b55000: 0...0, cookie 126
> >
> > This "0...0" means the s->tx_dma_len on the sci_dma_tx_work_fn will be zero. And,
> 
> How can this happen? schedule_work(&s->work_tx) is called only if
> !uart_circ_empty(), and while holding the port lock? So the circular
> buffer must be made empty in between the call to schedule_work() and the
> work function sci_dma_tx_work_fn() being called.
> 
> I think this can happen if uart_flush_buffer() is called at the right
> moment?

I think so. According to the log [2], the xmit->head and tail is set to zero.

278 [    4.331234] sh-sci e6e88000.serial: sci_dma_tx_work_fn: ffff800639b55000: 9...52, cookie 124 
279 [    4.334885] sh-sci e6e88000.serial: sci_dma_tx_complete(0) 
280 [    4.339992] sh-sci e6e88000.serial: sci_dma_tx_work_fn: ffff800639b55000: 52...100, cookie 125 
281 [    4.343340] sh-sci e6e88000.serial: sci_dma_tx_complete(0) 
282 [    4.379716] sh-sci e6e88000.serial: sci_dma_tx_work_fn: ffff800639b55000: 0...0, cookie 126 

> > > rcar-dmac e7300000.dma-controller: chan0: queue chunk (____ptrval____): 0@0xffff800639eb8090 -> 0x0000000000000000
> >
> > This means the chunk->dst_addr is not set to the "dst_addr" for SCIF because the len on rcar_dmac_chan_prep_sg is zero.
> > So, I'm thinking:
> >  - we have to fix the sh_sci driver to avoid "tx_dma_len = 0" transferring.
> 
> That sounds like just a simple check for !s->tx_dma_len in
> sci_dma_tx_work_fn(), to return early, _and_ reset s->cookie_tx to
> -EINVAL.
> 
> However, uart_flush_buffer() may still be called in between the check
> and the calls to dmaengine_prep_slave_single() /
> dma_sync_single_for_device(), clearing s->tx_dma_len again.
> Unless something has changed recently, these two calls cannot be moved
> inside the spinlock-protected section?

I also think these two calls (and dmaengine_submit() and dma_async_issue_pending())
should be moved inside the spinlock-protected section like sci_dma_rx_complete().

Also, sci_flush_buffer() should have the spinlock-protected section and
check the xmit and dma state somehow.

> Using a cached value of s->tx_dma_len for the dmaengine calls might
> work, though.
> 
> > and
> >
> >  - also we have to fix the rcar-dmac driver to avoid this issue because the DMA Engine API
> >    guide doesn't prevent the len = 0.
> 
> I guess returning an error makes most sense?
> Else we have to fix it deeper into the driver, where handling becomes
> more complex.

I see. I think so. (We should avoid more complex.)

Best regards,
Yoshihiro Shimoda

[PATCH 3/6] arm64: dts: renesas: r8a7795: zap dma configuration in scif2

[Eugeniu Rosca]

Both empirically and based on the existing documentation, it looks like
SCIF2 does not support DMA. Let's disable it. Full reasoning is given in
commit ("Revert "arm64: dts: renesas: r8a7796: Enable DMA for SCIF2"").

Fixes: eb21089c32054e ("arm64: dts: renesas: r8a7795: Add missing SYS-DMAC2 dmas")
Fixes: 49af46b4095672 ("arm64: renesas: r8a7795: Add all SCIF nodes")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 arch/arm64/boot/dts/renesas/r8a7795.dtsi | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index abeac3059383..7704bd46afdf 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -1323,9 +1323,6 @@
 				 <&cpg CPG_CORE R8A7795_CLK_S3D1>,
 				 <&scif_clk>;
 			clock-names = "fck", "brg_int", "scif_clk";
-			dmas = <&dmac1 0x13>, <&dmac1 0x12>,
-			       <&dmac2 0x13>, <&dmac2 0x12>;
-			dma-names = "tx", "rx", "tx", "rx";
 			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
 			resets = <&cpg 310>;
 			status = "disabled";
-- 
2.21.0

[PATCH 4/6] Revert "arm64: dts: renesas: r8a77965: Enable DMA for SCIF2"

[Eugeniu Rosca]

This reverts commit 05c8478abd485507c25aa565afab604af8d8fe46.

Both empirically and based on the existing documentation, it looks like
SCIF2 does not support DMA. Let's disable it. Full reasoning is given in
commit ("Revert "arm64: dts: renesas: r8a7796: Enable DMA for SCIF2"").

Fixes: 05c8478abd4855 ("arm64: dts: renesas: r8a77965: Enable DMA for SCIF2")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 arch/arm64/boot/dts/renesas/r8a77965.dtsi | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a77965.dtsi b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
index 9763d108e183..979f14d1fcc4 100644
--- a/arch/arm64/boot/dts/renesas/r8a77965.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77965.dtsi
@@ -1068,9 +1068,6 @@
 				 <&cpg CPG_CORE R8A77965_CLK_S3D1>,
 				 <&scif_clk>;
 			clock-names = "fck", "brg_int", "scif_clk";
-			dmas = <&dmac1 0x13>, <&dmac1 0x12>,
-			       <&dmac2 0x13>, <&dmac2 0x12>;
-			dma-names = "tx", "rx", "tx", "rx";
 			power-domains = <&sysc R8A77965_PD_ALWAYS_ON>;
 			resets = <&cpg 310>;
 			status = "disabled";
-- 
2.21.0

[PATCH 5/6] Revert "arm64: dts: renesas: r8a77990: Enable DMA for SCIF2"

[Eugeniu Rosca]

This reverts commit a99de47921565c233092b0d3c4652b3a10e715ec.

Both empirically and based on the existing documentation, it looks like
SCIF2 does not support DMA. Let's disable it. Full reasoning is given in
commit ("Revert "arm64: dts: renesas: r8a7796: Enable DMA for SCIF2"").

Fixes: a99de47921565c ("arm64: dts: renesas: r8a77990: Enable DMA for SCIF2")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 arch/arm64/boot/dts/renesas/r8a77990.dtsi | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a77990.dtsi b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
index d2ad665fe2d9..4bf32bfaafc0 100644
--- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
@@ -1018,9 +1018,7 @@
 				 <&cpg CPG_CORE R8A77990_CLK_S3D1C>,
 				 <&scif_clk>;
 			clock-names = "fck", "brg_int", "scif_clk";
-			dmas = <&dmac1 0x13>, <&dmac1 0x12>,
-			       <&dmac2 0x13>, <&dmac2 0x12>;
-			dma-names = "tx", "rx", "tx", "rx";
+
 			power-domains = <&sysc R8A77990_PD_ALWAYS_ON>;
 			resets = <&cpg 310>;
 			status = "disabled";
-- 
2.21.0

[PATCH 6/6] Revert "arm64: dts: renesas: r8a77995: add DMA for SCIF2"

[Eugeniu Rosca]

This reverts commit af2ea3df851ffa68ad07ff59d4dabadbf33b45ef.

Both empirically and based on the existing documentation, it looks like
SCIF2 does not support DMA. Let's disable it. Full reasoning is given in
commit ("Revert "arm64: dts: renesas: r8a7796: Enable DMA for SCIF2"").

Fixes: af2ea3df851ffa ("arm64: dts: renesas: r8a77995: add DMA for SCIF2")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 arch/arm64/boot/dts/renesas/r8a77995.dtsi | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a77995.dtsi b/arch/arm64/boot/dts/renesas/r8a77995.dtsi
index 5bf3af246e14..bd3ac5d00b2e 100644
--- a/arch/arm64/boot/dts/renesas/r8a77995.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a77995.dtsi
@@ -740,9 +740,6 @@
 				 <&cpg CPG_CORE R8A77995_CLK_S3D1C>,
 				 <&scif_clk>;
 			clock-names = "fck", "brg_int", "scif_clk";
-			dmas = <&dmac1 0x13>, <&dmac1 0x12>,
-			       <&dmac2 0x13>, <&dmac2 0x12>;
-			dma-names = "tx", "rx", "tx", "rx";
 			power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
 			resets = <&cpg 310>;
 			status = "disabled";
-- 
2.21.0