* [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues
@ 2019-06-24 12:35 Geert Uytterhoeven
2019-06-24 12:35 ` [PATCH 1/2] serial: sh-sci: Fix TX DMA buffer flushing and workqueue races Geert Uytterhoeven
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Geert Uytterhoeven @ 2019-06-24 12:35 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Eugeniu Rosca
Cc: Yoshihiro Shimoda, Vinod Koul, Dan Williams, Wolfram Sang,
linux-serial, linux-renesas-soc, dmaengine, 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] serial: sh-sci: Fix TX DMA buffer flushing and workqueue races
2019-06-24 12:35 [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues Geert Uytterhoeven
@ 2019-06-24 12:35 ` Geert Uytterhoeven
2019-06-28 12:53 ` Eugeniu Rosca
2019-06-24 12:35 ` [PATCH 2/2] serial: sh-sci: Terminate TX DMA during buffer flushing Geert Uytterhoeven
2019-06-26 17:34 ` [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues Eugeniu Rosca
2 siblings, 1 reply; 16+ messages in thread
From: Geert Uytterhoeven @ 2019-06-24 12:35 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Eugeniu Rosca
Cc: Yoshihiro Shimoda, Vinod Koul, Dan Williams, Wolfram Sang,
linux-serial, linux-renesas-soc, dmaengine, 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] serial: sh-sci: Terminate TX DMA during buffer flushing
2019-06-24 12:35 [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues Geert Uytterhoeven
2019-06-24 12:35 ` [PATCH 1/2] serial: sh-sci: Fix TX DMA buffer flushing and workqueue races Geert Uytterhoeven
@ 2019-06-24 12:35 ` Geert Uytterhoeven
2019-06-28 14:04 ` Eugeniu Rosca
2019-06-26 17:34 ` [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues Eugeniu Rosca
2 siblings, 1 reply; 16+ messages in thread
From: Geert Uytterhoeven @ 2019-06-24 12:35 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Eugeniu Rosca
Cc: Yoshihiro Shimoda, Vinod Koul, Dan Williams, Wolfram Sang,
linux-serial, linux-renesas-soc, dmaengine, 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues
2019-06-24 12:35 [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues Geert Uytterhoeven
2019-06-24 12:35 ` [PATCH 1/2] serial: sh-sci: Fix TX DMA buffer flushing and workqueue races Geert Uytterhoeven
2019-06-24 12:35 ` [PATCH 2/2] serial: sh-sci: Terminate TX DMA during buffer flushing Geert Uytterhoeven
@ 2019-06-26 17:34 ` Eugeniu Rosca
2019-06-28 11:51 ` Geert Uytterhoeven
2 siblings, 1 reply; 16+ messages in thread
From: Eugeniu Rosca @ 2019-06-26 17:34 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Jiri Slaby, Yoshihiro Shimoda, Vinod Koul,
Dan Williams, Wolfram Sang, linux-serial, linux-renesas-soc,
dmaengine, George G . Davis, Eugeniu Rosca, 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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues
2019-06-26 17:34 ` [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues Eugeniu Rosca
@ 2019-06-28 11:51 ` Geert Uytterhoeven
2019-06-28 12:39 ` Eugeniu Rosca
2019-06-28 16:29 ` George G. Davis
0 siblings, 2 replies; 16+ messages in thread
From: Geert Uytterhoeven @ 2019-06-28 11:51 UTC (permalink / raw)
To: Eugeniu Rosca
Cc: Geert Uytterhoeven, Greg Kroah-Hartman, Jiri Slaby,
Yoshihiro Shimoda, Vinod Koul, Dan Williams, Wolfram Sang,
open list:SERIAL DRIVERS, Linux-Renesas, dmaengine,
George G . Davis, Eugeniu Rosca
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues
2019-06-28 11:51 ` Geert Uytterhoeven
@ 2019-06-28 12:39 ` Eugeniu Rosca
2019-06-28 12:55 ` Wolfram Sang
2019-06-28 16:29 ` George G. Davis
1 sibling, 1 reply; 16+ messages in thread
From: Eugeniu Rosca @ 2019-06-28 12:39 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Eugeniu Rosca, Eugeniu Rosca, Geert Uytterhoeven,
Greg Kroah-Hartman, Jiri Slaby, Yoshihiro Shimoda, Vinod Koul,
Dan Williams, Wolfram Sang, open list:SERIAL DRIVERS,
Linux-Renesas, dmaengine, George G . Davis
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] serial: sh-sci: Fix TX DMA buffer flushing and workqueue races
2019-06-24 12:35 ` [PATCH 1/2] serial: sh-sci: Fix TX DMA buffer flushing and workqueue races Geert Uytterhoeven
@ 2019-06-28 12:53 ` Eugeniu Rosca
0 siblings, 0 replies; 16+ messages in thread
From: Eugeniu Rosca @ 2019-06-28 12:53 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Jiri Slaby, Yoshihiro Shimoda, Vinod Koul,
Dan Williams, Wolfram Sang, linux-serial, linux-renesas-soc,
dmaengine, Eugeniu Rosca, 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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues
2019-06-28 12:39 ` Eugeniu Rosca
@ 2019-06-28 12:55 ` Wolfram Sang
2019-06-28 13:02 ` Eugeniu Rosca
0 siblings, 1 reply; 16+ messages in thread
From: Wolfram Sang @ 2019-06-28 12:55 UTC (permalink / raw)
To: Eugeniu Rosca
Cc: Geert Uytterhoeven, Eugeniu Rosca, Geert Uytterhoeven,
Greg Kroah-Hartman, Jiri Slaby, Yoshihiro Shimoda, Vinod Koul,
Dan Williams, Wolfram Sang, open list:SERIAL DRIVERS,
Linux-Renesas, dmaengine, George G . Davis
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues
2019-06-28 12:55 ` Wolfram Sang
@ 2019-06-28 13:02 ` Eugeniu Rosca
2019-06-28 13:14 ` Wolfram Sang
2019-07-03 17:30 ` Greg Kroah-Hartman
0 siblings, 2 replies; 16+ messages in thread
From: Eugeniu Rosca @ 2019-06-28 13:02 UTC (permalink / raw)
To: Wolfram Sang
Cc: Geert Uytterhoeven, Eugeniu Rosca, Eugeniu Rosca,
Geert Uytterhoeven, Greg Kroah-Hartman, Jiri Slaby,
Yoshihiro Shimoda, Vinod Koul, Dan Williams, Wolfram Sang,
open list:SERIAL DRIVERS, Linux-Renesas, dmaengine,
George G . Davis
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues
2019-06-28 13:02 ` Eugeniu Rosca
@ 2019-06-28 13:14 ` Wolfram Sang
2019-07-03 17:30 ` Greg Kroah-Hartman
1 sibling, 0 replies; 16+ messages in thread
From: Wolfram Sang @ 2019-06-28 13:14 UTC (permalink / raw)
To: Eugeniu Rosca
Cc: Geert Uytterhoeven, Eugeniu Rosca, Geert Uytterhoeven,
Greg Kroah-Hartman, Jiri Slaby, Yoshihiro Shimoda, Vinod Koul,
Dan Williams, Wolfram Sang, open list:SERIAL DRIVERS,
Linux-Renesas, dmaengine, George G . Davis
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] serial: sh-sci: Terminate TX DMA during buffer flushing
2019-06-24 12:35 ` [PATCH 2/2] serial: sh-sci: Terminate TX DMA during buffer flushing Geert Uytterhoeven
@ 2019-06-28 14:04 ` Eugeniu Rosca
0 siblings, 0 replies; 16+ messages in thread
From: Eugeniu Rosca @ 2019-06-28 14:04 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Jiri Slaby, Yoshihiro Shimoda, Vinod Koul,
Dan Williams, Wolfram Sang, linux-serial, linux-renesas-soc,
dmaengine, Eugeniu Rosca, 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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues
2019-06-28 11:51 ` Geert Uytterhoeven
2019-06-28 12:39 ` Eugeniu Rosca
@ 2019-06-28 16:29 ` George G. Davis
1 sibling, 0 replies; 16+ messages in thread
From: George G. Davis @ 2019-06-28 16:29 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Eugeniu Rosca, Geert Uytterhoeven, Greg Kroah-Hartman,
Jiri Slaby, Yoshihiro Shimoda, Vinod Koul, Dan Williams,
Wolfram Sang, open list:SERIAL DRIVERS, Linux-Renesas, dmaengine,
Eugeniu Rosca
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues
2019-06-28 13:02 ` Eugeniu Rosca
2019-06-28 13:14 ` Wolfram Sang
@ 2019-07-03 17:30 ` Greg Kroah-Hartman
2019-07-03 18:15 ` Eugeniu Rosca
1 sibling, 1 reply; 16+ messages in thread
From: Greg Kroah-Hartman @ 2019-07-03 17:30 UTC (permalink / raw)
To: Eugeniu Rosca
Cc: Wolfram Sang, Geert Uytterhoeven, Eugeniu Rosca,
Geert Uytterhoeven, Jiri Slaby, Yoshihiro Shimoda, Vinod Koul,
Dan Williams, Wolfram Sang, open list:SERIAL DRIVERS,
Linux-Renesas, dmaengine, George G . Davis
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 :)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues
2019-07-03 17:30 ` Greg Kroah-Hartman
@ 2019-07-03 18:15 ` Eugeniu Rosca
2019-07-03 18:44 ` Greg Kroah-Hartman
0 siblings, 1 reply; 16+ messages in thread
From: Eugeniu Rosca @ 2019-07-03 18:15 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Eugeniu Rosca, Wolfram Sang, Geert Uytterhoeven, Eugeniu Rosca,
Geert Uytterhoeven, Jiri Slaby, Yoshihiro Shimoda, Vinod Koul,
Dan Williams, Wolfram Sang, open list:SERIAL DRIVERS,
Linux-Renesas, dmaengine, George G . Davis
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues
2019-07-03 18:15 ` Eugeniu Rosca
@ 2019-07-03 18:44 ` Greg Kroah-Hartman
2019-07-03 21:08 ` Wolfram Sang
0 siblings, 1 reply; 16+ messages in thread
From: Greg Kroah-Hartman @ 2019-07-03 18:44 UTC (permalink / raw)
To: Eugeniu Rosca
Cc: Eugeniu Rosca, Wolfram Sang, Geert Uytterhoeven,
Geert Uytterhoeven, Jiri Slaby, Yoshihiro Shimoda, Vinod Koul,
Dan Williams, Wolfram Sang, open list:SERIAL DRIVERS,
Linux-Renesas, dmaengine, George G . Davis
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues
2019-07-03 18:44 ` Greg Kroah-Hartman
@ 2019-07-03 21:08 ` Wolfram Sang
0 siblings, 0 replies; 16+ messages in thread
From: Wolfram Sang @ 2019-07-03 21:08 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Eugeniu Rosca, Eugeniu Rosca, Geert Uytterhoeven,
Geert Uytterhoeven, Jiri Slaby, Yoshihiro Shimoda, Vinod Koul,
Dan Williams, Wolfram Sang, open list:SERIAL DRIVERS,
Linux-Renesas, dmaengine, George G . Davis
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-07-03 21:09 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-24 12:35 [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues Geert Uytterhoeven
2019-06-24 12:35 ` [PATCH 1/2] serial: sh-sci: Fix TX DMA buffer flushing and workqueue races Geert Uytterhoeven
2019-06-28 12:53 ` Eugeniu Rosca
2019-06-24 12:35 ` [PATCH 2/2] serial: sh-sci: Terminate TX DMA during buffer flushing Geert Uytterhoeven
2019-06-28 14:04 ` Eugeniu Rosca
2019-06-26 17:34 ` [PATCH 0/2] serial: sh-sci: Fix .flush_buffer() issues Eugeniu Rosca
2019-06-28 11:51 ` Geert Uytterhoeven
2019-06-28 12:39 ` Eugeniu Rosca
2019-06-28 12:55 ` Wolfram Sang
2019-06-28 13:02 ` Eugeniu Rosca
2019-06-28 13:14 ` Wolfram Sang
2019-07-03 17:30 ` Greg Kroah-Hartman
2019-07-03 18:15 ` Eugeniu Rosca
2019-07-03 18:44 ` Greg Kroah-Hartman
2019-07-03 21:08 ` Wolfram Sang
2019-06-28 16:29 ` George G. Davis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).