All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/6] serial: imx: various improvements
@ 2017-09-21 16:18 ` Martyn Welch
  0 siblings, 0 replies; 24+ messages in thread
From: Martyn Welch @ 2017-09-21 16:18 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Nandor Han, Romain Perier, linux-serial, linux-arm-kernel,
	linux-kernel, u.kleine-koenig, Fabio Estevam, Martyn Welch

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.


Changes in v3:
- Re-worked commit message of `dma_is_rxing` patch.
- Correct patch order to avoid build failure.
- Added information to CTSC and CTS handling patch commit message

Changes in v2:
- Re-worked assignment of `dma_is_rxing` in patch 1/7
- Removed commit "serial: imx: move log from error to debug type"
- Removed commit "serial: imx: init dma_is_{rx|tx}ing variables"
- Improved commit log for "serial: imx: Simplify DMA disablement" 
  and splitted it in two commits, with one for CTSC and CTS
- Added commit "serial: imx: remove CTSC and CTS handling"
- Fixed typo in commit "unmap sg buffers when DMA channel is released"
- Re-worded commit log for "serial: imx: update the stop rx,tx procedures" and
  removed forward declaration
- Fixed typo in commit "serial: imx: Fix imx_shutdown procedure" and simplified
  locking

Nandor Han (5):
  serial: imx: remove CTSC and CTS handling from imx_disable_dma
  serial: imx: Simplify DMA disablement
  serial: imx: unmap sg buffers when DMA channel is released
  serial: imx: update the stop rx,tx procedures
  serial: imx: Fix imx_shutdown procedure

Romain Perier (1):
  serial: imx: only set dma_is_rxing when DMA starts

 drivers/tty/serial/imx.c | 117 ++++++++++++++++++++++++++---------------------
 1 file changed, 65 insertions(+), 52 deletions(-)

-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [PATCH v3 0/6] serial: imx: various improvements
@ 2017-09-21 16:18 ` Martyn Welch
  0 siblings, 0 replies; 24+ messages in thread
From: Martyn Welch @ 2017-09-21 16:18 UTC (permalink / raw)
  To: linux-arm-kernel

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.


Changes in v3:
- Re-worked commit message of `dma_is_rxing` patch.
- Correct patch order to avoid build failure.
- Added information to CTSC and CTS handling patch commit message

Changes in v2:
- Re-worked assignment of `dma_is_rxing` in patch 1/7
- Removed commit "serial: imx: move log from error to debug type"
- Removed commit "serial: imx: init dma_is_{rx|tx}ing variables"
- Improved commit log for "serial: imx: Simplify DMA disablement" 
  and splitted it in two commits, with one for CTSC and CTS
- Added commit "serial: imx: remove CTSC and CTS handling"
- Fixed typo in commit "unmap sg buffers when DMA channel is released"
- Re-worded commit log for "serial: imx: update the stop rx,tx procedures" and
  removed forward declaration
- Fixed typo in commit "serial: imx: Fix imx_shutdown procedure" and simplified
  locking

Nandor Han (5):
  serial: imx: remove CTSC and CTS handling from imx_disable_dma
  serial: imx: Simplify DMA disablement
  serial: imx: unmap sg buffers when DMA channel is released
  serial: imx: update the stop rx,tx procedures
  serial: imx: Fix imx_shutdown procedure

Romain Perier (1):
  serial: imx: only set dma_is_rxing when DMA starts

 drivers/tty/serial/imx.c | 117 ++++++++++++++++++++++++++---------------------
 1 file changed, 65 insertions(+), 52 deletions(-)

-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [PATCH v3 1/6] serial: imx: remove CTSC and CTS handling from imx_disable_dma
  2017-09-21 16:18 ` Martyn Welch
@ 2017-09-21 16:18   ` Martyn Welch
  -1 siblings, 0 replies; 24+ messages in thread
From: Martyn Welch @ 2017-09-21 16:18 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Nandor Han, Romain Perier, linux-serial, linux-arm-kernel,
	linux-kernel, u.kleine-koenig, Fabio Estevam, Martyn Welch

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

The CTSC and CTS bits affect operation of the CTS/RTS hardware flow
control signal (depending on whether the device is in DCE or DTE mode) and
are not related to DMA. When in RS-232 mode, the driver is using the
automatic CTSC control based on a rxFIFO fill level unless the state of
the CTS signal is explictly set via an ioctl call.

Previous improvements to the imx serial driver have resulted on
imx_disable_dma() only being called on shutdown, by which point the
serial core has already correctly deasserted CTS.

Testing shows that without this handling in imx_disable_dma() the CTS
signal state is set correctly when the device is open and TIOCM_RTS is
set/cleared via the TIOCMGET ioctl. The CTS signal is also correctly
deasserted when the device file is closed.

When in RS-485 mode, the driver uses the CTS signal very differently and
appears to control it via calls to imx_port_rts_active() and
imx_port_rts_inactive().

This configuration of the CTSC and CTS bits are therefore not needed.

Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---
 drivers/tty/serial/imx.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index fe368a4..d90dae3 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1225,11 +1225,6 @@ static void imx_disable_dma(struct imx_port *sport)
 	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_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
 
 	sport->dma_is_enabled = 0;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 1/6] serial: imx: remove CTSC and CTS handling from imx_disable_dma
@ 2017-09-21 16:18   ` Martyn Welch
  0 siblings, 0 replies; 24+ messages in thread
From: Martyn Welch @ 2017-09-21 16:18 UTC (permalink / raw)
  To: linux-arm-kernel

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

The CTSC and CTS bits affect operation of the CTS/RTS hardware flow
control signal (depending on whether the device is in DCE or DTE mode) and
are not related to DMA. When in RS-232 mode, the driver is using the
automatic CTSC control based on a rxFIFO fill level unless the state of
the CTS signal is explictly set via an ioctl call.

Previous improvements to the imx serial driver have resulted on
imx_disable_dma() only being called on shutdown, by which point the
serial core has already correctly deasserted CTS.

Testing shows that without this handling in imx_disable_dma() the CTS
signal state is set correctly when the device is open and TIOCM_RTS is
set/cleared via the TIOCMGET ioctl. The CTS signal is also correctly
deasserted when the device file is closed.

When in RS-485 mode, the driver uses the CTS signal very differently and
appears to control it via calls to imx_port_rts_active() and
imx_port_rts_inactive().

This configuration of the CTSC and CTS bits are therefore not needed.

Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---
 drivers/tty/serial/imx.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index fe368a4..d90dae3 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1225,11 +1225,6 @@ static void imx_disable_dma(struct imx_port *sport)
 	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_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
 
 	sport->dma_is_enabled = 0;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 2/6] serial: imx: only set dma_is_rxing when DMA starts
  2017-09-21 16:18 ` Martyn Welch
@ 2017-09-21 16:18   ` Martyn Welch
  -1 siblings, 0 replies; 24+ messages in thread
From: Martyn Welch @ 2017-09-21 16:18 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Nandor Han, Romain Perier, linux-serial, linux-arm-kernel,
	linux-kernel, u.kleine-koenig, Fabio Estevam, Martyn Welch

From: Romain Perier <romain.perier@collabora.com>

The variable dma_is_rxing is currently set to 1 in imx_disable_rx_int().
This is problematic as:

- whilst imx_disable_rx_int() is currently always called before
  start_rx_dma() this dependency isn't obvious.
- start_rx_dma() does error checking and might exit without
  enabling DMA. Currently this will result in dma_is_rxing suggesting
  that DMA is being used for recieving.

To avoid these issues, move the setting of dma_is_rxing to
start_rx_dma() when appropriate.

Signed-off-by: Romain Perier <romain.perier@collabora.com>
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---
 drivers/tty/serial/imx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index d90dae3..7835279 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -714,8 +714,6 @@ static void imx_disable_rx_int(struct imx_port *sport)
 {
 	unsigned long temp;
 
-	sport->dma_is_rxing = 1;
-
 	/* disable the receiver ready and aging timer interrupts */
 	temp = readl(sport->port.membase + UCR1);
 	temp &= ~(UCR1_RRDYEN);
@@ -1074,6 +1072,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;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 2/6] serial: imx: only set dma_is_rxing when DMA starts
@ 2017-09-21 16:18   ` Martyn Welch
  0 siblings, 0 replies; 24+ messages in thread
From: Martyn Welch @ 2017-09-21 16:18 UTC (permalink / raw)
  To: linux-arm-kernel

From: Romain Perier <romain.perier@collabora.com>

The variable dma_is_rxing is currently set to 1 in imx_disable_rx_int().
This is problematic as:

- whilst imx_disable_rx_int() is currently always called before
  start_rx_dma() this dependency isn't obvious.
- start_rx_dma() does error checking and might exit without
  enabling DMA. Currently this will result in dma_is_rxing suggesting
  that DMA is being used for recieving.

To avoid these issues, move the setting of dma_is_rxing to
start_rx_dma() when appropriate.

Signed-off-by: Romain Perier <romain.perier@collabora.com>
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---
 drivers/tty/serial/imx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index d90dae3..7835279 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -714,8 +714,6 @@ static void imx_disable_rx_int(struct imx_port *sport)
 {
 	unsigned long temp;
 
-	sport->dma_is_rxing = 1;
-
 	/* disable the receiver ready and aging timer interrupts */
 	temp = readl(sport->port.membase + UCR1);
 	temp &= ~(UCR1_RRDYEN);
@@ -1074,6 +1072,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;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 3/6] serial: imx: Simplify DMA disablement
  2017-09-21 16:18 ` Martyn Welch
@ 2017-09-21 16:18   ` Martyn Welch
  -1 siblings, 0 replies; 24+ messages in thread
From: Martyn Welch @ 2017-09-21 16:18 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Nandor Han, Romain Perier, linux-serial, linux-arm-kernel,
	linux-kernel, u.kleine-koenig, Fabio Estevam, Martyn Welch

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.
This is a preparation for the next commit.

Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---
 drivers/tty/serial/imx.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 7835279..2fb3210 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -350,6 +350,24 @@ static void imx_port_rts_auto(struct imx_port *sport, unsigned long *ucr2)
 	*ucr2 |= UCR2_CTSC;
 }
 
+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);
+}
+
 /*
  * interrupts disabled on entry
  */
@@ -1217,13 +1235,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);
-
+	imx_stop_rx_dma(sport);
+	imx_stop_tx_dma(sport);
 	imx_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
 
 	sport->dma_is_enabled = 0;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 3/6] serial: imx: Simplify DMA disablement
@ 2017-09-21 16:18   ` Martyn Welch
  0 siblings, 0 replies; 24+ messages in thread
From: Martyn Welch @ 2017-09-21 16:18 UTC (permalink / raw)
  To: linux-arm-kernel

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.
This is a preparation for the next commit.

Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---
 drivers/tty/serial/imx.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 7835279..2fb3210 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -350,6 +350,24 @@ static void imx_port_rts_auto(struct imx_port *sport, unsigned long *ucr2)
 	*ucr2 |= UCR2_CTSC;
 }
 
+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);
+}
+
 /*
  * interrupts disabled on entry
  */
@@ -1217,13 +1235,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);
-
+	imx_stop_rx_dma(sport);
+	imx_stop_tx_dma(sport);
 	imx_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
 
 	sport->dma_is_enabled = 0;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 4/6] serial: imx: unmap sg buffers when DMA channel is released
  2017-09-21 16:18 ` Martyn Welch
@ 2017-09-21 16:18   ` Martyn Welch
  -1 siblings, 0 replies; 24+ messages in thread
From: Martyn Welch @ 2017-09-21 16:18 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Nandor Han, Romain Perier, linux-serial, linux-arm-kernel,
	linux-kernel, u.kleine-koenig, Fabio Estevam, Martyn Welch

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

This commits unmaps sg buffers when the DMA channel is released. It also
sets to zero `dma_is_rxing` and `dma_is_txing` to state that the
corresponding channels cannot transmit/receive data, as these are
disabled.

Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---
 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 2fb3210..ed02783 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -357,6 +357,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)
@@ -366,6 +372,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;
+	}
 }
 
 /*
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 4/6] serial: imx: unmap sg buffers when DMA channel is released
@ 2017-09-21 16:18   ` Martyn Welch
  0 siblings, 0 replies; 24+ messages in thread
From: Martyn Welch @ 2017-09-21 16:18 UTC (permalink / raw)
  To: linux-arm-kernel

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

This commits unmaps sg buffers when the DMA channel is released. It also
sets to zero `dma_is_rxing` and `dma_is_txing` to state that the
corresponding channels cannot transmit/receive data, as these are
disabled.

Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---
 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 2fb3210..ed02783 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -357,6 +357,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)
@@ -366,6 +372,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;
+	}
 }
 
 /*
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 5/6] serial: imx: update the stop rx,tx procedures
  2017-09-21 16:18 ` Martyn Welch
@ 2017-09-21 16:18   ` Martyn Welch
  -1 siblings, 0 replies; 24+ messages in thread
From: Martyn Welch @ 2017-09-21 16:18 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Nandor Han, Romain Perier, linux-serial, linux-arm-kernel,
	linux-kernel, u.kleine-koenig, Fabio Estevam, Martyn Welch

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.

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

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index ed02783..256b128 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -388,15 +388,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 &&
@@ -423,21 +422,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

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 5/6] serial: imx: update the stop rx,tx procedures
@ 2017-09-21 16:18   ` Martyn Welch
  0 siblings, 0 replies; 24+ messages in thread
From: Martyn Welch @ 2017-09-21 16:18 UTC (permalink / raw)
  To: linux-arm-kernel

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.

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

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index ed02783..256b128 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -388,15 +388,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 &&
@@ -423,21 +422,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

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 6/6] serial: imx: Fix imx_shutdown procedure
  2017-09-21 16:18 ` Martyn Welch
@ 2017-09-21 16:18   ` Martyn Welch
  -1 siblings, 0 replies; 24+ messages in thread
From: Martyn Welch @ 2017-09-21 16:18 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Nandor Han, Romain Perier, linux-serial, linux-arm-kernel,
	linux-kernel, u.kleine-koenig, Fabio Estevam, Martyn Welch

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>
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---
 drivers/tty/serial/imx.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 256b128..393c1c0 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1368,44 +1368,40 @@ 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);
+
+		if (sport->dma_is_inited && sport->dma_is_enabled)
+			imx_disable_dma(sport);
+
 		spin_unlock_irqrestore(&sport->port.lock, flags);
 		imx_uart_dma_exit(sport);
 	}
 
-	mctrl_gpio_disable_ms(sport->gpios);
-
 	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

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [PATCH v3 6/6] serial: imx: Fix imx_shutdown procedure
@ 2017-09-21 16:18   ` Martyn Welch
  0 siblings, 0 replies; 24+ messages in thread
From: Martyn Welch @ 2017-09-21 16:18 UTC (permalink / raw)
  To: linux-arm-kernel

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>
Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---
 drivers/tty/serial/imx.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 256b128..393c1c0 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1368,44 +1368,40 @@ 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);
+
+		if (sport->dma_is_inited && sport->dma_is_enabled)
+			imx_disable_dma(sport);
+
 		spin_unlock_irqrestore(&sport->port.lock, flags);
 		imx_uart_dma_exit(sport);
 	}
 
-	mctrl_gpio_disable_ms(sport->gpios);
-
 	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

^ permalink raw reply related	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/6] serial: imx: only set dma_is_rxing when DMA starts
  2017-09-21 16:18   ` Martyn Welch
@ 2017-09-21 18:15     ` Uwe Kleine-König
  -1 siblings, 0 replies; 24+ messages in thread
From: Uwe Kleine-König @ 2017-09-21 18:15 UTC (permalink / raw)
  To: Martyn Welch
  Cc: Greg Kroah-Hartman, Nandor Han, Romain Perier, linux-serial,
	linux-arm-kernel, linux-kernel, Fabio Estevam

On Thu, Sep 21, 2017 at 05:18:13PM +0100, Martyn Welch wrote:
> From: Romain Perier <romain.perier@collabora.com>
> 
> The variable dma_is_rxing is currently set to 1 in imx_disable_rx_int().
> This is problematic as:
> 
> - whilst imx_disable_rx_int() is currently always called before
>   start_rx_dma() this dependency isn't obvious.
> - start_rx_dma() does error checking and might exit without
>   enabling DMA. Currently this will result in dma_is_rxing suggesting
>   that DMA is being used for recieving.
> 
> To avoid these issues, move the setting of dma_is_rxing to
> start_rx_dma() when appropriate.
> 
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

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

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [PATCH v3 2/6] serial: imx: only set dma_is_rxing when DMA starts
@ 2017-09-21 18:15     ` Uwe Kleine-König
  0 siblings, 0 replies; 24+ messages in thread
From: Uwe Kleine-König @ 2017-09-21 18:15 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Sep 21, 2017 at 05:18:13PM +0100, Martyn Welch wrote:
> From: Romain Perier <romain.perier@collabora.com>
> 
> The variable dma_is_rxing is currently set to 1 in imx_disable_rx_int().
> This is problematic as:
> 
> - whilst imx_disable_rx_int() is currently always called before
>   start_rx_dma() this dependency isn't obvious.
> - start_rx_dma() does error checking and might exit without
>   enabling DMA. Currently this will result in dma_is_rxing suggesting
>   that DMA is being used for recieving.
> 
> To avoid these issues, move the setting of dma_is_rxing to
> start_rx_dma() when appropriate.
> 
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
Reviewed-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

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

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/6] serial: imx: remove CTSC and CTS handling from imx_disable_dma
  2017-09-21 16:18   ` Martyn Welch
@ 2017-09-21 18:20     ` Uwe Kleine-König
  -1 siblings, 0 replies; 24+ messages in thread
From: Uwe Kleine-König @ 2017-09-21 18:20 UTC (permalink / raw)
  To: Martyn Welch
  Cc: Greg Kroah-Hartman, Nandor Han, Romain Perier, linux-serial,
	linux-arm-kernel, linux-kernel, Fabio Estevam

On Thu, Sep 21, 2017 at 05:18:12PM +0100, Martyn Welch wrote:
> From: Nandor Han <nandor.han@ge.com>
> 
> The CTSC and CTS bits affect operation of the CTS/RTS hardware flow
> control signal (depending on whether the device is in DCE or DTE mode) and
> are not related to DMA. When in RS-232 mode, the driver is using the
> automatic CTSC control based on a rxFIFO fill level unless the state of
> the CTS signal is explictly set via an ioctl call.
> 
> Previous improvements to the imx serial driver have resulted on
> imx_disable_dma() only being called on shutdown, by which point the
> serial core has already correctly deasserted CTS.
> 
> Testing shows that without this handling in imx_disable_dma() the CTS
> signal state is set correctly when the device is open and TIOCM_RTS is
> set/cleared via the TIOCMGET ioctl. The CTS signal is also correctly
> deasserted when the device file is closed.

With that block kept CTS set once more to inactive. So the block doesn't
hurt and is "only" superflous, right?

> When in RS-485 mode, the driver uses the CTS signal very differently and
> appears to control it via calls to imx_port_rts_active() and
> imx_port_rts_inactive().
> 
> This configuration of the CTSC and CTS bits are therefore not needed.
> 
> Signed-off-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> ---
>  drivers/tty/serial/imx.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index fe368a4..d90dae3 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1225,11 +1225,6 @@ static void imx_disable_dma(struct imx_port *sport)
>  	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);

The commit log doesn't mention ATEN, I guess that one just doesn't
matter any more at this stage? Would be nice to point out though.

Best regards
Uwe

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

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [PATCH v3 1/6] serial: imx: remove CTSC and CTS handling from imx_disable_dma
@ 2017-09-21 18:20     ` Uwe Kleine-König
  0 siblings, 0 replies; 24+ messages in thread
From: Uwe Kleine-König @ 2017-09-21 18:20 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Sep 21, 2017 at 05:18:12PM +0100, Martyn Welch wrote:
> From: Nandor Han <nandor.han@ge.com>
> 
> The CTSC and CTS bits affect operation of the CTS/RTS hardware flow
> control signal (depending on whether the device is in DCE or DTE mode) and
> are not related to DMA. When in RS-232 mode, the driver is using the
> automatic CTSC control based on a rxFIFO fill level unless the state of
> the CTS signal is explictly set via an ioctl call.
> 
> Previous improvements to the imx serial driver have resulted on
> imx_disable_dma() only being called on shutdown, by which point the
> serial core has already correctly deasserted CTS.
> 
> Testing shows that without this handling in imx_disable_dma() the CTS
> signal state is set correctly when the device is open and TIOCM_RTS is
> set/cleared via the TIOCMGET ioctl. The CTS signal is also correctly
> deasserted when the device file is closed.

With that block kept CTS set once more to inactive. So the block doesn't
hurt and is "only" superflous, right?

> When in RS-485 mode, the driver uses the CTS signal very differently and
> appears to control it via calls to imx_port_rts_active() and
> imx_port_rts_inactive().
> 
> This configuration of the CTSC and CTS bits are therefore not needed.
> 
> Signed-off-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> ---
>  drivers/tty/serial/imx.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index fe368a4..d90dae3 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1225,11 +1225,6 @@ static void imx_disable_dma(struct imx_port *sport)
>  	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);

The commit log doesn't mention ATEN, I guess that one just doesn't
matter any more at this stage? Would be nice to point out though.

Best regards
Uwe

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

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 4/6] serial: imx: unmap sg buffers when DMA channel is released
  2017-09-21 16:18   ` Martyn Welch
@ 2017-09-21 18:25     ` Uwe Kleine-König
  -1 siblings, 0 replies; 24+ messages in thread
From: Uwe Kleine-König @ 2017-09-21 18:25 UTC (permalink / raw)
  To: Martyn Welch
  Cc: Greg Kroah-Hartman, Nandor Han, Romain Perier, linux-serial,
	linux-arm-kernel, linux-kernel, Fabio Estevam

On Thu, Sep 21, 2017 at 05:18:15PM +0100, Martyn Welch wrote:
> From: Nandor Han <nandor.han@ge.com>
> 
> This commits unmaps sg buffers when the DMA channel is released. It also
> sets to zero `dma_is_rxing` and `dma_is_txing` to state that the
> corresponding channels cannot transmit/receive data, as these are
> disabled.

That's a fix for a race, right? The functions being fixed were
introduced just in the patch before and I guess the race was there
already before. It is best if you could resort your series to have fixes
first. This way they can easily be applied during the rc phase while the
rest waits for the merge window.

Best regards
Uwe

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

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [PATCH v3 4/6] serial: imx: unmap sg buffers when DMA channel is released
@ 2017-09-21 18:25     ` Uwe Kleine-König
  0 siblings, 0 replies; 24+ messages in thread
From: Uwe Kleine-König @ 2017-09-21 18:25 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Sep 21, 2017 at 05:18:15PM +0100, Martyn Welch wrote:
> From: Nandor Han <nandor.han@ge.com>
> 
> This commits unmaps sg buffers when the DMA channel is released. It also
> sets to zero `dma_is_rxing` and `dma_is_txing` to state that the
> corresponding channels cannot transmit/receive data, as these are
> disabled.

That's a fix for a race, right? The functions being fixed were
introduced just in the patch before and I guess the race was there
already before. It is best if you could resort your series to have fixes
first. This way they can easily be applied during the rc phase while the
rest waits for the merge window.

Best regards
Uwe

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

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 5/6] serial: imx: update the stop rx,tx procedures
  2017-09-21 16:18   ` Martyn Welch
@ 2017-09-21 18:28     ` Uwe Kleine-König
  -1 siblings, 0 replies; 24+ messages in thread
From: Uwe Kleine-König @ 2017-09-21 18:28 UTC (permalink / raw)
  To: Martyn Welch
  Cc: Greg Kroah-Hartman, Nandor Han, Romain Perier, linux-serial,
	linux-arm-kernel, linux-kernel, Fabio Estevam

On Thu, Sep 21, 2017 at 05:18:16PM +0100, Martyn Welch 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.
> 
> Signed-off-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> ---
>  drivers/tty/serial/imx.c | 36 +++++++++++++++++-------------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index ed02783..256b128 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -388,15 +388,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.
> -	 */

did you understand this comment you're removing here? I admit I don't
but unless it is non-sensical you're doing something wrong.

Best regards
Uwe

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

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [PATCH v3 5/6] serial: imx: update the stop rx,tx procedures
@ 2017-09-21 18:28     ` Uwe Kleine-König
  0 siblings, 0 replies; 24+ messages in thread
From: Uwe Kleine-König @ 2017-09-21 18:28 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Sep 21, 2017 at 05:18:16PM +0100, Martyn Welch 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.
> 
> Signed-off-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> ---
>  drivers/tty/serial/imx.c | 36 +++++++++++++++++-------------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index ed02783..256b128 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -388,15 +388,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.
> -	 */

did you understand this comment you're removing here? I admit I don't
but unless it is non-sensical you're doing something wrong.

Best regards
Uwe

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

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/6] serial: imx: remove CTSC and CTS handling from imx_disable_dma
  2017-09-21 18:20     ` Uwe Kleine-König
@ 2017-09-21 19:12       ` Martyn Welch
  -1 siblings, 0 replies; 24+ messages in thread
From: Martyn Welch @ 2017-09-21 19:12 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Greg Kroah-Hartman, Nandor Han, Romain Perier, linux-serial,
	linux-arm-kernel, linux-kernel, Fabio Estevam

On Thu, Sep 21, 2017 at 08:20:17PM +0200, Uwe Kleine-König wrote:
> On Thu, Sep 21, 2017 at 05:18:12PM +0100, Martyn Welch wrote:
> > From: Nandor Han <nandor.han@ge.com>
> > 
> > The CTSC and CTS bits affect operation of the CTS/RTS hardware flow
> > control signal (depending on whether the device is in DCE or DTE mode) and
> > are not related to DMA. When in RS-232 mode, the driver is using the
> > automatic CTSC control based on a rxFIFO fill level unless the state of
> > the CTS signal is explictly set via an ioctl call.
> > 
> > Previous improvements to the imx serial driver have resulted on
> > imx_disable_dma() only being called on shutdown, by which point the
> > serial core has already correctly deasserted CTS.
> > 
> > Testing shows that without this handling in imx_disable_dma() the CTS
> > signal state is set correctly when the device is open and TIOCM_RTS is
> > set/cleared via the TIOCMGET ioctl. The CTS signal is also correctly
> > deasserted when the device file is closed.
> 
> With that block kept CTS set once more to inactive. So the block doesn't
> hurt and is "only" superflous, right?
> 

That's my understanding, yes.

> > When in RS-485 mode, the driver uses the CTS signal very differently and
> > appears to control it via calls to imx_port_rts_active() and
> > imx_port_rts_inactive().
> > 
> > This configuration of the CTSC and CTS bits are therefore not needed.
> > 
> > Signed-off-by: Nandor Han <nandor.han@ge.com>
> > Signed-off-by: Romain Perier <romain.perier@collabora.com>
> > Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> > ---
> >  drivers/tty/serial/imx.c | 5 -----
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index fe368a4..d90dae3 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1225,11 +1225,6 @@ static void imx_disable_dma(struct imx_port *sport)
> >  	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);
> 
> The commit log doesn't mention ATEN, I guess that one just doesn't
> matter any more at this stage? Would be nice to point out though.
> 

Hmm, going to need to look at this again...

Patch 5 adds clearing ATEN into imx_stop_rx(), which is sensible given
that it's the aging timer on the rxFIFO, but I don't think we should be
removing that from here before it's in imx_stop_rx().

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

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [PATCH v3 1/6] serial: imx: remove CTSC and CTS handling from imx_disable_dma
@ 2017-09-21 19:12       ` Martyn Welch
  0 siblings, 0 replies; 24+ messages in thread
From: Martyn Welch @ 2017-09-21 19:12 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Sep 21, 2017 at 08:20:17PM +0200, Uwe Kleine-K?nig wrote:
> On Thu, Sep 21, 2017 at 05:18:12PM +0100, Martyn Welch wrote:
> > From: Nandor Han <nandor.han@ge.com>
> > 
> > The CTSC and CTS bits affect operation of the CTS/RTS hardware flow
> > control signal (depending on whether the device is in DCE or DTE mode) and
> > are not related to DMA. When in RS-232 mode, the driver is using the
> > automatic CTSC control based on a rxFIFO fill level unless the state of
> > the CTS signal is explictly set via an ioctl call.
> > 
> > Previous improvements to the imx serial driver have resulted on
> > imx_disable_dma() only being called on shutdown, by which point the
> > serial core has already correctly deasserted CTS.
> > 
> > Testing shows that without this handling in imx_disable_dma() the CTS
> > signal state is set correctly when the device is open and TIOCM_RTS is
> > set/cleared via the TIOCMGET ioctl. The CTS signal is also correctly
> > deasserted when the device file is closed.
> 
> With that block kept CTS set once more to inactive. So the block doesn't
> hurt and is "only" superflous, right?
> 

That's my understanding, yes.

> > When in RS-485 mode, the driver uses the CTS signal very differently and
> > appears to control it via calls to imx_port_rts_active() and
> > imx_port_rts_inactive().
> > 
> > This configuration of the CTSC and CTS bits are therefore not needed.
> > 
> > Signed-off-by: Nandor Han <nandor.han@ge.com>
> > Signed-off-by: Romain Perier <romain.perier@collabora.com>
> > Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> > ---
> >  drivers/tty/serial/imx.c | 5 -----
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index fe368a4..d90dae3 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1225,11 +1225,6 @@ static void imx_disable_dma(struct imx_port *sport)
> >  	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);
> 
> The commit log doesn't mention ATEN, I guess that one just doesn't
> matter any more at this stage? Would be nice to point out though.
> 

Hmm, going to need to look at this again...

Patch 5 adds clearing ATEN into imx_stop_rx(), which is sensible given
that it's the aging timer on the rxFIFO, but I don't think we should be
removing that from here before it's in imx_stop_rx().

> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-K?nig            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 24+ messages in thread
end of thread, other threads:[~2017-09-21 19:13 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-21 16:18 [PATCH v3 0/6] serial: imx: various improvements Martyn Welch
2017-09-21 16:18 ` Martyn Welch
2017-09-21 16:18 ` [PATCH v3 1/6] serial: imx: remove CTSC and CTS handling from imx_disable_dma Martyn Welch
2017-09-21 16:18   ` Martyn Welch
2017-09-21 18:20   ` Uwe Kleine-König
2017-09-21 18:20     ` Uwe Kleine-König
2017-09-21 19:12     ` Martyn Welch
2017-09-21 19:12       ` Martyn Welch
2017-09-21 16:18 ` [PATCH v3 2/6] serial: imx: only set dma_is_rxing when DMA starts Martyn Welch
2017-09-21 16:18   ` Martyn Welch
2017-09-21 18:15   ` Uwe Kleine-König
2017-09-21 18:15     ` Uwe Kleine-König
2017-09-21 16:18 ` [PATCH v3 3/6] serial: imx: Simplify DMA disablement Martyn Welch
2017-09-21 16:18   ` Martyn Welch
2017-09-21 16:18 ` [PATCH v3 4/6] serial: imx: unmap sg buffers when DMA channel is released Martyn Welch
2017-09-21 16:18   ` Martyn Welch
2017-09-21 18:25   ` Uwe Kleine-König
2017-09-21 18:25     ` Uwe Kleine-König
2017-09-21 16:18 ` [PATCH v3 5/6] serial: imx: update the stop rx,tx procedures Martyn Welch
2017-09-21 16:18   ` Martyn Welch
2017-09-21 18:28   ` Uwe Kleine-König
2017-09-21 18:28     ` Uwe Kleine-König
2017-09-21 16:18 ` [PATCH v3 6/6] serial: imx: Fix imx_shutdown procedure Martyn Welch
2017-09-21 16:18   ` Martyn Welch

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.