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

[sashiko-bot@kernel.org]

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