[PATCH v8 0/9] serial: max310x: RS485 delay and RTS fixes, software-timed delays

[PATCH v8 0/9] serial: max310x: RS485 delay and RTS fixes, software-timed delays

[Tapio Reijonen]

The MAX310X hardware can express at most 15 bit-times of RS485 RTS
setup/hold delay, while struct serial_rs485 expresses the delays in
milliseconds. The driver rejected anything above 0x0f with -ERANGE,
upon which uart_rs485_config() wipes port->rs485 and silently disables
RS485 - a device tree asking for a 20 ms setup delay boots with RS485
off and an unusable bus. The values that were accepted got written
into HDPIXDELAY unconverted, milliseconds as bit-times.

Patches 1-5 fix pre-existing bugs found on the way: a termios write
clobbering an active break; breaks never reaching the wire on RS485
ports because auto-RTS only drives the transceiver for FIFO data; the
milliseconds-as-bit-times unit bug (patch 3 first centralizes the
transceiver register programming with no functional change, patch 4
then converts the units); and close() powering the port down
mid-transmission, truncating the final character and, on the auto-RTS
path, leaving the transceiver asserted on the bus until the next open.
Patch 6 adds active-low RTS on the hardware path via IRDA.RTSINVERT.
Patch 7
is preparation, and patch 8 adds the software-timed RTS path that
takes over whenever the hardware cannot represent the requested
timing, clamping the delays to the UART core's maximum instead of
rejecting them. Patch 9 fixes a reconfigure-versus-write race the
asynchronous rs485 config application has had since 2016, which the
software path would have made worse.

v4 was all of this in a single patch; Greg asked for it to be broken
up into one change at a time [1]. Splitting it meant re-verifying each
patch in isolation on hardware, and that re-verification found two
bugs v4 contained: a set_termios() or TIOCSRS485 during an active
break released the transceiver mid-break while the break bookkeeping
still looked correct (prevented by the tx_break ownership guard in
patches 2 and 3), and the patch-9 race, where a TIOCSRS485 followed
immediately by a write could put an entire transfer on the wire with
the transceiver released.

Tested on a MAX14830 (SPI, i.MX6SX) driving RS485 transceivers: for
each patch the bug it fixes was first reproduced on the wire with a
logic analyzer against the kernel one patch earlier, then shown fixed.
The complete series additionally passed an automated regression
matrix, grown during the v5/v6 review rounds to more than 40
scenarios: both RTS paths, both polarities, RS485/RS232 mode
round-trips, close/hangup/SIGKILL landing in every envelope phase,
output stop and flush mid-burst, mid-transfer path switches in both
directions, XON/XOFF injection at idle and mid-burst, and termios/
TIOCSRS485 disturbances landing in every envelope phase (setup, data,
hold, break) - each scenario checked both on the wire and against the
driver's reported state.

Changes in v8:
- no code changes - the diff is byte-identical to v7
- patch 1: the commit message now leads with the user-visible failure
  (a TIOCSBRK break silently ended by any termios change) instead of
  the register mechanics (Hugo)
- patch 9: the message documents the lock-free chip-FIFO check in the
  deferred-write restart - a refill racing it costs one extra
  before-send pause with the transceiver held, the pre-patch-8
  behaviour (v7 review bot)
- patches 2, 4 and 5 are retitled to name the failure they fix
  instead of the change that fixes it (Hugo)
- patch 5 carries the Assisted-by tag again; the v7 reword of its
  message dropped it unnoticed
- the cover letter is back in its usual shape; v7's went out with the
  changelog above the title and no subject (tooling slip)
- Link to v7: https://lore.kernel.org/r/20261005-max310x-rs485-sw-delay-v7-0-6ae392eca08c@vaisala.com

Changes in v7:
- patch 5 reworked: instead of draining the FIFO at line rate - up to
  fifosize+1 character times of uninterruptible sleep per close(),
  pointed out by the v6 review bot, and unbounded with CTS flow
  control holding the FIFO - shutdown() now stops the transmitter
  (MODE1 TxDisabl): the character in flight completes, abandoned data
  is discarded, and the auto-RTS release is given the configured hold
  plus one bit time before power-off. close() is bounded by about one
  character plus the hold, independent of queued data. This also
  fixes a bug the drain still had: a truncating close() on the
  auto-RTS path powered down mid-transmission and froze the
  transceiver asserted on the bus until the next open (reproduced 9/9
  on the wire: 0.6-2.4 s of stuck DE plus a corrupt character at
  reopen), since the drain bound could expire with data left
- patch 8: shutdown() gates first - teardown interlock and interrupt
  mask before any wait - and the envelope wait honours only the
  after-send hold plus two character times; the final RTS release
  settle is tightened from one character to one bit time (the
  measured pin propagation is one tick of the 16x oversampling clock)
- patch 8: a write landing while the envelope is in its send phase no
  longer rewinds it to the before-send phase, which inserted a
  spurious setup delay mid-stream; the teardown interlock now also
  fences the in-flight transfer adoption, so a reconfigure racing a
  teardown cannot re-arm the cancelled delay timer; and an RS485
  disable flushes a queued RTS worker that could re-assert the pin
  after the settle (v6 review bot)
- patch 9: the deferred-TX release in the rs485 worker takes the port
  lock through uart_port_lock_irqsave() instead of a raw spinlock
  guard, which start_tx()'s lock drop/retake would unbalance against
  the nbcon console handling (v6 review bot)
- patch 9: a write deferred by the reconfigure gate is restarted as
  a fresh envelope: the reconfigure helper's transfer adoption
  otherwise claims it and the restart pumps it in the send phase,
  skipping the configured before-send delay (0.3 ms on the wire where
  20 ms was configured; caught by the v7 regression run)
- the shutdown rework was re-verified on the wire: the regression
  matrix plus close/hangup/SIGKILL truncation scenarios on both RTS
  paths and both polarities, at the baud extremes
- Link to v6: https://lore.kernel.org/r/20261004-max310x-rs485-sw-delay-v6-0-3a0ef13ed9e3@vaisala.com

Changes in v6:
- every patch now carries the Assisted-by tag, which v5 omitted [2]
- patch 1: the LCR clobber is now described as "overwrites the whole
  LCR register" in the message and as a whole-register write in the
  comments (Hugo)
- the character-time caching and the shutdown drain-wait comments now
  state their reasons in place: both values must be current before
  the RS485 helper runs, and the tty layer's wait-until-sent is
  bounded by closing_wait and absent on hangup (Hugo)
- the old patch 3 is split in two: patch 3 only introduces
  max310x_set_rts_ctl_params() and folds the three copies of the
  HDPIXDELAY/MODE1 programming into it, patch 4 then only adds the
  millisecond-to-bit-time conversion (Hugo); break_ctl() keeps a
  single max310x_rts_ctl() call at the end instead of duplicating it
  in both branches (Hugo)
- one_char_duration_us is renamed to char_time_us and the shutdown
  drain bounds to tries (Hugo)
- fix: disabling RS485 during an active break left the transceiver
  driving the bus indefinitely - break-off only ran its restore while
  RS485 was still enabled. Only the break assertion is now gated on
  RS485 being enabled; the break-off restore always runs and derives
  the register state from the current configuration (patch 2)
- fix: a reconfigure that moves the port off the hardware RTS path
  while a transmission is still in flight - a termios change or
  TIOCSRS485 pushing a delay above what the hardware can time at the
  new rate - released the transceiver mid-transfer, and the rest of
  the data was shifted out with the bus undriven, with no error
  reported. The helper now takes such a transfer over: it enters the
  software envelope and keeps RTS driven across the handover, and the
  normal drain path arms the after-send hold (patch 8)
- fix: a break that begins while the previous envelope's after-send
  hold is still armed - TIOCSBRK waits for the output to drain, which
  lands it exactly there - was cut short when the hold expired and
  released the transceiver mid-break. The RTS worker now derives the
  line state from break ownership as well, so the expiry re-asserts
  instead of releasing (patch 8)
- fix: an XON/XOFF character deferred by the reconfigure-pending gate
  was never transmitted when nothing else was queued, and the re-check
  after start_tx()'s dropped lock did not cover a freshly posted
  reconfigure (patch 9)
- fix: a delay of exactly 15 bit-times - the largest the chip can
  time - was routed to the software path, because the hardware
  ceiling was computed in nanoseconds from the truncated per-bit
  time (49999995 ns where 50 ms at 300 baud needs 50000000). The
  selection now converts to bit-times first and compares against the
  field maximum, which is exact at every baud rate (patch 8)
- fix: a close() racing the after-send hold expiry could leave the
  transceiver driving the bus: shutdown's manual RTS release reaches
  the register, but the RTS output stage is clocked by the UART
  channel clock that the power-off stops - stopped too soon, the pin
  stays asserted until the next open (register confirmed released,
  pin confirmed high, in every observed case). shutdown() now gives
  the release one character time, at least 100 us, before powering
  down; measured propagation is below 50 us at 1200 baud, and the
  delay-free alternative (handing the pin to the auto-RTS engine) was
  tried and rejected on the wire - for an active-low RTS every
  register ordering of that handover drives the pin to the asserted
  level mid-sequence (patch 8)
- fix: a reconfigure moving the port onto the hardware path
  mid-envelope stranded the software envelope state: nothing reset it
  in that direction, and the TX-empty hold arming ran only on the
  software path. A later reconfigure off the hardware path trusted
  the stale state, skipped the in-flight-transfer adoption, and
  released the transceiver under the running transfer. The TX-empty
  unwind now runs on both paths, the reconfigure asserts RTS for any
  transfer it believes exists before auto-RTS is disabled, and the
  hold arms only once no transmittable data remains (patch 8)
- the seven fixes above came out of further review and hardware
  testing of v5: each was first demonstrated on the wire, then shown
  fixed, and the full regression matrix was re-run on the result
- Link to v5: https://lore.kernel.org/r/20260929-max310x-rs485-sw-delay-v5-0-ae46afa583f2@vaisala.com

Changes in v5, beyond the split:
- teardown interlock (tx_teardown): shutdown() and the rs485-disable
  path set it under port->lock, and start_tx() checks it on entry and
  again after retaking the dropped lock, so a racing write can no
  longer re-arm the delay timer or queue RTS work against a port being
  torn down (addresses the remaining review-bot findings on v4)
- shutdown() also cancels tx_work, previously only cancelled in
  remove()
- the per-character duration is stored as unsigned int microseconds
  instead of ktime_t: single-copy atomic on 32-bit, so a torn read of
  the 64-bit value is gone by construction
- the TXEMPTY handling documents that the interrupt latches on the
  FIFO becoming empty, so a stale interrupt cannot pump data during an
  RTS setup delay
- new in v5: the tx_break ownership guard (patches 2/3) and the
  reconfigure-pending gate (patch 9), both found during the per-patch
  hardware re-testing described above
- also new in v5, from a review pass over the split series: startup()
  clears a latched break (nothing clears TXBREAK when a port is closed
  with a break still asserted - 8250 does the same); a reconfigure
  arriving during a break is now deferred and applied at break-end
  instead of partially dropped; the rs485-config worker runs under
  port->mutex so its break-guarded register writes cannot straddle a
  break edge; the termios-path idle settle re-checks tx_state after
  writing and requeues rts_work if an envelope started meanwhile; and
  the hardware-delay ceiling is computed in u64

[1] https://lore.kernel.org/all/2026092326-truth-unweave-c773@gregkh/
[2] https://lore.kernel.org/all/2026100116-saint-idealize-32cf@gregkh/

---
Tapio Reijonen (9):
      serial: max310x: don't clobber the TX break bit in set_termios
      serial: max310x: fix breaks not reaching the wire in RS485 mode
      serial: max310x: centralize the RS485 transceiver programming
      serial: max310x: fix RS485 delays being programmed as bit-times
      serial: max310x: don't power down the port mid-transmission on close
      serial: max310x: support active-low RTS on the hardware path
      serial: max310x: schedule tx_work directly from the IRQ handler
      serial: max310x: drive RTS in software when hardware delays are too short
      serial: max310x: don't transmit while an RS485 reconfigure is pending

 drivers/tty/serial/max310x.c | 632 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 595 insertions(+), 37 deletions(-)
---
base-commit: 9505146e885b1a842118aa6410f737290c4a5a32
change-id: 20260513-max310x-rs485-sw-delay-a306d783d529

Best regards,
-- 
Tapio Reijonen <tapio.reijonen@vaisala.com>

[PATCH v8 1/9] serial: max310x: don't clobber the TX break bit in set_termios

[Tapio Reijonen]

A TX break asserted with TIOCSBRK ends early, with no error reported,
as soon as any termios change lands on the port: a break is a state
that must hold until TIOCCBRK, but max310x_set_termios() overwrites
the whole LCR register, and LCR also carries the TX break bit that
break_ctl() drives. No concurrency is required - a baud or parity
change from the same process ends the break just as well, and
break-led protocols lose their break with nothing in the logs.

Update only the LCR bits that are derived from termios and leave the
TX break and RTS pin control bits untouched. Since nothing clears a
break when a port is closed with the break still asserted - the tty
core sends no break-off on release, and the unconditional write here
was the accidental recovery - clear TXBREAK in startup(), the same way
8250 does.

Fixes: f65444187a66 ("serial: New serial driver MAX310X")
Assisted-by: Claude:claude-fable-5
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 022502986c5fcf1ff4de9328746ddc71677be730..fead9c51163d8372d1b609ee9cd5b87faa917fc1 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -158,6 +158,8 @@
 #define MAX310X_LCR_FORCEPARITY_BIT	(1 << 5) /* 9-bit multidrop parity */
 #define MAX310X_LCR_TXBREAK_BIT		(1 << 6) /* TX break enable */
 #define MAX310X_LCR_RTS_BIT		(1 << 7) /* RTS pin control */
+/* LCR bits owned by termios; TX break and RTS are driven elsewhere */
+#define MAX310X_LCR_TERMIOS_MASK	GENMASK(5, 0)
 
 /* IRDA register bits */
 #define MAX310X_IRDA_IRDAEN_BIT		(1 << 0) /* IRDA mode enable */
@@ -969,8 +971,12 @@ static void max310x_set_termios(struct uart_port *port,
 	if (termios->c_cflag & CSTOPB)
 		lcr |= MAX310X_LCR_STOPLEN_BIT; /* 2 stops */
 
-	/* Update LCR register */
-	max310x_port_write(port, MAX310X_LCR_REG, lcr);
+	/*
+	 * Update LCR register. Leave the TX break bit alone: it is driven by
+	 * break_ctl(), and a whole-register write here would end a break in
+	 * progress.
+	 */
+	max310x_port_update(port, MAX310X_LCR_REG, MAX310X_LCR_TERMIOS_MASK, lcr);
 
 	/* Set read status mask */
 	port->read_status_mask = MAX310X_LSR_RXOVR_BIT;
@@ -1088,6 +1094,13 @@ static int max310x_startup(struct uart_port *port)
 
 	max310x_power(port, 1);
 
+	/*
+	 * Clear a latched break: nothing clears TXBREAK when a port is
+	 * closed with a break still asserted, and set_termios() no longer
+	 * rewrites it.
+	 */
+	max310x_port_update(port, MAX310X_LCR_REG, MAX310X_LCR_TXBREAK_BIT, 0);
+
 	/* Configure MODE1 register */
 	max310x_port_update(port, MAX310X_MODE1_REG,
 			    MAX310X_MODE1_TRNSCVCTRL_BIT, 0);

-- 
2.47.3

Re: [PATCH v8 1/9] serial: max310x: don't clobber the TX break bit in set_termios

[sashiko-bot]

> A TX break asserted with TIOCSBRK ends early, with no error reported,
> as soon as any termios change lands on the port: a break is a state
> that must hold until TIOCCBRK, but max310x_set_termios() overwrites
> the whole LCR register, and LCR also carries the TX break bit that
> break_ctl() drives. No concurrency is required - a baud or parity
> change from the same process ends the break just as well, and
> break-led protocols lose their break with nothing in the logs.
> 
> Update only the LCR bits that are derived from termios and leave the
> TX break and RTS pin control bits untouched. Since nothing clears a
> break when a port is closed with the break still asserted - the tty
> core sends no break-off on release, and the unconditional write here
> was the accidental recovery - clear TXBREAK in startup(), the same way
> 8250 does.
> 
> Fixes: f65444187a66 ("serial: New serial driver MAX310X")
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-max310x-rs485-sw-delay-v8-0-6ba881f81e1f@vaisala.com?part=1

[PATCH v8 2/9] serial: max310x: fix breaks not reaching the wire in RS485 mode

[Tapio Reijonen]

The chip's auto-RTS (MODE1.TRNSCVCTRL) asserts the RS485 transceiver
only while FIFO data is shifting out, and a break is not FIFO data: on
an RS485 port a requested break sets the TX break bit but the
transceiver is never enabled, so the break never reaches the wire.
Break-led protocols cannot work at all.

Disable auto-RTS for the break duration and drive RTS manually via the
LCR RTS bit, then restore auto-RTS when the break ends. Track the break
in tx_break and leave MODE1 alone in the rs485-config worker while it
is set - a TIOCSRS485 arriving mid-break would otherwise re-enable
auto-RTS on top of the manual RTS and release the transceiver before
the break ends; break_ctl() restores auto-RTS from the then-current
configuration when the break completes. The worker runs under
port->mutex - break_ctl() and set_termios() already do - so the
tx_break test and the MODE1 write cannot straddle a break starting or
ending, and startup() clears tx_break alongside the latched TXBREAK
bit, since a port can be closed with a break still asserted.

Only the break assertion is gated on RS485 being enabled; the restore
at break end runs unconditionally and derives MODE1 from the current
configuration. Returning early for a port whose RS485 was disabled
mid-break - that reconfigure is deferred like any other - would leak
the manually driven RTS and leave the transceiver holding the bus
indefinitely after the break ends. On a non-RS485 port the restore is
a no-op: nothing else writes the LCR RTS bit.

Fixes: 55367c620aed ("serial: max310x: Add support for RS-485 mode")
Assisted-by: Claude:claude-fable-5
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 49 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index fead9c51163d8372d1b609ee9cd5b87faa917fc1..319517bee63fdf5e67c311d2575083fd0d8a5870 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -298,6 +298,7 @@ struct max310x_one {
 	struct work_struct	md_work;
 	struct work_struct	rs_work;
 	struct regmap		*regmap;
+	bool			tx_break;	/* break_ctl() owns the transceiver */
 
 	u8 rx_buf[MAX310X_FIFO_SIZE];
 };
@@ -682,6 +683,12 @@ static void max310x_batch_read(struct uart_port *port, u8 *rxbuf, unsigned int l
 	regmap_noinc_read(one->regmap, MAX310X_RHR_REG, rxbuf, len);
 }
 
+static void max310x_rts_ctl(struct uart_port *port, bool rts_state)
+{
+	max310x_port_update(port, MAX310X_LCR_REG, MAX310X_LCR_RTS_BIT,
+			    rts_state ? MAX310X_LCR_RTS_BIT : 0);
+}
+
 static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
 {
 	struct max310x_one *one = to_max310x_port(port);
@@ -929,9 +936,32 @@ static void max310x_set_mctrl(struct uart_port *port, unsigned int mctrl)
 
 static void max310x_break_ctl(struct uart_port *port, int break_state)
 {
+	struct max310x_one *one = to_max310x_port(port);
+
+	one->tx_break = break_state;
+
 	max310x_port_update(port, MAX310X_LCR_REG,
 			    MAX310X_LCR_TXBREAK_BIT,
 			    break_state ? MAX310X_LCR_TXBREAK_BIT : 0);
+
+	/*
+	 * The chip's auto-RTS asserts the transceiver only while FIFO data is
+	 * shifting out, and a break is not FIFO data. Disable auto-RTS for the
+	 * break duration and drive RTS manually so the break reaches the wire;
+	 * restore auto-RTS when the break ends.
+	 */
+	if (break_state) {
+		if (!(port->rs485.flags & SER_RS485_ENABLED))
+			return;
+		max310x_port_update(port, MAX310X_MODE1_REG,
+				    MAX310X_MODE1_TRNSCVCTRL_BIT, 0);
+	} else {
+		max310x_port_update(port, MAX310X_MODE1_REG,
+				    MAX310X_MODE1_TRNSCVCTRL_BIT,
+				    (port->rs485.flags & SER_RS485_ENABLED) ?
+				    MAX310X_MODE1_TRNSCVCTRL_BIT : 0);
+	}
+	max310x_rts_ctl(port, break_state);
 }
 
 static void max310x_set_termios(struct uart_port *port,
@@ -1055,6 +1085,13 @@ static void max310x_rs_proc(struct work_struct *ws)
 	struct max310x_one *one = container_of(ws, struct max310x_one, rs_work);
 	unsigned int delay, mode1 = 0, mode2 = 0;
 
+	/*
+	 * Serialize against break_ctl() and set_termios(), which run under
+	 * port->mutex: the tx_break test below and the MODE1 write must not
+	 * straddle a break starting or ending.
+	 */
+	guard(mutex)(&one->port.state->port.mutex);
+
 	delay = (one->port.rs485.delay_rts_before_send << 4) |
 		one->port.rs485.delay_rts_after_send;
 	max310x_port_write(&one->port, MAX310X_HDPIXDELAY_REG, delay);
@@ -1066,8 +1103,14 @@ static void max310x_rs_proc(struct work_struct *ws)
 			mode2 = MAX310X_MODE2_ECHOSUPR_BIT;
 	}
 
-	max310x_port_update(&one->port, MAX310X_MODE1_REG,
-			MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
+	/*
+	 * A break owns the transceiver: break_ctl() disabled auto-RTS and
+	 * drives RTS manually, and restores it when the break ends. Leave
+	 * MODE1 alone meanwhile or the break goes undriven mid-way.
+	 */
+	if (!one->tx_break)
+		max310x_port_update(&one->port, MAX310X_MODE1_REG,
+				    MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
 	max310x_port_update(&one->port, MAX310X_MODE2_REG,
 			MAX310X_MODE2_ECHOSUPR_BIT, mode2);
 }
@@ -1090,6 +1133,7 @@ static int max310x_rs485_config(struct uart_port *port, struct ktermios *termios
 
 static int max310x_startup(struct uart_port *port)
 {
+	struct max310x_one *one = to_max310x_port(port);
 	unsigned int val;
 
 	max310x_power(port, 1);
@@ -1099,6 +1143,7 @@ static int max310x_startup(struct uart_port *port)
 	 * closed with a break still asserted, and set_termios() no longer
 	 * rewrites it.
 	 */
+	one->tx_break = false;
 	max310x_port_update(port, MAX310X_LCR_REG, MAX310X_LCR_TXBREAK_BIT, 0);
 
 	/* Configure MODE1 register */

-- 
2.47.3

Re: [PATCH v8 2/9] serial: max310x: fix breaks not reaching the wire in RS485 mode

[sashiko-bot]

> The chip's auto-RTS (MODE1.TRNSCVCTRL) asserts the RS485 transceiver
> only while FIFO data is shifting out, and a break is not FIFO data: on
> an RS485 port a requested break sets the TX break bit but the
> transceiver is never enabled, so the break never reaches the wire.
> Break-led protocols cannot work at all.
> 
> Disable auto-RTS for the break duration and drive RTS manually via the
> LCR RTS bit, then restore auto-RTS when the break ends. Track the break
> in tx_break and leave MODE1 alone in the rs485-config worker while it
> is set - a TIOCSRS485 arriving mid-break would otherwise re-enable
> auto-RTS on top of the manual RTS and release the transceiver before
> the break ends; break_ctl() restores auto-RTS from the then-current
> configuration when the break completes. The worker runs under
> port->mutex - break_ctl() and set_termios() already do - so the
> tx_break test and the MODE1 write cannot straddle a break starting or
> ending, and startup() clears tx_break alongside the latched TXBREAK
> bit, since a port can be closed with a break still asserted.
> 
> Only the break assertion is gated on RS485 being enabled; the restore
> at break end runs unconditionally and derives MODE1 from the current
> configuration. Returning early for a port whose RS485 was disabled
> mid-break - that reconfigure is deferred like any other - would leak
> the manually driven RTS and leave the transceiver holding the bus
> indefinitely after the break ends. On a non-RS485 port the restore is
> a no-op: nothing else writes the LCR RTS bit.
> 
> Fixes: 55367c620aed ("serial: max310x: Add support for RS-485 mode")
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-max310x-rs485-sw-delay-v8-0-6ba881f81e1f@vaisala.com?part=2

[PATCH v8 3/9] serial: max310x: centralize the RS485 transceiver programming

[Tapio Reijonen]

The HDPIXDELAY RTS delays and the MODE1 auto-transceiver enable are
programmed in three places - the rs485-config worker, startup() and
break_ctl()'s break-off restore - each with its own copy of the
register sequence. Move the sequence into a helper,
max310x_set_rts_ctl_params(), together with the tx_break guard that
keeps the MODE1 write away from a break in progress; the break-off
restore reapplies the current configuration through the same helper.

No functional change intended. The copies differed only in that
startup() clamped the delays to the 4-bit field while the worker wrote
them unclamped; the helper clamps, and the difference is unreachable
while rs485_config() still rejects delays above 15 ms with -ERANGE.
Break-off additionally rewrites HDPIXDELAY with the values it already
holds, a no-op on the wire.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 85 ++++++++++++++++++++++++--------------------
 1 file changed, 47 insertions(+), 38 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 319517bee63fdf5e67c311d2575083fd0d8a5870..85353401e09b1481996142432b1279f44fefff1d 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -934,6 +934,38 @@ static void max310x_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	schedule_work(&one->md_work);
 }
 
+/*
+ * Program the chip's RS485 RTS timing (HDPIXDELAY) and the auto
+ * transceiver control (MODE1) from the current configuration. The
+ * rs485-config worker, startup() and break_ctl()'s break-off restore
+ * each carried their own copy of this sequence.
+ */
+static void max310x_set_rts_ctl_params(struct max310x_one *one)
+{
+	struct uart_port *port = &one->port;
+	unsigned int delay;
+	u8 mode1 = 0;
+
+	delay = (clamp(port->rs485.delay_rts_before_send, 0U, 15U) << 4) |
+		clamp(port->rs485.delay_rts_after_send, 0U, 15U);
+	max310x_port_write(port, MAX310X_HDPIXDELAY_REG, delay);
+
+	if (port->rs485.flags & SER_RS485_ENABLED)
+		mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
+
+	/*
+	 * A break owns the transceiver: break_ctl() disabled auto-RTS and
+	 * drives RTS manually, and restores it from the current
+	 * configuration when the break ends. Touching MODE1 here would
+	 * release the transceiver mid-break.
+	 */
+	if (one->tx_break)
+		return;
+
+	max310x_port_update(port, MAX310X_MODE1_REG,
+			    MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
+}
+
 static void max310x_break_ctl(struct uart_port *port, int break_state)
 {
 	struct max310x_one *one = to_max310x_port(port);
@@ -956,10 +988,7 @@ static void max310x_break_ctl(struct uart_port *port, int break_state)
 		max310x_port_update(port, MAX310X_MODE1_REG,
 				    MAX310X_MODE1_TRNSCVCTRL_BIT, 0);
 	} else {
-		max310x_port_update(port, MAX310X_MODE1_REG,
-				    MAX310X_MODE1_TRNSCVCTRL_BIT,
-				    (port->rs485.flags & SER_RS485_ENABLED) ?
-				    MAX310X_MODE1_TRNSCVCTRL_BIT : 0);
+		max310x_set_rts_ctl_params(one);
 	}
 	max310x_rts_ctl(port, break_state);
 }
@@ -1083,36 +1112,23 @@ static void max310x_set_termios(struct uart_port *port,
 static void max310x_rs_proc(struct work_struct *ws)
 {
 	struct max310x_one *one = container_of(ws, struct max310x_one, rs_work);
-	unsigned int delay, mode1 = 0, mode2 = 0;
+	unsigned int mode2 = 0;
 
 	/*
 	 * Serialize against break_ctl() and set_termios(), which run under
-	 * port->mutex: the tx_break test below and the MODE1 write must not
+	 * port->mutex: the tx_break-guarded register writes must not
 	 * straddle a break starting or ending.
 	 */
 	guard(mutex)(&one->port.state->port.mutex);
 
-	delay = (one->port.rs485.delay_rts_before_send << 4) |
-		one->port.rs485.delay_rts_after_send;
-	max310x_port_write(&one->port, MAX310X_HDPIXDELAY_REG, delay);
+	max310x_set_rts_ctl_params(one);
 
-	if (one->port.rs485.flags & SER_RS485_ENABLED) {
-		mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
+	if (one->port.rs485.flags & SER_RS485_ENABLED &&
+	    !(one->port.rs485.flags & SER_RS485_RX_DURING_TX))
+		mode2 = MAX310X_MODE2_ECHOSUPR_BIT;
 
-		if (!(one->port.rs485.flags & SER_RS485_RX_DURING_TX))
-			mode2 = MAX310X_MODE2_ECHOSUPR_BIT;
-	}
-
-	/*
-	 * A break owns the transceiver: break_ctl() disabled auto-RTS and
-	 * drives RTS manually, and restores it when the break ends. Leave
-	 * MODE1 alone meanwhile or the break goes undriven mid-way.
-	 */
-	if (!one->tx_break)
-		max310x_port_update(&one->port, MAX310X_MODE1_REG,
-				    MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
 	max310x_port_update(&one->port, MAX310X_MODE2_REG,
-			MAX310X_MODE2_ECHOSUPR_BIT, mode2);
+			    MAX310X_MODE2_ECHOSUPR_BIT, mode2);
 }
 
 static int max310x_rs485_config(struct uart_port *port, struct ktermios *termios,
@@ -1156,21 +1172,14 @@ static int max310x_startup(struct uart_port *port)
 	max310x_port_update(port, MAX310X_MODE2_REG,
 			    MAX310X_MODE2_FIFORST_BIT, 0);
 
-	/* Configure mode1/mode2 to have rs485/rs232 enabled at startup */
-	val = (clamp(port->rs485.delay_rts_before_send, 0U, 15U) << 4) |
-		clamp(port->rs485.delay_rts_after_send, 0U, 15U);
-	max310x_port_write(port, MAX310X_HDPIXDELAY_REG, val);
-
-	if (port->rs485.flags & SER_RS485_ENABLED) {
-		max310x_port_update(port, MAX310X_MODE1_REG,
-				    MAX310X_MODE1_TRNSCVCTRL_BIT,
-				    MAX310X_MODE1_TRNSCVCTRL_BIT);
+	/* Configure the RS485 RTS timing and the RS485/RS232 mode bits. */
+	max310x_set_rts_ctl_params(one);
 
-		if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
-			max310x_port_update(port, MAX310X_MODE2_REG,
-					    MAX310X_MODE2_ECHOSUPR_BIT,
-					    MAX310X_MODE2_ECHOSUPR_BIT);
-	}
+	if (port->rs485.flags & SER_RS485_ENABLED &&
+	    !(port->rs485.flags & SER_RS485_RX_DURING_TX))
+		max310x_port_update(port, MAX310X_MODE2_REG,
+				    MAX310X_MODE2_ECHOSUPR_BIT,
+				    MAX310X_MODE2_ECHOSUPR_BIT);
 
 	/*
 	 * Configure flow control levels:

-- 
2.47.3

Re: [PATCH v8 3/9] serial: max310x: centralize the RS485 transceiver programming

[sashiko-bot]

> The HDPIXDELAY RTS delays and the MODE1 auto-transceiver enable are
> programmed in three places - the rs485-config worker, startup() and
> break_ctl()'s break-off restore - each with its own copy of the
> register sequence. Move the sequence into a helper,
> max310x_set_rts_ctl_params(), together with the tx_break guard that
> keeps the MODE1 write away from a break in progress; the break-off
> restore reapplies the current configuration through the same helper.
> 
> No functional change intended. The copies differed only in that
> startup() clamped the delays to the 4-bit field while the worker wrote
> them unclamped; the helper clamps, and the difference is unreachable
> while rs485_config() still rejects delays above 15 ms with -ERANGE.
> Break-off additionally rewrites HDPIXDELAY with the values it already
> holds, a no-op on the wire.
> 
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-max310x-rs485-sw-delay-v8-0-6ba881f81e1f@vaisala.com?part=3

[PATCH v8 4/9] serial: max310x: fix RS485 delays being programmed as bit-times

[Tapio Reijonen]

The HDPIXDELAY register counts the RTS setup and hold delays in
bit-times, four bits per direction, but the driver has been writing the
struct serial_rs485 delay_rts_before_send/delay_rts_after_send values
into it unconverted - and the uapi expresses those in milliseconds. A
requested 9 ms setup delay is programmed as 9 bit-times, which at 9600
baud is 0.94 ms, roughly a tenth of what userspace asked for; the error
grows with the baud rate.

Cache the baud rate in set_termios() and convert the delays to
bit-times at the current rate, rounding up so the delay on the wire is
never shorter than requested, and capping at the 15 bit-times the
4-bit field can hold. The conversion lives in
max310x_set_rts_ctl_params(); set_termios() now calls it as well,
since the conversion depends on the baud rate.

The delays a 4-bit bit-time field can represent still fall well short
of the milliseconds the uapi can express; requests beyond 15 bit-times
are capped, and the -ERANGE rejection of values above 15 ms remains in
place for now.

Fixes: 55367c620aed ("serial: max310x: Add support for RS-485 mode")
Assisted-by: Claude:claude-fable-5
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 85353401e09b1481996142432b1279f44fefff1d..2456a2af891f5296cfec833d6406916073d2865f 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -165,6 +165,10 @@
 #define MAX310X_IRDA_IRDAEN_BIT		(1 << 0) /* IRDA mode enable */
 #define MAX310X_IRDA_SIR_BIT		(1 << 1) /* SIR mode enable */
 
+/* HDPIXDELAY accessor macros */
+#define MAX310X_HDPIXDELAY_SETUP(val)	(((val) & 0x0f) << 4)
+#define MAX310X_HDPIXDELAY_HOLD(val)	((val) & 0x0f)
+
 /* Flow control trigger level register masks */
 #define MAX310X_FLOWLVL_HALT_MASK	GENMASK(3, 0) /* Flow control halt level */
 #define MAX310X_FLOWLVL_RES_MASK	GENMASK(7, 4) /* Flow control resume level */
@@ -298,6 +302,7 @@ struct max310x_one {
 	struct work_struct	md_work;
 	struct work_struct	rs_work;
 	struct regmap		*regmap;
+	unsigned int		baud;
 	bool			tx_break;	/* break_ctl() owns the transceiver */
 
 	u8 rx_buf[MAX310X_FIFO_SIZE];
@@ -935,23 +940,33 @@ static void max310x_set_mctrl(struct uart_port *port, unsigned int mctrl)
 }
 
 /*
- * Program the chip's RS485 RTS timing (HDPIXDELAY) and the auto
- * transceiver control (MODE1) from the current configuration. The
- * rs485-config worker, startup() and break_ctl()'s break-off restore
- * each carried their own copy of this sequence.
+ * Program the chip's RS485 RTS timing. The HDPIXDELAY setup and hold fields
+ * count bit-times, four bits per direction, while the uapi expresses the
+ * delays in milliseconds: convert at the current baud rate, rounding up, and
+ * cap at the field maximum.
  */
 static void max310x_set_rts_ctl_params(struct max310x_one *one)
 {
+	const unsigned int max_bit_dly = 15;
 	struct uart_port *port = &one->port;
-	unsigned int delay;
+	unsigned int setup = 0, hold = 0;
 	u8 mode1 = 0;
 
-	delay = (clamp(port->rs485.delay_rts_before_send, 0U, 15U) << 4) |
-		clamp(port->rs485.delay_rts_after_send, 0U, 15U);
-	max310x_port_write(port, MAX310X_HDPIXDELAY_REG, delay);
+	if (port->rs485.flags & SER_RS485_ENABLED) {
+		/* Convert milliseconds to bit-times, rounding up. */
+		setup = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_before_send,
+				     MSEC_PER_SEC);
+		hold  = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_after_send,
+				     MSEC_PER_SEC);
+		setup = min(setup, max_bit_dly);
+		hold  = min(hold,  max_bit_dly);
 
-	if (port->rs485.flags & SER_RS485_ENABLED)
 		mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
+	}
+
+	max310x_port_write(port, MAX310X_HDPIXDELAY_REG,
+			   MAX310X_HDPIXDELAY_SETUP(setup) |
+			   MAX310X_HDPIXDELAY_HOLD(hold));
 
 	/*
 	 * A break owns the transceiver: break_ctl() disabled auto-RTS and
@@ -1107,6 +1122,13 @@ static void max310x_set_termios(struct uart_port *port,
 
 	/* Update timeout according to new baud rate */
 	uart_update_timeout(port, termios->c_cflag, baud);
+
+	/*
+	 * Cache the new baud rate and reprogram the RS485 RTS delays, whose
+	 * millisecond-to-bit-time conversion depends on it.
+	 */
+	to_max310x_port(port)->baud = baud;
+	max310x_set_rts_ctl_params(to_max310x_port(port));
 }
 
 static void max310x_rs_proc(struct work_struct *ws)

-- 
2.47.3

Re: [PATCH v8 4/9] serial: max310x: fix RS485 delays being programmed as bit-times

[sashiko-bot]

> The HDPIXDELAY register counts the RTS setup and hold delays in
> bit-times, four bits per direction, but the driver has been writing the
> struct serial_rs485 delay_rts_before_send/delay_rts_after_send values
> into it unconverted - and the uapi expresses those in milliseconds. A
> requested 9 ms setup delay is programmed as 9 bit-times, which at 9600
> baud is 0.94 ms, roughly a tenth of what userspace asked for; the error
> grows with the baud rate.
> 
> Cache the baud rate in set_termios() and convert the delays to
> bit-times at the current rate, rounding up so the delay on the wire is
> never shorter than requested, and capping at the 15 bit-times the
> 4-bit field can hold. The conversion lives in
> max310x_set_rts_ctl_params(); set_termios() now calls it as well,
> since the conversion depends on the baud rate.
> 
> The delays a 4-bit bit-time field can represent still fall well short
> of the milliseconds the uapi can express; requests beyond 15 bit-times
> are capped, and the -ERANGE rejection of values above 15 ms remains in
> place for now.
> 
> Fixes: 55367c620aed ("serial: max310x: Add support for RS-485 mode")
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-max310x-rs485-sw-delay-v8-0-6ba881f81e1f@vaisala.com?part=4

[PATCH v8 5/9] serial: max310x: don't power down the port mid-transmission on close

[Tapio Reijonen]

max310x_shutdown() powers the port down regardless of what the
transmitter is doing. The power-off stops the UART clock mid-character,
truncating the final frame, and on the auto-RTS path it freezes the
RTS output at its current level: a close() with data still queued
leaves the RS485 transceiver asserted until the next open, which also
emits the interrupted character corrupted.

Waiting for the data to drain is no better: the tty layer's
wait-until-sent is bounded by closing_wait, configurable to none, and
a hangup arrives with no wait at all, so draining a full FIFO blocks
close() in uninterruptible sleep for seconds at low baud rates - and
forever when CTS flow control blocks the FIFO.

Instead, set MODE1 TxDisabl: the character in flight completes and the
transmitter ceases with TX_ at idle. Give that character one character
time (the chip has no transmitter-idle status), then reset the FIFOs
so the auto-RTS engine sees the transmitter empty and releases RTS
within the configured after-send hold; wait that hold plus one bit
time before powering off. Abandoned data was explicitly not waited
for, and startup() resets the FIFOs and TxDisabl on open anyway.

Measured on a MAX14830: a truncating close() at 50 baud takes 0.23 s,
RTS releases one character plus the hold after the last stop bit on
both polarities, and the reopen corruption is gone.

Fixes: f65444187a66 ("serial: New serial driver MAX310X")
Assisted-by: Claude:claude-fable-5
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 2456a2af891f5296cfec833d6406916073d2865f..e08e506360d72a3ef4c0ebdca2e60d864973f909 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -302,6 +302,7 @@ struct max310x_one {
 	struct work_struct	md_work;
 	struct work_struct	rs_work;
 	struct regmap		*regmap;
+	unsigned int		char_time_us;
 	unsigned int		baud;
 	bool			tx_break;	/* break_ctl() owns the transceiver */
 
@@ -1012,6 +1013,7 @@ static void max310x_set_termios(struct uart_port *port,
 				struct ktermios *termios,
 				const struct ktermios *old)
 {
+	unsigned int frame_bits = tty_get_frame_size(termios->c_cflag);
 	unsigned int lcr = 0, flow = 0;
 	int baud;
 
@@ -1124,10 +1126,13 @@ static void max310x_set_termios(struct uart_port *port,
 	uart_update_timeout(port, termios->c_cflag, baud);
 
 	/*
-	 * Cache the new baud rate and reprogram the RS485 RTS delays, whose
-	 * millisecond-to-bit-time conversion depends on it.
+	 * Cache the new baud rate and the time it takes to clock out one
+	 * character before reprogramming the RS485 RTS delays: the
+	 * millisecond-to-bit-time conversion divides by the baud rate.
 	 */
 	to_max310x_port(port)->baud = baud;
+	to_max310x_port(port)->char_time_us =
+		DIV_ROUND_UP(USEC_PER_SEC * frame_bits, baud);
 	max310x_set_rts_ctl_params(to_max310x_port(port));
 }
 
@@ -1184,9 +1189,10 @@ static int max310x_startup(struct uart_port *port)
 	one->tx_break = false;
 	max310x_port_update(port, MAX310X_LCR_REG, MAX310X_LCR_TXBREAK_BIT, 0);
 
-	/* Configure MODE1 register */
+	/* Configure MODE1: re-enable the transmitter shutdown() stopped */
 	max310x_port_update(port, MAX310X_MODE1_REG,
-			    MAX310X_MODE1_TRNSCVCTRL_BIT, 0);
+			    MAX310X_MODE1_TRNSCVCTRL_BIT |
+			    MAX310X_MODE1_TXDIS_BIT, 0);
 
 	/* Configure MODE2 register & Reset FIFOs*/
 	val = MAX310X_MODE2_RXEMPTINV_BIT | MAX310X_MODE2_FIFORST_BIT;
@@ -1223,9 +1229,39 @@ static int max310x_startup(struct uart_port *port)
 
 static void max310x_shutdown(struct uart_port *port)
 {
+	struct max310x_one *one = to_max310x_port(port);
+
 	/* Disable all interrupts */
 	max310x_port_write(port, MAX310X_IRQEN_REG, 0);
 
+	/*
+	 * Stop the transmitter: the character in flight completes, data
+	 * still queued is abandoned (the next startup() resets the FIFOs).
+	 */
+	max310x_port_update(port, MAX310X_MODE1_REG,
+			    MAX310X_MODE1_TXDIS_BIT, MAX310X_MODE1_TXDIS_BIT);
+
+	/*
+	 * Let the character in flight finish (the chip has no
+	 * transmitter-idle status), then empty the FIFO: auto-RTS releases
+	 * only once the transmitter is empty, and the power-off below
+	 * would freeze an asserted pin until the next open.
+	 */
+	fsleep(one->char_time_us);
+	max310x_port_update(port, MAX310X_MODE2_REG,
+			    MAX310X_MODE2_FIFORST_BIT,
+			    MAX310X_MODE2_FIFORST_BIT);
+	max310x_port_update(port, MAX310X_MODE2_REG,
+			    MAX310X_MODE2_FIFORST_BIT, 0);
+	if (port->rs485.flags & SER_RS485_ENABLED && one->baud) {
+		unsigned int hold = DIV_ROUND_UP(one->baud *
+				port->rs485.delay_rts_after_send,
+				MSEC_PER_SEC);
+
+		fsleep(max(DIV_ROUND_UP((hold + 1) * USEC_PER_SEC,
+					one->baud), 100U));
+	}
+
 	max310x_power(port, 0);
 }
 

-- 
2.47.3

Re: [PATCH v8 5/9] serial: max310x: don't power down the port mid-transmission on close

[sashiko-bot]

> max310x_shutdown() powers the port down regardless of what the
> transmitter is doing. The power-off stops the UART clock mid-character,
> truncating the final frame, and on the auto-RTS path it freezes the
> RTS output at its current level: a close() with data still queued
> leaves the RS485 transceiver asserted until the next open, which also
> emits the interrupted character corrupted.
> 
> Waiting for the data to drain is no better: the tty layer's
> wait-until-sent is bounded by closing_wait, configurable to none, and
> a hangup arrives with no wait at all, so draining a full FIFO blocks
> close() in uninterruptible sleep for seconds at low baud rates - and
> forever when CTS flow control blocks the FIFO.
> 
> Instead, set MODE1 TxDisabl: the character in flight completes and the
> transmitter ceases with TX_ at idle. Give that character one character
> time (the chip has no transmitter-idle status), then reset the FIFOs
> so the auto-RTS engine sees the transmitter empty and releases RTS
> within the configured after-send hold; wait that hold plus one bit
> time before powering off. Abandoned data was explicitly not waited
> for, and startup() resets the FIFOs and TxDisabl on open anyway.
> 
> Measured on a MAX14830: a truncating close() at 50 baud takes 0.23 s,
> RTS releases one character plus the hold after the last stop bit on
> both polarities, and the reopen corruption is gone.
> 
> Fixes: f65444187a66 ("serial: New serial driver MAX310X")
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-max310x-rs485-sw-delay-v8-0-6ba881f81e1f@vaisala.com?part=5

[PATCH v8 6/9] serial: max310x: support active-low RTS on the hardware path

[Tapio Reijonen]

The chip's auto-RTS engine asserts the RTS_ pin high while data is
shifting out, so a transceiver with an active-low driver-enable could
not use the hardware RS485 path at all: SER_RS485_RTS_AFTER_SEND is
not in the supported flags and the core normalizes it away with
"invalid RTS setting, using RTS_ON_SEND instead".

The output stage is invertible: program IRDA.RTSINVERT when the
requested polarity is active-low and advertise SER_RS485_RTS_AFTER_SEND
in rs485_supported. A break already drives break_state onto the RTS
bit unadjusted, which remains correct because RTSINVERT inverts the
output stage itself, not the register value.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index e08e506360d72a3ef4c0ebdca2e60d864973f909..0c40283f44ba1df712607218aef24cb642ba3918 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -164,6 +164,7 @@
 /* IRDA register bits */
 #define MAX310X_IRDA_IRDAEN_BIT		(1 << 0) /* IRDA mode enable */
 #define MAX310X_IRDA_SIR_BIT		(1 << 1) /* SIR mode enable */
+#define MAX310X_IRDA_RTSINVERT_BIT	(1 << 2) /* Invert RTS output */
 
 /* HDPIXDELAY accessor macros */
 #define MAX310X_HDPIXDELAY_SETUP(val)	(((val) & 0x0f) << 4)
@@ -951,7 +952,7 @@ static void max310x_set_rts_ctl_params(struct max310x_one *one)
 	const unsigned int max_bit_dly = 15;
 	struct uart_port *port = &one->port;
 	unsigned int setup = 0, hold = 0;
-	u8 mode1 = 0;
+	u8 mode1 = 0, irda = 0;
 
 	if (port->rs485.flags & SER_RS485_ENABLED) {
 		/* Convert milliseconds to bit-times, rounding up. */
@@ -963,6 +964,12 @@ static void max310x_set_rts_ctl_params(struct max310x_one *one)
 		hold  = min(hold,  max_bit_dly);
 
 		mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
+		/*
+		 * The auto-RTS engine asserts RTS high on send; for an
+		 * active-low RTS let IRDA.RTSINVERT invert the output stage.
+		 */
+		if (!(port->rs485.flags & SER_RS485_RTS_ON_SEND))
+			irda = MAX310X_IRDA_RTSINVERT_BIT;
 	}
 
 	max310x_port_write(port, MAX310X_HDPIXDELAY_REG,
@@ -980,6 +987,8 @@ static void max310x_set_rts_ctl_params(struct max310x_one *one)
 
 	max310x_port_update(port, MAX310X_MODE1_REG,
 			    MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
+	max310x_port_update(port, MAX310X_IRDA_REG,
+			    MAX310X_IRDA_RTSINVERT_BIT, irda);
 }
 
 static void max310x_break_ctl(struct uart_port *port, int break_state)
@@ -996,7 +1005,9 @@ static void max310x_break_ctl(struct uart_port *port, int break_state)
 	 * The chip's auto-RTS asserts the transceiver only while FIFO data is
 	 * shifting out, and a break is not FIFO data. Disable auto-RTS for the
 	 * break duration and drive RTS manually so the break reaches the wire;
-	 * restore auto-RTS when the break ends.
+	 * restore auto-RTS when the break ends. For an active-low RTS,
+	 * IRDA.RTSINVERT already inverts the RTS_ output stage, so break_state
+	 * is driven as it is.
 	 */
 	if (break_state) {
 		if (!(port->rs485.flags & SER_RS485_ENABLED))
@@ -1416,7 +1427,8 @@ static int max310x_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
 #endif
 
 static const struct serial_rs485 max310x_rs485_supported = {
-	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RX_DURING_TX,
+	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
+		 SER_RS485_RTS_AFTER_SEND | SER_RS485_RX_DURING_TX,
 	.delay_rts_before_send = 1,
 	.delay_rts_after_send = 1,
 };

-- 
2.47.3

Re: [PATCH v8 6/9] serial: max310x: support active-low RTS on the hardware path

[sashiko-bot]

> The chip's auto-RTS engine asserts the RTS_ pin high while data is
> shifting out, so a transceiver with an active-low driver-enable could
> not use the hardware RS485 path at all: SER_RS485_RTS_AFTER_SEND is
> not in the supported flags and the core normalizes it away with
> "invalid RTS setting, using RTS_ON_SEND instead".
> 
> The output stage is invertible: program IRDA.RTSINVERT when the
> requested polarity is active-low and advertise SER_RS485_RTS_AFTER_SEND
> in rs485_supported. A break already drives break_state onto the RTS
> bit unadjusted, which remains correct because RTSINVERT inverts the
> output stage itself, not the register value.
> 
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-max310x-rs485-sw-delay-v8-0-6ba881f81e1f@vaisala.com?part=6

[PATCH v8 7/9] serial: max310x: schedule tx_work directly from the IRQ handler

[Tapio Reijonen]

The TXEMPTY interrupt asks for a FIFO refill, and start_tx() does
nothing beyond scheduling tx_work, so going through it makes no
functional difference. It does conflate two distinct events, though:
start_tx() is the serial core starting a new transmission, while
TXEMPTY can only fire for a transmission that is already running -
the bit latches on the FIFO's non-empty to empty transition, and the
IRQ handler's read of IRQSTS consumes the latch, so a stale TXEMPTY
cannot exist on an idle port (if the bit were level-triggered, the
handler's read-until-clear loop would never terminate).

Schedule tx_work directly, keeping the interrupt path a pure FIFO
refill. This is preparation for a following patch that adds
software-timed RS485 RTS control, where start_tx() also starts the
RTS envelope and a refill must not restart it.

No functional change.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 0c40283f44ba1df712607218aef24cb642ba3918..c1fe4ad8878392613cbe4d7805eb8ecb454176d6 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -858,8 +858,12 @@ static irqreturn_t max310x_port_irq(struct max310x_port *s, int portno)
 		}
 		if (rxlen)
 			max310x_handle_rx(port, rxlen);
+		/*
+		 * TXEMPTY latches on the FIFO becoming empty, so a stale
+		 * interrupt cannot pump data during an RTS setup delay.
+		 */
 		if (ists & MAX310X_IRQ_TXEMPTY_BIT)
-			max310x_start_tx(port);
+			schedule_work(&s->p[portno].tx_work);
 	} while (1);
 
 	return res;

-- 
2.47.3

Re: [PATCH v8 7/9] serial: max310x: schedule tx_work directly from the IRQ handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tty: max310x: Use-After-Free of workqueues in probe error path and remove
--

commit 6bfa220718efcbb6292bc06e4e18a373318c9c1a
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>

serial: max310x: schedule tx_work directly from the IRQ handler

This commit schedules tx_work directly from the IRQ handler instead of going
through start_tx(). This keeps the interrupt path as a pure FIFO refill in
preparation for a following patch.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index 0c40283f44ba1..c1fe4ad887839 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
> @@ -858,8 +858,12 @@ static irqreturn_t max310x_port_irq(struct max310x_port *s, int portno)
>  		}
>  		if (rxlen)
>  			max310x_handle_rx(port, rxlen);
> +		/*
> +		 * TXEMPTY latches on the FIFO becoming empty, so a stale
> +		 * interrupt cannot pump data during an RTS setup delay.
> +		 */
>  		if (ists & MAX310X_IRQ_TXEMPTY_BIT)
> -			max310x_start_tx(port);
> +			schedule_work(&s->p[portno].tx_work);
>  	} while (1);
>  
>  	return res;

[Severity: High]
Does this code allow delayed works to outlive the driver's memory?

In max310x_remove(), works are cancelled before uart_remove_one_port():

drivers/tty/serial/max310x.c:max310x_remove() {
	for (i = 0; i < s->devtype->nr; i++) {
		cancel_work_sync(&s->p[i].tx_work);
		cancel_work_sync(&s->p[i].md_work);
		cancel_work_sync(&s->p[i].rs_work);

		if (test_and_clear_bit(s->p[i].port.line, max310x_lines))
			uart_remove_one_port(&max310x_uart, &s->p[i].port);
		...
	}
}

If the port is open, removing the port invokes a vhangup, dropping DTR/RTS via
uart_shutdown() -> uart_port_dtr_rts(0) -> max310x_set_mctrl().

This synchronously re-schedules md_work. Since it was already cancelled
earlier, it is left pending while max310x_remove() completes and devres
frees the max310x_port structure, resulting in a use-after-free.

Similarly, the probe error path completely fails to cancel works before devres
frees the structure:

drivers/tty/serial/max310x.c:max310x_probe() {
...
out_uart:
	for (i = 0; i < devtype->nr; i++) {
		if (test_and_clear_bit(s->p[i].port.line, max310x_lines))
			uart_remove_one_port(&max310x_uart, &s->p[i].port);
	}
...
}

This means md_work and rs_work scheduled by uart_add_one_port() will execute
after devres cleanup on probe failures.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-max310x-rs485-sw-delay-v8-0-6ba881f81e1f@vaisala.com?part=7

[PATCH v8 8/9] serial: max310x: drive RTS in software when hardware delays are too short

[Tapio Reijonen]

The chip's auto-RTS path can express at most 15 bit-times of RTS setup
and hold delay - a fraction of a millisecond at typical baud rates -
while the uapi expresses the delays in milliseconds up to the UART
core's RS485_MAX_RTS_DELAY. Requests beyond the field were rejected
with -ERANGE, which makes the core silently wipe port->rs485 and
disable RS485; a device tree asking for a 20 ms setup delay ends up
with no RS485 at all and an unusable bus.

Add a software-driven RTS path that takes over whenever the hardware
cannot represent the requested timing, and clamp the delays to
RS485_MAX_RTS_DELAY instead of rejecting them.

max310x_set_rts_ctl_params() picks the path: software if either delay
exceeds what 15 bit-times encode at the current baud rate, or if the
requested RTS polarity combination cannot be produced by the auto-RTS
engine; hardware otherwise, as before.

On the software path the RTS envelope is driven by a single hrtimer,
re-used for the before- and after-send phases (tracked in tx_state),
plus a single rts_work that toggles RTS. start_tx() begins the
envelope; rts_work asserts RTS and only then arms the before-send
timer, so data is never shifted before RTS is on the wire. The timer
expiry kicks tx_work; once the chip FIFO drains, the same timer is
re-armed for one character (the byte still in the shift register)
plus the after-send delay, after which rts_work releases RTS. One
timer and one work keep the phases mutually exclusive and the RTS
toggles ordered. A start_tx() landing while the envelope is already in
its send phase only pumps the new data: rewinding it to the
before-send phase would insert a spurious setup delay mid-stream.

Teardown is interlocked: shutdown() and an RS485-disabling
TIOCSRS485 set tx_teardown under port->lock before cancelling the
timer and works, and start_tx() checks it on entry and again after
the hrtimer_try_to_cancel(-1) path retakes the dropped lock -
otherwise a write racing the teardown could re-arm the timer or
queue rts_work against a port being shut down, leaving the
transceiver driving the bus after close. The rs485-disable path
additionally kicks tx_work afterwards, since a racing write may have
queued data with no envelope left to pump it, and shutdown() now also
cancels tx_work, which was previously cancelled only in remove(). The
interlock also fences the adoption path below, so a reconfigure racing
a teardown cannot re-enable the cancelled timer, and an RS485 disable
flushes a queued rts_work before the pin is settled: one already past
its tx_state read would re-assert RTS after the settle.

set_rts_ctl_params() publishes sw_rts_during_tx with a single store
and settles the RTS idle level only while tx_state is off, because
serial_core calls set_termios() without port->lock and rs485_config()
schedules a reconfigure on every TIOCSRS485 - either could otherwise
release the transceiver mid-envelope. The settle also re-checks
tx_state after its write and requeues rts_work if an envelope started
meanwhile: the state read and the register write are not atomic, and
rts_work re-derives the level from tx_state, so this converges without
locking. shutdown() gates first - the
interlock is set and the interrupts are masked before any wait, so
nothing can start a new envelope or requeue tx_work behind the cancels
- then honours a running after-send hold, bounded by the after-send
delay plus two character times (the transmitter is already stopped by
this point), and cancels the timer and works unconditionally: a
TIOCSRS485 can clear sw_rts_during_tx while an envelope is still in
flight, and neither may outlive the port. A hold that did not complete
within the bound is settled by the final release below regardless.
break_ctl() on the software path applies the
configured RTS polarity itself.

A reconfigure can also move the port off the hardware path while the
chip's auto-RTS still owns a transmission in flight - a termios change
or TIOCSRS485 pushing a delay above what the hardware can time at the
new rate. Such transfers are invisible to tx_state, so the idle settle
would release RTS and the MODE1 write would disable auto-RTS
mid-transfer, shifting the rest of the data out with the transceiver
released and no error reported anywhere. Adopt the transfer instead
when the chip TX FIFO or the xmit buffer is not empty: enter
MAX310X_TX_SEND and drive RTS manually before auto-RTS is disabled, so
the pin is never released across the handover, and the normal drain
path arms the after-send hold - or the adoption arms it directly if
the FIFO drained while this raced the TX-empty worker. A transfer
whose last character has already left the FIFO for the transmit shift
register is still invisible and keeps the old behavior: at most one
character of early release.

The reverse move strands state instead: a reconfigure onto the
hardware path mid-envelope left tx_state set, because nothing reset it
in that direction and the TX-empty hold arming only ran while the
software path was selected. A later reconfigure back off the hardware
path then trusted the stale tx_state, skipped both the adoption and
the idle settle, and disabled auto-RTS with the RTS register released
- the rest of the transfer in flight shifted out with the transceiver
off, again with no error reported (hit twice in 79 in-flight
reconfigures under a multi-process stress test). The TX-empty unwind
now runs regardless of the selected path - it is a no-op outside an
envelope - and the reconfigure asserts RTS for any transfer it
believes exists, stale or live, before auto-RTS is disabled. The hold
is armed only once no transmittable data remains, so that assert
cannot cut a burst short when it lands in a FIFO refill window;
output that is stopped still releases through the hold.

rts_work also derives the level from tx_break: a break can begin while
the previous envelope's after-send hold is still armed - TIOCSBRK waits
for the output to drain, which lands the break exactly there - and the
expiring hold would otherwise release the transceiver mid-break. With
break ownership in the derivation the hold expiry becomes an idempotent
re-assert, and break-off restores the idle state as it already does.

shutdown() gives its final RTS release one bit time, and at
least 100 us, to reach the pin before powering the port down. The
power-off stops the UART channel clock, and the RTS output stage is
clocked by it: a release written immediately before the clock stops
reaches the register but never moves the pin, which stays asserted
until the next startup restarts the clock - a close racing the
after-send hold expiry left the transceiver driving the bus, with the
register reading released and the pin high. The measured propagation
is about one tick of the 16x oversampling clock - below 50 us at
1200 baud - so one bit time keeps a 16x margin and scales with the
clock the output stage runs on; the floor covers rates where a bit is
shorter than the measured bound. Handing the pin to the
chip's auto-RTS engine instead was tried and rejected on the wire:
the pin level is RTSINVERT xor the selected source, so for an
active-low RTS every single-register step of that handover drives the
pin to the asserted level, and the same clock stop can freeze it
there.

Fixes: 55367c620aed ("serial: max310x: Add support for RS-485 mode")
Assisted-by: Claude:claude-fable-5
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 488 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 436 insertions(+), 52 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index c1fe4ad8878392613cbe4d7805eb8ecb454176d6..5fdb9dfca6027ff0a9284ed994000a5d7587408c 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -15,6 +15,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/gpio/driver.h>
+#include <linux/hrtimer.h>
 #include <linux/i2c.h>
 #include <linux/kconfig.h>
 #include <linux/module.h>
@@ -297,15 +298,29 @@ struct max310x_devtype {
 	u8	power_bit; /* Bit for sleep or power-off mode (active high). */
 };
 
+/* Software-timed RS485 RTS envelope phase */
+enum max310x_tx_state {
+	MAX310X_TX_OFF,			/* idle, RTS released */
+	MAX310X_TX_WAIT_BEFORE_SEND,	/* RTS asserted, before-send delay */
+	MAX310X_TX_SEND,		/* data in flight, awaiting TX-empty */
+	MAX310X_TX_WAIT_AFTER_SEND,	/* data drained, after-send hold */
+};
+
 struct max310x_one {
 	struct uart_port	port;
 	struct work_struct	tx_work;
 	struct work_struct	md_work;
 	struct work_struct	rs_work;
+	struct work_struct	rts_work;
+	struct hrtimer		tx_delay_tmr;
 	struct regmap		*regmap;
 	unsigned int		char_time_us;
 	unsigned int		baud;
+	bool			sw_rts_during_tx;
+	bool			cancel_tx_delay_tmr;
+	bool			tx_teardown;	/* envelope being torn down */
 	bool			tx_break;	/* break_ctl() owns the transceiver */
+	enum max310x_tx_state	tx_state;
 
 	u8 rx_buf[MAX310X_FIFO_SIZE];
 };
@@ -696,6 +711,38 @@ static void max310x_rts_ctl(struct uart_port *port, bool rts_state)
 			    rts_state ? MAX310X_LCR_RTS_BIT : 0);
 }
 
+/* RTS level for the transmitting or the idle phase of an RS485 envelope */
+static bool max310x_rts_level(struct uart_port *port, bool active)
+{
+	return active ? (port->rs485.flags & SER_RS485_RTS_ON_SEND) :
+			(port->rs485.flags & SER_RS485_RTS_AFTER_SEND);
+}
+
+/*
+ * Drive the RS485 RTS line to match the current tx_state and break
+ * ownership. This is the only place that touches RTS, and it reads the
+ * state rather than a fixed assert/deassert intent, so a newer assert is
+ * never clobbered by a stale release and an expiring after-send hold never
+ * releases a break in progress. It also arms the before-send timer once the
+ * RTS edge is on the wire, so data is never shifted before RTS is asserted.
+ */
+static void max310x_rts_work_proc(struct work_struct *ws)
+{
+	struct max310x_one *one = container_of(ws, struct max310x_one, rts_work);
+	struct uart_port *port = &one->port;
+	bool rts_on = READ_ONCE(one->tx_state) != MAX310X_TX_OFF ||
+		      READ_ONCE(one->tx_break);
+
+	max310x_rts_ctl(port, max310x_rts_level(port, rts_on));
+
+	guard(spinlock_irqsave)(&port->lock);
+	if (READ_ONCE(one->tx_state) == MAX310X_TX_WAIT_BEFORE_SEND &&
+	    !one->cancel_tx_delay_tmr && !hrtimer_active(&one->tx_delay_tmr))
+		hrtimer_start(&one->tx_delay_tmr,
+			      ms_to_ktime(port->rs485.delay_rts_before_send),
+			      HRTIMER_MODE_REL);
+}
+
 static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
 {
 	struct max310x_one *one = to_max310x_port(port);
@@ -792,6 +839,77 @@ static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
 	tty_flip_buffer_push(&port->state->port);
 }
 
+static enum hrtimer_restart max310x_tmr_tx(struct hrtimer *timer)
+{
+	struct max310x_one *one = container_of(timer, struct max310x_one,
+					       tx_delay_tmr);
+
+	guard(spinlock_irqsave)(&one->port.lock);
+	if (!one->cancel_tx_delay_tmr) {
+		if (READ_ONCE(one->tx_state) == MAX310X_TX_WAIT_AFTER_SEND) {
+			/* After-send hold elapsed: drop RTS via the rts worker. */
+			WRITE_ONCE(one->tx_state, MAX310X_TX_OFF);
+			schedule_work(&one->rts_work);
+		} else {
+			WRITE_ONCE(one->tx_state, MAX310X_TX_SEND);
+			schedule_work(&one->tx_work);
+		}
+	}
+
+	return HRTIMER_NORESTART;
+}
+
+static void max310x_delayed_stop_tx(struct uart_port *port)
+{
+	struct max310x_one *one = to_max310x_port(port);
+	unsigned int txlvl;
+
+	if (READ_ONCE(one->tx_state) == MAX310X_TX_OFF)
+		return;
+
+	/*
+	 * Data still queued for transmission defers the hold: a refill is
+	 * coming and the envelope is not over. Stopped output does not
+	 * count - it drains nowhere, and the envelope must end.
+	 */
+	if (!kfifo_is_empty(&port->state->port.xmit_fifo) &&
+	    !uart_tx_stopped(port))
+		return;
+
+	/*
+	 * The kfifo can be empty while the chip TX FIFO is still draining, so arm
+	 * the after-send hold only once the chip FIFO is empty too - the TX-empty
+	 * interrupt re-invokes us then. Otherwise the hold starts early and RTS
+	 * drops mid-character, clipping the last byte(s).
+	 */
+	txlvl = max310x_port_read(port, MAX310X_TXFIFOLVL_REG);
+	if (txlvl)
+		return;
+
+	/*
+	 * Runs from tx_work without port->lock, so re-check the state under it:
+	 * shutdown() may have cancelled the envelope meanwhile. Only
+	 * MAX310X_TX_SEND may arm the hold.
+	 */
+	guard(spinlock_irqsave)(&one->port.lock);
+	if (one->cancel_tx_delay_tmr ||
+	    READ_ONCE(one->tx_state) != MAX310X_TX_SEND)
+		return;
+
+	if (!hrtimer_active(&one->tx_delay_tmr)) {
+		/*
+		 * Add one character for the byte still in the shift register -
+		 * TX-empty fires as it enters, not as it leaves.
+		 */
+		ktime_t delay = us_to_ktime(one->char_time_us +
+					    port->rs485.delay_rts_after_send *
+					    USEC_PER_MSEC);
+
+		WRITE_ONCE(one->tx_state, MAX310X_TX_WAIT_AFTER_SEND);
+		hrtimer_start(&one->tx_delay_tmr, delay, HRTIMER_MODE_REL);
+	}
+}
+
 static void max310x_handle_tx(struct uart_port *port)
 {
 	struct tty_port *tport = &port->state->port;
@@ -803,8 +921,16 @@ static void max310x_handle_tx(struct uart_port *port)
 		return;
 	}
 
-	if (kfifo_is_empty(&tport->xmit_fifo) || uart_tx_stopped(port))
+	if (kfifo_is_empty(&tport->xmit_fifo) || uart_tx_stopped(port)) {
+		/*
+		 * Unwind the envelope even if a reconfigure has moved the
+		 * port to the hardware path mid-envelope: a stranded
+		 * tx_state would make a later reconfigure believe a
+		 * software envelope still owns RTS.
+		 */
+		max310x_delayed_stop_tx(port);
 		return;
+	}
 
 	/*
 	 * It's a circ buffer -- wrap around.
@@ -829,11 +955,64 @@ static void max310x_handle_tx(struct uart_port *port)
 		uart_write_wakeup(port);
 }
 
+/*
+ * Begin a software-timed RTS envelope: set the before-send phase and queue the
+ * rts worker to assert RTS. tx_state is set synchronously here (start_tx() holds
+ * port.lock) so close()/shutdown can see an envelope is in flight; rts_work then
+ * asserts RTS and arms the before-send timer (see there).
+ */
+static void max310x_delayed_start_tx(struct uart_port *port)
+{
+	struct max310x_one *one = to_max310x_port(port);
+
+	WRITE_ONCE(one->tx_state, MAX310X_TX_WAIT_BEFORE_SEND);
+	one->cancel_tx_delay_tmr = false;
+	schedule_work(&one->rts_work);
+}
+
+/* called with port.lock taken and irqs off */
 static void max310x_start_tx(struct uart_port *port)
 {
 	struct max310x_one *one = to_max310x_port(port);
 
-	schedule_work(&one->tx_work);
+	/* A teardown is in progress; nothing may start an envelope or TX. */
+	if (one->tx_teardown)
+		return;
+
+	if (READ_ONCE(one->sw_rts_during_tx)) {
+		/*
+		 * The before- and after-send phases share one delay timer. If an
+		 * after-send release is pending, cancel it before starting a new
+		 * TX so the just-asserted RTS is not yanked; re-arming the timer
+		 * for the before-send phase then supersedes the release.
+		 */
+		int res = 0;
+
+		if (READ_ONCE(one->tx_state) == MAX310X_TX_WAIT_AFTER_SEND)
+			res = hrtimer_try_to_cancel(&one->tx_delay_tmr);
+		if (unlikely(res == -1)) {
+			one->cancel_tx_delay_tmr = true;
+			uart_port_unlock(port);
+			hrtimer_cancel(&one->tx_delay_tmr);
+			uart_port_lock(port);
+			/*
+			 * The lock was dropped: a teardown may have run to
+			 * completion meanwhile. Re-check before starting.
+			 */
+			if (one->tx_teardown)
+				return;
+		}
+
+		/* Already sending: pump the new data, don't rewind. */
+		if (READ_ONCE(one->tx_state) == MAX310X_TX_SEND) {
+			schedule_work(&one->tx_work);
+			return;
+		}
+
+		max310x_delayed_start_tx(port);
+	} else {
+		schedule_work(&one->tx_work);
+	}
 }
 
 static irqreturn_t max310x_port_irq(struct max310x_port *s, int portno)
@@ -946,10 +1125,39 @@ static void max310x_set_mctrl(struct uart_port *port, unsigned int mctrl)
 }
 
 /*
- * Program the chip's RS485 RTS timing. The HDPIXDELAY setup and hold fields
- * count bit-times, four bits per direction, while the uapi expresses the
- * delays in milliseconds: convert at the current baud rate, rounding up, and
- * cap at the field maximum.
+ * A reconfigure can move the port off the hardware RTS path while the chip's
+ * auto-RTS still owns a transmission in flight; tx_state does not track
+ * those. Take such a transfer over before auto-RTS is disabled: enter
+ * MAX310X_TX_SEND, so the caller keeps RTS driven across the handover and
+ * the normal drain path arms the after-send hold. Returns true when a
+ * transmission owns RTS - adopted here or started concurrently - and false
+ * when the line is really idle.
+ */
+static bool max310x_adopt_hw_tx(struct max310x_one *one)
+{
+	struct uart_port *port = &one->port;
+
+	if (!max310x_port_read(port, MAX310X_TXFIFOLVL_REG) &&
+	    kfifo_is_empty(&port->state->port.xmit_fifo))
+		return false;
+
+	scoped_guard(spinlock_irqsave, &port->lock) {
+		if (one->tx_teardown)
+			return false;
+		if (READ_ONCE(one->tx_state) != MAX310X_TX_OFF)
+			return true;
+		WRITE_ONCE(one->tx_state, MAX310X_TX_SEND);
+		one->cancel_tx_delay_tmr = false;
+	}
+
+	return true;
+}
+
+/*
+ * Pick hardware or software RTS timing for the current port. The chip can
+ * deliver up to 15 bit-times of setup/hold delay via HDPIXDELAY; anything
+ * longer (or any RTS polarity the chip cannot produce automatically) must
+ * be driven by software via tx_delay_tmr and rts_work.
  */
 static void max310x_set_rts_ctl_params(struct max310x_one *one)
 {
@@ -957,25 +1165,35 @@ static void max310x_set_rts_ctl_params(struct max310x_one *one)
 	struct uart_port *port = &one->port;
 	unsigned int setup = 0, hold = 0;
 	u8 mode1 = 0, irda = 0;
+	bool sw_rts = false;
 
-	if (port->rs485.flags & SER_RS485_ENABLED) {
-		/* Convert milliseconds to bit-times, rounding up. */
-		setup = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_before_send,
-				     MSEC_PER_SEC);
-		hold  = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_after_send,
-				     MSEC_PER_SEC);
-		setup = min(setup, max_bit_dly);
-		hold  = min(hold,  max_bit_dly);
-
-		mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
-		/*
-		 * The auto-RTS engine asserts RTS high on send; for an
-		 * active-low RTS let IRDA.RTSINVERT invert the output stage.
-		 */
-		if (!(port->rs485.flags & SER_RS485_RTS_ON_SEND))
-			irda = MAX310X_IRDA_RTSINVERT_BIT;
+	if (!(port->rs485.flags & SER_RS485_ENABLED))
+		goto out;
+
+	/* Convert milliseconds to bit-times, rounding up. */
+	setup = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_before_send,
+			     MSEC_PER_SEC);
+	hold  = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_after_send,
+			     MSEC_PER_SEC);
+
+	/*
+	 * Compare in bit-times: a delay of exactly max_bit_dly bit-times
+	 * must stay on the hardware path, which a nanosecond ceiling
+	 * computed from the truncated per-bit time would reject. Without
+	 * a baud rate the conversion is meaningless - use software.
+	 */
+	if (!one->baud || setup > max_bit_dly || hold > max_bit_dly ||
+	    !!(port->rs485.flags & SER_RS485_RTS_ON_SEND) ==
+	    !!(port->rs485.flags & SER_RS485_RTS_AFTER_SEND)) {
+		sw_rts = true;
+		setup = 0;
+		hold  = 0;
 	}
 
+out:
+	/* Assign once; a transient false would be seen by other readers. */
+	WRITE_ONCE(one->sw_rts_during_tx, sw_rts);
+
 	max310x_port_write(port, MAX310X_HDPIXDELAY_REG,
 			   MAX310X_HDPIXDELAY_SETUP(setup) |
 			   MAX310X_HDPIXDELAY_HOLD(hold));
@@ -983,12 +1201,66 @@ static void max310x_set_rts_ctl_params(struct max310x_one *one)
 	/*
 	 * A break owns the transceiver: break_ctl() disabled auto-RTS and
 	 * drives RTS manually, and restores it from the current
-	 * configuration when the break ends. Touching MODE1 here would
+	 * configuration when the break ends. Touching them here would
 	 * release the transceiver mid-break.
 	 */
 	if (one->tx_break)
 		return;
 
+	if (port->rs485.flags & SER_RS485_ENABLED) {
+		if (sw_rts) {
+			/*
+			 * Only settle RTS at idle when no transmission owns it.
+			 * A reconfigure while one is in flight - rs_work runs
+			 * on every TIOCSRS485 - would otherwise release the
+			 * transceiver mid-character. A transfer the hardware
+			 * path owns is invisible to tx_state - adopt it
+			 * instead of settling. The reverse strand also
+			 * exists: an envelope the hardware path inherited
+			 * mid-envelope leaves tx_state set with nothing
+			 * driving the pin.
+			 */
+			if (READ_ONCE(one->tx_state) != MAX310X_TX_OFF ||
+			    max310x_adopt_hw_tx(one)) {
+				/*
+				 * The transfer may be running under auto-RTS:
+				 * assert before auto-RTS is disabled below so
+				 * the pin is never released under it, and arm
+				 * the after-send hold - with the chip FIFO
+				 * already drained no TX-empty interrupt
+				 * arrives to arm it.
+				 */
+				max310x_rts_ctl(port,
+						max310x_rts_level(port, true));
+				max310x_delayed_stop_tx(port);
+			} else {
+				max310x_rts_ctl(port,
+						max310x_rts_level(port, false));
+				/*
+				 * serial_core calls set_termios() without
+				 * port->lock, so an envelope may have started
+				 * while the idle level was written and the
+				 * settle can land after its RTS assert.
+				 * rts_work re-derives the level from
+				 * tx_state; requeue it to converge.
+				 */
+				if (READ_ONCE(one->tx_state) != MAX310X_TX_OFF)
+					schedule_work(&one->rts_work);
+			}
+		} else {
+			mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
+			/*
+			 * The auto-RTS engine asserts RTS high on send; for an
+			 * active-low RTS let IRDA.RTSINVERT invert the output
+			 * stage.
+			 */
+			if (!(port->rs485.flags & SER_RS485_RTS_ON_SEND))
+				irda = MAX310X_IRDA_RTSINVERT_BIT;
+		}
+	} else {
+		max310x_rts_ctl(port, 0);
+	}
+
 	max310x_port_update(port, MAX310X_MODE1_REG,
 			    MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
 	max310x_port_update(port, MAX310X_IRDA_REG,
@@ -1016,12 +1288,27 @@ static void max310x_break_ctl(struct uart_port *port, int break_state)
 	if (break_state) {
 		if (!(port->rs485.flags & SER_RS485_ENABLED))
 			return;
-		max310x_port_update(port, MAX310X_MODE1_REG,
-				    MAX310X_MODE1_TRNSCVCTRL_BIT, 0);
+		if (READ_ONCE(one->sw_rts_during_tx)) {
+			max310x_rts_ctl(port, max310x_rts_level(port, 1));
+		} else {
+			max310x_port_update(port, MAX310X_MODE1_REG,
+					    MAX310X_MODE1_TRNSCVCTRL_BIT, 0);
+			max310x_rts_ctl(port, 1);
+		}
 	} else {
+		/*
+		 * Reapply the current configuration: a reconfigure that
+		 * arrived during the break was deferred by the tx_break
+		 * guard, and RS485 may have been disabled outright - the
+		 * manually driven RTS must not leak past the break. On the
+		 * software path the helper also settles the idle level; on
+		 * the hardware path release the manual RTS - auto-RTS owns
+		 * the pin again.
+		 */
 		max310x_set_rts_ctl_params(one);
+		if (!READ_ONCE(one->sw_rts_during_tx))
+			max310x_rts_ctl(port, 0);
 	}
-	max310x_rts_ctl(port, break_state);
 }
 
 static void max310x_set_termios(struct uart_port *port,
@@ -1063,9 +1350,10 @@ static void max310x_set_termios(struct uart_port *port,
 		lcr |= MAX310X_LCR_STOPLEN_BIT; /* 2 stops */
 
 	/*
-	 * Update LCR register. Leave the TX break bit alone: it is driven by
-	 * break_ctl(), and a whole-register write here would end a break in
-	 * progress.
+	 * Update LCR register. Leave the TX break and RTS bits alone: they are
+	 * driven by break_ctl() and by the software-timed RS485 RTS, and a
+	 * whole-register write here would end a break in progress or release
+	 * the transceiver mid-character.
 	 */
 	max310x_port_update(port, MAX310X_LCR_REG, MAX310X_LCR_TERMIOS_MASK, lcr);
 
@@ -1143,7 +1431,9 @@ static void max310x_set_termios(struct uart_port *port,
 	/*
 	 * Cache the new baud rate and the time it takes to clock out one
 	 * character before reprogramming the RS485 RTS delays: the
-	 * millisecond-to-bit-time conversion divides by the baud rate.
+	 * millisecond-to-bit-time conversion divides by the baud rate, and
+	 * taking over an in-flight transfer arms the after-send hold from
+	 * the character time.
 	 */
 	to_max310x_port(port)->baud = baud;
 	to_max310x_port(port)->char_time_us =
@@ -1163,6 +1453,10 @@ static void max310x_rs_proc(struct work_struct *ws)
 	 */
 	guard(mutex)(&one->port.state->port.mutex);
 
+	/* Flush an rts_work that read tx_state before the disable cleared it. */
+	if (!(one->port.rs485.flags & SER_RS485_ENABLED))
+		cancel_work_sync(&one->rts_work);
+
 	max310x_set_rts_ctl_params(one);
 
 	if (one->port.rs485.flags & SER_RS485_ENABLED &&
@@ -1173,14 +1467,37 @@ static void max310x_rs_proc(struct work_struct *ws)
 			    MAX310X_MODE2_ECHOSUPR_BIT, mode2);
 }
 
+/* called with port.lock taken and irqs off */
 static int max310x_rs485_config(struct uart_port *port, struct ktermios *termios,
 				struct serial_rs485 *rs485)
 {
 	struct max310x_one *one = to_max310x_port(port);
 
-	if ((rs485->delay_rts_before_send > 0x0f) ||
-	    (rs485->delay_rts_after_send > 0x0f))
-		return -ERANGE;
+	rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
+	rs485->delay_rts_after_send  = min(rs485->delay_rts_after_send,  100U);
+
+	/*
+	 * Make sure no SW-timed RTS toggle survives an RS485 disable, even
+	 * if the delay timer happens to be running right now.
+	 */
+	if (!(rs485->flags & SER_RS485_ENABLED)) {
+		one->tx_teardown = true;
+		one->cancel_tx_delay_tmr = true;
+		if (hrtimer_try_to_cancel(&one->tx_delay_tmr) == -1) {
+			uart_port_unlock(port);
+			hrtimer_cancel(&one->tx_delay_tmr);
+			uart_port_lock(port);
+		}
+		WRITE_ONCE(one->tx_state, MAX310X_TX_OFF);
+		one->tx_teardown = false;
+		/*
+		 * The port stays alive, and a write that raced the teardown
+		 * may have left data queued with no envelope left to pump it.
+		 * Kick tx_work; RS485 is disabled, so the plain path is right.
+		 */
+		if (!kfifo_is_empty(&port->state->port.xmit_fifo))
+			schedule_work(&one->tx_work);
+	}
 
 	port->rs485 = *rs485;
 
@@ -1215,7 +1532,15 @@ static int max310x_startup(struct uart_port *port)
 	max310x_port_update(port, MAX310X_MODE2_REG,
 			    MAX310X_MODE2_FIFORST_BIT, 0);
 
-	/* Configure the RS485 RTS timing and the RS485/RS232 mode bits. */
+	one->tx_teardown = false;
+
+	/*
+	 * Configure the RS485 RTS timing (HW auto-RTS vs software-driven) and
+	 * the RS485/RS232 mode bits. Don't hardcode HW auto-RTS here - let
+	 * max310x_set_rts_ctl_params() pick HW or SW per the configured
+	 * delays, otherwise the chip's auto-RTS would override the software
+	 * RTS hold and the after-send delay is lost.
+	 */
 	max310x_set_rts_ctl_params(one);
 
 	if (port->rs485.flags & SER_RS485_ENABLED &&
@@ -1246,7 +1571,15 @@ static void max310x_shutdown(struct uart_port *port)
 {
 	struct max310x_one *one = to_max310x_port(port);
 
-	/* Disable all interrupts */
+	/*
+	 * Gate new transmissions and interrupts first: a concurrent
+	 * start_tx() either sees the interlock or happens-before it, and
+	 * nothing below may start a new envelope. The timer and the works
+	 * stay live for now - they are what completes a running after-send
+	 * hold.
+	 */
+	scoped_guard(spinlock_irqsave, &port->lock)
+		one->tx_teardown = true;
 	max310x_port_write(port, MAX310X_IRQEN_REG, 0);
 
 	/*
@@ -1256,25 +1589,69 @@ static void max310x_shutdown(struct uart_port *port)
 	max310x_port_update(port, MAX310X_MODE1_REG,
 			    MAX310X_MODE1_TXDIS_BIT, MAX310X_MODE1_TXDIS_BIT);
 
+	if (READ_ONCE(one->sw_rts_during_tx)) {
+		/*
+		 * Honour a running after-send hold before the port is
+		 * powered down. The bound covers the final character -
+		 * tx_empty() cannot see the transmit shift register - plus
+		 * the hold itself. Data beyond that is abandoned: it is
+		 * only still queued when the tty layer was told not to
+		 * wait, and the release below drives RTS to idle
+		 * regardless.
+		 */
+		unsigned int tries = port->rs485.delay_rts_after_send +
+			2 * DIV_ROUND_UP(one->char_time_us, USEC_PER_MSEC);
+
+		while (READ_ONCE(one->tx_state) != MAX310X_TX_OFF && tries-- > 0)
+			fsleep(USEC_PER_MSEC);
+	} else {
+		/*
+		 * Let the character in flight finish (the chip has no
+		 * transmitter-idle status), then empty the FIFO: auto-RTS
+		 * releases only once the transmitter is empty, and the
+		 * power-off below would freeze an asserted pin until the
+		 * next open. The hold is at most 15 bit-times.
+		 */
+		fsleep(one->char_time_us);
+		max310x_port_update(port, MAX310X_MODE2_REG,
+				    MAX310X_MODE2_FIFORST_BIT,
+				    MAX310X_MODE2_FIFORST_BIT);
+		max310x_port_update(port, MAX310X_MODE2_REG,
+				    MAX310X_MODE2_FIFORST_BIT, 0);
+		if (port->rs485.flags & SER_RS485_ENABLED && one->baud) {
+			unsigned int hold = DIV_ROUND_UP(one->baud *
+					port->rs485.delay_rts_after_send,
+					MSEC_PER_SEC);
+
+			fsleep(max(DIV_ROUND_UP((hold + 1) * USEC_PER_SEC,
+						one->baud), 100U));
+		}
+	}
+
 	/*
-	 * Let the character in flight finish (the chip has no
-	 * transmitter-idle status), then empty the FIFO: auto-RTS releases
-	 * only once the transmitter is empty, and the power-off below
-	 * would freeze an asserted pin until the next open.
+	 * The wind-down is over: cancel unconditionally. The SW/HW decision
+	 * is recomputed on every reconfigure, so a TIOCSRS485 can clear
+	 * sw_rts_during_tx while an envelope is still in flight, and
+	 * neither the timer nor the works may outlive the port.
 	 */
-	fsleep(one->char_time_us);
-	max310x_port_update(port, MAX310X_MODE2_REG,
-			    MAX310X_MODE2_FIFORST_BIT,
-			    MAX310X_MODE2_FIFORST_BIT);
-	max310x_port_update(port, MAX310X_MODE2_REG,
-			    MAX310X_MODE2_FIFORST_BIT, 0);
-	if (port->rs485.flags & SER_RS485_ENABLED && one->baud) {
-		unsigned int hold = DIV_ROUND_UP(one->baud *
-				port->rs485.delay_rts_after_send,
-				MSEC_PER_SEC);
-
-		fsleep(max(DIV_ROUND_UP((hold + 1) * USEC_PER_SEC,
-					one->baud), 100U));
+	scoped_guard(spinlock_irqsave, &port->lock)
+		one->cancel_tx_delay_tmr = true;
+	cancel_work_sync(&one->tx_work);
+	hrtimer_cancel(&one->tx_delay_tmr);
+	cancel_work_sync(&one->rts_work);
+	WRITE_ONCE(one->tx_state, MAX310X_TX_OFF);
+
+	if (READ_ONCE(one->sw_rts_during_tx)) {
+		max310x_rts_ctl(port, max310x_rts_level(port, false));
+		/*
+		 * The power-off below stops the UART clock that moves the
+		 * RTS pin: released too late, the write reaches the register
+		 * but the pin stays at its old level until the next startup
+		 * restarts the clock. Give the release one bit time (at
+		 * least 100 us) to reach the pin - the measured propagation
+		 * is about one tick of the 16x oversampling clock.
+		 */
+		fsleep(max(one->char_time_us / 10, 100U));
 	}
 
 	max310x_power(port, 0);
@@ -1566,6 +1943,11 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
 		INIT_WORK(&s->p[i].md_work, max310x_md_proc);
 		/* Initialize queue for changing RS485 mode */
 		INIT_WORK(&s->p[i].rs_work, max310x_rs_proc);
+		/* Initialize queue for software-driven RTS toggling */
+		INIT_WORK(&s->p[i].rts_work, max310x_rts_work_proc);
+		hrtimer_setup(&s->p[i].tx_delay_tmr, max310x_tmr_tx,
+			      CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+		s->p[i].tx_state = MAX310X_TX_OFF;
 	}
 
 #ifdef CONFIG_GPIOLIB
@@ -1676,6 +2058,8 @@ static void max310x_remove(struct device *dev)
 	int i;
 
 	for (i = 0; i < s->devtype->nr; i++) {
+		hrtimer_cancel(&s->p[i].tx_delay_tmr);
+		cancel_work_sync(&s->p[i].rts_work);
 		cancel_work_sync(&s->p[i].tx_work);
 		cancel_work_sync(&s->p[i].md_work);
 		cancel_work_sync(&s->p[i].rs_work);

-- 
2.47.3

Re: [PATCH v8 8/9] serial: max310x: drive RTS in software when hardware delays are too short

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] tty: use-after-free in max310x_remove due to premature workqueue cancellation
- [High] tty: lockless kfifo check in max310x_delayed_stop_tx truncates RS485 transmission
- [High] tty: TOCTOU race in max310x_rts_work_proc decouples timer arming from RTS assertion
- [High] tty: unprotected tx_state read in max310x_set_rts_ctl_params drops RTS mid-transmission
- [Medium] tty: trailing hrtimer running state in max310x_delayed_stop_tx strands RTS
- [Medium] tty: TOCTOU in max310x_rts_work_proc clobbers break_ctl RTS assertion
--

commit 05eccd91a3e45c1a75cc25465960c12d3c83b68c
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>

serial: max310x: drive RTS in software when hardware delays are too short
    
The commit adds a software-driven RTS path for max310x when requested RS485
delays exceed the hardware's 15 bit-time limit. It uses an hrtimer and a
workqueue to track before-send and after-send RTS envelope phases and
handle teardown interlocking.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index c1fe4ad887839..5fdb9dfca6027 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c

[ ... ]

> +static void max310x_rts_work_proc(struct work_struct *ws)
> +{
> +	struct max310x_one *one = container_of(ws, struct max310x_one, rts_work);
> +	struct uart_port *port = &one->port;
> +	bool rts_on = READ_ONCE(one->tx_state) != MAX310X_TX_OFF ||
> +		      READ_ONCE(one->tx_break);
> +
> +	max310x_rts_ctl(port, max310x_rts_level(port, rts_on));

[Severity: Medium]
Can this sequence clobber a concurrent break_ctl() RTS assertion?

A concurrent max310x_break_ctl() sets tx_break and manually asserts the RTS
pin. If this function evaluates tx_break as false locklessly, gets preempted
while break_ctl() asserts RTS, and then resumes, it could write its stale
'false' evaluation to the hardware and drop the RTS pin prematurely.

> +
> +	guard(spinlock_irqsave)(&port->lock);
> +	if (READ_ONCE(one->tx_state) == MAX310X_TX_WAIT_BEFORE_SEND &&
> +	    !one->cancel_tx_delay_tmr && !hrtimer_active(&one->tx_delay_tmr))
> +		hrtimer_start(&one->tx_delay_tmr,
> +			      ms_to_ktime(port->rs485.delay_rts_before_send),
> +			      HRTIMER_MODE_REL);
> +}

[Severity: High]
Does this lockless read decouple the timer arming from the physical RTS pin
assertion?

If a concurrent start_tx() sets tx_state to MAX310X_TX_WAIT_BEFORE_SEND and
queues a new rts_work after the lockless evaluation of rts_on but before
taking port->lock, the timer could be armed while the pin is still
deasserted, shortening the setup delay.

[ ... ]

> +static void max310x_delayed_stop_tx(struct uart_port *port)
> +{
> +	struct max310x_one *one = to_max310x_port(port);
> +	unsigned int txlvl;
> +
> +	if (READ_ONCE(one->tx_state) == MAX310X_TX_OFF)
> +		return;
> +
> +	/*
> +	 * Data still queued for transmission defers the hold: a refill is
> +	 * coming and the envelope is not over. Stopped output does not
> +	 * count - it drains nowhere, and the envelope must end.
> +	 */
> +	if (!kfifo_is_empty(&port->state->port.xmit_fifo) &&
> +	    !uart_tx_stopped(port))
> +		return;
> +
> +	/*
> +	 * The kfifo can be empty while the chip TX FIFO is still draining, so arm
> +	 * the after-send hold only once the chip FIFO is empty too - the TX-empty
> +	 * interrupt re-invokes us then. Otherwise the hold starts early and RTS
> +	 * drops mid-character, clipping the last byte(s).
> +	 */
> +	txlvl = max310x_port_read(port, MAX310X_TXFIFOLVL_REG);
> +	if (txlvl)
> +		return;
> +
> +	/*
> +	 * Runs from tx_work without port->lock, so re-check the state under it:
> +	 * shutdown() may have cancelled the envelope meanwhile. Only
> +	 * MAX310X_TX_SEND may arm the hold.
> +	 */
> +	guard(spinlock_irqsave)(&one->port.lock);
> +	if (one->cancel_tx_delay_tmr ||
> +	    READ_ONCE(one->tx_state) != MAX310X_TX_SEND)
> +		return;

[Severity: High]
Is it possible for the lockless checks performed earlier in this function
to evaluate as empty, but for new data to be queued concurrently before
acquiring port->lock?

If new data is queued by a concurrent start_tx(), the function will observe
tx_state is still MAX310X_TX_SEND without re-evaluating the FIFO, and arm
the after-send timer. This could drop the RTS pin mid-transmission when the
timer expires, truncating the RS485 frame.

> +
> +	if (!hrtimer_active(&one->tx_delay_tmr)) {

[Severity: Medium]
Could this check strand the RTS pin asserted?

The before-send timer callback releases the port lock and returns
HRTIMER_NORESTART, but the hrtimer core retains the running state until the
callback fully returns.

If tx_work executes concurrently, drains the FIFO, and enters this path
before the hrtimer core clears the running state, this check evaluates to
true. This skips starting the after-send hold, leaving tx_state stuck in
MAX310X_TX_SEND and never releasing the RTS pin.

> +		/*
> +		 * Add one character for the byte still in the shift register -
> +		 * TX-empty fires as it enters, not as it leaves.
> +		 */
> +		ktime_t delay = us_to_ktime(one->char_time_us +
> +					    port->rs485.delay_rts_after_send *
> +					    USEC_PER_MSEC);
> +
> +		WRITE_ONCE(one->tx_state, MAX310X_TX_WAIT_AFTER_SEND);
> +		hrtimer_start(&one->tx_delay_tmr, delay, HRTIMER_MODE_REL);
> +	}
> +}

[ ... ]

> @@ -957,25 +1165,35 @@ static void max310x_set_rts_ctl_params(struct max310x_one *one)
>  	struct uart_port *port = &one->port;
>  	unsigned int setup = 0, hold = 0;
>  	u8 mode1 = 0, irda = 0;
> +	bool sw_rts = false;

[ ... ]

> +	if (port->rs485.flags & SER_RS485_ENABLED) {
> +		if (sw_rts) {
> +			/*
> +			 * Only settle RTS at idle when no transmission owns it.
> +			 * A reconfigure while one is in flight - rs_work runs
> +			 * on every TIOCSRS485 - would otherwise release the
> +			 * transceiver mid-character. A transfer the hardware
> +			 * path owns is invisible to tx_state - adopt it
> +			 * instead of settling. The reverse strand also
> +			 * exists: an envelope the hardware path inherited
> +			 * mid-envelope leaves tx_state set with nothing
> +			 * driving the pin.
> +			 */
> +			if (READ_ONCE(one->tx_state) != MAX310X_TX_OFF ||
> +			    max310x_adopt_hw_tx(one)) {
> +				/*
> +				 * The transfer may be running under auto-RTS:
> +				 * assert before auto-RTS is disabled below so
> +				 * the pin is never released under it, and arm
> +				 * the after-send hold - with the chip FIFO
> +				 * already drained no TX-empty interrupt
> +				 * arrives to arm it.
> +				 */
> +				max310x_rts_ctl(port,
> +						max310x_rts_level(port, true));
> +				max310x_delayed_stop_tx(port);
> +			} else {
> +				max310x_rts_ctl(port,
> +						max310x_rts_level(port, false));
> +				/*
> +				 * serial_core calls set_termios() without
> +				 * port->lock, so an envelope may have started
> +				 * while the idle level was written and the
> +				 * settle can land after its RTS assert.
> +				 * rts_work re-derives the level from
> +				 * tx_state; requeue it to converge.
> +				 */
> +				if (READ_ONCE(one->tx_state) != MAX310X_TX_OFF)
> +					schedule_work(&one->rts_work);
> +			}

[Severity: High]
Is there a race that could cause the RTS pin to drop mid-transmission?

Since tx_state is read locklessly, a concurrent start_tx() could initiate a
transmission and assert the RTS pin before this slow hardware write executes.

The mitigation to requeue rts_work only re-asserts the RTS pin after it
was already dropped, which would irreparably corrupt the active RS485 frame.

[ ... ]

> @@ -1676,6 +2058,8 @@ static void max310x_remove(struct device *dev)
>  	int i;
>  
>  	for (i = 0; i < s->devtype->nr; i++) {
> +		hrtimer_cancel(&s->p[i].tx_delay_tmr);
> +		cancel_work_sync(&s->p[i].rts_work);
>  		cancel_work_sync(&s->p[i].tx_work);
>  		cancel_work_sync(&s->p[i].md_work);
>  		cancel_work_sync(&s->p[i].rs_work);
>  
>  		if (test_and_clear_bit(s->p[i].port.line, max310x_lines))
>  			uart_remove_one_port(&max310x_uart, &s->p[i].port);

[Severity: Critical]
Can this sequence cause a use-after-free by cancelling background tasks
too early?

uart_remove_one_port() is called after these workqueues and timers are
cancelled. Because the TTY port is still fully registered and active during
this window, userspace operations or hardware interrupts could re-queue
these works.

Once max310x_remove() returns and the memory is freed, any re-queued
works would execute on freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-max310x-rs485-sw-delay-v8-0-6ba881f81e1f@vaisala.com?part=8

[PATCH v8 9/9] serial: max310x: don't transmit while an RS485 reconfigure is pending

[Tapio Reijonen]

TIOCSRS485 applies its register changes asynchronously: rs485_config()
stores the new configuration and schedules rs_work, which programs
HDPIXDELAY, MODE1.TRNSCVCTRL and the RTS path. A write() issued right
after the ioctl therefore transmits against the old, half-switched
state. On a single core the ordering is even deterministic: start_tx()
picks the stale path first, then rs_work reprograms the chip, then
tx_work pumps the data - with the transceiver already released. A
TIOCSRS485 switching from the hardware to the software RTS path
followed immediately by a write puts the whole transfer on the wire
with the transceiver disabled: nothing reaches the bus and no error is
reported anywhere. The inverse direction is as old as the asynchronous
reconfigure itself: enabling RS485 and writing immediately shifts the
first bytes out before rs_work has enabled the chip's auto-RTS.

Defer instead: rs485_config() marks the reconfigure pending under
port->lock, start_tx() leaves the data in the kfifo while the mark is
set, and rs_work restarts the transmission itself once the new
configuration is fully applied. The rs485-disable path's direct
tx_work kick is replaced by the same mechanism, which also orders that
flush after the reconfigure instead of before it.

The restart kick also checks port->x_char: uart_send_xchar() reaches
start_tx() too, and a lone x_char deferred by the pending gate leaves
the kfifo empty, so the kick would otherwise skip it and nothing else
would ever send it - an idle chip FIFO raises no TXEMPTY interrupt.
And start_tx() re-checks the pending mark after the
hrtimer_try_to_cancel(-1) path retakes the dropped lock, mirroring the
tx_teardown re-check there: a TIOCSRS485 posted inside that window
would otherwise let the transmission proceed on the stale path.

rs_work takes port->lock for that restart through
uart_port_lock_irqsave(): start_tx() may drop and retake the lock
through the uart_port API on its hrtimer_try_to_cancel(-1) path, and a
raw spinlock guard around the call would unbalance the nbcon console
handling underneath.

The restart also rewinds a send state adopted for the deferred write
itself: set_rts_ctl_params() cannot tell data the pending gate
deferred apart from an in-flight transfer's refill by the xmit buffer
alone, and a deferred write pumped in the adopted send phase would
skip the configured before-send delay. With the chip FIFO empty the
adopted state can only be the deferred write: rewind it and let the
restart run the full envelope.

The chip-FIFO check for that rewind is deliberately lock-free, so a
TX-empty refill can land between the read and the lock and the rewind
can hit a live envelope. The consequence is bounded: the rewind runs
under port->lock in a state the RTS worker reads as asserted, so the
envelope pauses one before-send delay with the transceiver held - the
same pause the unconditional rewind produced before patch 8's guard.

Fixes: 5bdb48b501e8 ("serial: max310x: Fix RS485 handling")
Assisted-by: Claude:claude-fable-5
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 51 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 42 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 5fdb9dfca6027ff0a9284ed994000a5d7587408c..833ab4d461235d89438b1a7c84c46609fff64453 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -319,6 +319,7 @@ struct max310x_one {
 	bool			sw_rts_during_tx;
 	bool			cancel_tx_delay_tmr;
 	bool			tx_teardown;	/* envelope being torn down */
+	bool			rs485_pending;	/* rs_work not yet applied */
 	bool			tx_break;	/* break_ctl() owns the transceiver */
 	enum max310x_tx_state	tx_state;
 
@@ -979,6 +980,16 @@ static void max310x_start_tx(struct uart_port *port)
 	if (one->tx_teardown)
 		return;
 
+	/*
+	 * An RS485 reconfigure is scheduled but not applied yet: transmitting
+	 * now would use the old path against half-programmed registers - a
+	 * TIOCSRS485 switching paths followed immediately by a write puts the
+	 * data on the wire with the transceiver released. Leave the data in
+	 * the kfifo; rs_work restarts TX once the configuration is applied.
+	 */
+	if (one->rs485_pending)
+		return;
+
 	if (READ_ONCE(one->sw_rts_during_tx)) {
 		/*
 		 * The before- and after-send phases share one delay timer. If an
@@ -997,9 +1008,10 @@ static void max310x_start_tx(struct uart_port *port)
 			uart_port_lock(port);
 			/*
 			 * The lock was dropped: a teardown may have run to
-			 * completion meanwhile. Re-check before starting.
+			 * completion or a reconfigure may have been posted
+			 * meanwhile. Re-check before starting.
 			 */
-			if (one->tx_teardown)
+			if (one->tx_teardown || one->rs485_pending)
 				return;
 		}
 
@@ -1445,6 +1457,8 @@ static void max310x_rs_proc(struct work_struct *ws)
 {
 	struct max310x_one *one = container_of(ws, struct max310x_one, rs_work);
 	unsigned int mode2 = 0;
+	unsigned long flags;
+	bool chip_tx_empty;
 
 	/*
 	 * Serialize against break_ctl() and set_termios(), which run under
@@ -1465,6 +1479,31 @@ static void max310x_rs_proc(struct work_struct *ws)
 
 	max310x_port_update(&one->port, MAX310X_MODE2_REG,
 			    MAX310X_MODE2_ECHOSUPR_BIT, mode2);
+
+	/*
+	 * The configuration is applied: release any TX that start_tx()
+	 * deferred while the reconfigure was pending, now on the right
+	 * path. start_tx() can drop and retake the lock through the
+	 * uart_port API - a raw spinlock guard here would unbalance it.
+	 */
+	chip_tx_empty = !max310x_port_read(&one->port,
+					   MAX310X_TXFIFOLVL_REG);
+
+	uart_port_lock_irqsave(&one->port, &flags);
+	one->rs485_pending = false;
+	if (one->port.x_char ||
+	    !kfifo_is_empty(&one->port.state->port.xmit_fifo)) {
+		/*
+		 * A send state adopted without chip data is the deferred
+		 * write itself: restart it as a fresh envelope so the
+		 * before-send delay is honoured.
+		 */
+		if (chip_tx_empty &&
+		    READ_ONCE(one->tx_state) == MAX310X_TX_SEND)
+			WRITE_ONCE(one->tx_state, MAX310X_TX_OFF);
+		max310x_start_tx(&one->port);
+	}
+	uart_port_unlock_irqrestore(&one->port, flags);
 }
 
 /* called with port.lock taken and irqs off */
@@ -1490,17 +1529,11 @@ static int max310x_rs485_config(struct uart_port *port, struct ktermios *termios
 		}
 		WRITE_ONCE(one->tx_state, MAX310X_TX_OFF);
 		one->tx_teardown = false;
-		/*
-		 * The port stays alive, and a write that raced the teardown
-		 * may have left data queued with no envelope left to pump it.
-		 * Kick tx_work; RS485 is disabled, so the plain path is right.
-		 */
-		if (!kfifo_is_empty(&port->state->port.xmit_fifo))
-			schedule_work(&one->tx_work);
 	}
 
 	port->rs485 = *rs485;
 
+	one->rs485_pending = true;
 	schedule_work(&one->rs_work);
 
 	return 0;

-- 
2.47.3

Re: [PATCH v8 9/9] serial: max310x: don't transmit while an RS485 reconfigure is pending

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] tty: max310x tx_work bypasses rs485_pending deferral
- [High] tty: max310x: kfifo corruption in max310x_handle_tx()
- [Medium] tty: max310x: rs_work outlives port shutdown causing suspended device access
--

commit fddc249318df6e2dd69c19bbf8a221d220eb0e4a
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>

serial: max310x: don't transmit while an RS485 reconfigure is pending

TIOCSRS485 applies its register changes asynchronously. This patch defers
transmission by adding an rs485_pending flag to start_tx() to prevent
transmitting against a half-switched state.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index 5fdb9dfca6027..833ab4d461235 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
> [ ... ]
> @@ -979,6 +980,16 @@ static void max310x_start_tx(struct uart_port *port)
>  	if (one->tx_teardown)
>  		return;
>  
> +	/*
> +	 * An RS485 reconfigure is scheduled but not applied yet: transmitting
> +	 * now would use the old path against half-programmed registers - a
> +	 * TIOCSRS485 switching paths followed immediately by a write puts the
> +	 * data on the wire with the transceiver released. Leave the data in
> +	 * the kfifo; rs_work restarts TX once the configuration is applied.
> +	 */
> +	if (one->rs485_pending)
> +		return;

[Severity: High]
Can the asynchronous transmit worker tx_work bypass this deferral check?

If a trailing TXEMPTY interrupt from a previous transmission schedules
tx_work right as rs485_config() marks the reconfigure pending and new data
is written to the kfifo, it looks like tx_work will execute
max310x_handle_tx() and unconditionally drain the newly written data to the
chip's FIFO.

Does this put data on the wire while rs_work is concurrently modifying the
transceiver configuration registers, causing the same half-switched
transmission this was meant to fix?

[Severity: High]
Does the lockless execution of max310x_handle_tx() via tx_work create a
kfifo corruption risk against uart_flush_buffer()?

max310x_handle_tx() computes the buffer length and performs a blocking
SPI/I2C write locklessly. If a concurrent ioctl(fd, TCFLSH, TCOFLUSH)
triggers uart_flush_buffer(), the serial core acquires port->lock and calls
kfifo_reset(&tport->xmit_fifo).

When max310x_batch_write() returns, does uart_xmit_advance() increment the
reset kfifo->out pointer, causing it to underflow past kfifo->in and
potentially leaking up to UART_XMIT_SIZE bytes of stale kernel memory to
the serial bus?

> [ ... ]
> @@ -1465,6 +1479,31 @@ static void max310x_rs_proc(struct work_struct *ws)
>  
>  	max310x_port_update(&one->port, MAX310X_MODE2_REG,
>  			    MAX310X_MODE2_ECHOSUPR_BIT, mode2);
> +
> +	/*
> +	 * The configuration is applied: release any TX that start_tx()
> +	 * deferred while the reconfigure was pending, now on the right
> +	 * path. start_tx() can drop and retake the lock through the
> +	 * uart_port API - a raw spinlock guard here would unbalance it.
> +	 */
> +	chip_tx_empty = !max310x_port_read(&one->port,
> +					   MAX310X_TXFIFOLVL_REG);

[Severity: Medium]
Can rs_work outlive port shutdown and access a powered-off device here?

max310x_shutdown() cancels tx_work and rts_work and powers down the port,
but deliberately omits canceling rs_work. A pending rs_work will wait on
port->mutex while uart_shutdown is executing.

After uart_shutdown finishes and releases port->mutex, max310x_rs_proc()
acquires the mutex and executes. Will it then perform SPI/I2C register reads
and writes, such as this new MAX310X_TXFIFOLVL_REG read, on the device that
was just powered off?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-max310x-rs485-sw-delay-v8-0-6ba881f81e1f@vaisala.com?part=9