Re: [PATCH v4 4/4] serial: abort TIOCMIWAIT on hangup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The `TIOCMIWAIT` implementation in `drivers/tty/mxser.c` suffers from the exact same missing hangup check as `serial_core.c`, causing indefinite sleep on hangup.
- [High] The `TIOCMIWAIT` loop in `drivers/tty/amiserial.c` is fundamentally broken, instantly returns `-EIO` without waiting, and lacks a hangup check.
--

commit a8c074101564c35207f8f5e3fb06b52d01a55d88
Author: Johan Hovold <johan@kernel.org>
Subject: serial: abort TIOCMIWAIT on hangup

The commit adds a missing hangup check to the TIOCMIWAIT wait loop in
serial_core.c to avoid indefinite sleep.

Link: https://lore.kernel.org/r/20260904115542.E20D11F00A3D@smtp.kernel.org

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index f7b30f77b2312..33c8f3b6ac816 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1244,6 +1244,11 @@ static int uart_wait_modem_status(struct tty_struct *tty, struct uart_state *sta
>  			break;
>  		}
>  
> +		if (tty_io_error(tty)) {
> +			ret = -EIO;
> +			break;
> +		}
> +

[Severity: High]
This isn't a bug introduced by this patch, but the TIOCMIWAIT implementation
in drivers/tty/mxser.c suffers from the exact same missing hangup check,
causing indefinite sleep on hangup.

In drivers/tty/mxser.c:mxser_cflags_changed(), the condition for
wait_event_interruptible() is evaluated without checking for a hangup:

	ret =	((arg & TIOCM_RNG) && (cnow.rng != cprev->rng)) ||
		((arg & TIOCM_DSR) && (cnow.dsr != cprev->dsr)) ||
		((arg & TIOCM_CD)  && (cnow.dcd != cprev->dcd)) ||
		((arg & TIOCM_CTS) && (cnow.cts != cprev->cts));

Does mxser.c also need a tty_io_error() check here to prevent the process
from going back to sleep indefinitely instead of aborting when woken on
hangup?

[Severity: High]
This is a pre-existing issue, but the TIOCMIWAIT loop in
drivers/tty/amiserial.c is fundamentally broken, instantly returning
-EIO without waiting, and lacks a hangup check.

In drivers/tty/amiserial.c:rs_ioctl(), cprev is captured right before
the wait loop. Immediately inside the loop, cnow is captured again and
checked:

				cnow = info->icount; /* atomic copy */
				local_irq_restore(flags);
				if (cnow.rng == cprev.rng && cnow.dsr == cprev.dsr && 
				    cnow.dcd == cprev.dcd && cnow.cts == cprev.cts) {
					ret = -EIO; /* no change => error */
					break;
				}

Because the copies happen almost back-to-back, the counters will almost
always be identical on the first iteration before schedule() is ever
called, causing an instant failure instead of waiting. 

Should this faulty check be removed, and a proper hangup check like the
one you're adding in this patch be included instead?

>  		schedule();
>  
>  		/* see if a signal did it */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910130816.642699-1-johan@kernel.org?part=4