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

[Tapio Reijonen <tapio.reijonen@vaisala.com>]

A belated follow-up on the three findings the automated review raised
against v4, since v5 (split into a series, as Greg asked) is about to
be posted and changes course on some of what earlier replies claimed.

The short version: two of the three findings led to changes in v5, and
one of those corrects a claim made in my reply on v3. The third finding
is refuted, and v5 adds a comment at the spot so the reasoning is in
the code rather than in a mail archive.

On "races from dropping port->lock in start_tx/rs485_config":

Right bug, and my earlier assessment was too narrow - both of its
scenarios are real, although the dropped lock is not the mechanism.
->shutdown() runs under port->mutex and ->start_tx() under port->lock,
so they never excluded each other to begin with: the window is the
whole of shutdown(), not the unlock. The same shape exists in the
rs485-disable path, where the sharp end is silent data loss - a
write() racing the disable leaves its bytes queued with no envelope
left to pump them, and a following close() discards them without an
error. In fact this finding and the two shutdown-related findings from
the v3 round collapse into one defect: starting a transmission had no
teardown interlock. v5 adds one (a tx_teardown flag set under
port->lock by shutdown() and the rs485-disable path, checked by
start_tx() on entry and again after the dropped lock is retaken), the
disable path now restarts TX once the reconfigure is applied so the
queued data goes out, and shutdown() also cancels tx_work, which was
previously only cancelled in remove().

On "torn read of the 64-bit one_character_duration":

Valid, and my reply on v3 overreached when it said every value the
driver can hold has a zero upper word. That was board-specific
reasoning stated as a driver-wide claim: with a low reference clock
the no-PLL configuration can win, the baud floor collapses, and a
12-bit frame at single-digit baud does exceed 2^32 ns - at which point
the bounded-shutdown-loop argument from the first round falls with it.
Rather than argue reachability, v5 stores the per-character duration
as unsigned int microseconds: single-copy atomic on 32-bit, so the
torn read is gone by construction, and three of the four users
converted straight back to microseconds anyway.

On "tx_work bypasses the before_send delay":

The premise about the code is correct - nothing in the TXEMPTY path
checks tx_state - but the scenario is unreachable. TXEMPTY latches on
the FIFO's non-empty to empty transition and the IRQ handler's read of
IRQSTS consumes the latch; that read sits upstream of the only path
that ever deasserts RTS, so whenever RTS is low there is no pending
latch left to fire. (If the bit were level-triggered instead, the
handler's read-until-clear loop would never terminate on an idle
port.) A write landing mid-envelope can reset the phase and pay one
extra before-send delay, but RTS is already asserted then, so nothing
is shifted with the transceiver released. v5 adds a comment at the
TXEMPTY handling to that effect.

Of the two pre-existing concerns, shutdown() not cancelling tx_work is
addressed in v5 as part of the interlock work; the remove() ordering
remains out of scope for this series.

Tapio