Re: [PATCH RESEND v2] serial: max310x: drive RTS in software when hardware delays are too short

[Tapio Reijonen]

> Sashiko AI review found 8 potential issue(s) to consider:
> - [Critical] Unprivileged Denial of Service (Hung Task Panic) via
>   unbounded uninterruptible sleep.
> - [High] Use-After-Free due to timer resurrection during teardown.
> - [High] Illegal drop of port->lock in UART callbacks, breaking core
>   serialization.
> - [High] RS485 data loss via software state machine bypass.
> - [High] Software RTS state and TX-break bits are wiped out by
>   absolute LCR write.
> - [High] Broken RS485 break signaling ignores transceiver polarity.
> - [High] Resource leak and Use-After-Free of timers/workqueues on
>   probe failure.
> - [Medium] Spurious before-send delays corrupt the RS485 timing
>   envelope.

Two of these are real. Please do not apply this version; a v3 follows.

5. Correct, and this patch is what creates it. MAX310X_LCR_RTS_BIT was
defined but never written before; the patch makes it the software RS485
RTS. That leaves two ways to clobber an envelope in flight:

  - max310x_set_termios() writes LCR absolutely, and the core calls
    ->set_termios() without port->lock (uart_change_line_settings()
    takes the lock only after the call), so a concurrent tcsetattr()
    clears RTS mid-character.
  - max310x_set_rts_ctl_params() drives RTS to the idle level
    unconditionally, and it also runs from rs_work, which
    max310x_rs485_config() schedules on every TIOCSRS485.

Either way the rest of the burst is shifted out with the transceiver
disabled. v3 will leave RTS alone while tx_state is not MAX310X_TX_OFF.
The TX-break half of your point is real too, but it predates this patch.

6. Correct. max310x_break_ctl() passes break_state straight to
max310x_rts_ctl(), while every other RTS write in the driver selects on
SER_RS485_RTS_ON_SEND / SER_RS485_RTS_AFTER_SEND, so an
rs485-rts-active-low port drives the wrong level for the break duration.
v3 will use the same polarity as the rest. On the hardware path
IRDA.RTSINVERT may invert the pin and mask it; I will confirm that
against the datasheet rather than assume it.

8. Correct, and known: a write landing while tx_state is MAX310X_TX_SEND
does reset the state to MAX310X_TX_WAIT_BEFORE_SEND and re-arm. The
txlvl == 0 gate in max310x_delayed_stop_tx() exists for exactly the
mid-drain call that results, so the envelope stays correct, but the data
does pay an extra before-send delay. I will skip the re-arm when the
state is already MAX310X_TX_SEND.

The rest I do not think are bugs:

1. Both loops are bounded - the software path by the computed loops
count, the hardware path by port->fifosize + 1. The hung task detector
also cannot fire here: it looks for a task whose switch count has not
changed across its timeout, and both loops sleep and wake every
iteration. The longest single sleep is one character duration at the
lowest baud the BRG can produce, well under the 120 s default.

2. max310x_delayed_stop_tx() returns early while tx_state is
MAX310X_TX_OFF, before it touches cancel_tx_delay_tmr, and
max310x_shutdown() sets that state before it returns, so a late tx_work
cannot re-arm the timer. tx_work has never been cancelled in shutdown,
including before this patch; it is cancelled in max310x_remove().

3. Dropping and retaking port->lock around hrtimer_cancel() is
deliberate: max310x_tmr_tx() takes port->lock, so cancelling while
holding it would deadlock. The timer is not soft, so
hrtimer_cancel_wait_running() only does cpu_relax() and nothing sleeps,
and several other serial drivers drop the lock the same way. I will
switch the two sites to uart_port_unlock()/uart_port_lock() in v3 for
consistency, although for this driver they are no-ops as it has no
console support.

4. I could not construct this one. max310x_handle_tx() does fill the
FIFO without consulting tx_state, but the TX-empty interrupt latches on
the FIFO going empty, so from idle there is no tx_work already pending
to run ahead of rts_work, and mid-stream RTS is still asserted by the
burst in progress. If you have a concrete ordering in mind, I would like
to see it.

7. hrtimer_setup() and INIT_WORK() only initialise; nothing is armed or
queued until a port is opened, and the out_uart path calls
uart_remove_one_port(), which hangs the tty up and runs ->shutdown(),
which cancels both. The pre-existing tx_work, md_work and rs_work have
the same shape.

Tapio