Re: [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues

[Eugeniu Rosca <roscaeugeniu@gmail.com>]

Hi Geert, CC: George

On Mon, Jun 24, 2019 at 02:35:38PM +0200, Geert Uytterhoeven wrote:
> Hi Greg, Jiri,
> 
> This patch series attempts to fix the issues Eugeniu Rosca reported
> seeing, where .flush_buffer() interfered with transmit DMA operation[*].
> 
> There's a third patch "dmaengine: rcar-dmac: Reject zero-length slave
> DMA requests", which is related to the issue, but further independent,
> hence submitted separately.
> 
> Eugeniu: does this fix the issues you were seeing?

Many thanks for both sh-sci and the rcar-dmac patches.
The fixes are very much appreciated.

> Geert Uytterhoeven (2):
>   serial: sh-sci: Fix TX DMA buffer flushing and workqueue races
>   serial: sh-sci: Terminate TX DMA during buffer flushing
> 
>  drivers/tty/serial/sh-sci.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)

I reserved some time to get a feeling about how the patches behave on
a real system (H3-ES2.0-ULCB-KF-M06), so here come my observations.

First of all, the issue I have originally reported in [0] is only
reproducible in absence of [4]. So, one of my questions would be how
do you yourself see the relationship between [1-3] and [4]?

That said, all my testing assumes:
 - Vanilla tip v5.2-rc6-15-g249155c20f9b with [4] reverted.
 - DEBUG is undefined in {sh-sci.c,rcar-dmac.c}, since I've noticed
   new issues arising in the debug build, which are unrelated to [0].

Below is the summary of my findings:

 Version         IS [0]       Is console       Error message when
(vanilla+X)    reproduced?  usable after [0]   [0] is reproduced
                             is reproduced?
 ------------------------------------------------------------
 -[4]             Yes           No                [5]
 -[4]+[1]         Yes           No                -
 -[4]+[2]         Yes           Yes               [5]
 -[4]+[3]         Yes           Yes               [6]
 -[4]+[1]+[2]     No            -                 -
 -[4]+[1]+[2]+[3] No            -                 -
 pure vanilla     No            -                 -

This looks a little too verbose, but I thought it might be interesting.

The story which I see is that [1] does not fix [0] alone, but it seems
to depend on [2]. Furthermore, if cherry picked alone, [1] makes the
matters somewhat worse in the sense that it hides the error [5].

My only question is whether [1-3] are supposed to replace [4] or they
are supposed to happily coexist. Since I don't see [0] being reproduced
with [1-3], I personally prefer to re-enable DMA on SCIF (when the
latter is used as console) so that more features and code paths are
exercised to increase test coverage.

[0] https://lore.kernel.org/lkml/20190504004258.23574-3-erosca@de.adit-jv.com/
[1] https://patchwork.kernel.org/patch/11012983/
    ("serial: sh-sci: Fix TX DMA buffer flushing and workqueue races")
[2] https://patchwork.kernel.org/patch/11012987/
    ("serial: sh-sci: Terminate TX DMA during buffer flushing")
[3] https://patchwork.kernel.org/patch/11012991/
    ("dmaengine: rcar-dmac: Reject zero-length slave DMA requests")
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=099506cbbc79c0
    ("serial: sh-sci: disable DMA for uart_console")

[5] rcar-dmac e7300000.dma-controller: Channel Address Error
[6] rcar-dmac e7300000.dma-controller: rcar_dmac_prep_slave_sg: bad parameter: len=1, id=19
    sh-sci e6e88000.serial: Failed preparing Tx DMA descriptor

Thanks!

-- 
Best regards,
Eugeniu.