[PATCH] serial: xilinx_uartps: fix oops when disabling RS-485

[PATCH] serial: xilinx_uartps: fix oops when disabling RS-485

[Niko Mauno]

cdns_rs485_config() sets up tx_timer only when RS-485 is enabled, but
cancels it when RS-485 is disabled. A TIOCSRS485 that disables RS-485
on a port that was never in RS-485 mode cancels a zero-filled hrtimer
and oopses:

  Unable to handle kernel NULL pointer dereference at virtual address 0000000c when read
  PC is at hrtimer_active+0x4/0x94
  LR is at hrtimer_cancel+0x10/0x64
  ...
  Call trace:
   hrtimer_active from hrtimer_cancel+0x10/0x64
   hrtimer_cancel from cdns_rs485_config+0x10c/0x11c
   cdns_rs485_config from uart_ioctl+0x7a4/0xc3c
   uart_ioctl from tty_ioctl+0x3ec/0xbcc

Set the timer up once in probe instead. This also stops a TIOCSRS485
that enables RS-485 again from re-initialising the timer while a
transmit delay may have it queued. The callback is switched at run
time by the existing call sites, so its initial value does not matter.

Fixes: fccc9d9233f9 ("tty: serial: uartps: Add rs485 support to uartps driver")
Cc: stable@vger.kernel.org
Signed-off-by: Niko Mauno <niko.mauno@vaisala.com>
---
Tested on a Zynq-7000 board on 6.12.y: without this patch the first
TIOCSRS485 that disables RS-485 after boot oopses as above; with it,
repeated enable/transmit/disable cycles run clean. Also reproduced and
verified on v7.4-rc1 in QEMU.

 drivers/tty/serial/xilinx_uartps.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index e8c4b1fa7658..f93e374f6d0e 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1620,10 +1620,6 @@ static int cdns_rs485_config(struct uart_port *port, struct ktermios *termios,
 		val &= ~CDNS_UART_MODEMCR_FCM;
 		writel(val, port->membase + CDNS_UART_MODEMCR);
 
-		/* Timer setup */
-		hrtimer_setup(&cdns_uart->tx_timer, &cdns_rs485_tx_callback, CLOCK_MONOTONIC,
-			      HRTIMER_MODE_REL);
-
 		/* Disable transmitter and make Rx setup*/
 		cdns_uart_stop_tx(port);
 	} else {
@@ -1782,6 +1778,8 @@ static int cdns_uart_probe(struct platform_device *pdev)
 	port->rs485_config = cdns_rs485_config;
 	port->rs485_supported = cdns_rs485_supported;
 	cdns_uart_data->port = port;
+	hrtimer_setup(&cdns_uart_data->tx_timer, &cdns_rs485_tx_callback,
+		      CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	platform_set_drvdata(pdev, port);
 
 	rc = uart_get_rs485_mode(port);

base-commit: 067c5c24cef23d913b67eaaf71ae67e7169a6620
-- 
2.47.3

Re: [PATCH] serial: xilinx_uartps: fix oops when disabling RS-485

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] serial: xilinx_uartps: uncancelled tx_timer in
  cdns_rs485_config()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] serial: xilinx_uartps: missing port lock in
  cdns_rs485_rx_callback() [1]
--

commit 3210e7db33392b91c77b40821a8026e9d89c30d1
Author: Niko Mauno <niko.mauno@vaisala.com>

serial: xilinx_uartps: fix oops when disabling RS-485

This commit moves the tx_timer hrtimer initialization from
cdns_rs485_config() to cdns_uart_probe() so that disabling RS-485 on a
port that was never in RS-485 mode does not cancel an uninitialized
timer.

> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index e8c4b1fa7658b..f93e374f6d0e5 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -1620,10 +1620,6 @@ static int cdns_rs485_config(struct uart_port *port, struct ktermios *termios,
>  		val &= ~CDNS_UART_MODEMCR_FCM;
>  		writel(val, port->membase + CDNS_UART_MODEMCR);
>  
> -		/* Timer setup */
> -		hrtimer_setup(&cdns_uart->tx_timer, &cdns_rs485_tx_callback, CLOCK_MONOTONIC,
> -			      HRTIMER_MODE_REL);
> -
>  		/* Disable transmitter and make Rx setup*/
>  		cdns_uart_stop_tx(port);
>  	} else {

[Severity: Medium]
Should cdns_rs485_config() also cancel tx_timer when SER_RS485_ENABLED
is set?

When TIOCSRS485 reconfigures a port that already has tx_timer queued for
a transmit delay, cdns_uart_stop_tx() -> cdns_rs485_rx_setup() clears
rs485_tx_started without cancelling tx_timer.  If cdns_uart_start_tx()
runs before the timer expires, it sees !rs485_tx_started and calls
hrtimer_update_function() on the still-queued timer:

kernel/time/hrtimer.c:hrtimer_update_function() {
	...
	if (WARN_ON_ONCE(hrtimer_is_queued(timer)))
		return;
	...
}

This triggers a warning under CONFIG_PROVE_LOCKING and leaves the
existing callback in place.  If tx_timer was queued with
cdns_rs485_rx_callback, re-arming it in cdns_uart_start_tx() will run
cdns_rs485_rx_callback instead of cdns_rs485_tx_callback and stall TX.

> @@ -1782,6 +1778,8 @@ static int cdns_uart_probe(struct platform_device *pdev)
>  	port->rs485_config = cdns_rs485_config;
>  	port->rs485_supported = cdns_rs485_supported;
>  	cdns_uart_data->port = port;
> +	hrtimer_setup(&cdns_uart_data->tx_timer, &cdns_rs485_tx_callback,
> +		      CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>  	platform_set_drvdata(pdev, port);
>  
>  	rc = uart_get_rs485_mode(port);

[Severity: High]
This problem wasn't introduced by this patch, but can
cdns_rs485_rx_callback() race with cdns_uart_set_mctrl() on
CDNS_UART_MODEMCR [1]?

When tx_timer is switched to cdns_rs485_rx_callback() for the after-send
delay, the callback invokes cdns_rs485_rx_setup() ->
cdns_rts_gpio_enable() without holding port->lock:

CPU0                              CPU1
cdns_rs485_rx_callback()          cdns_uart_set_mctrl()
  cdns_rs485_rx_setup()             uart_port_lock_irqsave(port)
    cdns_rts_gpio_enable()
      val = readl(...MODEMCR)
                                    val = readl(...MODEMCR)
                                    val |= CDNS_UART_MODEMCR_DTR
                                    writel(val, ...MODEMCR)
      val &= ~CDNS_UART_MODEMCR_RTS
      writel(val, ...MODEMCR)  <-- overwrites CPU1's update

The unlocked read-modify-write can drop concurrent modem control updates
such as DTR changes or leave RTS in the wrong state on the RS-485 bus.

[1] https://sashiko.dev/#/bug/linux-f32cec5a-9677-4aba-b408-e83f8b26002d

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113145000.17691-1-niko.mauno@vaisala.com?part=1