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

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

[Geert Uytterhoeven]

	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?

Thanks for your comments!

[*] '[PATCH 2/6] Revert "arm64: dts: renesas: r8a7796: Enable DMA for SCIF2"'
(https://lore.kernel.org/lkml/20190504004258.23574-3-erosca@de.adit-jv.com/).

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

-- 
2.17.1

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

[PATCH 1/2] serial: sh-sci: Fix TX DMA buffer flushing and workqueue races

[Geert Uytterhoeven]

When uart_flush_buffer() is called, the .flush_buffer() callback zeroes
the tx_dma_len field.  This may race with the work queue function
handling transmit DMA requests:

  1. If the buffer is flushed before the first DMA API call,
     dmaengine_prep_slave_single() may be called with a zero length,
     causing the DMA request to never complete, leading to messages
     like:

        rcar-dmac e7300000.dma-controller: Channel Address Error happen

     and, with debug enabled:

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

     and DMA timeouts.

  2. If the buffer is flushed after the first DMA API call, but before
     the second, dma_sync_single_for_device() may be called with a zero
     length, causing the transmit data not to be flushed to RAM, and
     leading to stale data being output.

Fix this by:
  1. Letting sci_dma_tx_work_fn() return immediately if the transmit
     buffer is empty,
  2. Extending the critical section to cover all DMA preparational work,
     so tx_dma_len stays consistent for all of it,
  3. Using local copies of circ_buf.head and circ_buf.tail, to make sure
     they match the actual operation above.

Reported-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Suggested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/sh-sci.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index abc705716aa094fd..d4504daff99263f5 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1398,6 +1398,7 @@ static void sci_dma_tx_work_fn(struct work_struct *work)
 	struct circ_buf *xmit = &port->state->xmit;
 	unsigned long flags;
 	dma_addr_t buf;
+	int head, tail;
 
 	/*
 	 * DMA is idle now.
@@ -1407,16 +1408,23 @@ static void sci_dma_tx_work_fn(struct work_struct *work)
 	 * consistent xmit buffer state.
 	 */
 	spin_lock_irq(&port->lock);
-	buf = s->tx_dma_addr + (xmit->tail & (UART_XMIT_SIZE - 1));
+	head = xmit->head;
+	tail = xmit->tail;
+	buf = s->tx_dma_addr + (tail & (UART_XMIT_SIZE - 1));
 	s->tx_dma_len = min_t(unsigned int,
-		CIRC_CNT(xmit->head, xmit->tail, UART_XMIT_SIZE),
-		CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE));
-	spin_unlock_irq(&port->lock);
+		CIRC_CNT(head, tail, UART_XMIT_SIZE),
+		CIRC_CNT_TO_END(head, tail, UART_XMIT_SIZE));
+	if (!s->tx_dma_len) {
+		/* Transmit buffer has been flushed */
+		spin_unlock_irq(&port->lock);
+		return;
+	}
 
 	desc = dmaengine_prep_slave_single(chan, buf, s->tx_dma_len,
 					   DMA_MEM_TO_DEV,
 					   DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 	if (!desc) {
+		spin_unlock_irq(&port->lock);
 		dev_warn(port->dev, "Failed preparing Tx DMA descriptor\n");
 		goto switch_to_pio;
 	}
@@ -1424,18 +1432,18 @@ static void sci_dma_tx_work_fn(struct work_struct *work)
 	dma_sync_single_for_device(chan->device->dev, buf, s->tx_dma_len,
 				   DMA_TO_DEVICE);
 
-	spin_lock_irq(&port->lock);
 	desc->callback = sci_dma_tx_complete;
 	desc->callback_param = s;
-	spin_unlock_irq(&port->lock);
 	s->cookie_tx = dmaengine_submit(desc);
 	if (dma_submit_error(s->cookie_tx)) {
+		spin_unlock_irq(&port->lock);
 		dev_warn(port->dev, "Failed submitting Tx DMA descriptor\n");
 		goto switch_to_pio;
 	}
 
+	spin_unlock_irq(&port->lock);
 	dev_dbg(port->dev, "%s: %p: %d...%d, cookie %d\n",
-		__func__, xmit->buf, xmit->tail, xmit->head, s->cookie_tx);
+		__func__, xmit->buf, tail, head, s->cookie_tx);
 
 	dma_async_issue_pending(chan);
 	return;
-- 
2.17.1

Re: [PATCH 1/2] serial: sh-sci: Fix TX DMA buffer flushing and workqueue races

[Eugeniu Rosca]

Hi Geert,

One bikeshedding below.

On Mon, Jun 24, 2019 at 02:35:39PM +0200, Geert Uytterhoeven wrote:
[..]
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index abc705716aa094fd..d4504daff99263f5 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1398,6 +1398,7 @@ static void sci_dma_tx_work_fn(struct work_struct *work)
>  	struct circ_buf *xmit = &port->state->xmit;
>  	unsigned long flags;
>  	dma_addr_t buf;
> +	int head, tail;
>  
>  	/*
>  	 * DMA is idle now.
> @@ -1407,16 +1408,23 @@ static void sci_dma_tx_work_fn(struct work_struct *work)
>  	 * consistent xmit buffer state.
>  	 */
>  	spin_lock_irq(&port->lock);
> -	buf = s->tx_dma_addr + (xmit->tail & (UART_XMIT_SIZE - 1));
> +	head = xmit->head;
> +	tail = xmit->tail;
> +	buf = s->tx_dma_addr + (tail & (UART_XMIT_SIZE - 1));
>  	s->tx_dma_len = min_t(unsigned int,
> -		CIRC_CNT(xmit->head, xmit->tail, UART_XMIT_SIZE),
> -		CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE));
> -	spin_unlock_irq(&port->lock);
> +		CIRC_CNT(head, tail, UART_XMIT_SIZE),
> +		CIRC_CNT_TO_END(head, tail, UART_XMIT_SIZE));
> +	if (!s->tx_dma_len) {
> +		/* Transmit buffer has been flushed */
> +		spin_unlock_irq(&port->lock);
> +		return;

Since we can now return before using the result stored in 'buf', we
could relocate the 'buf' calculation next to the return statement, just
before calling dmaengine_prep_slave_single(), as micro-optimization.

> +	}
>  
>  	desc = dmaengine_prep_slave_single(chan, buf, s->tx_dma_len,
>  					   DMA_MEM_TO_DEV,
>  					   DMA_PREP_INTERRUPT | DMA_CTRL_ACK);

Otherwise:

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

-- 
Best Regards,
Eugeniu.

[PATCH 2/2] serial: sh-sci: Terminate TX DMA during buffer flushing

[Geert Uytterhoeven]

While the .flush_buffer() callback clears sci_port.tx_dma_len since
commit 1cf4a7efdc71cab8 ("serial: sh-sci: Fix race condition causing
garbage during shutdown"), it does not terminate a transmit DMA
operation that may be in progress.

Fix this by terminating any pending DMA operations, and resetting the
corresponding cookie.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/sh-sci.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index d4504daff99263f5..d18c680aa64b3427 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1656,11 +1656,18 @@ static void sci_free_dma(struct uart_port *port)
 
 static void sci_flush_buffer(struct uart_port *port)
 {
+	struct sci_port *s = to_sci_port(port);
+
 	/*
 	 * In uart_flush_buffer(), the xmit circular buffer has just been
-	 * cleared, so we have to reset tx_dma_len accordingly.
+	 * cleared, so we have to reset tx_dma_len accordingly, and stop any
+	 * pending transfers
 	 */
-	to_sci_port(port)->tx_dma_len = 0;
+	s->tx_dma_len = 0;
+	if (s->chan_tx) {
+		dmaengine_terminate_async(s->chan_tx);
+		s->cookie_tx = -EINVAL;
+	}
 }
 #else /* !CONFIG_SERIAL_SH_SCI_DMA */
 static inline void sci_request_dma(struct uart_port *port)
-- 
2.17.1

Re: [PATCH 2/2] serial: sh-sci: Terminate TX DMA during buffer flushing

[Eugeniu Rosca]

On Mon, Jun 24, 2019 at 02:35:40PM +0200, Geert Uytterhoeven wrote:
[..]
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index d4504daff99263f5..d18c680aa64b3427 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1656,11 +1656,18 @@ static void sci_free_dma(struct uart_port *port)
>  
>  static void sci_flush_buffer(struct uart_port *port)
>  {
> +	struct sci_port *s = to_sci_port(port);
> +
>  	/*
>  	 * In uart_flush_buffer(), the xmit circular buffer has just been
> -	 * cleared, so we have to reset tx_dma_len accordingly.
> +	 * cleared, so we have to reset tx_dma_len accordingly, and stop any
> +	 * pending transfers
>  	 */
> -	to_sci_port(port)->tx_dma_len = 0;
> +	s->tx_dma_len = 0;
> +	if (s->chan_tx) {
> +		dmaengine_terminate_async(s->chan_tx);
> +		s->cookie_tx = -EINVAL;
> +	}

I am seeing a similar solution being employed by other tty/serial
drivers [1]. One major difference is that those drivers make use of
dmaengine_terminate_all(). Since the latter is deprecated starting
with commit [2], the API choice looks reasonable to me.

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

[1] git grep -W "static void.*flush_buffer" -- drivers/tty/serial/ | grep dmaengine_terminate
drivers/tty/serial/fsl_lpuart.c-		dmaengine_terminate_all(sport->dma_tx_chan);
drivers/tty/serial/imx.c-	dmaengine_terminate_all(sport->dma_chan_tx);
drivers/tty/serial/serial-tegra.c-		dmaengine_terminate_all(tup->tx_dma_chan);

[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b36f09c3c441a6
    ("dmaengine: Add transfer termination synchronization support")

-- 
Best Regards,
Eugeniu.

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

[Eugeniu Rosca]

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.

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

[Geert Uytterhoeven]

Hi Eugeniu,

On Wed, Jun 26, 2019 at 7:34 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> On Mon, Jun 24, 2019 at 02:35:38PM +0200, Geert Uytterhoeven wrote:
> > 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.

Thanks for your extensive testing!

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

I consider them independent.
Just applying [4] would fix the issue for the console only, while the
race condition can still be triggered on other serial ports.

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

Thanks, it's very helpful to provide these results.

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

OK.

> 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

They are meant to coexist.

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

If a serial port is used as a console, the port is used for both DMA
(normal use) and PIO (serial console output).  The latter can have a
negative impact on the former, aggravating existing bugs, or triggering
more races, even in the hardware.  So I think it's better to be more
cautious and keep DMA disabled for the console.

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

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 0/2] serial: sh-sci: Fix .flush_buffer() issues

[Eugeniu Rosca]

Hi Geert,

On Fri, Jun 28, 2019 at 01:51:25PM +0200, Geert Uytterhoeven wrote:

[..]

> If a serial port is used as a console, the port is used for both DMA
> (normal use) and PIO (serial console output).  The latter can have a
> negative impact on the former, aggravating existing bugs, or triggering
> more races, even in the hardware.  So I think it's better to be more
> cautious and keep DMA disabled for the console.

Thanks for the extensive and comprehensible replies.
No more questions from my end.
Looking forward to picking the patches from vanilla/stable trees.

-- 
Best Regards,
Eugeniu.

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

[Wolfram Sang]

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


> > If a serial port is used as a console, the port is used for both DMA
> > (normal use) and PIO (serial console output).  The latter can have a
> > negative impact on the former, aggravating existing bugs, or triggering
> > more races, even in the hardware.  So I think it's better to be more
> > cautious and keep DMA disabled for the console.
> 
> Thanks for the extensive and comprehensible replies.
> No more questions from my end.
> Looking forward to picking the patches from vanilla/stable trees.

If you could formally add such a tag:

Tested-by: <your email>

(maybe also Acked-by: or Reviewed-by:, dunno if you think it is
apropriate)

to the patches, this would be much appreciated and will usually speed up
the patches getting applied.

Thanks for your help!


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

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

[Eugeniu Rosca]

Hi Wolfram,

On Fri, Jun 28, 2019 at 02:55:34PM +0200, Wolfram Sang wrote:
[..]
> If you could formally add such a tag:
> 
> Tested-by: <your email>
> 
> (maybe also Acked-by: or Reviewed-by:, dunno if you think it is
> apropriate)
> 
> to the patches, this would be much appreciated and will usually speed up
> the patches getting applied.
> 
> Thanks for your help!

I am doing this per-patch to allow patchwork to reflect the status of
each patch on the linux-renesas-soc front-page. AFAIK patchwork ignores
series-wide '*-by: Name <email>' signatures/tags.

-- 
Best Regards,
Eugeniu.

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

[Wolfram Sang]

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


> I am doing this per-patch to allow patchwork to reflect the status of
> each patch on the linux-renesas-soc front-page. AFAIK patchwork ignores
> series-wide '*-by: Name <email>' signatures/tags.

AFAIK, yes.

Thanks!


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

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

[Greg Kroah-Hartman]

On Fri, Jun 28, 2019 at 03:02:00PM +0200, Eugeniu Rosca wrote:
> Hi Wolfram,
> 
> On Fri, Jun 28, 2019 at 02:55:34PM +0200, Wolfram Sang wrote:
> [..]
> > If you could formally add such a tag:
> > 
> > Tested-by: <your email>
> > 
> > (maybe also Acked-by: or Reviewed-by:, dunno if you think it is
> > apropriate)
> > 
> > to the patches, this would be much appreciated and will usually speed up
> > the patches getting applied.
> > 
> > Thanks for your help!
> 
> I am doing this per-patch to allow patchwork to reflect the status of
> each patch on the linux-renesas-soc front-page. AFAIK patchwork ignores
> series-wide '*-by: Name <email>' signatures/tags.

I don't use patchwork :)

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

[Eugeniu Rosca]

Hi Greg,

On Wed, Jul 03, 2019 at 07:30:50PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Jun 28, 2019 at 03:02:00PM +0200, Eugeniu Rosca wrote:
[..]
> > I am doing this per-patch to allow patchwork to reflect the status of
> > each patch on the linux-renesas-soc front-page. AFAIK patchwork ignores
> > series-wide '*-by: Name <email>' signatures/tags.
> 
> I don't use patchwork :)

How do you then collect all the "{Reviewed,Tested,etc}-by:" signatures
(each of which means sometimes hours of effort) in the hairy ML threads?
Patchwork makes it a matter of one click.

-- 
Best regards,
Eugeniu.

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

[Greg Kroah-Hartman]

On Wed, Jul 03, 2019 at 08:15:19PM +0200, Eugeniu Rosca wrote:
> Hi Greg,
> 
> On Wed, Jul 03, 2019 at 07:30:50PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Jun 28, 2019 at 03:02:00PM +0200, Eugeniu Rosca wrote:
> [..]
> > > I am doing this per-patch to allow patchwork to reflect the status of
> > > each patch on the linux-renesas-soc front-page. AFAIK patchwork ignores
> > > series-wide '*-by: Name <email>' signatures/tags.
> > 
> > I don't use patchwork :)
> 
> How do you then collect all the "{Reviewed,Tested,etc}-by:" signatures
> (each of which means sometimes hours of effort) in the hairy ML threads?
> Patchwork makes it a matter of one click.

I've been doing this for a very long time now, before patchwork was even
around.  It's pretty trivial to collect them on my own.

thanks,

greg k-h

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

[Wolfram Sang]

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


> I've been doing this for a very long time now, before patchwork was even
> around.  It's pretty trivial to collect them on my own.

It's a workflow thing. Mileages vary. I ask people to tag patches
individually for reasonable small series.


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

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

[George G. Davis]

Hello All,

On Fri, Jun 28, 2019 at 01:51:25PM +0200, Geert Uytterhoeven wrote:
> Hi Eugeniu,
> 
> On Wed, Jun 26, 2019 at 7:34 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> > On Mon, Jun 24, 2019 at 02:35:38PM +0200, Geert Uytterhoeven wrote:
> > > 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.
> 
> Thanks for your extensive testing!
> 
> > 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]?
> 
> I consider them independent.
> Just applying [4] would fix the issue for the console only, while the
> race condition can still be triggered on other serial ports.
> 
> > 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.
> 
> Thanks, it's very helpful to provide these results.
> 
> > 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].
> 
> OK.
> 
> > 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
> 
> They are meant to coexist.
> 
> > 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.
> 
> If a serial port is used as a console, the port is used for both DMA
> (normal use) and PIO (serial console output).  The latter can have a
> negative impact on the former, aggravating existing bugs, or triggering
> more races, even in the hardware.  So I think it's better to be more
> cautious and keep DMA disabled for the console.

Agreed.

Just a note for the record that [4] was the easiest way to resolve the
reported problem [0] but an alternative solution would be to implement DMA
support for ttySC console ports which will be non-trivial to implement and test
due to the potential for deadlocks in console write critical paths where
various locks are held with interrupts disabled. I see only one tty serial
driver which implements console DMA support, drivers/tty/serial/mpsc.c,
and perhaps there is a good reason why there are no other examples?

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

-- 
Regards,
George