[PATCH 0/2] serial: imx: cleanup shutdown

[PATCH 0/2] serial: imx: cleanup shutdown

[Sebastian Reichel]

Hi,

Here is a simple cleanup patch for the imx serial driver. The second
one fixes a missing dma resource free. I tested both patches on imx53
based GE PPD device.

-- Sebastian

Sebastian Reichel (2):
  serial: imx: cleanup imx_uart_disable_dma()
  serial: imx: dma_unmap_sg buffers on shutdown

 drivers/tty/serial/imx.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

-- 
2.17.0

[PATCH 1/2] serial: imx: cleanup imx_uart_disable_dma()

[Sebastian Reichel]

Remove unrelated CTSC/CTS disabling from imx_uart_disable_dma() and
move it to imx_uart_shutdown(), which is the only user of the DMA
disabling function. This should not change the driver's behaviour,
but improves readability. After this change imx_uart_disable_dma()
does the reverse thing of imx_uart_enable_dma().

Suggested-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 drivers/tty/serial/imx.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index c2fc6bef7a6f..3ca767b1162a 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1291,18 +1291,13 @@ static void imx_uart_enable_dma(struct imx_port *sport)
 
 static void imx_uart_disable_dma(struct imx_port *sport)
 {
-	u32 ucr1, ucr2;
+	u32 ucr1;
 
 	/* clear UCR1 */
 	ucr1 = imx_uart_readl(sport, UCR1);
 	ucr1 &= ~(UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN);
 	imx_uart_writel(sport, ucr1, UCR1);
 
-	/* clear UCR2 */
-	ucr2 = imx_uart_readl(sport, UCR2);
-	ucr2 &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
-	imx_uart_writel(sport, ucr2, UCR2);
-
 	imx_uart_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
 
 	sport->dma_is_enabled = 0;
@@ -1447,7 +1442,7 @@ static void imx_uart_shutdown(struct uart_port *port)
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 	ucr2 = imx_uart_readl(sport, UCR2);
-	ucr2 &= ~(UCR2_TXEN | UCR2_ATEN);
+	ucr2 &= ~(UCR2_TXEN | UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
 	imx_uart_writel(sport, ucr2, UCR2);
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 
-- 
2.17.0

[PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown

[Sebastian Reichel]

This properly unmaps DMA SG on device shutdown.

Reported-by: Nandor Han <nandor.han@ge.com>
Suggested-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 drivers/tty/serial/imx.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 3ca767b1162a..6c53e74244ec 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
 	u32 ucr1, ucr2;
 
 	if (sport->dma_is_enabled) {
-		sport->dma_is_rxing = 0;
-		sport->dma_is_txing = 0;
 		dmaengine_terminate_sync(sport->dma_chan_tx);
+		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;
+		}
 		dmaengine_terminate_sync(sport->dma_chan_rx);
+		if (sport->dma_is_rxing) {
+			dma_unmap_sg(sport->port.dev, &sport->rx_sgl,
+				     1, DMA_FROM_DEVICE);
+			sport->dma_is_rxing = 0;
+		}
 
 		spin_lock_irqsave(&sport->port.lock, flags);
 		imx_uart_stop_tx(port);
-- 
2.17.0

Re: [PATCH 1/2] serial: imx: cleanup imx_uart_disable_dma()

[Uwe Kleine-König]

Hello,

On Mon, May 07, 2018 at 11:36:09PM +0200, Sebastian Reichel wrote:
> Remove unrelated CTSC/CTS disabling from imx_uart_disable_dma() and
> move it to imx_uart_shutdown(), which is the only user of the DMA
> disabling function. This should not change the driver's behaviour,
> but improves readability. After this change imx_uart_disable_dma()
> does the reverse thing of imx_uart_enable_dma().
> 
> Suggested-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
>  drivers/tty/serial/imx.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index c2fc6bef7a6f..3ca767b1162a 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1291,18 +1291,13 @@ static void imx_uart_enable_dma(struct imx_port *sport)
>  
>  static void imx_uart_disable_dma(struct imx_port *sport)
>  {
> -	u32 ucr1, ucr2;
> +	u32 ucr1;
>  
>  	/* clear UCR1 */
>  	ucr1 = imx_uart_readl(sport, UCR1);
>  	ucr1 &= ~(UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN);
>  	imx_uart_writel(sport, ucr1, UCR1);
>  
> -	/* clear UCR2 */
> -	ucr2 = imx_uart_readl(sport, UCR2);
> -	ucr2 &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> -	imx_uart_writel(sport, ucr2, UCR2);
> -
>  	imx_uart_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
>  
>  	sport->dma_is_enabled = 0;
> @@ -1447,7 +1442,7 @@ static void imx_uart_shutdown(struct uart_port *port)
>  
>  	spin_lock_irqsave(&sport->port.lock, flags);
>  	ucr2 = imx_uart_readl(sport, UCR2);
> -	ucr2 &= ~(UCR2_TXEN | UCR2_ATEN);
> +	ucr2 &= ~(UCR2_TXEN | UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
>  	imx_uart_writel(sport, ucr2, UCR2);
>  	spin_unlock_irqrestore(&sport->port.lock, flags);

While this doesn't change behaviour (which is of course good and
intended here) I wonder if changing RTS is right here.

According to Documentation/serial/driver it is not.

Best regards
Uwe

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

Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown

[Uwe Kleine-König]

On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> This properly unmaps DMA SG on device shutdown.
> 
> Reported-by: Nandor Han <nandor.han@ge.com>
> Suggested-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
>  drivers/tty/serial/imx.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 3ca767b1162a..6c53e74244ec 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
>  	u32 ucr1, ucr2;
>  
>  	if (sport->dma_is_enabled) {
> -		sport->dma_is_rxing = 0;
> -		sport->dma_is_txing = 0;
>  		dmaengine_terminate_sync(sport->dma_chan_tx);
> +		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;
> +		}

did you find this because the kernel crashed or consumed more and more
memory, or is this "only" a finding of reading the source code? If the
former it would be great to point out in the commit log, if the latter,
I wonder if this is a real problem that warrants a stable backport.

Best regards
Uwe

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

Re: [PATCH 1/2] serial: imx: cleanup imx_uart_disable_dma()

[Sebastian Reichel]

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

Hi,

On Tue, May 08, 2018 at 08:40:47AM +0200, Uwe Kleine-König wrote:
> On Mon, May 07, 2018 at 11:36:09PM +0200, Sebastian Reichel wrote:
> > Remove unrelated CTSC/CTS disabling from imx_uart_disable_dma() and
> > move it to imx_uart_shutdown(), which is the only user of the DMA
> > disabling function. This should not change the driver's behaviour,
> > but improves readability. After this change imx_uart_disable_dma()
> > does the reverse thing of imx_uart_enable_dma().
> > 
> > Suggested-by: Nandor Han <nandor.han@ge.com>
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > ---
> >  drivers/tty/serial/imx.c | 9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index c2fc6bef7a6f..3ca767b1162a 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1291,18 +1291,13 @@ static void imx_uart_enable_dma(struct imx_port *sport)
> >  
> >  static void imx_uart_disable_dma(struct imx_port *sport)
> >  {
> > -	u32 ucr1, ucr2;
> > +	u32 ucr1;
> >  
> >  	/* clear UCR1 */
> >  	ucr1 = imx_uart_readl(sport, UCR1);
> >  	ucr1 &= ~(UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN);
> >  	imx_uart_writel(sport, ucr1, UCR1);
> >  
> > -	/* clear UCR2 */
> > -	ucr2 = imx_uart_readl(sport, UCR2);
> > -	ucr2 &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> > -	imx_uart_writel(sport, ucr2, UCR2);
> > -
> >  	imx_uart_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
> >  
> >  	sport->dma_is_enabled = 0;
> > @@ -1447,7 +1442,7 @@ static void imx_uart_shutdown(struct uart_port *port)
> >  
> >  	spin_lock_irqsave(&sport->port.lock, flags);
> >  	ucr2 = imx_uart_readl(sport, UCR2);
> > -	ucr2 &= ~(UCR2_TXEN | UCR2_ATEN);
> > +	ucr2 &= ~(UCR2_TXEN | UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> >  	imx_uart_writel(sport, ucr2, UCR2);
> >  	spin_unlock_irqrestore(&sport->port.lock, flags);
> 
> While this doesn't change behaviour (which is of course good and
> intended here) I wonder if changing RTS is right here.
> 
> According to Documentation/serial/driver it is not.

Yes, looks like it should be removed completly. I suggest to keep
this in two different patches (move UCR2 handling first, then remove
RTS handling), so that we end up with simple and comprehensible patches.

-- Sebastian

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

Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown

[Sebastian Reichel]

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

Hi,

On Tue, May 08, 2018 at 08:43:51AM +0200, Uwe Kleine-König wrote:
> On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> > This properly unmaps DMA SG on device shutdown.
> > 
> > Reported-by: Nandor Han <nandor.han@ge.com>
> > Suggested-by: Nandor Han <nandor.han@ge.com>
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > ---
> >  drivers/tty/serial/imx.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 3ca767b1162a..6c53e74244ec 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
> >  	u32 ucr1, ucr2;
> >  
> >  	if (sport->dma_is_enabled) {
> > -		sport->dma_is_rxing = 0;
> > -		sport->dma_is_txing = 0;
> >  		dmaengine_terminate_sync(sport->dma_chan_tx);
> > +		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;
> > +		}
> 
> did you find this because the kernel crashed or consumed more and more
> memory, or is this "only" a finding of reading the source code? If the
> former it would be great to point out in the commit log, if the latter,
> I wonder if this is a real problem that warrants a stable backport.

A bit of both. One of Collabora's customers had a (scarce) kernel crash
in imx-serial and modified multiple things in the driver. The crash is
gone, but it's not clear which change fixed it. I could not
reproduce the crash so far and I'm currently rebasing and splitting
their changes into upstreamable portions with proper patch
descriptions. From reading the source this looked like a real issue.

-- Sebastian

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

Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown

[Uwe Kleine-König]

Hello Sebastian,

On Tue, May 08, 2018 at 03:40:47PM +0200, Sebastian Reichel wrote:
> On Tue, May 08, 2018 at 08:43:51AM +0200, Uwe Kleine-König wrote:
> > On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> > > This properly unmaps DMA SG on device shutdown.
> > > 
> > > Reported-by: Nandor Han <nandor.han@ge.com>
> > > Suggested-by: Nandor Han <nandor.han@ge.com>
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > > ---
> > >  drivers/tty/serial/imx.c | 12 ++++++++++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > index 3ca767b1162a..6c53e74244ec 100644
> > > --- a/drivers/tty/serial/imx.c
> > > +++ b/drivers/tty/serial/imx.c
> > > @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
> > >  	u32 ucr1, ucr2;
> > >  
> > >  	if (sport->dma_is_enabled) {
> > > -		sport->dma_is_rxing = 0;
> > > -		sport->dma_is_txing = 0;
> > >  		dmaengine_terminate_sync(sport->dma_chan_tx);
> > > +		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;
> > > +		}
> > 
> > did you find this because the kernel crashed or consumed more and more
> > memory, or is this "only" a finding of reading the source code? If the
> > former it would be great to point out in the commit log, if the latter,
> > I wonder if this is a real problem that warrants a stable backport.
> 
> A bit of both. One of Collabora's customers had a (scarce) kernel crash
> in imx-serial and modified multiple things in the driver. The crash is
> gone, but it's not clear which change fixed it. I could not
> reproduce the crash so far and I'm currently rebasing and splitting
> their changes into upstreamable portions with proper patch
> descriptions. From reading the source this looked like a real issue.

In which context (kernel version, operating mode (e.g. rs485)) did these
happen? What does "crash" mean? The kernel did just hang or produced an
oops? If the latter, can you show it/them?

Best regards
Uwe

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

Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown

[Sebastian Reichel]

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

Hi,

On Tue, May 08, 2018 at 08:46:12PM +0200, Uwe Kleine-König wrote:
> On Tue, May 08, 2018 at 03:40:47PM +0200, Sebastian Reichel wrote:
> > On Tue, May 08, 2018 at 08:43:51AM +0200, Uwe Kleine-König wrote:
> > > On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> > > > This properly unmaps DMA SG on device shutdown.
> > > > 
> > > > Reported-by: Nandor Han <nandor.han@ge.com>
> > > > Suggested-by: Nandor Han <nandor.han@ge.com>
> > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > > > ---
> > > >  drivers/tty/serial/imx.c | 12 ++++++++++--
> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > index 3ca767b1162a..6c53e74244ec 100644
> > > > --- a/drivers/tty/serial/imx.c
> > > > +++ b/drivers/tty/serial/imx.c
> > > > @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
> > > >  	u32 ucr1, ucr2;
> > > >  
> > > >  	if (sport->dma_is_enabled) {
> > > > -		sport->dma_is_rxing = 0;
> > > > -		sport->dma_is_txing = 0;
> > > >  		dmaengine_terminate_sync(sport->dma_chan_tx);
> > > > +		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;
> > > > +		}
> > > 
> > > did you find this because the kernel crashed or consumed more and more
> > > memory, or is this "only" a finding of reading the source code? If the
> > > former it would be great to point out in the commit log, if the latter,
> > > I wonder if this is a real problem that warrants a stable backport.
> > 
> > A bit of both. One of Collabora's customers had a (scarce) kernel crash
> > in imx-serial and modified multiple things in the driver. The crash is
> > gone, but it's not clear which change fixed it. I could not
> > reproduce the crash so far and I'm currently rebasing and splitting
> > their changes into upstreamable portions with proper patch
> > descriptions. From reading the source this looked like a real issue.
> 
> In which context (kernel version, operating mode (e.g. rs485)) did these
> happen? What does "crash" mean? The kernel did just hang or produced an
> oops? If the latter, can you show it/them?

I pasted the oops, that triggered writing the patches (Linux 4.8, no
rs485, 4MHz baudrate). I think, that the actual issue has already been
fixed upstream between 4.13 and current master.

-- Sebastian

...
[  302.516696] Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0938000
[  302.524394] pgd = 80004000
[  302.527111] [f0938000] *pgd=de81a811, *pte=53fc0653, *ppte=53fc0453
[  302.533451] Internal error: : 1008 [#1] SMP ARM
[  302.537994] Modules linked in: smsc95xx usbnet atmel_mxt_ts
[  302.543651] CPU: 0 PID: 357 Comm: _ACFRead Not tainted 4.8.0 #1
[  302.549578] Hardware name: Freescale i.MX53 (Device Tree Support)
[  302.555679] task: edbf1f00 task.stack: ed5ce000
[  302.560228] PC is at imx_rxint+0x5c/0x228
[  302.564261] LR is at lock_acquired+0x494/0x57c
...
[  303.055953] Backtrace:
[  303.058430] [<804b3b48>] (imx_rxint) from [<804b4df8>] (imx_int+0x2ec/0x404)
[  303.065484]  r10:00000000 r9:00000000 r8:00000030 r7:00000030 r6:00005099 r5:00007240
[  303.073406]  r4:eeafdc10
[  303.075975] [<804b4b0c>] (imx_int) from [<80182f10>] (__handle_irq_event_percpu+0x4c/0x3e8)
[  303.084331]  r10:00000000 r9:810025c4 r8:00000030 r7:00000001 r6:ee81c210 r5:ee81c200
[  303.092252]  r4:edcbe040
[  303.094814] [<80182ec4>] (__handle_irq_event_percpu) from [<801832d8>] (handle_irq_event_percpu+0x2
c/0x68)
[  303.104472]  r10:1240dc08 r9:00000001 r8:ee81e400 r7:00000001 r6:ee81c210 r5:ee81c200
[  303.112393]  r4:ee81c200
[  303.114954] [<801832ac>] (handle_irq_event_percpu) from [<8018335c>] (handle_irq_event+0x48/0x6c)
[  303.123832]  r5:ee81c260 r4:ee81c200
[  303.127454] [<80183314>] (handle_irq_event) from [<80186ba0>] (handle_level_irq+0xb8/0x154)
[  303.135810]  r7:00000001 r6:ee81c210 r5:ee81c260 r4:ee81c200
[  303.141547] [<80186ae8>] (handle_level_irq) from [<80182420>] (generic_handle_irq+0x30/0x44)
[  303.149990]  r7:00000001 r6:00000000 r5:00000030 r4:80ff1e8c
[  303.155728] [<801823f0>] (generic_handle_irq) from [<801827a0>] (__handle_domain_irq+0x60/0xc8)
[  303.164439] [<80182740>] (__handle_domain_irq) from [<80101530>] (tzic_handle_irq+0x74/0x9c)
[  303.172882]  r9:00000001 r8:ed5cfae8 r7:00000001 r6:00000020 r5:8108a50c r4:00000000
[  303.180734] [<801014bc>] (tzic_handle_irq) from [<8094c630>] (__irq_svc+0x70/0x98)
[  303.188311] Exception stack(0xed5cfae8 to 0xed5cfb30)
[  303.193374] fae0:                   00000283 edbf23c0 edbf1f00 600b0113 edbf2458 00000004
[  303.201563] fb00: 00000014 00000004 818762ec edbf23c8 1240dc08 ed5cfb94 eee55200 ed5cfb38
[  303.209748] fb20: 00000282 80177d1c 600b0113 ffffffff
[  303.214805]  r9:ed5ce000 r8:818762ec r7:ed5cfb1c r6:ffffffff r5:600b0113 r4:80177d1c
[  303.222649] [<80177a50>] (lock_release) from [<8094bba0>] (_raw_spin_unlock+0x28/0x34)
[  303.230572]  r10:00000008 r9:ed5a5500 r8:eeafdc10 r7:ef07c000 r6:00000000 r5:edb86f40
[  303.238493]  r4:8101bd68
[  303.241057] [<8094bb78>] (_raw_spin_unlock) from [<8025fdb4>] (remove_vm_area+0x54/0x70)
[  303.249153]  r5:edb86f40 r4:edb86100
[  303.252771] [<8025fd60>] (remove_vm_area) from [<8025fdfc>] (__vunmap+0x2c/0xf8)
[  303.260173]  r5:f0c0b000 r4:f0c0b000
[  303.263791] [<8025fdd0>] (__vunmap) from [<8026000c>] (vunmap+0x50/0x5c)
[  303.270497]  r7:ef07c000 r6:00001000 r5:f0c0b000 r4:00000000
[  303.276240] [<8025ffbc>] (vunmap) from [<805262e4>] (dma_common_free_remap+0x64/0x74)
[  303.284076]  r5:20000008 r4:f0c0b000
[  303.287697] [<80526280>] (dma_common_free_remap) from [<80117404>] (cma_allocator_free+0x7c/0x84)
[  303.296574]  r7:ef07c000 r6:00000000 r5:00001000 r4:effd9f80
[  303.302312] [<80117388>] (cma_allocator_free) from [<80116dcc>] (__arm_dma_free+0xf0/0x13c)
[  303.310668]  r7:ef07c000 r6:f0c0b000 r5:f0c0b000 r4:edb869c0
[  303.316405] [<80116cdc>] (__arm_dma_free) from [<80116e78>] (arm_dma_free+0x2c/0x34)
[  303.324153]  r5:edcc2010 r4:80116e4c
[  303.327779] [<80116e4c>] (arm_dma_free) from [<8047dba8>] (sdma_free_chan_resources+0xc4/0x110)
[  303.336490] [<8047dae4>] (sdma_free_chan_resources) from [<8047a9d8>] (dma_chan_put+0x88/0xbc)
[  303.345107]  r7:600b0113 r6:00000000 r5:edcc07ec r4:edcc07ec
[  303.350845] [<8047a950>] (dma_chan_put) from [<8047aa44>] (dma_release_channel+0x38/0xa8)
[  303.359028]  r5:edcc07ec r4:edcc07ec
[  303.362647] [<8047aa0c>] (dma_release_channel) from [<804b3e1c>] (imx_uart_dma_exit+0x50/0xfc)
[  303.371263]  r5:edcc07ec r4:eeafdc10
[  303.374882] [<804b3dcc>] (imx_uart_dma_exit) from [<804b5028>] (imx_shutdown+0x118/0x20c)
[  303.383065]  r5:00000b01 r4:eeafdc10
[  303.386689] [<804b4f10>] (imx_shutdown) from [<804ae834>] (uart_shutdown+0x120/0x17c)
[  303.394525]  r7:81031774 r6:ee8863a4 r5:eeafdc10 r4:ee886258
[  303.400264] [<804ae714>] (uart_shutdown) from [<804b0624>] (uart_close+0x164/0x254)
[  303.407926]  r9:ed5a5500 r8:ee8863ac r7:ee886310 r6:eeafdc10 r5:edba9800 r4:ee886258
[  303.415768] [<804b04c0>] (uart_close) from [<80492300>] (tty_release+0x104/0x498)
[  303.423256]  r9:ed5a5500 r8:00000000 r7:ee00e000 r6:00000000 r5:eeac2e60 r4:edba9800
[  303.431100] [<804921fc>] (tty_release) from [<80271404>] (__fput+0x98/0x1e8)
[  303.438154]  r10:00000008 r9:eeac2e60 r8:00000000 r7:ee00e000 r6:edebea10 r5:eeac2e60
[  303.446075]  r4:ed5a5500
[  303.448635] [<8027136c>] (__fput) from [<802715c4>] (____fput+0x18/0x1c)
[  303.455342]  r10:ed5cfedc r9:ed496780 r8:edbf1f00 r7:edbf2340 r6:8108b054 r5:00000000
[  303.463263]  r4:edbf2300
[  303.465832] [<802715ac>] (____fput) from [<80146034>] (task_work_run+0xc8/0xf8)
[  303.473165] [<80145f6c>] (task_work_run) from [<801265e0>] (do_exit+0x330/0xb74)
[  303.480567]  r9:edfd07dc r8:00000000 r7:81084dcc r6:810025c4 r5:81083a25 r4:edbf1f00
[  303.488408] [<801262b0>] (do_exit) from [<80128678>] (do_group_exit+0x4c/0xcc)
[  303.495636]  r7:00000009
[  303.498205] [<8012862c>] (do_group_exit) from [<80135450>] (get_signal+0x2a8/0x990)
[  303.505866]  r7:00000009 r6:00418004 r5:ed5cfec8 r4:ed5eeca0
[  303.511616] [<801351a8>] (get_signal) from [<8010c6e4>] (do_signal+0xd8/0x47c)
[  303.518844]  r10:00000000 r9:ed5ce000 r8:00000001 r7:766d443c r6:766d4438 r5:ed5cfec8
[  303.526764]  r4:ed5cffb0
[  303.529326] [<8010c60c>] (do_signal) from [<8010cc78>] (do_work_pending+0xc0/0xd0)
[  303.536901]  r10:00000000 r9:ed5ce000 r8:801086c4 r7:801086c4 r6:ed5cffb0 r5:ed5ce000
[  303.544824]  r4:00000001
[  303.547385] [<8010cbb8>] (do_work_pending) from [<80108548>] (slow_work_pending+0xc/0x20)
[  303.555568]  r7:0000008e r6:747ad088 r5:747ad188 r4:747ad188
[  303.561306] Code: e5943094 e594202c e2833001 e5843094 (e592a000)
[  303.567419] ---[ end trace 741daf1a1655e1be ]---
[  303.572048] Kernel panic - not syncing: Fatal exception in interrupt
[  303.578432] ---[ end Kernel panic - not syncing: Fatal exception in interrupt
...

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

Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown

[Uwe Kleine-König]

On Wed, May 09, 2018 at 12:20:58PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, May 08, 2018 at 08:46:12PM +0200, Uwe Kleine-König wrote:
> > On Tue, May 08, 2018 at 03:40:47PM +0200, Sebastian Reichel wrote:
> > > On Tue, May 08, 2018 at 08:43:51AM +0200, Uwe Kleine-König wrote:
> > > > On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> > > > > This properly unmaps DMA SG on device shutdown.
> > > > > 
> > > > > Reported-by: Nandor Han <nandor.han@ge.com>
> > > > > Suggested-by: Nandor Han <nandor.han@ge.com>
> > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > > > > ---
> > > > >  drivers/tty/serial/imx.c | 12 ++++++++++--
> > > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > > index 3ca767b1162a..6c53e74244ec 100644
> > > > > --- a/drivers/tty/serial/imx.c
> > > > > +++ b/drivers/tty/serial/imx.c
> > > > > @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
> > > > >  	u32 ucr1, ucr2;
> > > > >  
> > > > >  	if (sport->dma_is_enabled) {
> > > > > -		sport->dma_is_rxing = 0;
> > > > > -		sport->dma_is_txing = 0;
> > > > >  		dmaengine_terminate_sync(sport->dma_chan_tx);
> > > > > +		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;
> > > > > +		}
> > > > 
> > > > did you find this because the kernel crashed or consumed more and more
> > > > memory, or is this "only" a finding of reading the source code? If the
> > > > former it would be great to point out in the commit log, if the latter,
> > > > I wonder if this is a real problem that warrants a stable backport.
> > > 
> > > A bit of both. One of Collabora's customers had a (scarce) kernel crash
> > > in imx-serial and modified multiple things in the driver. The crash is
> > > gone, but it's not clear which change fixed it. I could not
> > > reproduce the crash so far and I'm currently rebasing and splitting
> > > their changes into upstreamable portions with proper patch
> > > descriptions. From reading the source this looked like a real issue.
> > 
> > In which context (kernel version, operating mode (e.g. rs485)) did these
> > happen? What does "crash" mean? The kernel did just hang or produced an
> > oops? If the latter, can you show it/them?
> 
> I pasted the oops, that triggered writing the patches (Linux 4.8, no
> rs485, 4MHz baudrate). I think, that the actual issue has already been
> fixed upstream between 4.13 and current master.
> 
> -- Sebastian
> 
> ...
> [  302.516696] Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0938000

This is usually a missing clk. Alternatively RX is disabled and the
RXDATA register is read. Scrolling through

	git log v4.13..linus/master -- drivers/tty/serial/imx.c

I didn't find a candidate for fixing that.

> [  302.524394] pgd = 80004000
> [  302.527111] [f0938000] *pgd=de81a811, *pte=53fc0653, *ppte=53fc0453
> [  302.533451] Internal error: : 1008 [#1] SMP ARM
> [  302.537994] Modules linked in: smsc95xx usbnet atmel_mxt_ts
> [  302.543651] CPU: 0 PID: 357 Comm: _ACFRead Not tainted 4.8.0 #1
> [  302.549578] Hardware name: Freescale i.MX53 (Device Tree Support)
> [  302.555679] task: edbf1f00 task.stack: ed5ce000
> [  302.560228] PC is at imx_rxint+0x5c/0x228
> [  302.564261] LR is at lock_acquired+0x494/0x57c

If you still have this kernel, disabling imx_rxint would be helpful.

Best regards
Uwe

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

Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown

[Sebastian Reichel]

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

Hi,

On Wed, May 09, 2018 at 12:42:56PM +0200, Uwe Kleine-König wrote:
> On Wed, May 09, 2018 at 12:20:58PM +0200, Sebastian Reichel wrote:
> > Hi,
> > 
> > On Tue, May 08, 2018 at 08:46:12PM +0200, Uwe Kleine-König wrote:
> > > On Tue, May 08, 2018 at 03:40:47PM +0200, Sebastian Reichel wrote:
> > > > On Tue, May 08, 2018 at 08:43:51AM +0200, Uwe Kleine-König wrote:
> > > > > On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> > > > > > This properly unmaps DMA SG on device shutdown.
> > > > > > 
> > > > > > Reported-by: Nandor Han <nandor.han@ge.com>
> > > > > > Suggested-by: Nandor Han <nandor.han@ge.com>
> > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > > > > > ---
> > > > > >  drivers/tty/serial/imx.c | 12 ++++++++++--
> > > > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > > > index 3ca767b1162a..6c53e74244ec 100644
> > > > > > --- a/drivers/tty/serial/imx.c
> > > > > > +++ b/drivers/tty/serial/imx.c
> > > > > > @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
> > > > > >  	u32 ucr1, ucr2;
> > > > > >  
> > > > > >  	if (sport->dma_is_enabled) {
> > > > > > -		sport->dma_is_rxing = 0;
> > > > > > -		sport->dma_is_txing = 0;
> > > > > >  		dmaengine_terminate_sync(sport->dma_chan_tx);
> > > > > > +		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;
> > > > > > +		}
> > > > > 
> > > > > did you find this because the kernel crashed or consumed more and more
> > > > > memory, or is this "only" a finding of reading the source code? If the
> > > > > former it would be great to point out in the commit log, if the latter,
> > > > > I wonder if this is a real problem that warrants a stable backport.
> > > > 
> > > > A bit of both. One of Collabora's customers had a (scarce) kernel crash
> > > > in imx-serial and modified multiple things in the driver. The crash is
> > > > gone, but it's not clear which change fixed it. I could not
> > > > reproduce the crash so far and I'm currently rebasing and splitting
> > > > their changes into upstreamable portions with proper patch
> > > > descriptions. From reading the source this looked like a real issue.
> > > 
> > > In which context (kernel version, operating mode (e.g. rs485)) did these
> > > happen? What does "crash" mean? The kernel did just hang or produced an
> > > oops? If the latter, can you show it/them?
> > 
> > I pasted the oops, that triggered writing the patches (Linux 4.8, no
> > rs485, 4MHz baudrate). I think, that the actual issue has already been
> > fixed upstream between 4.13 and current master.
> > 
> > -- Sebastian
> > 
> > ...
> > [  302.516696] Unhandled fault: external abort on non-linefetch (0x1008) at 0xf0938000
> 
> This is usually a missing clk. Alternatively RX is disabled and the
> RXDATA register is read. Scrolling through
> 
> 	git log v4.13..linus/master -- drivers/tty/serial/imx.c
> 
> I didn't find a candidate for fixing that.

It happens while the port is being torn apart. I think the following
patches from you are very good candidates. Especially since the
remaining diff fixing the issue in the customer's old kernel has a
smilar change:

76821e222c18 - serial: imx: ensure that RX irqs are off if RX is off (9 weeks ago) <Uwe Kleine-König>
dedc64e02f5d - serial: imx: Stop to receive in .stop_rx() (9 weeks ago) <Uwe Kleine-König>

-- Sebastian

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