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

Message ID 20190504004258.23574-3-erosca@de.adit-jv.com
State Rejected
Delegated to: Simon Horman
Headers show
Series
  • Zap SCIF2 DMA configuration in R-Car Gen3 DTS
Related show

Commit Message

Eugeniu Rosca May 4, 2019, 12:42 a.m. UTC
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(-)

Comments

Geert Uytterhoeven May 6, 2019, 10:02 a.m. UTC | #1
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
Eugeniu Rosca May 6, 2019, 7:42 p.m. UTC | #2
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
Eugeniu Rosca May 10, 2019, 5:10 p.m. UTC | #3
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
>
George G. Davis May 10, 2019, 6:38 p.m. UTC | #4
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.
Eugeniu Rosca May 13, 2019, 10:28 a.m. UTC | #5
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.
Geert Uytterhoeven May 13, 2019, 11:13 a.m. UTC | #6
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
Simon Horman May 13, 2019, 11:42 a.m. UTC | #7
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>
Wolfram Sang May 13, 2019, 1:51 p.m. UTC | #8
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.
George G. Davis May 13, 2019, 3:43 p.m. UTC | #9
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.
Yoshihiro Shimoda May 20, 2019, 2:18 a.m. UTC | #10
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.
Geert Uytterhoeven May 20, 2019, 7:37 a.m. UTC | #11
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
Yoshihiro Shimoda May 20, 2019, 9:52 a.m. UTC | #12
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
diff mbox series

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