Some Freescale 8250 implementations have the problem that a single long
break results in one irq per character frame time. The code in
fsl8250_handle_irq() that is supposed to handle that uses the BI bit in
lsr_saved_flags to detect such a situation and then skip the second
received character. However it also stores other error bits and so after
a single frame error the character received in the next irq handling is
passed to the upper layer with a frame error, too.
So after a spike on the data line (which is correctly recognized as a
frame error) the following valid character is thrown away, because the
driver reports a frame error for that one, too.
To weaken this problem restrict saving LSR to only the BI bit.
Note however that the handling is still broken:
- lsr_saved_flags is updated using orig_lsr which is the LSR content
for the first received char, but there might be more in the FIFO, so
a character is thrown away that is received later and not necessarily
the one following the break.
- The doubled break might be the 2nd and 3rd char in the FIFO, so the
workaround doesn't catch these, because serial8250_rx_chars() doesn't
handle the workaround.
- lsr_saved_flags might have set UART_LSR_BI at the entry of
fsl8250_handle_irq() which doesn't originate from
fsl8250_handle_irq()'s "up->lsr_saved_flags |= orig_lsr &
UART_LSR_BI;" but from e.g. from serial8250_tx_empty().
- For a long or a short break this isn't about two characters, but more
or only a single one.
Fixes: 9deaa53ac7 ("serial: add irq handler for Freescale 16550 errata.")
Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Link: https://lore.kernel.org/r/20220704085119.55900-1-u.kleine-koenig@pengutronix.de
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The current implementation queues up new control and user packets as needed
and processes this queue down to the ldisc in the same code path.
That means that the upper and the lower layer are hard coupled in the code.
Due to this deadlocks can happen as seen below while transmitting data,
especially during ldisc congestion. Furthermore, the data channels starve
the control channel on high transmission load on the ldisc.
Introduce an additional control channel data queue to prevent timeouts and
link hangups during ldisc congestion. This is being processed before the
user channel data queue in gsm_data_kick(), i.e. with the highest priority.
Put the queue to ldisc data path into a workqueue and trigger it whenever
new data has been put into the transmission queue. Change
gsm_dlci_data_sweep() accordingly to fill up the transmission queue until
TX_THRESH_HI. This solves the locking issue, keeps latency low and provides
good performance on high data load.
Note that now all packets from a DLCI are removed from the internal queue
if the associated DLCI was closed. This ensures that no data is sent by the
introduced write task to an already closed DLCI.
BUG: spinlock recursion on CPU#0, test_v24_loop/124
lock: serial8250_ports+0x3a8/0x7500, .magic: dead4ead, .owner: test_v24_loop/124, .owner_cpu: 0
CPU: 0 PID: 124 Comm: test_v24_loop Tainted: G O 5.18.0-rc2 #3
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
Call Trace:
<IRQ>
dump_stack_lvl+0x34/0x44
do_raw_spin_lock+0x76/0xa0
_raw_spin_lock_irqsave+0x72/0x80
uart_write_room+0x3b/0xc0
gsm_data_kick+0x14b/0x240 [n_gsm]
gsmld_write_wakeup+0x35/0x70 [n_gsm]
tty_wakeup+0x53/0x60
tty_port_default_wakeup+0x1b/0x30
serial8250_tx_chars+0x12f/0x220
serial8250_handle_irq.part.0+0xfe/0x150
serial8250_default_handle_irq+0x48/0x80
serial8250_interrupt+0x56/0xa0
__handle_irq_event_percpu+0x78/0x1f0
handle_irq_event+0x34/0x70
handle_fasteoi_irq+0x90/0x1e0
__common_interrupt+0x69/0x100
common_interrupt+0x48/0xc0
asm_common_interrupt+0x1e/0x40
RIP: 0010:__do_softirq+0x83/0x34e
Code: 2a 0a ff 0f b7 ed c7 44 24 10 0a 00 00 00 48 c7 c7 51 2a 64 82 e8 2d
e2 d5 ff 65 66 c7 05 83 af 1e 7e 00 00 fb b8 ff ff ff ff <49> c7 c2 40 61
80 82 0f bc c5 41 89 c4 41 83 c4 01 0f 84 e6 00 00
RSP: 0018:ffffc90000003f98 EFLAGS: 00000286
RAX: 00000000ffffffff RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff82642a51 RDI: ffffffff825bb5e7
RBP: 0000000000000200 R08: 00000008de3271a8 R09: 0000000000000000
R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000030 R14: 0000000000000000 R15: 0000000000000000
? __do_softirq+0x73/0x34e
irq_exit_rcu+0xb5/0x100
common_interrupt+0xa4/0xc0
</IRQ>
<TASK>
asm_common_interrupt+0x1e/0x40
RIP: 0010:_raw_spin_unlock_irqrestore+0x2e/0x50
Code: 00 55 48 89 fd 48 83 c7 18 53 48 89 f3 48 8b 74 24 10 e8 85 28 36 ff
48 89 ef e8 cd 58 36 ff 80 e7 02 74 01 fb bf 01 00 00 00 <e8> 3d 97 33 ff
65 8b 05 96 23 2b 7e 85 c0 74 03 5b 5d c3 0f 1f 44
RSP: 0018:ffffc9000020fd08 EFLAGS: 00000202
RAX: 0000000000000000 RBX: 0000000000000246 RCX: 0000000000000000
RDX: 0000000000000004 RSI: ffffffff8257fd74 RDI: 0000000000000001
RBP: ffff8880057de3a0 R08: 00000008de233000 R09: 0000000000000000
R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000100 R14: 0000000000000202 R15: ffff8880057df0b8
? _raw_spin_unlock_irqrestore+0x23/0x50
gsmtty_write+0x65/0x80 [n_gsm]
n_tty_write+0x33f/0x530
? swake_up_all+0xe0/0xe0
file_tty_write.constprop.0+0x1b1/0x320
? n_tty_flush_buffer+0xb0/0xb0
new_sync_write+0x10c/0x190
vfs_write+0x282/0x310
ksys_write+0x68/0xe0
do_syscall_64+0x3b/0x90
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f3e5e35c15c
Code: 8b 7c 24 08 89 c5 e8 c5 ff ff ff 89 ef 89 44 24 08 e8 58 bc 02 00 8b
44 24 08 48 83 c4 10 5d c3 48 63 ff b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff
ff 76 10 48 8b 15 fd fc 05 00 f7 d8 64 89 02 48 83
RSP: 002b:00007ffcee77cd18 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00007ffcee77cd70 RCX: 00007f3e5e35c15c
RDX: 0000000000000100 RSI: 00007ffcee77cd90 RDI: 0000000000000003
RBP: 0000000000000100 R08: 0000000000000000 R09: 7efefefefefefeff
R10: 00007f3e5e3bddeb R11: 0000000000000246 R12: 00007ffcee77ce8f
R13: 0000000000000001 R14: 000056214404e010 R15: 00007ffcee77cd90
</TASK>
Fixes: e1eaea46bb ("tty: n_gsm line discipline")
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
Link: https://lore.kernel.org/r/20220701122332.2039-1-daniel.starke@siemens.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The function may be used by the user directly and also by the n_gsm
internal functions. They can lead into a race condition which results in
interleaved frames if both are writing at the same time. The receiving side
is not able to decode those interleaved frames correctly.
Add a lock around the low side tty write to avoid race conditions and frame
interleaving between user originated writes and n_gsm writes.
Fixes: e1eaea46bb ("tty: n_gsm line discipline")
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
Link: https://lore.kernel.org/r/20220701061652.39604-9-daniel.starke@siemens.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
In the current implementation control packets are re-transmitted even if
the control channel closed down during T2. This is wrong.
Check whether the control channel is open before re-transmitting any
packets. Note that control channel open/close is handled by T1 and not T2
and remains unaffected by this.
Fixes: e1eaea46bb ("tty: n_gsm line discipline")
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
Link: https://lore.kernel.org/r/20220701061652.39604-7-daniel.starke@siemens.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
the newer 27.010 here. Chapter 5.4.6.3.6 states that FCoff stops the
transmission on all channels except the control channel. This is already
implemented in gsm_data_kick(). However, chapter 5.4.8.1 explains that this
shall result in the same behavior as software flow control on the ldisc in
advanced option mode. That means only flow control frames shall be sent
during flow off. The current implementation does not consider this case.
Change gsm_data_kick() to send only flow control frames if constipated to
abide the standard. gsm_read_ea_val() and gsm_is_flow_ctrl_msg() are
introduced as helper functions for this.
It is planned to use gsm_read_ea_val() in later code cleanups for other
functions, too.
Fixes: c01af4fec2 ("n_gsm : Flow control handling in Mux driver")
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
Link: https://lore.kernel.org/r/20220701061652.39604-5-daniel.starke@siemens.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The current implementation does not handle the situation that no data is in
the internal queue and needs to be sent out while the user tty fifo is
full.
Add a timer that moves more data from user tty down to the internal queue
which is then serialized on the ldisc. This timer is triggered if no data
was moved from a user tty to the internal queue within 10 * T1.
Fixes: e1eaea46bb ("tty: n_gsm line discipline")
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
Link: https://lore.kernel.org/r/20220701061652.39604-4-daniel.starke@siemens.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
1) The function drains the fifo for the given user tty/DLCI without
considering 'TX_THRESH_HI' and different to gsm_dlci_data_output_framed(),
which moves only one packet from the user side to the internal transmission
queue. We can only handle one packet at a time here if we want to allow
DLCI priority handling in gsm_dlci_data_sweep() to avoid link starvation.
2) Furthermore, the additional header octet from convergence layer type 2
is not counted against MTU. It is part of the UI/UIH frame message which
needs to be limited to MTU. Hence, it is wrong not to consider this octet.
3) Finally, the waiting user tty is not informed about freed space in its
send queue.
Take at most one packet worth of data out of the DLCI fifo to fix 1).
Limit the max user data size per packet to MTU - 1 in case of convergence
layer type 2 to leave space for the control signal octet which is added in
the later part of the function. This fixes 2).
Add tty_port_tty_wakeup() to wake up the user tty if new write space has
been made available to fix 3).
Fixes: 268e526b93 ("tty/n_gsm: avoid fifo overflow in gsm_dlci_data_output")
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
Link: https://lore.kernel.org/r/20220701061652.39604-3-daniel.starke@siemens.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
The current implementation registers/deregisters the user ttys at mux
attach/detach. That means that the user devices are available before any
control channel is open. However, user channel initialization requires an
open control channel. Furthermore, the user is not informed if the mux
restarts due to configuration changes.
Put the registration/deregistration procedure into separate function to
improve readability.
Move registration to mux activation and deregistration to mux cleanup to
keep the user devices only open as long as a control channel exists. The
user will be informed via the device driver if the mux was reconfigured in
a way that required a mux re-activation.
This makes it necessary to add T2 initialization to gsmld_open() for the
ldisc open code path (not the reconfiguration code path) to avoid deletion
of an uninitialized T2 at mux cleanup.
Fixes: d50f6dcaf2 ("tty: n_gsm: expose gsmtty device nodes at ldisc open time")
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
Link: https://lore.kernel.org/r/20220701061652.39604-2-daniel.starke@siemens.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
After setting up the control channel on both sides the responder side may
want to open a virtual tty to listen on until the initiator starts an
application on a user channel. The current implementation allows the
open() but no other operation, like termios. These fail with EINVAL.
The responder sided application has no means to detect an open by the
initiator sided application this way. And the initiator sided applications
usually expect the responder sided application to listen on the user
channel upon open.
Set the user channel into half-open state on responder side once a user
application opens the virtual tty to allow IO operations on it.
Furthermore, keep the user channel constipated until the initiator side
opens it to give the responder sided application the chance to detect the
new connection and to avoid data loss if the responder sided application
starts sending before the user channel is open.
Fixes: e1eaea46bb ("tty: n_gsm line discipline")
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
Link: https://lore.kernel.org/r/20220701061652.39604-1-daniel.starke@siemens.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
A memory overlapping copy occurs when deleting a long line. This memory
overlapping copy can cause data corruption when scr_memcpyw is optimized
to memcpy because memcpy does not ensure its behavior if the destination
buffer overlaps with the source buffer. The line buffer is not always
broken, because the memcpy utilizes the hardware acceleration, whose
result is not deterministic.
Fix this problem by using replacing the scr_memcpyw with scr_memmovew.
Fixes: 81732c3b2f ("tty vt: Fix line garbage in virtual console on command line edition")
Cc: stable <stable@kernel.org>
Signed-off-by: Yangxi Xiang <xyangxi5@gmail.com>
Link: https://lore.kernel.org/r/20220628093322.5688-1-xyangxi5@gmail.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Functions tty_termios_encode_baud_rate() and uart_update_timeout() should
be called with the baudrate value which was set to hardware. Linux then
report exact values via ioctl(TCGETS2) to userspace.
Change mvebu_uart_baud_rate_set() function to return baudrate value which
was set to hardware and propagate this value to above mentioned functions.
With this change userspace would see precise value in termios c_ospeed
field.
Fixes: 68a0db1d7d ("serial: mvebu-uart: add function to change baudrate")
Cc: stable <stable@kernel.org>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Pali Rohár <pali@kernel.org>
Link: https://lore.kernel.org/r/20220628100922.10717-1-pali@kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
When console is enabled, univ8250_console_setup() calls
serial8250_console_setup() before .dev is set to uart_port. Therefore,
it will not call pm_runtime_get_sync(). Later, when the actual driver
is going to take over univ8250_console_exit() is called. As .dev is
already set, serial8250_console_exit() makes pm_runtime_put_sync() call
with usage count being zero triggering PM usage count warning
(extra debug for univ8250_console_setup(), univ8250_console_exit(), and
serial8250_register_ports()):
[ 0.068987] univ8250_console_setup ttyS0 nodev
[ 0.499670] printk: console [ttyS0] enabled
[ 0.717955] printk: console [ttyS0] printing thread started
[ 1.960163] serial8250_register_ports assigned dev for ttyS0
[ 1.976830] printk: console [ttyS0] disabled
[ 1.976888] printk: console [ttyS0] printing thread stopped
[ 1.977073] univ8250_console_exit ttyS0 usage:0
[ 1.977075] serial8250 serial8250: Runtime PM usage count underflow!
[ 1.977429] dw-apb-uart.6: ttyS0 at MMIO 0x4010006000 (irq = 33, base_baud = 115200) is a 16550A
[ 1.977812] univ8250_console_setup ttyS0 usage:2
[ 1.978167] printk: console [ttyS0] printing thread started
[ 1.978203] printk: console [ttyS0] enabled
To fix the issue, call pm_runtime_get_sync() in
serial8250_register_ports() as soon as .dev is set for an uart_port
if it has console enabled.
This problem became apparent only recently because 82586a7215 ("PM:
runtime: Avoid device usage count underflows") added the warning
printout. I confirmed this problem also occurs with v5.18 (w/o the
warning printout, obviously).
Fixes: bedb404e91 ("serial: 8250_port: Don't use power management for kernel console")
Cc: stable <stable@kernel.org>
Tested-by: Tony Lindgren <tony@atomide.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Link: https://lore.kernel.org/r/b4f428e9-491f-daf2-2232-819928dc276e@linux.intel.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
If port->mapbase = NULL in serial8250_request_std_resource() , it need
return a error code instead of 0. If uart_set_info() fail to request new
regions by serial8250_request_std_resource() but the return value of
serial8250_request_std_resource() is 0, The system incorrectly considers
that the resource application is successful and does not attempt to
restore the old setting. A null pointer reference is triggered when the
port resource is later invoked.
Signed-off-by: Yi Yang <yiyang13@huawei.com>
Cc: stable <stable@kernel.org>
Link: https://lore.kernel.org/r/20220628083515.64138-1-yiyang13@huawei.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
If the earlycon parameter is given twice, the kernel will spit out a
WARN() in register_console() because it was already registered. The
non-dt variant setup_earlycon() already handles that gracefully. The dt
variant of_setup_earlycon() doesn't. Add the check there and add the
-EALREADY handling in early_init_dt_scan_chosen_stdout().
FWIW, this doesn't happen if CONFIG_ACPI_SPCR_TABLE is set. In that case
the registration is delayed until after earlycon parameter(s) are
parsed.
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Michael Walle <michael@walle.cc>
Link: https://lore.kernel.org/r/20220628120705.200617-1-michael@walle.cc
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Regarding Exynos Auto v9 SoC, it supports uarts up to 12. However, the
maximum number of the ports has been derived from
CONFIG_SERIAL_SAMSUNG_UARTS and tightly coupled with the config for
previous Samsung SoCs such as s3c24xx and s3c64xx. To overcome this
limitation, this changes the usage of the definition to UART_NR which is
widely used from other serial drivers. This also defines the value to 12
only for ARM64 SoCs to not affect the change to previous arm32 SoCs.
Instead of enumerating all the ports as predefined arrays, this
introduces s3c24xx_serial_init_port_default that is initializing the
structure as the default value.
Reviewed-by: Hector Martin <marcan@marcan.st>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Chanho Park <chanho61.park@samsung.com>
Link: https://lore.kernel.org/r/20220629005538.60132-1-chanho61.park@samsung.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Commit e8ffbb71f7 ("serial: 8250: use THRE & __stop_tx also with
DMA") changed __dma_tx_complete() to enable THRI that is cleared in
__stop_tx() once THRE is asserted as UART runs out bits to transmit. It
is possible, however, that more data arrives in between in which case
serial8250_tx_dma() resumes Tx. THRI is not supposed to be on during
DMA Tx because DMA is based on completion handler, therefore THRI must
be cleared unconditionally in serial8250_tx_dma().
When Tx is about to start, another race window exists with
serial8250_handle_irq() leading to a call into __stop_tx() while the
Tx has already been resumed:
__tx_complete():
-> spin_lock(port->lock)
-> dma->tx_running = 0
-> serial8250_set_THRI()
-> spin_unlock(port->lock)
uart_start():
serial8250_handle_irq():
-> spin_lock(port->lock)
-> serial8250_tx_dma():
-> dma->tx_running = 1
-> spin_unlock(port->lock)
-> spin_lock(port->lock)
-> __stop_tx()
Close this race by checking !dma->tx_running before calling into
__stop_tx().
Fixes: e8ffbb71f7 ("serial: 8250: use THRE & __stop_tx also with DMA")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Link: https://lore.kernel.org/r/20220615090651.15340-2-ilpo.jarvinen@linux.intel.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Add support for RS-485 multipoint addressing using 9th bit [*]. The
addressing mode is configured through ->rs485_config().
ADDRB in termios indicates 9th bit addressing mode is enabled. In this
mode, 9th bit is used to indicate an address (byte) within the
communication line. ADDRB can only be enabled/disabled through
->rs485_config() that is also responsible for setting the destination and
receiver (filter) addresses.
Add traps to detect unwanted changes to struct serial_rs485 layout using
static_assert().
[*] Technically, RS485 is just an electronic spec and does not itself
specify the 9th bit addressing mode but 9th bit seems at least
"semi-standard" way to do addressing with RS485.
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Link: https://lore.kernel.org/r/20220624204210.11112-6-ilpo.jarvinen@linux.intel.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Use 32-bit reads in order to not lose higher bits of DW UART regs. This
change does not fix any known issue as the high bits are not used for
anything related to 8250 driver (dw8250_readl_ext and dw8250_writel_ext
used within the dwlib are already doing
readl/writel/ioread32be/iowrite32be anyway).
This change is necessary to enables 9th bit address mode. DW UART
reports address frames with BIT(8) of LSR.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Link: https://lore.kernel.org/r/20220624204210.11112-4-ilpo.jarvinen@linux.intel.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Since commit 31f6bd7fad ("serial: Store character timing information
to uart_port"), per frame timing information is available on uart_port.
Uart port's timeout can be derived from frame_time by multiplying with
fifosize.
Most callers of uart_poll_timeout are not made under port's lock. To be
on the safe side, make sure frame_time is only accessed once. As
fifo_size is effectively a constant, it shouldn't cause any issues.
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Link: https://lore.kernel.org/r/20220613113905.22962-1-ilpo.jarvinen@linux.intel.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
