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

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Jan 04, 2021 at 08:41:26PM +0800, zhangqiumiao1@huawei.com wrote:
> From: Qiumiao Zhang <zhangqiumiao1@huawei.com>
> 
> Serial port writing will be suspended when the buffer is continuously full. When
> the serial port's TX and RX are short circuited, there is a certain probability
> that this will happed. The concrete representation is n_tty_write blocking in
> wait_woken and the process is trapped in a loop waiting. After testing, when the
> the serial port runs well, wait_woken wait time will not exceed 60 seconds. So
> for serial port, set the timeout value of wait_woken function to 60s. Wake up and
> flush the buffer after timeout.
> 

Why would we ever want to wait longer than 60 seconds?  What problems
does this cause if we just always set the timeout to 60 seconds?

> Signed-off-by: Qiumiao Zhang <zhangqiumiao1@huawei.com>
> ---
> v4:
>  fix wrong expression in path description
>  remove unnecessary macro definition and debugging code
> 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
> 
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 319d68c..0e6f202 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -2375,7 +2375,17 @@ 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 for serial port*/

Extra space please before the "*/"

> +		if (tty->driver->type == TTY_DRIVER_TYPE_SERIAL) {

Why is serial unique here?

> +			if (wait_woken(&wait, TASK_INTERRUPTIBLE, 60 * HZ) == 0)
> +				if (tty && tty->ops->flush_buffer) {
> +					tty->ops->flush_buffer(tty);
> +				} else {
> +					tty_err(tty, "tty related structure not registered\n");

How is this an error now?  What can a user do about it?  Do you need to
fix up some drivers because of this change?

Did you run this through checkpatch?  Are you sure that it does not
complain about the { } use here?



> +				}
> +		} else {
> +			wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
> +		}

This whole thing still feels really wrong.  If things are stuck, why
will this fix them?  Shouldn't the serial driver properly recover no
matter what?

What hardware and workload are you seeing this problem on?

thanks,

greg k-h