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

[Jiri Slaby <jirislaby@kernel.org>]

Hi,

On 08. 02. 24, 8:52, Yicong Yang wrote:
> From: Yicong Yang <yangyicong@hisilicon.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