RE: [PATCH v3] serial: sh-sci: Stop dmaengine transfer in sci_stop_tx()

Hi Greg, Geert,

From: Greg KH, Sent: Thursday, June 10, 2021 4:02 PM

On Thu, Jun 10, 2021 at 08:58:04AM +0200, Geert Uytterhoeven wrote:

quoted

Hi Shimoda-san,

On Thu, Jun 10, 2021 at 2:50 AM Yoshihiro Shimoda
[off-list ref] wrote:

quoted

quoted

From: Geert Uytterhoeven, Sent: Thursday, June 10, 2021 12:30 AM
On Wed, Jun 9, 2021 at 5:09 PM Sergei Shtylyov
[off-list ref] wrote:

quoted

On 6/9/21 4:49 AM, Yoshihiro Shimoda wrote:

quoted

Stop dmaengine transfer in sci_stop_tx(). Otherwise, the following
message is possible output when system enters suspend and while
transferring data, because clearing TIE bit in SCSCR is not able to
stop any dmaengine transfer.

    sh-sci e6550000.serial: ttySC1: Unable to drain transmitter

Note that this patch uses dmaengine_terminate_async() so that
we can apply this patch into longterm kernel v4.9.x or later.

Fixes: 73a19e4c0301 ("serial: sh-sci: Add DMA support.")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---

 drivers/tty/serial/sh-sci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 4baf1316ea72..2d5487bf6855 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c

@@ -610,6 +610,14 @@ static void sci_stop_tx(struct uart_port *port)
      ctrl &= ~SCSCR_TIE;

      serial_port_out(port, SCSCR, ctrl);
+
+#ifdef CONFIG_SERIAL_SH_SCI_DMA

   Why not use IS_ENABLED() instead? Gets rid of #ifdef. :-)

quoted

+     if (to_sci_port(port)->chan_tx &&
+         !dma_submit_error(to_sci_port(port)->cookie_tx)) {
+             dmaengine_terminate_async(to_sci_port(port)->chan_tx);
+             to_sci_port(port)->cookie_tx = -EINVAL;

Because chan_tx and cookie_tx do not exist if CONFIG_SERIAL_SH_SCI_DMA
is disabled.

This is a nit though, chan_tx always exists.

I stand corrected, only cookie_tx depends on CONFIG_SERIAL_SH_SCI_DMA.

quoted

quoted

Yes, that's why all the DMA code in this driver (.c file) is protected by
#ifdef CONFIG_SERIAL_SH_SCI_DMA.

I'm thinking we have to remove #ifdef from sh-sci.c file at first...

While I don't disagree that would be worthwhile, do we really need
to refactor a driver first, before a fix that follows the existing
driver style can be applied (and backported)?

No we do not.  Sorry if this usage is already in the driver, might as
well keep it there, I thought this was an exception and was being added
for the first time here.

Thank you for your comments. I understood we should keep this driver
and fix it at first. So, I'll add a description why we need #ifdef in
.c file on v4 patch.

Best regards,
Yoshihiro Shimoda