[PATCH 0/7] serial: imx: various improvements

[PATCH 0/7] serial: imx: various improvements

[Romain Perier]

During shutdown when a userspace service is disabled (which generates
an uart close), we got kernel crashes in the imx serial driver :

[ 1257.657423] Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0938000
[ 1257.665122] pgd = ecf20000
[ 1257.667838] [f0938000] *pgd=de819811, *pte=53fc0653, *ppte=53fc0453
[ 1257.674179] Internal error: : 1008 [#1] SMP ARM
[ 1257.678722] Modules linked in:
[ 1257.681807] CPU: 0 PID: 3850 Comm: emerald_acq Not tainted 4.8.0 #10
[ 1257.688168] Hardware name: Freescale i.MX53 (Device Tree Support)
[ 1257.694269] task: e5c48000 task.stack: ed0b4000
[ 1257.698827] PC is at imx_rxint+0x5c/0x228
[ 1257.702859] LR is at lock_acquired+0x494/0x57c
[ 1257.707312] pc : [<80484884>]    lr : [<80173aa0>]    psr: 20070193
[ 1257.707312] sp : ed0b5c60  ip : ed0b5be8  fp : ed0b5c9c
[ 1257.718795] r10: 00000000  r9 : 00000000  r8 : 00000004
[ 1257.724027] r7 : 00000030  r6 : ee83e258  r5 : 00000000  r4 : ee09f410
[ 1257.730561] r3 : 0015c30c  r2 : f0938000  r1 : 00000135  r0 : 40070193
[ 1257.737099] Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
[ 1257.744327] Control: 10c5387d  Table: dcf20019  DAC: 00000051
[ 1257.750080] Process emerald_acq (pid: 3850, stack limit = 0xed0b4210)
[ 1257.756527] Stack: (0xed0b5c60 to 0xed0b6000)
[ 1257.760898] 5c60: ed0b5cf4 40070193 80175f64 80faf384 e5c484c0 ee09f410 00007240 00005099
[ 1257.769087] 5c80: 00000030 00000030 80e025c4 00000000 ed0b5cf4 ed0b5ca0 80485b18 80484834
[ 1257.777275] 5ca0: 8012fe26 00000000 00000000 60070193 ee81b210 00000001 ed0b5cdc ed0b5cc8
[ 1257.785463] 5cc0: 80171d28 80171c3c 80e2d634 ee096740 ee81b200 ee81b210 00000001 00000030
[ 1257.793651] 5ce0: 80e025c4 00000000 ed0b5d34 ed0b5cf8 80182f10 804857fc e5c484c0 00000002
[ 1257.801839] 5d00: ee81b200 ed0b5d3c 60070193 ee81b200 ee81b200 ee81b210 00000001 ee81c400
[ 1257.810027] 5d20: 00000001 00000008 ed0b5d54 ed0b5d38 801832d8 80182ed0 808f4b60 00000000
[ 1257.818216] 5d40: ee81b200 ee81b260 ed0b5d74 ed0b5d58 8018335c 801832b8 ee81b200 ee81b260
[ 1257.826404] 5d60: ee81b210 00000001 ed0b5d94 ed0b5d78 80186ba0 80183320 80debe8c 00000030
[ 1257.834592] 5d80: 00000000 00000001 ed0b5da4 ed0b5d98 80182420 80186af4 ed0b5dcc ed0b5da8
[ 1257.842780] 5da0: 801827a0 801823fc 00000000 80e8350c 00000020 00000001 ed0b5df8 00000001
[ 1257.850968] 5dc0: ed0b5df4 ed0b5dd0 80101530 8018274c 808f4bec 20070013 ffffffff ed0b5e2c
[ 1257.859156] 5de0: ee09f410 ed0b4000 ed0b5e5c ed0b5df8 808f55f0 801014c8 00000001 00000130
[ 1257.867345] 5e00: 00000000 e5c48000 60070013 ee09f410 00000000 60070013 ee09f410 ed06a640
[ 1257.875533] 5e20: 00000008 ed0b5e5c ed0b5df0 ed0b5e48 801756a8 808f4bec 20070013 ffffffff
[ 1257.883722] 5e40: 00000051 7f000000 ee09f410 00000b01 ed0b5e7c ed0b5e60 80485d74 808f4bb4
[ 1257.891912] 5e60: ee83e258 ee09f410 ee83e3a4 80e2d634 ed0b5ea4 ed0b5e80 8047f514 80485c70
[ 1257.900100] 5e80: ee83e258 ed375000 ee09f410 ee83e310 ee83e3ac ed06a640 ed0b5ecc ed0b5ea8
[ 1257.908288] 5ea0: 80481304 8047f400 ed375000 eeabce60 00000000 ee7973e8 00000000 ed06a640
[ 1257.916477] 5ec0: ed0b5f14 ed0b5ed0 80462fe0 804811ac 00000008 eeabce60 00000001 00000001
[ 1257.924665] 5ee0: 00000000 802713dc ed0b5f54 ed06a640 eeabce60 ee2c6910 ee7973e8 00000000
[ 1257.932854] 5f00: eeabce60 00000008 ed0b5f54 ed0b5f18 80271404 80462ee8 00000000 00000000
[ 1257.941044] 5f20: ed06c640 ed06a648 ed0b5f4c e5c48400 00000000 80e84054 e5c48440 e5c48000
[ 1257.949232] 5f40: 00000000 00000000 ed0b5f64 ed0b5f58 802715c4 80271378 ed0b5f8c ed0b5f68
[ 1257.957420] 5f60: 80146034 802715b8 00000000 ed0b4000 ed0b5fb0 801086c4 801086c4 ed0b4000
[ 1257.965610] 5f80: ed0b5fac ed0b5f90 8010cc68 80145f78 0054756c 00000000 767474b4 00000006
[ 1257.973798] 5fa0: 00000000 ed0b5fb0 80108548 8010cbc4 00000000 76f2a084 00000002 00000000
[ 1257.981986] 5fc0: 0054756c 00000000 767474b4 00000006 0225e880 00000000 76f36000 7e836d34
[ 1257.990175] 5fe0: 00000000 7e836d10 76f2a4c0 76a5db68 80070010 00000062 00000000 00000000
[ 1257.998357] Backtrace: 
[ 1258.000837] [<80484828>] (imx_rxint) from [<80485b18>] (imx_int+0x328/0x474)
[ 1258.007892]  r10:00000000 r9:80e025c4 r8:00000030 r7:00000030 r6:00005099 r5:00007240
[ 1258.015815]  r4:ee09f410
[ 1258.018386] [<804857f0>] (imx_int) from [<80182f10>] (__handle_irq_event_percpu+0x4c/0x3e8)
[ 1258.026742]  r10:00000000 r9:80e025c4 r8:00000030 r7:00000001 r6:ee81b210 r5:ee81b200
[ 1258.034664]  r4:ee096740
[ 1258.037226] [<80182ec4>] (__handle_irq_event_percpu) from [<801832d8>] (handle_irq_event_percpu+0x2c/0x68)
[ 1258.046885]  r10:00000008 r9:00000001 r8:ee81c400 r7:00000001 r6:ee81b210 r5:ee81b200
[ 1258.054806]  r4:ee81b200
[ 1258.057369] [<801832ac>] (handle_irq_event_percpu) from [<8018335c>] (handle_irq_event+0x48/0x6c)
[ 1258.066246]  r5:ee81b260 r4:ee81b200
[ 1258.069866] [<80183314>] (handle_irq_event) from [<80186ba0>] (handle_level_irq+0xb8/0x154)
[ 1258.078222]  r7:00000001 r6:ee81b210 r5:ee81b260 r4:ee81b200
[ 1258.083961] [<80186ae8>] (handle_level_irq) from [<80182420>] (generic_handle_irq+0x30/0x44)
[ 1258.092404]  r7:00000001 r6:00000000 r5:00000030 r4:80debe8c
[ 1258.098143] [<801823f0>] (generic_handle_irq) from [<801827a0>] (__handle_domain_irq+0x60/0xc8)
[ 1258.106854] [<80182740>] (__handle_domain_irq) from [<80101530>] (tzic_handle_irq+0x74/0x9c)
[ 1258.115297]  r9:00000001 r8:ed0b5df8 r7:00000001 r6:00000020 r5:80e8350c r4:00000000
[ 1258.123139] [<801014bc>] (tzic_handle_irq) from [<808f55f0>] (__irq_svc+0x70/0x98)
[ 1258.130715] Exception stack(0xed0b5df8 to 0xed0b5e40)
[ 1258.135773] 5de0:                                                       00000001 00000130
[ 1258.143962] 5e00: 00000000 e5c48000 60070013 ee09f410 00000000 60070013 ee09f410 ed06a640
[ 1258.152151] 5e20: 00000008 ed0b5e5c ed0b5df0 ed0b5e48 801756a8 808f4bec 20070013 ffffffff
[ 1258.160333]  r9:ed0b4000 r8:ee09f410 r7:ed0b5e2c r6:ffffffff r5:20070013 r4:808f4bec
[ 1258.168188] [<808f4ba8>] (_raw_spin_unlock_irqrestore) from [<80485d74>] (imx_shutdown+0x110/0x214)
[ 1258.177239]  r5:00000b01 r4:ee09f410
[ 1258.180860] [<80485c64>] (imx_shutdown) from [<8047f514>] (uart_shutdown+0x120/0x17c)
[ 1258.188695]  r7:80e2d634 r6:ee83e3a4 r5:ee09f410 r4:ee83e258
[ 1258.194434] [<8047f3f4>] (uart_shutdown) from [<80481304>] (uart_close+0x164/0x254)
[ 1258.202096]  r9:ed06a640 r8:ee83e3ac r7:ee83e310 r6:ee09f410 r5:ed375000 r4:ee83e258
[ 1258.209948] [<804811a0>] (uart_close) from [<80462fe0>] (tty_release+0x104/0x498)
[ 1258.217439]  r9:ed06a640 r8:00000000 r7:ee7973e8 r6:00000000 r5:eeabce60 r4:ed375000
[ 1258.225285] [<80462edc>] (tty_release) from [<80271404>] (__fput+0x98/0x1e8)
[ 1258.232339]  r10:00000008 r9:eeabce60 r8:00000000 r7:ee7973e8 r6:ee2c6910 r5:eeabce60
[ 1258.240261]  r4:ed06a640
[ 1258.242823] [<8027136c>] (__fput) from [<802715c4>] (____fput+0x18/0x1c)
[ 1258.249528]  r10:00000000 r9:00000000 r8:e5c48000 r7:e5c48440 r6:80e84054 r5:00000000
[ 1258.257450]  r4:e5c48400
[ 1258.260021] [<802715ac>] (____fput) from [<80146034>] (task_work_run+0xc8/0xf8)
[ 1258.267352] [<80145f6c>] (task_work_run) from [<8010cc68>] (do_work_pending+0xb0/0xd0)
[ 1258.275274]  r9:ed0b4000 r8:801086c4 r7:801086c4 r6:ed0b5fb0 r5:ed0b4000 r4:00000000
[ 1258.283116] [<8010cbb8>] (do_work_pending) from [<80108548>] (slow_work_pending+0xc/0x20)
[ 1258.291298]  r7:00000006 r6:767474b4 r5:00000000 r4:0054756c
[ 1258.297037] Code: e5943094 e594202c e2833001 e5843094 (e592a000) 
[ 1258.303148] ---[ end trace 7a50198148a54c4d ]---
[ 1258.307776] Kernel panic - not syncing: Fatal exception in interrupt
[ 1258.314160] ---[ end Kernel panic - not syncing: Fatal exception in interrupt


After investigations, we found several issues:
It looks that interrupts can happen after the dma was disabled and
port was not yet shutdown. This will result in interrupts handled by
imx_rxint. Analyzed the imx_shutdown function, and found that:

- Some interrupts were not disabled during shutdown (AWAKEN, UCR4_OREN,
  UCR1_TRDYEN) 
- TX was not stopped in all situations (if dma is enabled and
  transmitting)
- Using deprecated dmaengine_terminate_all method.
- Trying to close DMA channels several times. 

This set of patches proposes fix regarding these issues and problems we
found during debugging.

Nandor Han (7):
  serial: imx: only set DMA rx-ing when DMA starts
  serial: imx: move log from error to debug type
  serial: imx: init dma_is_{rx|tx}ing variables
  serial: imx: Simplify DMA disablement
  serial: imx: umap sg buffers when DMA channel is released
  serial: imx: update the stop rx,tx procedures
  serial: imx: Fix imx_shutdown procedure

 drivers/tty/serial/imx.c | 134 ++++++++++++++++++++++++++++-------------------
 1 file changed, 80 insertions(+), 54 deletions(-)

-- 
1.8.3.1

[PATCH 1/7] serial: imx: only set DMA rx-ing when DMA starts

[Romain Perier]

From: Nandor Han <nandor.han@ge.com>

Avoid the situation when `dma_is_rxing` could incorrectly signal that
DMA RX channel is receiving data in case DMA preparation or sg mapping
fails.

Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
 drivers/tty/serial/imx.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 5437b34..1d35293 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -725,11 +725,11 @@ static irqreturn_t imx_rxint(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static void imx_disable_rx_int(struct imx_port *sport)
+static void imx_disable_rx_int(struct imx_port *sport, bool is_rxing)
 {
 	unsigned long temp;
 
-	sport->dma_is_rxing = 1;
+	sport->dma_is_rxing = is_rxing;
 
 	/* disable the receiver ready and aging timer interrupts */
 	temp = readl(sport->port.membase + UCR1);
@@ -762,7 +762,7 @@ static void imx_dma_rxint(struct imx_port *sport)
 	temp = readl(sport->port.membase + USR2);
 	if ((temp & USR2_RDR) && !sport->dma_is_rxing) {
 
-		imx_disable_rx_int(sport);
+		imx_disable_rx_int(sport, false);
 
 		/* tell the DMA to receive the data. */
 		start_rx_dma(sport);
@@ -1083,6 +1083,7 @@ static int start_rx_dma(struct imx_port *sport)
 	desc->callback_param = sport;
 
 	dev_dbg(dev, "RX: prepare for the DMA.\n");
+	sport->dma_is_rxing = 1;
 	sport->rx_cookie = dmaengine_submit(desc);
 	dma_async_issue_pending(chan);
 	return 0;
@@ -1362,7 +1363,7 @@ static int imx_startup(struct uart_port *port)
 		spin_unlock(&tty->files_lock);
 
 		if (readcnt > 0) {
-			imx_disable_rx_int(sport);
+			imx_disable_rx_int(sport, true);
 			start_rx_dma(sport);
 		}
 	}
-- 
1.8.3.1

[PATCH 2/7] serial: imx: move log from error to debug type

[Romain Perier]

From: Nandor Han <nandor.han@ge.com>

During DMA startup we have a data race condition since UART port can
receive data that can generate different type of errors.

This is not necessarily an error since DMA didn't yet started. The
situation is minimized but still present even if we try to clear up the
error before starting the DMA.

Therefore changing the log to debug type we avoid having "false" error
messages.

Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
 drivers/tty/serial/imx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 1d35293..188063d 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -994,7 +994,7 @@ static void dma_rx_callback(void *data)
 	status = dmaengine_tx_status(chan, (dma_cookie_t)0, &state);
 
 	if (status == DMA_ERROR) {
-		dev_err(sport->port.dev, "DMA transaction error.\n");
+		dev_dbg(sport->port.dev, "DMA transaction error.\n");
 		clear_rx_errors(sport);
 		return;
 	}
-- 
1.8.3.1

[PATCH 3/7] serial: imx: init dma_is_{rx|tx}ing variables

[Romain Perier]

From: Nandor Han <nandor.han@ge.com>

Initialize both dma_is_{rx|tx}ing variables when DMA is enabled to avoid
checking uninitialized variables if port shutdown is requested before
DMA channels get a chance to start.

Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
 drivers/tty/serial/imx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 188063d..81fb413 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1225,6 +1225,9 @@ static void imx_enable_dma(struct imx_port *sport)
 
 	imx_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
 
+	sport->dma_is_rxing = 0;
+	sport->dma_is_txing = 0;
+
 	sport->dma_is_enabled = 1;
 }
 
-- 
1.8.3.1

[PATCH 4/7] serial: imx: Simplify DMA disablement

[Romain Perier]

From: Nandor Han <nandor.han@ge.com>

This commits simplify the function imx_disable_dma() by moving
the code for disabling RX and TX DMAs to dedicated functions.
Also move away CTSC and CTS as this is not related to DMA.

Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
 drivers/tty/serial/imx.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 81fb413..e8cf7cf 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1208,6 +1208,24 @@ static int imx_uart_dma_init(struct imx_port *sport)
 	return ret;
 }
 
+static void imx_stop_tx_dma(struct imx_port *sport)
+{
+	unsigned long temp;
+
+	temp = readl(sport->port.membase + UCR1);
+	temp &= ~UCR1_TDMAEN;
+	writel(temp, sport->port.membase + UCR1);
+}
+
+static void imx_stop_rx_dma(struct imx_port *sport)
+{
+	unsigned long temp;
+
+	temp = readl(sport->port.membase + UCR1);
+	temp &= ~(UCR1_RDMAEN | UCR1_ATDMAEN);
+	writel(temp, sport->port.membase + UCR1);
+}
+
 static void imx_enable_dma(struct imx_port *sport)
 {
 	unsigned long temp;
@@ -1233,17 +1251,8 @@ static void imx_enable_dma(struct imx_port *sport)
 
 static void imx_disable_dma(struct imx_port *sport)
 {
-	unsigned long temp;
-
-	/* clear UCR1 */
-	temp = readl(sport->port.membase + UCR1);
-	temp &= ~(UCR1_RDMAEN | UCR1_TDMAEN | UCR1_ATDMAEN);
-	writel(temp, sport->port.membase + UCR1);
-
-	/* clear UCR2 */
-	temp = readl(sport->port.membase + UCR2);
-	temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
-	writel(temp, sport->port.membase + UCR2);
+	imx_stop_rx_dma(sport);
+	imx_stop_tx_dma(sport);
 
 	imx_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
 
-- 
1.8.3.1

[PATCH 5/7] serial: imx: umap sg buffers when DMA channel is released

[Romain Perier]

From: Nandor Han <nandor.han@ge.com>

This commits unmaps sg buffers when the DMA channel is released

Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
 drivers/tty/serial/imx.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index e8cf7cf..58d6b1c 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1215,6 +1215,12 @@ static void imx_stop_tx_dma(struct imx_port *sport)
 	temp = readl(sport->port.membase + UCR1);
 	temp &= ~UCR1_TDMAEN;
 	writel(temp, sport->port.membase + UCR1);
+
+	if (sport->dma_is_txing) {
+		dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
+		sport->dma_tx_nents, DMA_TO_DEVICE);
+		sport->dma_is_txing = 0;
+	}
 }
 
 static void imx_stop_rx_dma(struct imx_port *sport)
@@ -1224,6 +1230,12 @@ static void imx_stop_rx_dma(struct imx_port *sport)
 	temp = readl(sport->port.membase + UCR1);
 	temp &= ~(UCR1_RDMAEN | UCR1_ATDMAEN);
 	writel(temp, sport->port.membase + UCR1);
+
+	if (sport->dma_is_rxing) {
+		dma_unmap_sg(sport->port.dev, &sport->rx_sgl, 1,
+			DMA_FROM_DEVICE);
+		sport->dma_is_rxing = 0;
+	}
 }
 
 static void imx_enable_dma(struct imx_port *sport)
-- 
1.8.3.1

[PATCH 6/7] serial: imx: update the stop rx,tx procedures

[Romain Perier]

From: Nandor Han <nandor.han@ge.com>

According to "Documentation/serial/driver" both procedures should stop
receiving or sending data. Based on this the procedures should stop the
activity regardless if DMA is enabled or not.

This commit updates both imx_stop_{rx|tx} procedures to stop the
activity and disable the interrupts related to that. In case DMA is used
the sg buffers are also un-maped.

Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
 drivers/tty/serial/imx.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 58d6b1c..d5b6e09 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -360,6 +360,9 @@ static void imx_port_rts_auto(struct imx_port *sport, unsigned long *ucr2)
 	*ucr2 |= UCR2_CTSC;
 }
 
+static void imx_stop_rx_dma(struct imx_port *sport);
+static void imx_stop_tx_dma(struct imx_port *sport);
+
 /*
  * interrupts disabled on entry
  */
@@ -368,15 +371,14 @@ static void imx_stop_tx(struct uart_port *port)
 	struct imx_port *sport = (struct imx_port *)port;
 	unsigned long temp;
 
-	/*
-	 * We are maybe in the SMP context, so if the DMA TX thread is running
-	 * on other cpu, we have to wait for it to finish.
-	 */
-	if (sport->dma_is_enabled && sport->dma_is_txing)
-		return;
+	sport->tx_bytes = 0;
+
+	if (sport->dma_is_enabled)
+		imx_stop_tx_dma(sport);
 
 	temp = readl(port->membase + UCR1);
-	writel(temp & ~UCR1_TXMPTYEN, port->membase + UCR1);
+	temp &= ~(UCR1_TXMPTYEN | UCR1_TRDYEN | UCR1_RTSDEN);
+	writel(temp, port->membase + UCR1);
 
 	/* in rs485 mode disable transmitter if shifter is empty */
 	if (port->rs485.flags & SER_RS485_ENABLED &&
@@ -403,21 +405,20 @@ static void imx_stop_rx(struct uart_port *port)
 	struct imx_port *sport = (struct imx_port *)port;
 	unsigned long temp;
 
-	if (sport->dma_is_enabled && sport->dma_is_rxing) {
-		if (sport->port.suspended) {
-			dmaengine_terminate_all(sport->dma_chan_rx);
-			sport->dma_is_rxing = 0;
-		} else {
-			return;
-		}
-	}
+	if (sport->dma_is_enabled)
+		imx_stop_rx_dma(sport);
+
+	temp = readl(sport->port.membase + UCR1);
+	temp &= ~UCR1_RRDYEN;
+	writel(temp, sport->port.membase + UCR1);
 
 	temp = readl(sport->port.membase + UCR2);
-	writel(temp & ~UCR2_RXEN, sport->port.membase + UCR2);
+	temp &= ~UCR2_ATEN;
+	writel(temp, sport->port.membase + UCR2);
 
-	/* disable the `Receiver Ready Interrrupt` */
-	temp = readl(sport->port.membase + UCR1);
-	writel(temp & ~UCR1_RRDYEN, sport->port.membase + UCR1);
+	temp = readl(sport->port.membase + UCR3);
+	temp &= ~UCR3_AWAKEN;
+	writel(temp, sport->port.membase + UCR3);
 }
 
 /*
-- 
1.8.3.1

[PATCH 7/7] serial: imx: Fix imx_shutdown procedure

[Romain Perier]

From: Nandor Han <nandor.han@ge.com>

In some cases, It looks that interrupts can happen after the dma was
disabled and port was not yet shutdown. This will result in interrupts
handled by imx_rxint.

This commits updates the shutdown function to ensure that underlying
components are disabled in the right order. This disables RX and TX
blocks, then it disabled interrupts. In case DMA is enabled, it disables
DMA and free corresponding resources. It disables UART port and stop
clocks.

Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
 drivers/tty/serial/imx.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index d5b6e09..7dc6f0c 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1404,44 +1404,44 @@ static void imx_shutdown(struct uart_port *port)
 	unsigned long temp;
 	unsigned long flags;
 
-	if (sport->dma_is_enabled) {
-		sport->dma_is_rxing = 0;
-		sport->dma_is_txing = 0;
-		dmaengine_terminate_sync(sport->dma_chan_tx);
-		dmaengine_terminate_sync(sport->dma_chan_rx);
-
+	if (!sport->port.suspended) {
 		spin_lock_irqsave(&sport->port.lock, flags);
 		imx_stop_tx(port);
 		imx_stop_rx(port);
-		imx_disable_dma(sport);
 		spin_unlock_irqrestore(&sport->port.lock, flags);
-		imx_uart_dma_exit(sport);
 	}
 
-	mctrl_gpio_disable_ms(sport->gpios);
+	if (sport->dma_is_inited) {
+		if (sport->dma_is_enabled) {
+			spin_lock_irqsave(&sport->port.lock, flags);
+			imx_disable_dma(sport);
+			spin_unlock_irqrestore(&sport->port.lock, flags);
+		}
+		imx_uart_dma_exit(sport);
+	}
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 	temp = readl(sport->port.membase + UCR2);
-	temp &= ~(UCR2_TXEN);
+	temp &= ~(UCR2_TXEN | UCR2_RXEN);
 	writel(temp, sport->port.membase + UCR2);
+	temp = readl(sport->port.membase + UCR4);
+	temp &= ~UCR4_OREN;
+	writel(temp, sport->port.membase + UCR4);
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 
-	/*
-	 * Stop our timer.
-	 */
-	del_timer_sync(&sport->timer);
+	mctrl_gpio_disable_ms(sport->gpios);
 
-	/*
-	 * Disable all interrupts, port and break condition.
-	 */
+	/* Stop our timer. */
+	del_timer_sync(&sport->timer);
 
+	/* Disable port. */
 	spin_lock_irqsave(&sport->port.lock, flags);
 	temp = readl(sport->port.membase + UCR1);
-	temp &= ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN);
-
+	temp &= ~UCR1_UARTEN;
 	writel(temp, sport->port.membase + UCR1);
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 
+	/* Disable clocks. */
 	clk_disable_unprepare(sport->clk_per);
 	clk_disable_unprepare(sport->clk_ipg);
 }
-- 
1.8.3.1

Re: [PATCH 3/7] serial: imx: init dma_is_{rx|tx}ing variables

[Lothar Waßmann]

Hi,

On Fri, 30 Jun 2017 14:04:42 +0200 Romain Perier wrote:
> From: Nandor Han <nandor.han@ge.com>
> 
> Initialize both dma_is_{rx|tx}ing variables when DMA is enabled to avoid
> checking uninitialized variables if port shutdown is requested before
> DMA channels get a chance to start.
> 
> Signed-off-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> ---
>  drivers/tty/serial/imx.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 188063d..81fb413 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1225,6 +1225,9 @@ static void imx_enable_dma(struct imx_port *sport)
>  
>  	imx_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
>  
> +	sport->dma_is_rxing = 0;
> +	sport->dma_is_txing = 0;
> +
>  	sport->dma_is_enabled = 1;
>  }
>  
sport is devm_kzalloc()ed, so the variables are initialized to 0 anyway.


Lothar Waßmann

Re: [PATCH 1/7] serial: imx: only set DMA rx-ing when DMA starts

[Uwe Kleine-König]

Hello,

On Fri, Jun 30, 2017 at 02:04:40PM +0200, Romain Perier wrote:
> From: Nandor Han <nandor.han@ge.com>
> 
> Avoid the situation when `dma_is_rxing` could incorrectly signal that
> DMA RX channel is receiving data in case DMA preparation or sg mapping
> fails.

Just from reading the commit log and the patch I didn't understand the
problem that is supposed to be fixed here. After looking at it for some
I understood. I don't suggest a new commit log here as it has to look
different if you pick up my comment below.

> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 5437b34..1d35293 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -725,11 +725,11 @@ static irqreturn_t imx_rxint(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> -static void imx_disable_rx_int(struct imx_port *sport)
> +static void imx_disable_rx_int(struct imx_port *sport, bool is_rxing)
>  {
>  	unsigned long temp;
>  
> -	sport->dma_is_rxing = 1;
> +	sport->dma_is_rxing = is_rxing;
>  
>  	/* disable the receiver ready and aging timer interrupts */
>  	temp = readl(sport->port.membase + UCR1);
> @@ -762,7 +762,7 @@ static void imx_dma_rxint(struct imx_port *sport)
>  	temp = readl(sport->port.membase + USR2);
>  	if ((temp & USR2_RDR) && !sport->dma_is_rxing) {
>  
> -		imx_disable_rx_int(sport);
> +		imx_disable_rx_int(sport, false);
>  
>  		/* tell the DMA to receive the data. */
>  		start_rx_dma(sport);
> @@ -1083,6 +1083,7 @@ static int start_rx_dma(struct imx_port *sport)
>  	desc->callback_param = sport;
>  
>  	dev_dbg(dev, "RX: prepare for the DMA.\n");
> +	sport->dma_is_rxing = 1;
>  	sport->rx_cookie = dmaengine_submit(desc);
>  	dma_async_issue_pending(chan);
>  	return 0;
> @@ -1362,7 +1363,7 @@ static int imx_startup(struct uart_port *port)
>  		spin_unlock(&tty->files_lock);
>  
>  		if (readcnt > 0) {
> -			imx_disable_rx_int(sport);
> +			imx_disable_rx_int(sport, true);

I wonder if it would be saner to not assign to dma_is_rxing in
imx_disable_rx_int() at all but do this only in start_rx_dma() and
imx_dma_rxint().

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/7] serial: imx: move log from error to debug type

[Uwe Kleine-König]

On Fri, Jun 30, 2017 at 02:04:41PM +0200, Romain Perier wrote:
> From: Nandor Han <nandor.han@ge.com>
> 
> During DMA startup we have a data race condition since UART port can
> receive data that can generate different type of errors.
> 
> This is not necessarily an error since DMA didn't yet started. The
> situation is minimized but still present even if we try to clear up the
> error before starting the DMA.
> 
> Therefore changing the log to debug type we avoid having "false" error
> messages.

This doesn't look right. You say the message "DMA transaction error." is
wrong sometimes and so hide it a bit by using dev_dbg instead of
dev_err.

I don't like that.

Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 3/7] serial: imx: init dma_is_{rx|tx}ing variables

[Uwe Kleine-König]

On Fri, Jun 30, 2017 at 02:13:29PM +0200, Lothar Waßmann wrote:
> Hi,
> 
> On Fri, 30 Jun 2017 14:04:42 +0200 Romain Perier wrote:
> > From: Nandor Han <nandor.han@ge.com>
> > 
> > Initialize both dma_is_{rx|tx}ing variables when DMA is enabled to avoid
> > checking uninitialized variables if port shutdown is requested before
> > DMA channels get a chance to start.
> > 
> > Signed-off-by: Nandor Han <nandor.han@ge.com>
> > Signed-off-by: Romain Perier <romain.perier@collabora.com>
> > ---
> >  drivers/tty/serial/imx.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 188063d..81fb413 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1225,6 +1225,9 @@ static void imx_enable_dma(struct imx_port *sport)
> >  
> >  	imx_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
> >  
> > +	sport->dma_is_rxing = 0;
> > +	sport->dma_is_txing = 0;
> > +
> >  	sport->dma_is_enabled = 1;
> >  }
> >  
> sport is devm_kzalloc()ed, so the variables are initialized to 0 anyway.

I'd agree to Lothar's statement. Did you find this issue by inspection,
or does it fix a compiler warning? Do you think there is an actual
problem?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 4/7] serial: imx: Simplify DMA disablement

[Uwe Kleine-König]

On Fri, Jun 30, 2017 at 02:04:43PM +0200, Romain Perier wrote:
> From: Nandor Han <nandor.han@ge.com>
> 
> This commits simplify the function imx_disable_dma() by moving
> the code for disabling RX and TX DMAs to dedicated functions.

I'd point out there that this prepares the next commit because in the
current state I'd prefer just improved comments.

> Also move away CTSC and CTS as this is not related to DMA.

Hmm, maybe this is related to the rs485 breakage we're seeing and
deserves a separate commit?

This also has the advantage that the refactoring change is semantically
a no-op.

> Signed-off-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> ---
>  drivers/tty/serial/imx.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 81fb413..e8cf7cf 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1208,6 +1208,24 @@ static int imx_uart_dma_init(struct imx_port *sport)
>  	return ret;
>  }
>  
> +static void imx_stop_tx_dma(struct imx_port *sport)
> +{
> +	unsigned long temp;
> +
> +	temp = readl(sport->port.membase + UCR1);
> +	temp &= ~UCR1_TDMAEN;
> +	writel(temp, sport->port.membase + UCR1);
> +}
> +
> +static void imx_stop_rx_dma(struct imx_port *sport)
> +{
> +	unsigned long temp;
> +
> +	temp = readl(sport->port.membase + UCR1);
> +	temp &= ~(UCR1_RDMAEN | UCR1_ATDMAEN);
> +	writel(temp, sport->port.membase + UCR1);
> +}
> +
>  static void imx_enable_dma(struct imx_port *sport)
>  {
>  	unsigned long temp;
> @@ -1233,17 +1251,8 @@ static void imx_enable_dma(struct imx_port *sport)
>  
>  static void imx_disable_dma(struct imx_port *sport)
>  {
> -	unsigned long temp;
> -
> -	/* clear UCR1 */
> -	temp = readl(sport->port.membase + UCR1);
> -	temp &= ~(UCR1_RDMAEN | UCR1_TDMAEN | UCR1_ATDMAEN);
> -	writel(temp, sport->port.membase + UCR1);
> -
> -	/* clear UCR2 */
> -	temp = readl(sport->port.membase + UCR2);
> -	temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);

You also dropped clearing ATEN without mentioning that in the commit
log. Is this done on purpose?

> -	writel(temp, sport->port.membase + UCR2);
> +	imx_stop_rx_dma(sport);
> +	imx_stop_tx_dma(sport);
>  
>  	imx_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 5/7] serial: imx: umap sg buffers when DMA channel is released

[Uwe Kleine-König]

$Subject ~= s/umap/unmap/

On Fri, Jun 30, 2017 at 02:04:44PM +0200, Romain Perier wrote:
> From: Nandor Han <nandor.han@ge.com>
> 
> This commits unmaps sg buffers when the DMA channel is released
> 
> Signed-off-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> ---
>  drivers/tty/serial/imx.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index e8cf7cf..58d6b1c 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1215,6 +1215,12 @@ static void imx_stop_tx_dma(struct imx_port *sport)
>  	temp = readl(sport->port.membase + UCR1);
>  	temp &= ~UCR1_TDMAEN;
>  	writel(temp, sport->port.membase + UCR1);
> +
> +	if (sport->dma_is_txing) {
> +		dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
> +		sport->dma_tx_nents, DMA_TO_DEVICE);
> +		sport->dma_is_txing = 0;

You don't motivate setting dma_is_txing to zero in the commit log. 

Does this mean the driver leaks memory in the current state?

> +	}
>  }
>  
>  static void imx_stop_rx_dma(struct imx_port *sport)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 6/7] serial: imx: update the stop rx,tx procedures

[Uwe Kleine-König]

On Fri, Jun 30, 2017 at 02:04:45PM +0200, Romain Perier wrote:
> From: Nandor Han <nandor.han@ge.com>
> 
> According to "Documentation/serial/driver" both procedures should stop
> receiving or sending data. Based on this the procedures should stop the
> activity regardless if DMA is enabled or not.
> 
> This commit updates both imx_stop_{rx|tx} procedures to stop the
> activity and disable the interrupts related to that. In case DMA is used
> the sg buffers are also un-maped.

This unmapping is implicit, becuae imx_stop_rx_dma unmaps since the
previous commit, right?
> 
> Signed-off-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> ---
>  drivers/tty/serial/imx.c | 39 ++++++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 58d6b1c..d5b6e09 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -360,6 +360,9 @@ static void imx_port_rts_auto(struct imx_port *sport, unsigned long *ucr2)
>  	*ucr2 |= UCR2_CTSC;
>  }
>  
> +static void imx_stop_rx_dma(struct imx_port *sport);
> +static void imx_stop_tx_dma(struct imx_port *sport);

Is it possible to reshuffle the order of functions to make this forward
declaration redundant?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 7/7] serial: imx: Fix imx_shutdown procedure

[Uwe Kleine-König]

On Fri, Jun 30, 2017 at 02:04:46PM +0200, Romain Perier wrote:
> From: Nandor Han <nandor.han@ge.com>
> 
> In some cases, It looks that interrupts can happen after the dma was
s/It/it/

> disabled and port was not yet shutdown. This will result in interrupts
> handled by imx_rxint.
> 
> This commits updates the shutdown function to ensure that underlying
> components are disabled in the right order. This disables RX and TX
> blocks, then it disabled interrupts. In case DMA is enabled, it disables
> DMA and free corresponding resources. It disables UART port and stop
> clocks.
> 
> Signed-off-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> ---
>  drivers/tty/serial/imx.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index d5b6e09..7dc6f0c 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1404,44 +1404,44 @@ static void imx_shutdown(struct uart_port *port)
>  	unsigned long temp;
>  	unsigned long flags;
>  
> -	if (sport->dma_is_enabled) {
> -		sport->dma_is_rxing = 0;
> -		sport->dma_is_txing = 0;
> -		dmaengine_terminate_sync(sport->dma_chan_tx);
> -		dmaengine_terminate_sync(sport->dma_chan_rx);
> -
> +	if (!sport->port.suspended) {
>  		spin_lock_irqsave(&sport->port.lock, flags);
>  		imx_stop_tx(port);
>  		imx_stop_rx(port);
> -		imx_disable_dma(sport);
>  		spin_unlock_irqrestore(&sport->port.lock, flags);
> -		imx_uart_dma_exit(sport);
>  	}
>  
> -	mctrl_gpio_disable_ms(sport->gpios);
> +	if (sport->dma_is_inited) {
> +		if (sport->dma_is_enabled) {
> +			spin_lock_irqsave(&sport->port.lock, flags);
> +			imx_disable_dma(sport);
> +			spin_unlock_irqrestore(&sport->port.lock, flags);
> +		}
> +		imx_uart_dma_exit(sport);
> +	}
>  
>  	spin_lock_irqsave(&sport->port.lock, flags);
>  	temp = readl(sport->port.membase + UCR2);
> -	temp &= ~(UCR2_TXEN);
> +	temp &= ~(UCR2_TXEN | UCR2_RXEN);
>  	writel(temp, sport->port.membase + UCR2);
> +	temp = readl(sport->port.membase + UCR4);
> +	temp &= ~UCR4_OREN;
> +	writel(temp, sport->port.membase + UCR4);
>  	spin_unlock_irqrestore(&sport->port.lock, flags);

The function already had two critical blocks protected by
spin_lock_irqsave on sport->port.lock. With your patch there are three.
Is it possible to grab the lock only once?

>  
> -	/*
> -	 * Stop our timer.
> -	 */
> -	del_timer_sync(&sport->timer);
> +	mctrl_gpio_disable_ms(sport->gpios);
>  
> -	/*
> -	 * Disable all interrupts, port and break condition.
> -	 */
> +	/* Stop our timer. */

Updating the comment style in such a commit makes the diff unnecessarily
hard to read.

> +	del_timer_sync(&sport->timer);
>  
> +	/* Disable port. */
>  	spin_lock_irqsave(&sport->port.lock, flags);
>  	temp = readl(sport->port.membase + UCR1);
> -	temp &= ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN);
> -
> +	temp &= ~UCR1_UARTEN;
>  	writel(temp, sport->port.membase + UCR1);
>  	spin_unlock_irqrestore(&sport->port.lock, flags);
>  
> +	/* Disable clocks. */
>  	clk_disable_unprepare(sport->clk_per);
>  	clk_disable_unprepare(sport->clk_ipg);
>  }
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/7] serial: imx: only set DMA rx-ing when DMA starts

[Romain Perier]

Hello,


Le 03/07/2017 à 08:48, Uwe Kleine-König a écrit :
> Hello,
>
> On Fri, Jun 30, 2017 at 02:04:40PM +0200, Romain Perier wrote:
>> From: Nandor Han <nandor.han@ge.com>
>>
>> Avoid the situation when `dma_is_rxing` could incorrectly signal that
>> DMA RX channel is receiving data in case DMA preparation or sg mapping
>> fails.
> Just from reading the commit log and the patch I didn't understand the
> problem that is supposed to be fixed here. After looking at it for some
> I understood. I don't suggest a new commit log here as it has to look
> different if you pick up my comment below.
>
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 5437b34..1d35293 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -725,11 +725,11 @@ static irqreturn_t imx_rxint(int irq, void *dev_id)
>>  	return IRQ_HANDLED;
>>  }
>>  
>> -static void imx_disable_rx_int(struct imx_port *sport)
>> +static void imx_disable_rx_int(struct imx_port *sport, bool is_rxing)
>>  {
>>  	unsigned long temp;
>>  
>> -	sport->dma_is_rxing = 1;
>> +	sport->dma_is_rxing = is_rxing;
>>  
>>  	/* disable the receiver ready and aging timer interrupts */
>>  	temp = readl(sport->port.membase + UCR1);
>> @@ -762,7 +762,7 @@ static void imx_dma_rxint(struct imx_port *sport)
>>  	temp = readl(sport->port.membase + USR2);
>>  	if ((temp & USR2_RDR) && !sport->dma_is_rxing) {
>>  
>> -		imx_disable_rx_int(sport);
>> +		imx_disable_rx_int(sport, false);
>>  
>>  		/* tell the DMA to receive the data. */
>>  		start_rx_dma(sport);
>> @@ -1083,6 +1083,7 @@ static int start_rx_dma(struct imx_port *sport)
>>  	desc->callback_param = sport;
>>  
>>  	dev_dbg(dev, "RX: prepare for the DMA.\n");
>> +	sport->dma_is_rxing = 1;
>>  	sport->rx_cookie = dmaengine_submit(desc);
>>  	dma_async_issue_pending(chan);
>>  	return 0;
>> @@ -1362,7 +1363,7 @@ static int imx_startup(struct uart_port *port)
>>  		spin_unlock(&tty->files_lock);
>>  
>>  		if (readcnt > 0) {
>> -			imx_disable_rx_int(sport);
>> +			imx_disable_rx_int(sport, true);
> I wonder if it would be saner to not assign to dma_is_rxing in
> imx_disable_rx_int() at all but do this only in start_rx_dma() and
> imx_dma_rxint().

It was "mostly" done, in 4.11 in fact.  dma_is_rxing was set to 0 from
imx_dma_rxint() and was not set
at all from start_rx_dma(), resulting to the same issue. We could move
assignment of dma_is_rxing out of imx_disable_rx_int()
and set it to one from start_rx_dma() only. What do you think ?

Best Regards,
Romain

>
> Best regards
> Uwe
>

Re: [PATCH 3/7] serial: imx: init dma_is_{rx|tx}ing variables

[Romain Perier]

Hello,


Le 03/07/2017 à 08:52, Uwe Kleine-König a écrit :
> On Fri, Jun 30, 2017 at 02:13:29PM +0200, Lothar Waßmann wrote:
>> Hi,
>>
>> On Fri, 30 Jun 2017 14:04:42 +0200 Romain Perier wrote:
>>> From: Nandor Han <nandor.han@ge.com>
>>>
>>> Initialize both dma_is_{rx|tx}ing variables when DMA is enabled to avoid
>>> checking uninitialized variables if port shutdown is requested before
>>> DMA channels get a chance to start.
>>>
>>> Signed-off-by: Nandor Han <nandor.han@ge.com>
>>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>>> ---
>>>  drivers/tty/serial/imx.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>>> index 188063d..81fb413 100644
>>> --- a/drivers/tty/serial/imx.c
>>> +++ b/drivers/tty/serial/imx.c
>>> @@ -1225,6 +1225,9 @@ static void imx_enable_dma(struct imx_port *sport)
>>>  
>>>  	imx_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
>>>  
>>> +	sport->dma_is_rxing = 0;
>>> +	sport->dma_is_txing = 0;
>>> +
>>>  	sport->dma_is_enabled = 1;
>>>  }
>>>  
>> sport is devm_kzalloc()ed, so the variables are initialized to 0 anyway.
> I'd agree to Lothar's statement. Did you find this issue by inspection,
> or does it fix a compiler warning? Do you think there is an actual
> problem?
>
> Best regards
> Uwe
>
What does happen if the UART port is shutdown and then re-enabled ? I
don't think that kzalloc will work in this case

Regards,
Romain

Re: [PATCH 3/7] serial: imx: init dma_is_{rx|tx}ing variables

[Uwe Kleine-König]

Hello Romain,

On Wed, Jul 05, 2017 at 12:14:57PM +0200, Romain Perier wrote:
> Le 03/07/2017 à 08:52, Uwe Kleine-König a écrit :
> > On Fri, Jun 30, 2017 at 02:13:29PM +0200, Lothar Waßmann wrote:
> >> On Fri, 30 Jun 2017 14:04:42 +0200 Romain Perier wrote:
> >>> From: Nandor Han <nandor.han@ge.com>
> >>>
> >>> Initialize both dma_is_{rx|tx}ing variables when DMA is enabled to avoid
> >>> checking uninitialized variables if port shutdown is requested before
> >>> DMA channels get a chance to start.
> >>>
> >>> Signed-off-by: Nandor Han <nandor.han@ge.com>
> >>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> >>> ---
> >>>  drivers/tty/serial/imx.c | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >>> index 188063d..81fb413 100644
> >>> --- a/drivers/tty/serial/imx.c
> >>> +++ b/drivers/tty/serial/imx.c
> >>> @@ -1225,6 +1225,9 @@ static void imx_enable_dma(struct imx_port *sport)
> >>>  
> >>>  	imx_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
> >>>  
> >>> +	sport->dma_is_rxing = 0;
> >>> +	sport->dma_is_txing = 0;
> >>> +
> >>>  	sport->dma_is_enabled = 1;
> >>>  }
> >>>  
> >> sport is devm_kzalloc()ed, so the variables are initialized to 0 anyway.
> > I'd agree to Lothar's statement. Did you find this issue by inspection,
> > or does it fix a compiler warning? Do you think there is an actual
> > problem?
> >
> > Best regards
> > Uwe
> >
> What does happen if the UART port is shutdown and then re-enabled ? I
> don't think that kzalloc will work in this case

imx_shutdown has:

        if (sport->dma_is_enabled) {
		sport->dma_is_rxing = 0;
		sport->dma_is_txing = 0;

which might be good enough. Can dma_is_[rt]xing be != 0 if
dma_is_enabled is false? It seems it cannot, the only place where
dma_is_enabled is set to 0 (apart from the kzalloc where dma_is_[rt]xing
is set to zero) is imx_disable_dma(). The only caller sets both
dma_is_[rt]xing to zero before.

So this patch should be dropped or its commit log improved to point out
the actual problem.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |