RE: [PATCH v2] serial: max3100: Fix a data race on s->rts in max3100_work()

[Maarten Brock]

> -----Original Message-----
> From: Ginger Li <ginger.jzllee@xxxxxxxxx>
> Sent: Tuesday 22 September 2026 19:03
> To: jirislaby@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx
> Cc: linux-serial@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Maarten Brock <Maarten.Brock@xxxxxxxx>
> Subject: [PATCH v2] serial: max3100: Fix a data race on s->rts in max3100_work()
> 
> max3100_set_mctrl() stores the new RTS state and raises s->rts_commit to tell
> max3100_work() that it has to program it:
> 
> 	spin_lock(&s->conf_lock);
> 	if (s->rts != rts) {
> 		s->rts = rts;
> 		s->rts_commit = 1;
> 	}
> 	if (s->loopback_commit || s->rts_commit)
> 		max3100_dowork(s);
> 	spin_unlock(&s->conf_lock);
> 
> max3100_work() consumes s->rts_commit under s->conf_lock, but reads s->rts
> itself outside of the lock, both when it services that pending update and when
> it later transmits a character, so the state it programs into the hardware can
> be stale.
> 
> Read s->rts into a local variable in the s->conf_lock protected snapshot at
> the top of the loop, before s->rts_commit is consumed, in the same way as
> s->conf is read before s->conf_commit.
> 
> Fixes: 7831d56b0a35 ("tty: MAX3100")
> Signed-off-by: Ginger Li <ginger.jzllee@xxxxxxxxx>

Reviewed-by: Maarten Brock <maarten.brock@xxxxxxxx>

> ---
> v2:
>  - read s->rts into "rts" before s->rts_commit is consumed, as suggested by
>    Maarten
>  - mention s->rts_commit in the commit message
>  - left the s->rts / s->rts_commit split alone; folding both into one byte
>    would be a separate cleanup
> ---
>  drivers/tty/serial/max3100.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
> --- a/drivers/tty/serial/max3100.c
> +++ b/drivers/tty/serial/max3100.c
> @@ -236,6 +236,7 @@ static void max3100_work(struct work_struct *w)
>  	struct tty_port *tport = &s->port.state->port;
>  	unsigned char ch;
>  	int conf, cconf, cloopback, crts;
> +	bool rts;
>  	int rxchars;
>  	u16 tx, rx;
> 
> @@ -249,6 +250,7 @@ static void max3100_work(struct work_struct *w)
>  		s->conf_commit = 0;
>  		cloopback = s->loopback_commit;
>  		s->loopback_commit = 0;
> +		rts = s->rts;
>  		crts = s->rts_commit;
>  		s->rts_commit = 0;
>  		spin_unlock(&s->conf_lock);
> @@ -258,7 +260,7 @@ static void max3100_work(struct work_struct *w)
>  			max3100_sr(s, 0x4001, &rx);
>  		if (crts) {
>  			max3100_sr(s, MAX3100_WD | MAX3100_TE |
> -				   (s->rts ? MAX3100_RTS : 0), &rx);
> +				   (rts ? MAX3100_RTS : 0), &rx);
>  			rxchars += max3100_handlerx(s, rx);
>  		}
> 
> @@ -277,7 +279,7 @@ static void max3100_work(struct work_struct *w)
>  			}
>  			if (tx != 0xffff) {
>  				max3100_calc_parity(s, &tx);
> -				tx |= MAX3100_WD | (s->rts ? MAX3100_RTS : 0);
> +				tx |= MAX3100_WD | (rts ? MAX3100_RTS : 0);
>  				max3100_sr(s, tx, &rx);
>  				rxchars += max3100_handlerx(s, rx);
>  			}
> --
> 2.43.0