Re: [PATCH v3] tty: resolve loopback wait problem for aging serial port

[Greg KH]

On Wed, Dec 30, 2020 at 09:45:29PM +0800, zhangqiumiao1@xxxxxxxxxx wrote:
> From: Qiumiao Zhang <zhangqiumiao1@xxxxxxxxxx>
> 
> Because serial port is low-speed device, serial port writing will be suspended
> when the buffer of driver layer is full due to serial port aging.

What do you mean here exactly?  What is "serial port aging"?

And since when is a serial port a low-speed device?  Compared to what?

> The concrete
> representation is n_tty_write blocking in the process of wait_woken, the process
> of writing serial port exists without feedback, and becomes a zombie process.

I do not understand this sentance, sorry.  Can you rephrase it?

> So for serial port, set the timeout value of wait_woken function to 60s. Wake up
> and flush the buffer after timeout.

Where did the 60 second number come from?  Why that value and not any
other specific value?


> 
> Signed-off-by: Qiumiao Zhang <zhangqiumiao1@xxxxxxxxxx>
> ---
> v3:
>  add changes from v1 to v2
> 
> v2:
>  change to use "real name"
>  fix wrong expression in path description
>  remove config option
>  use driver type to judge tty device type
> 
>  drivers/tty/n_tty.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 319d68c..8e7591a 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -87,6 +87,8 @@
>  # define n_tty_trace(f, args...)	no_printk(f, ##args)
>  #endif
> 
> +#define SIXTY_SEC_TIMEOUT (60 * HZ)

Is that define really needed?

> +
>  struct n_tty_data {
>  	/* producer-published */
>  	size_t read_head;
> @@ -2375,7 +2377,18 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file,
>  		}
>  		up_read(&tty->termios_rwsem);
> 
> -		wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
> +		/* Resolve the problem of loopback waiting when the serial port is aging seriously */
> +		if (tty->driver->type == TTY_DRIVER_TYPE_SERIAL) {
> +			if (wait_woken(&wait, TASK_INTERRUPTIBLE, SIXTY_SEC_TIMEOUT) == 0)
> +				if (tty && tty->ops->flush_buffer) {
> +					printk(KERN_INFO "n_tty_write flush buffer\n");

Why have you left debugging code in here?

> +					tty->ops->flush_buffer(tty);
> +				} else {
> +					printk(KERN_ERR "tty related structure not registered\n");

What can a user do with this?  How can it be hit?

And never use printk() in a driver, please use dev_err() and friends
instead.

thanks,

greg k-h