Re: [PATCH 05/11] USB: serial: io_edgeport: serialise MCR accesses

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: serial: io_edgeport: change_port_settings() locks mutex in atomic context
--

commit 12f657a4474c4b2a9d60008b5bffca174179c50e
Author: Johan Hovold <johan@kernel.org>

USB: serial: io_edgeport: serialise MCR accesses

This commit adds a mutex to struct edgeport_port to serialise updates and
reads of shadowMCR across port operations.

> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
> index 38f58ca115ebd..0b50c6b99b0a8 100644
> --- a/drivers/usb/serial/io_edgeport.c
> +++ b/drivers/usb/serial/io_edgeport.c
[ ... ]
> @@ -2477,12 +2489,14 @@ static void change_port_settings(struct tty_struct *tty,
>  		return;
>  
>  	/* set up the MCR register and send it to the EdgePort */
> +	mutex_lock(&edge_port->mutex);
>  	edge_port->shadowMCR = MCR_MASTER_IE;
>  	if (cflag & CBAUD)
>  		edge_port->shadowMCR |= (MCR_DTR | MCR_RTS);
>  
>  	status = send_cmd_write_uart_register(edge_port, MCR,
>  						edge_port->shadowMCR);
> +	mutex_unlock(&edge_port->mutex);

[Severity: High]
Can this sleep in atomic context when a port is opened?

When edge_open() sends an open command to the device and waits on
edge_port->wait_open, the open response is handled in the bulk-in urb
completion callback while holding edge_serial->es_lock with interrupts
disabled:

drivers/usb/serial/io_edgeport.c:
edge_bulk_in_callback()
    spin_lock_irqsave(&edge_serial->es_lock, flags);
    ...
    process_rcvd_data(edge_serial, data, urb->actual_length);
        -> process_rcvd_status()
            if (code == IOSP_STATUS_OPEN_RSP) {
                ...
                tty = tty_port_tty_get(&edge_port->port->port);
                if (tty) {
                    change_port_settings(tty,
                        edge_port, &tty->termios);
                    tty_kref_put(tty);
                }

Calling mutex_lock(&edge_port->mutex) inside change_port_settings() acquires
a sleepable mutex under spin_lock_irqsave() in hardirq or softirq context.

A subsequent commit in this series moves the initial termios update from
process_rcvd_status() to edge_open(), but having this patch ordered before
that change leaves an intermediate bisection window where opening a port
triggers a sleeping-in-atomic bug, and could also cause a regression when
this commit is backported to stable trees without the follow-up refactoring.

Should the change moving the initial termios update into edge_open() come
before this patch and be included as a prerequisite for stable?

>  	if (status != 0)
>  		return;
>  
[ ... ]

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