linux-kernel - Re: [PATCH v3] serial: port: Don't suspend if the port is still busy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <beb599e2-ebc5-4b26-82c3-908830effd5a@kernel.org>
Date: Thu, 8 Feb 2024 09:27:57 +0100
From: Jiri Slaby <jirislaby@...nel.org>
To: Yicong Yang <yangyicong@...wei.com>, gregkh@...uxfoundation.org,
 tony@...mide.com, linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org
Cc: john.ogness@...utronix.de, andriy.shevchenko@...ux.intel.com,
 tglx@...utronix.de, yangyicong@...ilicon.com, linuxarm@...wei.com,
 prime.zeng@...ilicon.com, jonathan.cameron@...wei.com, fanghao11@...wei.com
Subject: Re: [PATCH v3] serial: port: Don't suspend if the port is still busy

Hi,

On 08. 02. 24, 8:52, Yicong Yang wrote:
> From: Yicong Yang <yangyicong@...ilicon.com>
> 
> We accidently met the issue that the bash prompt is not shown after the
> previous command done and until the next input if there's only one CPU
> (In our issue other CPUs are isolated by isolcpus=). Further analysis
> shows it's because the port entering runtime suspend even if there's
> still pending chars in the buffer and the pending chars will only be
> processed in next device resuming. We are using amba-pl011 and the
> problematic flow is like below:

..
> --- a/drivers/tty/serial/serial_port.c
> +++ b/drivers/tty/serial/serial_port.c
> @@ -19,8 +19,13 @@
>   /* Only considers pending TX for now. Caller must take care of locking */
>   static int __serial_port_busy(struct uart_port *port)
>   {
> -	return !uart_tx_stopped(port) &&
> -		uart_circ_chars_pending(&port->state->xmit);
> +	if (uart_tx_stopped(port))
> +		return 0;
> +
> +	if (uart_circ_chars_pending(&port->state->xmit))
> +		return -EBUSY;

Why do you do this change at all? If anything, __serial_port_busy() 
should be made to return a bool and not to return an error. Look how it 
is named -- returning EBUSY is sort of unexpected in my eyes. And if 
this needed to be done, it should have been in a separate patch anyway.

And then:

> @@ -46,8 +51,33 @@ static int serial_port_runtime_resume(struct device *dev)
>   	return 0;
>   }
>   
> +static int serial_port_runtime_suspend(struct device *dev)
> +{
> +	struct serial_port_device *port_dev = to_serial_base_port_device(dev);
> +	struct uart_port *port;
> +	unsigned long flags;
> +	int ret;

bool busy;

> +
> +	port = port_dev->port;
> +
> +	if (port->flags & UPF_DEAD)
> +		return 0;
> +
> +	uart_port_lock_irqsave(port, &flags);
> +	ret = __serial_port_busy(port);
> +	if (ret)

busy = ...
if (busy)

> +		port->ops->start_tx(port);
> +	uart_port_unlock_irqrestore(port, flags);
> +
> +	if (ret)

if (busy)

> +		pm_runtime_mark_last_busy(dev);
> +
> +	return ret;

return busy ? -EBUSY : 0;

> +}

thanks,
-- 
js
suse labs