* [PATCH 1/2] serial: 8250_dw: unregister 8250 port if clk_notifier_register() fails
From: Stepan Ionichev @ 2026-05-14 14:37 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: andriy.shevchenko, gregkh, jirislaby, linux-serial, linux-kernel,
stable, sozdayvek
In-Reply-To: <20260514143746.23671-1-sozdayvek@gmail.com>
dw8250_probe() registers the 8250 port via serial8250_register_8250_port()
and then, if the device has a clock, registers a clock notifier. If
clk_notifier_register() fails, probe returns the error but leaves the
8250 port registered. The matching serial8250_unregister_port() lives
in dw8250_remove(), which is not called when probe fails, so the port
slot stays occupied until the device is rebound or the system is
rebooted. The devm-allocated driver data is freed while the port still
references it (via the saved private_data and serial_in/serial_out
callbacks), so any access to that port slot before a rebind is a
use-after-free hazard.
Unregister the port on the clk_notifier_register() error path.
Fixes: cc816969d7b5 ("serial: 8250_dw: Fix common clocks usage race condition")
Cc: stable@vger.kernel.org
Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
---
drivers/tty/serial/8250/8250_dw.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 94beadb40..7dbd79a91 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -850,8 +850,10 @@ static int dw8250_probe(struct platform_device *pdev)
*/
if (data->clk) {
err = clk_notifier_register(data->clk, &data->clk_notifier);
- if (err)
+ if (err) {
+ serial8250_unregister_port(data->data.line);
return dev_err_probe(dev, err, "Failed to set the clock notifier\n");
+ }
queue_work(system_dfl_wq, &data->clk_work);
}
--
2.43.0
^ permalink raw reply related
* [PATCH 0/2] serial: 8250_dw: clock-notifier cleanup
From: Stepan Ionichev @ 2026-05-14 14:37 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: andriy.shevchenko, gregkh, jirislaby, linux-serial, linux-kernel,
stable, sozdayvek
Two-patch series addressing Andy's review of the leak-fix on v1.
Patch 1 keeps the same single-line leak fix as v1, but with:
- the correct "serial: 8250_dw:" prefix (underscore),
- a Fixes: tag pointing at the original clk_notifier introduction,
- Cc: stable@ so the fix gets picked up by stable branches that
still carry the notifier code.
Patch 2 drops the clock-notifier infrastructure entirely from
mainline, as suggested by Andy. The notifier was introduced for the
Baikal-T1 SoC (shared baudclk between UART ports) and has no other
in-tree user; Baikal-T1 support has been removed from the kernel.
If a future platform needs the cross-device baudclk-rate notification
pattern again, it can be reintroduced in a more general form.
Stepan Ionichev (2):
serial: 8250_dw: unregister 8250 port if clk_notifier_register() fails
serial: 8250_dw: remove clock-notifier infrastructure
drivers/tty/serial/8250/8250_dw.c | 79 -------------------------------
1 file changed, 79 deletions(-)
--
2.43.0
^ permalink raw reply
* [PATCH 2/2] amba/serial: amba-pl011: Bring back zx29 UART support
From: Stefan Dösinger @ 2026-05-13 21:34 UTC (permalink / raw)
To: Russell King, Greg Kroah-Hartman, Jiri Slaby
Cc: linux-arm-kernel, linux-kernel, linux-serial,
Stefan Dösinger, Linus Walleij
In-Reply-To: <20260514-zx29uart-v1-0-68470ecc3977@gmail.com>
This is based on code removed in commit 89d4f98ae90d ("ARM: remove zte
zx platform"). I did not bring back the zx29-uart .compatible as the
arm,primecell-periphid does the job.
Reviewed-by: Linus Walleij <linusw@kernel.org>
Signed-off-by: Stefan Dösinger <stefandoesinger@gmail.com>
---
Changes since v4:
Use ZTE's JEDEC ID instead of 0xfe for the DT-Provided AMBA ID.
---
drivers/tty/serial/amba-pl011.c | 42 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 028e37ad8d79..8ed91e1da22b 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -240,6 +240,38 @@ static struct vendor_data vendor_nvidia = {
.get_fifosize = get_fifosize_nvidia,
};
+static const u16 pl011_zte_offsets[REG_ARRAY_SIZE] = {
+ [REG_DR] = ZX_UART011_DR,
+ [REG_FR] = ZX_UART011_FR,
+ [REG_LCRH_RX] = ZX_UART011_LCRH,
+ [REG_LCRH_TX] = ZX_UART011_LCRH,
+ [REG_IBRD] = ZX_UART011_IBRD,
+ [REG_FBRD] = ZX_UART011_FBRD,
+ [REG_CR] = ZX_UART011_CR,
+ [REG_IFLS] = ZX_UART011_IFLS,
+ [REG_IMSC] = ZX_UART011_IMSC,
+ [REG_RIS] = ZX_UART011_RIS,
+ [REG_MIS] = ZX_UART011_MIS,
+ [REG_ICR] = ZX_UART011_ICR,
+ [REG_DMACR] = ZX_UART011_DMACR,
+};
+
+static unsigned int get_fifosize_zte(struct amba_device *dev)
+{
+ return 16;
+}
+
+static struct vendor_data vendor_zte = {
+ .reg_offset = pl011_zte_offsets,
+ .access_32b = true,
+ .ifls = UART011_IFLS_RX4_8 | UART011_IFLS_TX4_8,
+ .fr_busy = ZX_UART01x_FR_BUSY,
+ .fr_dsr = ZX_UART01x_FR_DSR,
+ .fr_cts = ZX_UART01x_FR_CTS,
+ .fr_ri = ZX_UART011_FR_RI,
+ .get_fifosize = get_fifosize_zte,
+};
+
/* Deals with DMA transactions */
struct pl011_dmabuf {
@@ -3180,6 +3212,16 @@ static const struct amba_id pl011_ids[] = {
.mask = 0x000fffff,
.data = &vendor_nvidia,
},
+ {
+ /* This is an invented ID. The actual hardware that contains
+ * these ZTE UARTs (zx29 boards) has no AMBA PIDs stored. ZTE
+ * JEDEC ID (ignoring banks) and the "011" part number as used
+ * by ARM.
+ */
+ .id = 0x0008c011,
+ .mask = 0x000fffff,
+ .data = &vendor_zte,
+ },
{ 0, 0 },
};
--
2.53.0
^ permalink raw reply related
* [PATCH 1/2] ARM: zte: Add support for zx29 low level debug
From: Stefan Dösinger @ 2026-05-13 21:34 UTC (permalink / raw)
To: Russell King, Greg Kroah-Hartman, Jiri Slaby
Cc: linux-arm-kernel, linux-kernel, linux-serial,
Stefan Dösinger, Linus Walleij
In-Reply-To: <20260514-zx29uart-v1-0-68470ecc3977@gmail.com>
This is based on the removed zx29 code. A separate (more complicated)
patch will re-add the register map to the pl011 serial driver.
Reviewed-by: Linus Walleij <linusw@kernel.org>
Signed-off-by: Stefan Dösinger <stefandoesinger@gmail.com>
---
Patch changelog:
v8: Adjust UART01x_FR_BUSY to match the different ZX UART registers
(Sashiko). I am unsure about UART01x_FR_TXFF and my boards do not expose
flow control pins to allow me to test if it works.
I am unsure about the virtual address. It doesn't seem to matter, as
long as it is a valid address. This address is based on the old removed
code. Is there a rule-of-thumb physical to virtual mapping I can use to
give a sensible default value?
---
arch/arm/Kconfig.debug | 12 ++++++++++++
arch/arm/include/debug/pl01x.S | 9 +++++++++
2 files changed, 21 insertions(+)
diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 366f162e147d..98d8a5a60048 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -1331,6 +1331,16 @@ choice
This option selects UART0 on VIA/Wondermedia System-on-a-chip
devices, including VT8500, WM8505, WM8650 and WM8850.
+ config DEBUG_ZTE_ZX
+ bool "Kernel low-level debugging via zx29 UART"
+ select DEBUG_UART_PL01X
+ depends on ARCH_ZTE
+ help
+ Say Y here if you are enabling ZTE zx297520v3 SOC and need
+ debug UART support. This UART is a PL011 with different
+ register addresses. The UART for boot messages on zx29 boards
+ is usually UART1 and is operating at 921600 8N1.
+
config DEBUG_ZYNQ_UART0
bool "Kernel low-level debugging on Xilinx Zynq using UART0"
depends on ARCH_ZYNQ
@@ -1545,6 +1555,7 @@ config DEBUG_UART_8250
config DEBUG_UART_PHYS
hex "Physical base address of debug UART"
+ default 0x01408000 if DEBUG_ZTE_ZX
default 0x01c28000 if DEBUG_SUNXI_UART0
default 0x01c28400 if DEBUG_SUNXI_UART1
default 0x01d0c000 if DEBUG_DAVINCI_DA8XX_UART1
@@ -1701,6 +1712,7 @@ config DEBUG_UART_VIRT
default 0xf31004c0 if DEBUG_MESON_UARTAO
default 0xf4090000 if DEBUG_LPC32XX
default 0xf4200000 if DEBUG_GEMINI
+ default 0xf4708000 if DEBUG_ZTE_ZX
default 0xf6200000 if DEBUG_PXA_UART1
default 0xf7000000 if DEBUG_SUN9I_UART0
default 0xf7000000 if DEBUG_S3C64XX_UART && DEBUG_S3C_UART0
diff --git a/arch/arm/include/debug/pl01x.S b/arch/arm/include/debug/pl01x.S
index c7e02d0628bf..9dcdeed2357d 100644
--- a/arch/arm/include/debug/pl01x.S
+++ b/arch/arm/include/debug/pl01x.S
@@ -8,6 +8,15 @@
*/
#include <linux/amba/serial.h>
+#ifdef CONFIG_DEBUG_ZTE_ZX
+#undef UART01x_DR
+#undef UART01x_FR
+#undef UART01x_FR_BUSY
+#define UART01x_DR 0x04
+#define UART01x_FR 0x14
+#define UART01x_FR_BUSY (1<<8)
+#endif
+
#ifdef CONFIG_DEBUG_UART_PHYS
.macro addruart, rp, rv, tmp
ldr \rp, =CONFIG_DEBUG_UART_PHYS
--
2.53.0
^ permalink raw reply related
* [PATCH 0/2] amba/serial: amba-pl011: Bring back zx29 UART support
From: Stefan Dösinger @ 2026-05-13 21:34 UTC (permalink / raw)
To: Russell King, Greg Kroah-Hartman, Jiri Slaby
Cc: linux-arm-kernel, linux-kernel, linux-serial,
Stefan Dösinger, Linus Walleij
This is based on code removed in commit 89d4f98ae90d ("ARM: remove zte
zx platform"). It was previously discussed and reviewed on the
linux-arm-kernel mailing list as part of my patchset to add initial
support for ZTE's zx297520v3 router board. I am sending the two UART
driver patches to linux-serial. The rest goes through the soc list.
Note that the first patch (LLDEBUG) uses a Kconfig symbol introduced by
my platform patch, which was sent to the soc mailing list. I don't know
what the correct way to handle this is. I can delay/resend this patchset
after the soc changes are merged.
Signed-off-by: Stefan Dösinger <stefandoesinger@gmail.com>
---
Stefan Dösinger (2):
ARM: zte: Add support for zx29 low level debug
amba/serial: amba-pl011: Bring back zx29 UART support
arch/arm/Kconfig.debug | 12 ++++++++++++
arch/arm/include/debug/pl01x.S | 9 +++++++++
drivers/tty/serial/amba-pl011.c | 42 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 63 insertions(+)
---
base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
change-id: 20260513-zx29uart-ef44af731390
Best regards,
--
Stefan Dösinger <stefandoesinger@gmail.com>
^ permalink raw reply
* Re: [PATCH] serial: 8250-dw: unregister 8250 port if clk_notifier_register() fails
From: Andy Shevchenko @ 2026-05-13 20:35 UTC (permalink / raw)
To: Stepan Ionichev
Cc: ilpo.jarvinen, gregkh, jirislaby, linux-serial, linux-kernel
In-Reply-To: <agTgbk24MtHqgkHm@ashevche-desk.local>
On Wed, May 13, 2026 at 11:34:58PM +0300, Andy Shevchenko wrote:
> On Wed, May 13, 2026 at 08:05:03PM +0500, Stepan Ionichev wrote:
Also note, the Subject prefix should be "serial: 8250_dw: ...".
> > dw8250_probe() registers the 8250 port via serial8250_register_8250_port()
> > and then, if the device has a clock, registers a clock notifier:
> >
> > data->data.line = serial8250_register_8250_port(up);
> > if (data->data.line < 0)
> > return data->data.line;
> > ...
> > if (data->clk) {
> > err = clk_notifier_register(data->clk, &data->clk_notifier);
> > if (err)
> > return dev_err_probe(dev, err,
> > "Failed to set the clock notifier\n");
> > ...
> > }
> >
> > If clk_notifier_register() fails, probe returns the error but leaves
> > the 8250 port registered. The matching serial8250_unregister_port()
> > lives in dw8250_remove(), which is not called when probe fails, so
> > the port slot in the serial8250 array stays occupied until the
> > device is rebound or the system is rebooted. The devm-allocated
> > driver data is freed while the port still references it (via the
> > saved private_data and serial_in/serial_out callbacks), so any
> > access to that port slot before a rebind would be a use-after-free.
> >
> > Unregister the port on the clk_notifier_register() error path.
>
> Maybe as a fix for backporting. For the current cycle can you remove that
> notifier and all that crap that was brought by Baikal upstreaming which won't
> ever be finished (as we actually dropped Baikal code in the kernel)?
>
> Or maybe series of two: this one as backport and the other one as fix / remove
> Baikal support entirely from this driver.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] serial: 8250-dw: unregister 8250 port if clk_notifier_register() fails
From: Andy Shevchenko @ 2026-05-13 20:34 UTC (permalink / raw)
To: Stepan Ionichev
Cc: ilpo.jarvinen, gregkh, jirislaby, linux-serial, linux-kernel
In-Reply-To: <20260513150503.11037-1-sozdayvek@gmail.com>
On Wed, May 13, 2026 at 08:05:03PM +0500, Stepan Ionichev wrote:
> dw8250_probe() registers the 8250 port via serial8250_register_8250_port()
> and then, if the device has a clock, registers a clock notifier:
>
> data->data.line = serial8250_register_8250_port(up);
> if (data->data.line < 0)
> return data->data.line;
> ...
> if (data->clk) {
> err = clk_notifier_register(data->clk, &data->clk_notifier);
> if (err)
> return dev_err_probe(dev, err,
> "Failed to set the clock notifier\n");
> ...
> }
>
> If clk_notifier_register() fails, probe returns the error but leaves
> the 8250 port registered. The matching serial8250_unregister_port()
> lives in dw8250_remove(), which is not called when probe fails, so
> the port slot in the serial8250 array stays occupied until the
> device is rebound or the system is rebooted. The devm-allocated
> driver data is freed while the port still references it (via the
> saved private_data and serial_in/serial_out callbacks), so any
> access to that port slot before a rebind would be a use-after-free.
>
> Unregister the port on the clk_notifier_register() error path.
Maybe as a fix for backporting. For the current cycle can you remove that
notifier and all that crap that was brought by Baikal upstreaming which won't
ever be finished (as we actually dropped Baikal code in the kernel)?
Or maybe series of two: this one as backport and the other one as fix / remove
Baikal support entirely from this driver.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH tty v4 2/6] serial: Replace driver usage of UPF_CONS_FLOW
From: Krzysztof Kozlowski @ 2026-05-13 19:50 UTC (permalink / raw)
To: John Ogness, Greg Kroah-Hartman, Jiri Slaby, Alim Akhtar
Cc: Andy Shevchenko, linux-kernel, David S. Miller,
Ilpo Järvinen, Andy Shevchenko, Thomas Fourier, Kees Cook,
linux-serial, linux-arm-kernel, linux-samsung-soc, sparclinux,
Peter Griffin, Tudor Ambarus, André Draszik, Alexey Klimov
In-Reply-To: <87lddvy2dl.fsf@jogness.linutronix.de>
On 07/05/2026 11:50, John Ogness wrote:
> Hi,
>
> (This email is particularly directed at the Samsung folks.)
>
> Responding to Sashiko:
>
> https://sashiko.dev/#/patchset/20260506121606.5805-1-john.ogness%40linutronix.de
>
> On 2026-05-06, Sashiko wrote:
>>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>>> index e27806bf2cf3e..2f94fc798cffb 100644
>>> --- a/drivers/tty/serial/samsung_tty.c
>>> +++ b/drivers/tty/serial/samsung_tty.c
>>> @@ -319,7 +319,7 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port)
>>> ourport->tx_enabled = 0;
>>> ourport->tx_in_progress = 0;
>>>
>>> - if (port->flags & UPF_CONS_FLOW)
>>> + if (uart_cons_flow_enabled(port))
>>> s3c24xx_serial_rx_enable(port);
>>
>> This isn't a new bug introduced by this patch, but does this code
>> result in a recursive spinlock acquisition and deadlock when console
>> flow control is enabled?
>>
>> The serial core framework calls port->ops->stop_tx() with the
>> port->lock spinlock already held. When uart_cons_flow_enabled(port)
>> evaluates to true, s3c24xx_serial_stop_tx() invokes
>> s3c24xx_serial_rx_enable().
>>
>> Looking at s3c24xx_serial_rx_enable(), it unconditionally attempts to
>> acquire the exact same lock again:
>>
>> static void s3c24xx_serial_rx_enable(struct uart_port *port)
>> {
>> struct s3c24xx_uart_port *ourport = to_ourport(port);
>> unsigned long flags;
>> ...
>> uart_port_lock_irqsave(port, &flags);
>> ...
>> }
>>
>> Because spinlocks are not recursive in the kernel, it appears this
>> sequence would cause an immediate deadlock on the same CPU.
>
> Although out of scope for my series, the reported issue is real. Perhaps
> something for Krzysztof or Alim to look into.
>
>>> @@ -493,7 +493,7 @@ static void s3c24xx_serial_start_tx(struct uart_port *port)
>>> struct tty_port *tport = &port->state->port;
>>>
>>> if (!ourport->tx_enabled) {
>>> - if (port->flags & UPF_CONS_FLOW)
>>> + if (uart_cons_flow_enabled(port))
>>> s3c24xx_serial_rx_disable(port);
>>
>> Similarly here, s3c24xx_serial_start_tx() is also called with
>> port->lock held, and s3c24xx_serial_rx_disable() will attempt to
>> acquire port->lock again:
>>
>> static void s3c24xx_serial_rx_disable(struct uart_port *port)
>> {
>> struct s3c24xx_uart_port *ourport = to_ourport(port);
>> unsigned long flags;
>> ...
>> uart_port_lock_irqsave(port, &flags);
>> ...
>> }
>>
>> Could this pre-existing locking issue in the samsung_tty driver be
>> addressed so that the rx enable/disable helpers do not try to take the
>> port lock when it is already held by the caller?
>
> Also legitimate. But out of scope for my series.
Thanks for letting us know. Deadlock did not happen so far, so something
is missing in Sashiko's report. :)
I am just back from vacation, so I won't have time to dig into this, but
I will keep your email in the inbox. Cc-ing also a few Linaro folks
which are using this platform and might be able to help us here.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/2] Add ZTE LRX UART driver
From: Krzysztof Kozlowski @ 2026-05-13 18:13 UTC (permalink / raw)
To: liu.qingtao2, gregkh, jirislaby, robh, krzk+dt, conor+dt, marex,
pjw, palmer, aou, alex, rdunlap, geert+renesas, quic_zongjian,
arturs.artamonovs, robert.marko, hvilleneuve, thierry.bultel.yh,
julianbraha, flavra, prabhakar.mahadev-lad.rj, linux-serial,
linux-kernel, devicetree, linux-riscv, liu.wenhong35, liu.fei16,
dai.hualiang, deng.weixian, jia.yunxiang, he.yilin, bai.lu5,
yang.susheng, shen.lin1, zuo.jiang, hu.shengming, gao.rui, tan.hu
In-Reply-To: <202605130851.64D8prqc083672@mse-fl1.zte.com.cn>
On 13/05/2026 10:46, liu.qingtao2@zte.com.cn wrote:
> From 08610be731b6fc3919d5eebfd2ff9d67f38a094c Mon Sep 17 00:00:00 2001
> From: Wenhong Liu <liu.wenhong35@zte.com.cn>
> Date: Tue, 28 Apr 2026 22:30:31 +0800
> Subject: [PATCH v2 1/2] dt-bindings: serial: Add zte,lrx-uart
>
> Add devicetree binding for ZTE LRX UART controller.
>
> Co-developed-by: Qingtao Liu <liu.qingtao2@zte.com.cn>
> Signed-off-by: Qingtao Liu <liu.qingtao2@zte.com.cn>
> Signed-off-by: Wenhong Liu <liu.wenhong35@zte.com.cn>
This is pretty broken posting. Also, several previous comments stay
valid. What is LRX?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v1 1/2] LRW UART: dt-bindings: Add binding for LRW UART
From: Krzysztof Kozlowski @ 2026-05-13 18:07 UTC (permalink / raw)
To: liu.qingtao2
Cc: gregkh, jirislaby, robh, krzk+dt, conor+dt, marex, pjw, palmer,
aou, alex, rdunlap, geert+renesas, quic_zongjian,
arturs.artamonovs, robert.marko, hvilleneuve, thierry.bultel.yh,
julianbraha, flavra, prabhakar.mahadev-lad.rj, linux-serial,
linux-kernel, devicetree, linux-riscv, liu.wenhong35,
dai.hualiang, deng.weixian, jia.yunxiang, bai.lu5, yang.susheng,
shen.lin1, zuo.jiang, hu.shengming, gao.rui, tan.hu
In-Reply-To: <202605130853.64D8r4bc045011@mse-fl2.zte.com.cn>
On 13/05/2026 10:43, liu.qingtao2@zte.com.cn wrote:
> Thanks for your kindly review. Sorry for the delay.
>
>> On 13/02/2026 10:33, LiuQingtao wrote:
>>> From: Wenhong Liu <liu.wenhong35@zte.com.cn>
>>>
>>> Add documentation for LRW UART devicetree bindings.
>>>
>>> Signed-off-by: Wenhong Liu <liu.wenhong35@zte.com.cn>
>>> Signed-off-by: Qingtao Liu <liu.qingtao2@zte.com.cn>
>>
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching. For bindings, the preferred subjects are
>> explained here:
>> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
>>
>>
>> A nit, subject: drop second/last, redundant "bindings". The
>> "dt-bindings" prefix is already stating that these are bindings.
>> See also:
>> https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>
> Thanks for the notice. I will change the subject to "Add devicetree binding for ZTE LRX UART controller"
> in the v2 patch series.
NAK. Read the comment again. You improved nothing.
>
>
>>> ---
>>> .../bindings/serial/lrw,lrw-uart.yaml | 49 +++++++++++++++++++
>>> .../devicetree/bindings/vendor-prefixes.yaml | 2 +
>>> MAINTAINERS | 7 +++
>>> 3 files changed, 58 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/serial/lrw,lrw-uart.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/lrw,lrw-uart.yaml b/Documentation/devicetree/bindings/> serial/lrw,lrw-uart.yaml
>>> new file mode 100644
>>> index 000000000000..a2d41c278c4f
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/serial/lrw,lrw-uart.yaml
>>> @@ -0,0 +1,49 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/serial/lrw-uart.yaml#
>>
>> Never tested, NAK. There are several other issues here, but I am not
>> going through rest of review if you did not bother to even build test
>> it. Please open any other recent binding and apply same style here
>> (filename, descriptions etc), so you won't be repeating SAME mistakes.
>>
>>
>
> Sorry, i tested on an older version kernel.
> No errors or warnings ever showed up running the command
> "make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/serial/zte,lrx-uart.yaml".
>
Don't work on older kernel. It's not an excuse.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v1 2/2] LRW UART: serial: add driver for the LRW UART
From: Krzysztof Kozlowski @ 2026-05-13 18:05 UTC (permalink / raw)
To: liu.qingtao2
Cc: gregkh, jirislaby, robh, krzk+dt, conor+dt, marex, pjw, palmer,
aou, alex, rdunlap, geert+renesas, quic_zongjian,
arturs.artamonovs, robert.marko, hvilleneuve, thierry.bultel.yh,
julianbraha, flavra, prabhakar.mahadev-lad.rj, linux-serial,
linux-kernel, devicetree, linux-riscv, liu.wenhong35,
dai.hualiang, deng.weixian, jia.yunxiang, bai.lu5, yang.susheng,
shen.lin1, zuo.jiang, hu.shengming, gao.rui, tan.hu
In-Reply-To: <202605130852.64D8qugc044740@mse-fl2.zte.com.cn>
On 13/05/2026 10:43, liu.qingtao2@zte.com.cn wrote:
>> On 13/02/2026 10:33, LiuQingtao wrote:
>>> From: Wenhong Liu <liu.wenhong35@zte.com.cn>
>>>
>>> This commit introduces a serial driver for the LRW UART controller
>> Please do not use "This commit/patch/change", but imperative mood. See
>> longer explanation here:
>> https://elixir.bootlin.com/linux/v6.16/source/Documentation/process/submitting-patches.rst#L94
>
> Thanks. I change subject to "Add support for the ZTE LRX UART controller with the following features"
> in the following patches.
You did not improve much. Read all comments again.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 0/3] serial: 8250: fix BREAK+SysRq dispatch on guard()-locked IRQ handlers
From: Andy Shevchenko @ 2026-05-13 17:51 UTC (permalink / raw)
To: Jacques Nilo
Cc: Greg Kroah-Hartman, Jiri Slaby, Ilpo Järvinen, linux-serial,
linux-kernel, stable
In-Reply-To: <cover.1778675349.git.jnilo@free.fr>
On Wed, May 13, 2026 at 03:30:22PM +0200, Jacques Nilo wrote:
> This series fixes a silent regression where a SysRq character entered as
> BREAK + key on the serial console is consumed by the kernel but never
> dispatched to handle_sysrq(). Same description as v1 [1].
I have read the v1 discussion and v2 makes sense to me and looks good
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH] serial: 8250-dw: unregister 8250 port if clk_notifier_register() fails
From: Stepan Ionichev @ 2026-05-13 15:05 UTC (permalink / raw)
To: ilpo.jarvinen
Cc: andriy.shevchenko, gregkh, jirislaby, linux-serial, linux-kernel,
sozdayvek
dw8250_probe() registers the 8250 port via serial8250_register_8250_port()
and then, if the device has a clock, registers a clock notifier:
data->data.line = serial8250_register_8250_port(up);
if (data->data.line < 0)
return data->data.line;
...
if (data->clk) {
err = clk_notifier_register(data->clk, &data->clk_notifier);
if (err)
return dev_err_probe(dev, err,
"Failed to set the clock notifier\n");
...
}
If clk_notifier_register() fails, probe returns the error but leaves
the 8250 port registered. The matching serial8250_unregister_port()
lives in dw8250_remove(), which is not called when probe fails, so
the port slot in the serial8250 array stays occupied until the
device is rebound or the system is rebooted. The devm-allocated
driver data is freed while the port still references it (via the
saved private_data and serial_in/serial_out callbacks), so any
access to that port slot before a rebind would be a use-after-free.
Unregister the port on the clk_notifier_register() error path.
Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
---
drivers/tty/serial/8250/8250_dw.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 94beadb40..7dbd79a91 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -850,8 +850,10 @@ static int dw8250_probe(struct platform_device *pdev)
*/
if (data->clk) {
err = clk_notifier_register(data->clk, &data->clk_notifier);
- if (err)
+ if (err) {
+ serial8250_unregister_port(data->data.line);
return dev_err_probe(dev, err, "Failed to set the clock notifier\n");
+ }
queue_work(system_dfl_wq, &data->clk_work);
}
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2 1/3] serial: core: introduce guard(uart_port_lock_check_sysrq_irqsave)
From: Ilpo Järvinen @ 2026-05-13 13:35 UTC (permalink / raw)
To: Jacques Nilo
Cc: Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko, linux-serial,
LKML, stable
In-Reply-To: <3849af4bc55d5d2a424fa850844e94d641b2f8a6.1778675349.git.jnilo@free.fr>
[-- Attachment #1: Type: text/plain, Size: 2806 bytes --]
On Wed, 13 May 2026, Jacques Nilo wrote:
> uart_handle_break() and uart_prepare_sysrq_char() (in
> include/linux/serial_core.h) capture a SysRq character into
> port->sysrq_ch while the port lock is held and rely on the unlock
> helper -- uart_unlock_and_check_sysrq_irqrestore() -- to dispatch the
> captured character to handle_sysrq() on scope exit.
>
> The existing guard(uart_port_lock_irqsave) cannot be used by IRQ
> handlers that process RX, because its destructor calls plain
> uart_port_unlock_irqrestore() and silently drops port->sysrq_ch.
>
> Add a dedicated guard(uart_port_lock_check_sysrq_irqsave) variant
> whose destructor is the sysrq-aware unlock helper. The lock side is
> identical to uart_port_lock_irqsave -- only the unlock-time behaviour
> differs. Callers that may capture SysRq characters must use
> guard(uart_port_lock_check_sysrq_irqsave); the existing
> guard(uart_port_lock_irqsave) keeps its current plain-unlock semantics
> for the many callers that do not process RX.
>
> The new macro is placed after the CONFIG_MAGIC_SYSRQ_SERIAL block so
> both definitions of uart_unlock_and_check_sysrq_irqrestore() (sysrq
> enabled and disabled) are visible at expansion time. When
> CONFIG_MAGIC_SYSRQ_SERIAL=n the destructor degenerates to plain
> uart_port_unlock_irqrestore(), so there is no overhead.
>
> No functional change on its own; users are converted in the following
> patches.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jacques Nilo <jnilo@free.fr>
> ---
> include/linux/serial_core.h | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 4f7bbdd90..d1404c97d 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -1286,6 +1286,18 @@ static inline void uart_unlock_and_check_sysrq_irqrestore(struct uart_port *port
> }
> #endif /* CONFIG_MAGIC_SYSRQ_SERIAL */
>
> +/*
> + * Variant of guard(uart_port_lock_irqsave) for IRQ handlers that may capture
> + * a SysRq character via uart_prepare_sysrq_char(). The destructor uses the
> + * sysrq-aware unlock helper so that a captured port->sysrq_ch is dispatched
> + * to handle_sysrq() on scope exit. The plain guard variant silently drops
> + * sysrq_ch and must not be used by callers that process RX.
> + */
> +DEFINE_LOCK_GUARD_1(uart_port_lock_check_sysrq_irqsave, struct uart_port,
> + uart_port_lock_irqsave(_T->lock, &_T->flags),
> + uart_unlock_and_check_sysrq_irqrestore(_T->lock, _T->flags),
> + unsigned long flags);
> +
> /*
> * We do the SysRQ and SAK checking like this...
> */
>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
--
i.
^ permalink raw reply
* [PATCH v2 3/3] serial: 8250_dw: dispatch SysRq character in dw8250_handle_irq()
From: Jacques Nilo @ 2026-05-13 13:30 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Ilpo Järvinen, Andy Shevchenko, linux-serial, linux-kernel,
stable, Jacques Nilo
In-Reply-To: <cover.1778675349.git.jnilo@free.fr>
dw8250_handle_irq() calls serial8250_handle_irq_locked() with the port
lock held via guard(uart_port_lock_irqsave). The guard destructor is
plain uart_port_unlock_irqrestore(), so a SysRq character captured into
port->sysrq_ch by uart_prepare_sysrq_char() is dropped without ever
being dispatched to handle_sysrq().
This is the same regression pattern as in serial8250_handle_irq(),
introduced when 883c5a2bc934 ("serial: 8250_dw: Rework
dw8250_handle_irq() locking and IIR handling") moved the function to
the guard()-based locking scheme without using the sysrq-aware unlock
helper.
Switch to guard(uart_port_lock_check_sysrq_irqsave) so that captured
sysrq_ch is dispatched on scope exit, matching the fix in
serial8250_handle_irq().
Fixes: 883c5a2bc934 ("serial: 8250_dw: Rework dw8250_handle_irq() locking and IIR handling")
Cc: stable@vger.kernel.org
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Jacques Nilo <jnilo@free.fr>
---
drivers/tty/serial/8250/8250_dw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 55e40c10f..9d552b224 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -416,7 +416,7 @@ static int dw8250_handle_irq(struct uart_port *p)
unsigned int quirks = d->pdata->quirks;
unsigned int status;
- guard(uart_port_lock_irqsave)(p);
+ guard(uart_port_lock_check_sysrq_irqsave)(p);
switch (FIELD_GET(DW_UART_IIR_IID, iir)) {
case UART_IIR_NO_INT:
--
2.43.0
^ permalink raw reply related
* [PATCH v2 2/3] serial: 8250: dispatch SysRq character in serial8250_handle_irq()
From: Jacques Nilo @ 2026-05-13 13:30 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Ilpo Järvinen, Andy Shevchenko, linux-serial, linux-kernel,
stable, Jacques Nilo
In-Reply-To: <cover.1778675349.git.jnilo@free.fr>
serial8250_handle_irq() captures a SysRq character into port->sysrq_ch
inside serial8250_handle_irq_locked() via uart_prepare_sysrq_char()
(reached from serial8250_read_char()). Dispatch of that captured
character to handle_sysrq() is expected to happen at port-unlock time,
through uart_unlock_and_check_sysrq[_irqrestore]().
After commit 8324a54f604d ("serial: 8250: Add
serial8250_handle_irq_locked()") the function was reduced to a wrapper
that takes the port lock via guard(uart_port_lock_irqsave) whose
destructor is plain uart_port_unlock_irqrestore(). The sysrq-aware
unlock helper is no longer called, so port->sysrq_ch is captured but
never dispatched: BREAK + SysRq key is consumed silently.
This was the very condition Johan Hovold's 853a9ae29e978 ("serial:
8250: fix handle_irq locking", 2021) introduced
uart_unlock_and_check_sysrq_irqrestore() to address.
Switch to the new guard(uart_port_lock_check_sysrq_irqsave), whose
destructor is the sysrq-aware unlock helper, restoring the pre-split
behaviour. Update the Context: comment on serial8250_handle_irq_locked()
so future HW-specific 8250 wrappers know to use the same guard or the
explicit sysrq-aware unlock.
Verified on RTL8196E with CONFIG_MAGIC_SYSRQ_SERIAL=y: BREAK + 'h' on
the console UART produces the SysRq help dump in dmesg and the brk
counter in /proc/tty/driver/serial increments correctly.
Fixes: 8324a54f604d ("serial: 8250: Add serial8250_handle_irq_locked()")
Cc: stable@vger.kernel.org
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Jacques Nilo <jnilo@free.fr>
---
drivers/tty/serial/8250/8250_port.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index e4e6a53eb..59203bbfb 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1786,7 +1786,10 @@ static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
}
/*
- * Context: port's lock must be held by the caller.
+ * Context: port's lock must be held by the caller. The caller must
+ * release it via guard(uart_port_lock_check_sysrq_irqsave) or
+ * uart_unlock_and_check_sysrq_irqrestore(), which captures SysRq
+ * character on unlock.
*/
void serial8250_handle_irq_locked(struct uart_port *port, unsigned int iir)
{
@@ -1839,7 +1842,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
if (iir & UART_IIR_NO_INT)
return 0;
- guard(uart_port_lock_irqsave)(port);
+ guard(uart_port_lock_check_sysrq_irqsave)(port);
serial8250_handle_irq_locked(port, iir);
return 1;
--
2.43.0
^ permalink raw reply related
* [PATCH v2 1/3] serial: core: introduce guard(uart_port_lock_check_sysrq_irqsave)
From: Jacques Nilo @ 2026-05-13 13:30 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Ilpo Järvinen, Andy Shevchenko, linux-serial, linux-kernel,
stable, Jacques Nilo
In-Reply-To: <cover.1778675349.git.jnilo@free.fr>
uart_handle_break() and uart_prepare_sysrq_char() (in
include/linux/serial_core.h) capture a SysRq character into
port->sysrq_ch while the port lock is held and rely on the unlock
helper -- uart_unlock_and_check_sysrq_irqrestore() -- to dispatch the
captured character to handle_sysrq() on scope exit.
The existing guard(uart_port_lock_irqsave) cannot be used by IRQ
handlers that process RX, because its destructor calls plain
uart_port_unlock_irqrestore() and silently drops port->sysrq_ch.
Add a dedicated guard(uart_port_lock_check_sysrq_irqsave) variant
whose destructor is the sysrq-aware unlock helper. The lock side is
identical to uart_port_lock_irqsave -- only the unlock-time behaviour
differs. Callers that may capture SysRq characters must use
guard(uart_port_lock_check_sysrq_irqsave); the existing
guard(uart_port_lock_irqsave) keeps its current plain-unlock semantics
for the many callers that do not process RX.
The new macro is placed after the CONFIG_MAGIC_SYSRQ_SERIAL block so
both definitions of uart_unlock_and_check_sysrq_irqrestore() (sysrq
enabled and disabled) are visible at expansion time. When
CONFIG_MAGIC_SYSRQ_SERIAL=n the destructor degenerates to plain
uart_port_unlock_irqrestore(), so there is no overhead.
No functional change on its own; users are converted in the following
patches.
Cc: stable@vger.kernel.org
Signed-off-by: Jacques Nilo <jnilo@free.fr>
---
include/linux/serial_core.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 4f7bbdd90..d1404c97d 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -1286,6 +1286,18 @@ static inline void uart_unlock_and_check_sysrq_irqrestore(struct uart_port *port
}
#endif /* CONFIG_MAGIC_SYSRQ_SERIAL */
+/*
+ * Variant of guard(uart_port_lock_irqsave) for IRQ handlers that may capture
+ * a SysRq character via uart_prepare_sysrq_char(). The destructor uses the
+ * sysrq-aware unlock helper so that a captured port->sysrq_ch is dispatched
+ * to handle_sysrq() on scope exit. The plain guard variant silently drops
+ * sysrq_ch and must not be used by callers that process RX.
+ */
+DEFINE_LOCK_GUARD_1(uart_port_lock_check_sysrq_irqsave, struct uart_port,
+ uart_port_lock_irqsave(_T->lock, &_T->flags),
+ uart_unlock_and_check_sysrq_irqrestore(_T->lock, _T->flags),
+ unsigned long flags);
+
/*
* We do the SysRQ and SAK checking like this...
*/
--
2.43.0
^ permalink raw reply related
* [PATCH v2 0/3] serial: 8250: fix BREAK+SysRq dispatch on guard()-locked IRQ handlers
From: Jacques Nilo @ 2026-05-13 13:30 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Ilpo Järvinen, Andy Shevchenko, linux-serial, linux-kernel,
stable, Jacques Nilo
In-Reply-To: <cover.1778592805.git.jnilo@free.fr>
This series fixes a silent regression where a SysRq character entered as
BREAK + key on the serial console is consumed by the kernel but never
dispatched to handle_sysrq(). Same description as v1 [1].
v1 -> v2 (per Ilpo's review [2]):
- Renamed the new lock guard from uart_port_lock_sysrq_irqsave to
uart_port_lock_check_sysrq_irqsave, preserving the "check" semantics
of the destructor's underlying helper
uart_unlock_and_check_sysrq_irqrestore(). Mechanical rename across
patches 2/3 and 3/3; Ilpo's Reviewed-by trailers from v1 carried
forward.
- Patch 1/3 commit message reflowed: the "guard(...)" form is spelled
out, the "lock side is identical" sentence moved up next to the
variant introduction, the now-redundant naming-rationale sentence
removed, and "opt in by using" tightened to "must use".
- Added Cc: stable@vger.kernel.org to patch 1/3 (prerequisite for the
stable backport of 2/3 and 3/3); no Fixes: tag, since 1/3 adds new
API rather than fixing existing code.
- Collapsed the DEFINE_LOCK_GUARD_1 destructor expression to a single
line, which fits within the expected indentation.
No re-test of the BREAK + 'h' path was performed for v2 since the
diff against v1 is purely a textual rename plus the commit-message
reflow above; the v1 RTL8196E validation (BREAK + 'h' on the console
UART producing the SysRq help dump, brk counter incrementing in
/proc/tty/driver/serial) continues to apply unchanged. Built and
booted on tty-next (base 16e95bfb79b5).
[1] https://lore.kernel.org/linux-serial/cover.1778592805.git.jnilo@free.fr/
[2] https://lore.kernel.org/linux-serial/3439217b-90b5-5d21-e777-d238b3ffc1a0@linux.intel.com/
Jacques Nilo (3):
serial: core: introduce guard(uart_port_lock_check_sysrq_irqsave)
serial: 8250: dispatch SysRq character in serial8250_handle_irq()
serial: 8250_dw: dispatch SysRq character in dw8250_handle_irq()
drivers/tty/serial/8250/8250_dw.c | 2 +-
drivers/tty/serial/8250/8250_port.c | 7 +++++--
include/linux/serial_core.h | 12 ++++++++++++
3 files changed, 18 insertions(+), 3 deletions(-)
base-commit: 16e95bfb79b5d9d01dc7651d98caf3c2ace331cd
--
2.43.0
^ permalink raw reply
* Re: [PATCH 1/3] serial: core: introduce guard(uart_port_lock_sysrq_irqsave)
From: Ilpo Järvinen @ 2026-05-13 12:21 UTC (permalink / raw)
To: Jacques Nilo
Cc: Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko, linux-serial,
LKML
In-Reply-To: <20260513121205.45921-1-jnilo@free.fr>
[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]
On Wed, 13 May 2026, Jacques Nilo wrote:
> On Wed, 13 May 2026, Ilpo Järvinen wrote:
>
> > > +DEFINE_LOCK_GUARD_1(uart_port_lock_sysrq_irqsave, struct uart_port,
> >
> > I suppose the "check" in the name is kind of important detail so maybe
> > it shouldn't be dropped from the guard name.
>
> Quick clarification before I respin: do you want this renamed in v2?
>
> I dropped the "check_" segment because the existing
> guard(uart_port_lock_irqsave) doesn't mirror its destructor's name
> either (it expands to uart_port_unlock_irqrestore, not
> uart_port_lock_irqsave_and_unlock_irqrestore), and the longer
> uart_port_lock_check_sysrq_irqsave starts to feel verbose. But I see
> the symmetry argument with uart_unlock_and_check_sysrq_irqrestore() and
> I have no strong attachment to the shorter name.
I meant guard(uart_port_lock_check_sysrq_irqsave)
The point is it does "check" sysrq. Though I admit I'm starting to see now
why you had irqsave earlier placed before sysrq.
It still fits to the expected indentation levels pretty well.
> If you'd like the rename, I'll do it for v2. If you're fine either
> way, I'll keep the current name -- patches 2/3 and 3/3 already have
> your Reviewed-by trailers on the call sites and I'd rather not
> invalidate those over a naming choice.
>
> The other points (commit-message reflow, Cc: stable on 1/3,
> single-line destructor formatting) are unambiguous and will land in
> v2 regardless.
>
> Thanks for the review.
>
> --
> Jacques
>
--
i.
^ permalink raw reply
* Re: [PATCH 1/3] serial: core: introduce guard(uart_port_lock_sysrq_irqsave)
From: Jacques Nilo @ 2026-05-13 12:10 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko, linux-serial,
linux-kernel, Jacques Nilo
In-Reply-To: <3439217b-90b5-5d21-e777-d238b3ffc1a0@linux.intel.com>
On Wed, 13 May 2026, Ilpo Järvinen wrote:
> > +DEFINE_LOCK_GUARD_1(uart_port_lock_sysrq_irqsave, struct uart_port,
>
> I suppose the "check" in the name is kind of important detail so maybe
> it shouldn't be dropped from the guard name.
Quick clarification before I respin: do you want this renamed in v2?
I dropped the "check_" segment because the existing
guard(uart_port_lock_irqsave) doesn't mirror its destructor's name
either (it expands to uart_port_unlock_irqrestore, not
uart_port_lock_irqsave_and_unlock_irqrestore), and the longer
uart_port_lock_check_sysrq_irqsave starts to feel verbose. But I see
the symmetry argument with uart_unlock_and_check_sysrq_irqrestore() and
I have no strong attachment to the shorter name.
If you'd like the rename, I'll do it for v2. If you're fine either
way, I'll keep the current name -- patches 2/3 and 3/3 already have
your Reviewed-by trailers on the call sites and I'd rather not
invalidate those over a naming choice.
The other points (commit-message reflow, Cc: stable on 1/3,
single-line destructor formatting) are unambiguous and will land in
v2 regardless.
Thanks for the review.
--
Jacques
^ permalink raw reply
* Re: [PATCH 1/3] serial: core: introduce guard(uart_port_lock_sysrq_irqsave)
From: Ilpo Järvinen @ 2026-05-13 12:01 UTC (permalink / raw)
To: Jacques Nilo
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, LKML, Johan Hovold
In-Reply-To: <b65b5237542c1c1c694a20e9d8fd0e18434a82b9.1778592805.git.jnilo@free.fr>
On Tue, 12 May 2026, Jacques Nilo wrote:
> uart_handle_break() and uart_prepare_sysrq_char() (in
> include/linux/serial_core.h) capture a SysRq character into
> port->sysrq_ch while the port lock is held and rely on the unlock
> helper -- uart_unlock_and_check_sysrq_irqrestore() -- to dispatch the
> captured character to handle_sysrq() on scope exit.
>
> The existing guard(uart_port_lock_irqsave) cannot be used by IRQ
> handlers that process RX, because its destructor calls plain
> uart_port_unlock_irqrestore() and silently drops port->sysrq_ch.
>
> Add a dedicated guard variant whose destructor is the sysrq-aware
guard(...) (put the full form here already).
> unlock helper. Callers that may capture SysRq characters opt in by
> using guard(uart_port_lock_sysrq_irqsave); the existing
opt in by using -> must use
> uart_port_lock_irqsave guard keeps its current plain-unlock semantics
guard(uart_port_lock_irqsave)
> for the many callers that do not process RX.
>
> The lock side is identical to uart_port_lock_irqsave -- only the
> unlock-time behaviour differs.
Move this to the previous paragraph after the first sentence.
> Naming mirrors
> uart_unlock_and_check_sysrq_irqrestore() (sysrq before irqsave/irqrestore).
I'd remove the sentence about the name, it's kind of obvious.
> The new macro is placed after the CONFIG_MAGIC_SYSRQ_SERIAL block so
> both definitions of uart_unlock_and_check_sysrq_irqrestore() (sysrq
> enabled and disabled) are visible at expansion time. When
> CONFIG_MAGIC_SYSRQ_SERIAL=n the destructor degenerates to plain
> uart_port_unlock_irqrestore(), so there is no overhead.
>
> No functional change on its own; users are converted in the following
> patches.
>
You should add Cc: stable here too as this is prerequisite for the other
two patches (but no Fixes tag).
> Signed-off-by: Jacques Nilo <jnilo@free.fr>
> ---
> include/linux/serial_core.h | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 4f7bbdd90..4fa079dc9 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -1286,6 +1286,19 @@ static inline void uart_unlock_and_check_sysrq_irqrestore(struct uart_port *port
> }
> #endif /* CONFIG_MAGIC_SYSRQ_SERIAL */
>
> +/*
> + * Variant of guard(uart_port_lock_irqsave) for IRQ handlers that may capture
> + * a SysRq character via uart_prepare_sysrq_char(). The destructor uses the
> + * sysrq-aware unlock helper so that a captured port->sysrq_ch is dispatched
> + * to handle_sysrq() on scope exit. The plain guard variant silently drops
> + * sysrq_ch and must not be used by callers that process RX.
> + */
> +DEFINE_LOCK_GUARD_1(uart_port_lock_sysrq_irqsave, struct uart_port,
I suppose the "check" in the name is kind of important detail so maybe it
shouldn't be dropped from the guard name.
> + uart_port_lock_irqsave(_T->lock, &_T->flags),
> + uart_unlock_and_check_sysrq_irqrestore(_T->lock,
> + _T->flags),
This fits to one line.
> + unsigned long flags);
> +
> /*
> * We do the SysRQ and SAK checking like this...
> */
>
--
i.
^ permalink raw reply
* Re: [PATCH 3/3] serial: 8250_dw: dispatch SysRq character in dw8250_handle_irq()
From: Ilpo Järvinen @ 2026-05-13 11:50 UTC (permalink / raw)
To: Jacques Nilo
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, LKML, Johan Hovold,
stable
In-Reply-To: <340a4a76e5dbeb2e49ad4b8d41b9631e09e94bec.1778592805.git.jnilo@free.fr>
[-- Attachment #1: Type: text/plain, Size: 1705 bytes --]
On Tue, 12 May 2026, Jacques Nilo wrote:
> dw8250_handle_irq() calls serial8250_handle_irq_locked() with the port
> lock held via guard(uart_port_lock_irqsave). The guard destructor is
> plain uart_port_unlock_irqrestore(), so a SysRq character captured into
> port->sysrq_ch by uart_prepare_sysrq_char() is dropped without ever
> being dispatched to handle_sysrq().
>
> This is the same regression pattern as in serial8250_handle_irq(),
> introduced when 883c5a2bc934 ("serial: 8250_dw: Rework
> dw8250_handle_irq() locking and IIR handling") moved the function to
> the guard()-based locking scheme without using the sysrq-aware unlock
> helper.
>
> Switch to guard(uart_port_lock_sysrq_irqsave) so that captured
> sysrq_ch is dispatched on scope exit, matching the fix in
> serial8250_handle_irq().
>
> Fixes: 883c5a2bc934 ("serial: 8250_dw: Rework dw8250_handle_irq() locking and IIR handling")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jacques Nilo <jnilo@free.fr>
> ---
> drivers/tty/serial/8250/8250_dw.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 55e40c10f..237543fa7 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -416,7 +416,7 @@ static int dw8250_handle_irq(struct uart_port *p)
> unsigned int quirks = d->pdata->quirks;
> unsigned int status;
>
> - guard(uart_port_lock_irqsave)(p);
> + guard(uart_port_lock_sysrq_irqsave)(p);
>
> switch (FIELD_GET(DW_UART_IIR_IID, iir)) {
> case UART_IIR_NO_INT:
>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
--
i.
^ permalink raw reply
* Re: [PATCH 2/3] serial: 8250: dispatch SysRq character in serial8250_handle_irq()
From: Ilpo Järvinen @ 2026-05-13 11:49 UTC (permalink / raw)
To: Jacques Nilo
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, LKML, Johan Hovold,
stable
In-Reply-To: <5d6d693aa9e92f05412d0f9395872d41e1b8e444.1778592805.git.jnilo@free.fr>
[-- Attachment #1: Type: text/plain, Size: 2858 bytes --]
On Tue, 12 May 2026, Jacques Nilo wrote:
> serial8250_handle_irq() captures a SysRq character into port->sysrq_ch
> inside serial8250_handle_irq_locked() via uart_prepare_sysrq_char()
> (reached from serial8250_read_char()). Dispatch of that captured
> character to handle_sysrq() is expected to happen at port-unlock time,
> through uart_unlock_and_check_sysrq[_irqrestore]().
>
> After commit 8324a54f604d ("serial: 8250: Add
> serial8250_handle_irq_locked()") the function was reduced to a wrapper
> that takes the port lock via guard(uart_port_lock_irqsave) whose
> destructor is plain uart_port_unlock_irqrestore(). The sysrq-aware
> unlock helper is no longer called, so port->sysrq_ch is captured but
> never dispatched: BREAK + SysRq key is consumed silently.
>
> This was the very condition Johan Hovold's 853a9ae29e978 ("serial:
> 8250: fix handle_irq locking", 2021) introduced
> uart_unlock_and_check_sysrq_irqrestore() to address.
>
> Switch to the new guard(uart_port_lock_sysrq_irqsave), whose destructor
> is the sysrq-aware unlock helper, restoring the pre-split behaviour.
> Update the Context: comment on serial8250_handle_irq_locked() so future
> HW-specific 8250 wrappers know to use the same guard or the explicit
> sysrq-aware unlock.
>
> Verified on RTL8196E with CONFIG_MAGIC_SYSRQ_SERIAL=y: BREAK + 'h' on
> the console UART produces the SysRq help dump in dmesg and the brk
> counter in /proc/tty/driver/serial increments correctly.
>
> Fixes: 8324a54f604d ("serial: 8250: Add serial8250_handle_irq_locked()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jacques Nilo <jnilo@free.fr>
> ---
> drivers/tty/serial/8250/8250_port.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index e4e6a53eb..64f3487e8 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1786,7 +1786,10 @@ static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
> }
>
> /*
> - * Context: port's lock must be held by the caller.
> + * Context: port's lock must be held by the caller. The caller must
> + * release it via guard(uart_port_lock_sysrq_irqsave) or
> + * uart_unlock_and_check_sysrq_irqrestore(), which captures SysRq
> + * character on unlock.
> */
> void serial8250_handle_irq_locked(struct uart_port *port, unsigned int iir)
> {
> @@ -1839,7 +1842,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
> if (iir & UART_IIR_NO_INT)
> return 0;
>
> - guard(uart_port_lock_irqsave)(port);
> + guard(uart_port_lock_sysrq_irqsave)(port);
> serial8250_handle_irq_locked(port, iir);
>
> return 1;
>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
--
i.
^ permalink raw reply
* Re: [PATCH v2 2/2] tty: serial: Add LRX UART driver
From: Greg KH @ 2026-05-13 10:11 UTC (permalink / raw)
To: liu.qingtao2
Cc: krzk, jirislaby, robh, krzk+dt, conor+dt, marex, pjw, palmer, aou,
alex, rdunlap, geert+renesas, quic_zongjian, arturs.artamonovs,
robert.marko, hvilleneuve, thierry.bultel.yh, julianbraha, flavra,
prabhakar.mahadev-lad.rj, linux-serial, linux-kernel, devicetree,
linux-riscv, liu.wenhong35, liu.fei16, dai.hualiang, deng.weixian,
jia.yunxiang, he.yilin, bai.lu5, yang.susheng, shen.lin1,
zuo.jiang, hu.shengming, gao.rui, tan.hu
In-Reply-To: <202605130852.64D8qEux084060@mse-fl1.zte.com.cn>
On Wed, May 13, 2026 at 04:46:57PM +0800, liu.qingtao2@zte.com.cn wrote:
> >From 9eba3be2e9b4d5c77956258e3c5db95049c3a895 Mon Sep 17 00:00:00 2001
> From: Wenhong Liu <liu.wenhong35@zte.com.cn>
> Date: Tue, 28 Apr 2026 22:30:40 +0800
> Subject: [PATCH v2 2/2] tty: serial: Add LRX UART driver
>
> Add support for the ZTE LRX UART controller with the following features:
> - Support for FIFO mode (16-byte depth)
> - Baud rate configuration
> - Standard asynchronous communication formats:
> * Data bits: 5, 6, 7, 8, 9 bits
> * Parity: odd, even, fixed, none
> * Stop bits: 1 or 2 bits
> - Hardware flow control (RTS/CTS)
> - Multiple interrupt reporting mechanisms
> - DMA support for improved performance
>
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202602140029.NXkDToZ7-lkp@intel.com/
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202602140108.kLMOYbwS-lkp@intel.com/
Why are these 4 lines here? The kernel test robot found bugs in your
previous versions, so fix that, but no need to list that here, right?
> --- /dev/null
> +++ b/drivers/tty/serial/lrx_uart.c
> @@ -0,0 +1,2822 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Serial Port driver for ZTE LRX
> + *
> + * Copyright (c) 2025, ZTE Corporation. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/ioport.h>
> +#include <linux/init.h>
> +#include <linux/console.h>
> +#include <linux/platform_device.h>
> +#include <linux/sysrq.h>
> +#include <linux/device.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/serial_core.h>
> +#include <linux/serial.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/slab.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/scatterlist.h>
> +#include <linux/delay.h>
> +#include <linux/types.h>
> +#include <linux/of.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/sizes.h>
> +#include <linux/io.h>
> +#include <linux/acpi.h>
> +
> +#define UART_NR 14
Why 14? Why limit yourself at all?
> +
> +#define ISR_PASS_LIMIT 256
> +
> +#define LRX_UART_NAME "lrx-uart"
KBUILD_MODNAME?
> +#define LRX_UART_TTY_PREFIX "ttyLRX"
Why are you using a new name and not the existing ones? Why is this not
just a variant of the existing 8250 serial driver? Surely this is not a
totally new UART that looks nothing like that one, right?
> +/* There is by now at least one vendor with differing details, so handle it */
So a new UART already has conflicting implementations? How can that
happen?
> +/* Deals with DMA transactions */
> +
> +struct lrx_uart_dmabuf {
> + dma_addr_t dma;
> + size_t len;
> + char *buf;
u8?
> +/*
> + * We wrap our port structure around the generic uart_port.
> + */
> +struct lrx_uart_port {
> + struct uart_port port;
> + const u16 *reg_offset;
> + struct clk *clk;
> + const struct vendor_data *vendor;
> + unsigned int im; /* interrupt mask */
> + unsigned int old_status;
> + unsigned int fifosize; /* vendor-specific */
> + unsigned int fixed_baud; /* vendor-set fixed baud rate */
> + char type[16];
> + bool rs485_tx_started;
> + unsigned int rs485_tx_drain_interval; /* usecs */
> +#ifdef CONFIG_DMA_ENGINE
Why would this UART not use the DMA engine? When would that ever be
disabled?
> +/*
> + * Reads up to 256 characters from the FIFO or until it's empty and
This implies that some tool wrote this code. Please always properly
document that and where it copied the information from in order to write
this code.
> +/*
> + * All the DMA operation mode stuff goes inside this ifdef.
> + * This assumes that you have a generic DMA device interface,
> + * no custom DMA interfaces are supported.
> + */
> +#ifdef CONFIG_DMA_ENGINE
> +
> +#define LRX_UART_DMA_BUFFER_SIZE PAGE_SIZE
Why not just use PAGE_SIZE? And how do you know the dma buffer is the
same size always? When using 16k pages, the hardware knows this?
> +/*
> + * Try to refill the TX DMA buffer.
> + * Locking: called with port lock held and IRQs disabled.
> + * Returns:
> + * 1 if we queued up a TX DMA buffer.
> + * 0 if we didn't want to handle this by DMA
Again, fix your tooling :(
> +/*
> + * Flush the transmit buffer.
> + * Locking: called with port lock held and IRQs disabled.
> + */
> +static void lrx_uart_dma_flush_buffer(struct uart_port *port)
> +__releases(&sup->port.lock)
> +__acquires(&sup->port.lock)
Ok, but:
> +{
> + struct lrx_uart_port *sup =
> + container_of(port, struct lrx_uart_port, port);
> +
> + if (!sup->using_tx_dma)
> + return;
> +
> + dmaengine_terminate_async(sup->dmatx.chan);
> +
> + if (sup->dmatx.queued) {
> + dma_unmap_single(sup->dmatx.chan->device->dev, sup->dmatx.dma,
> + sup->dmatx.len, DMA_TO_DEVICE);
> + sup->dmatx.queued = false;
> + sup->dmacr &= ~UARTFCCR_TXDMAE;
> + lrx_uart_write(sup->dmacr, sup, REG_FCCR);
> + }
> +}
I don't see those locks being touched, where did that happen?
> +static irqreturn_t lrx_uart_int(int irq, void *dev_id)
> +{
> + struct lrx_uart_port *sup = dev_id;
> + unsigned int status, pass_counter = ISR_PASS_LIMIT;
> + int handled = 0;
bool?
> +static int lrx_uart_hwinit(struct uart_port *port)
> +{
> + struct lrx_uart_port *sup =
> + container_of(port, struct lrx_uart_port, port);
You do this "container_of()" everywhere, why not write a simple macro
for it to make it more obvious like to_lxr_uart_port()?
And why not use container_of_const()?
> +static int lrx_uart_allocate_irq(struct lrx_uart_port *sup)
> +{
> + lrx_uart_write(sup->im, sup, REG_IMSC);
> +
> + return request_irq(sup->port.irq, lrx_uart_int, IRQF_SHARED, "lrx-uart", sup);
You had a driver name way above, why not use that instead of this string
here?
> + /* Set baud rate */
> + lrx_uart_write(quot & 0x3f, sup, REG_FD);
> + lrx_uart_write(quot >> 6, sup, REG_IND);
> +
> + /*
> + * ----------v----------v----------v----------v-----
> + * NOTE: REG_FRCR MUST BE WRITTEN AFTER REG_FD & REG_IND.
> + * ----------^----------^----------^----------^-----
> + */
Why the odd comment style?
> + lrx_uart_write(frcr, sup, REG_FRCR);
> +
> + lrx_uart_write(fccr, sup, REG_FCCR);
Why the blank line between these?
> +static struct lrx_uart_port *lrx_uart_console_ports[UART_NR];
Why isn't this a dynamic list?
> +/*
> + * While this can be a module, if builtin it's most likely the console
> + * So let's leave module_exit but move module_init to an earlier place
Again, your tooling is broken :(
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v2 0/2] Add ZTE LRX UART driver
From: Greg KH @ 2026-05-13 10:01 UTC (permalink / raw)
To: liu.qingtao2
Cc: krzk, jirislaby, robh, krzk+dt, conor+dt, marex, pjw, palmer, aou,
alex, rdunlap, geert+renesas, quic_zongjian, arturs.artamonovs,
robert.marko, hvilleneuve, thierry.bultel.yh, julianbraha, flavra,
prabhakar.mahadev-lad.rj, linux-serial, linux-kernel, devicetree,
linux-riscv, liu.wenhong35, liu.fei16, dai.hualiang, deng.weixian,
jia.yunxiang, he.yilin, bai.lu5, yang.susheng, shen.lin1,
zuo.jiang, hu.shengming, gao.rui, tan.hu
In-Reply-To: <202605130852.64D8qMgL084414@mse-fl1.zte.com.cn>
On Wed, May 13, 2026 at 04:46:42PM +0800, liu.qingtao2@zte.com.cn wrote:
> >From 9eba3be2e9b4d5c77956258e3c5db95049c3a895 Mon Sep 17 00:00:00 2001
> From: Wenhong Liu
> Date: Mon, 12 May 2026 10:15:55 +0800
> Subject: [PATCH v2 0/2] Add ZTE LRX UART driver
Again, why is this here?
Just use 'git send-email' to send patches out please. Your patches here
are not threaded together at all and our tooling can not pick them up
:(
thanks,
greg k-h
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox