Re: [PATCH 2/6] USB: serial: mxuport: initialize modem status on open

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: serial: mxuport: TOCTOU race drops MSR events during open
--

commit e30f4f38ee9a11d678d89a99a4f6221fd7d285d1
Author: Crescent Hsieh <crescentcy.hsieh@moxa.com>

USB: serial: mxuport: initialize modem status on open

The cached modem status is currently cleared whenever a port is opened,
regardless of the current modem-status inputs. This commit fetches the
modem status register via a vendor request during port open so that
TIOCMGET reports the correct initial input signals.

> diff --git a/drivers/usb/serial/mxuport.c b/drivers/usb/serial/mxuport.c
> index fa75f844050dc..3f1989afde30e 100644
> --- a/drivers/usb/serial/mxuport.c
> +++ b/drivers/usb/serial/mxuport.c

[ ... ]

> @@ -1477,13 +1478,19 @@ static int mxuport_open(struct tty_struct *tty, struct usb_serial_port *port)
>  	mxport->hold_reason &= ~MX_WAIT_FOR_SEND_NEXT;
>  	spin_unlock_irqrestore(&port->lock, flags);
>  
> -	/*
> -	 * TODO: use RQ_VENDOR_GET_MSR, once we know what it
> -	 * returns.
> -	 */
> -	mxport->msr_state = 0;
> +	err = mxuport_recv_ctrl(serial, RQ_VENDOR_GET_MSR, 0,
> +				port->port_number, &msr_state,
> +				sizeof(msr_state));

[Severity: Medium]
Does this introduce a race condition where modem status events can be
dropped?

If a physical modem status line changes immediately after this
RQ_VENDOR_GET_MSR request completes, but before the core layer marks the
port as initialized, it appears the completion handler drops the incoming
hardware events.

Looking at mxuport_process_read_urb_demux_event():

    if (tty_port_initialized(&demux_port->port)) {
        ch = data + HEADER_SIZE;
        rcv_event = get_unaligned_be16(data + 2);
        mxuport_process_read_urb_event(demux_port, ch, rcv_event);
    } else {
        dev_dbg(&demux_port->dev, "%s - event for closed port\n", __func__);
    }

Since tty_port_initialized() is not set to true until after mxuport_open()
returns to the core layer, any state transition in this window is silently
dropped. Can this leave the cached msr_state permanently desynchronized
from the actual hardware state until a subsequent event occurs?

> +	if (err)
> +		msr_state = 0;
> +	else
> +		msr_state &= ~UART_MSR_ANY_DELTA;
>  
> -	return err;
> +	spin_lock_irqsave(&mxport->spinlock, flags);
> +	mxport->msr_state = msr_state;
> +	spin_unlock_irqrestore(&mxport->spinlock, flags);
> +
> +	return 0;
>  }
>  
>  static void mxuport_close(struct usb_serial_port *port)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016100351.681390-1-crescentcy.hsieh@moxa.com?part=2