Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 10 May 2023 14:43:25 +0300 (EEST)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Lukasz Majczak <lma@semihalf.com>
cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        linux-serial <linux-serial@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, upstream@semihalf.com,
        stable@vger.kernel.org
Subject: Re: [PATCH v2 2/2] serial: core: enable FIFO after resume
In-Reply-To: <20230405111559.110220-3-lma@semihalf.com>
Message-ID: <2b3f5b4e-c75e-5c20-bb86-c5628e7e91a7@linux.intel.com>
References: <20230405111559.110220-1-lma@semihalf.com> <20230405111559.110220-3-lma@semihalf.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 5 Apr 2023, Lukasz Majczak wrote:

> The "serial/8250: Use fifo in 8250 console driver" commit

Use canonical formatting when referring to commit:

commit 12char_SHA1 ("shortlog")

> has revealed an issue of not re-enabling FIFO after resume.
> The problematic path is inside uart_resume_port() function.
> First, when the console device is re-enabled,
> a call to uport->ops->set_termios() internally initializes FIFO
> (in serial8250_do_set_termios()), although further code

I'd drop "First," and start with "When" and change "although" to "then"

> disables it by issuing ops->startup() (pointer to serial8250_do_startup,
> internally calling serial8250_clear_fifos()).
> There is even a comment saying "Clear the FIFO buffers and disable them.
> (they will be reenabled in set_termios())", but in this scenario,
> set_termios() has been called already and FIFO remains disabled.

Also, you should reflow the text to 72 chars per line.

> This patch address the issue by reversing the order - first checks
> if tty port is suspended and performs actions accordingly
> (e.g. call to ops->startup()), then tries to re-enable
> the console device after suspend (and call to uport->ops->set_termios()).
> 
> Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> Cc: <stable@vger.kernel.org> # 6.1+
> ---
>  drivers/tty/serial/serial_core.c | 54 ++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 394a05c09d87..57a153adba3a 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2406,33 +2406,6 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
>  	put_device(tty_dev);
>  	uport->suspended = 0;
>  
> -	/*
> -	 * Re-enable the console device after suspending.
> -	 */
> -	if (uart_console(uport)) {
> -		/*
> -		 * First try to use the console cflag setting.
> -		 */
> -		memset(&termios, 0, sizeof(struct ktermios));
> -		termios.c_cflag = uport->cons->cflag;
> -		termios.c_ispeed = uport->cons->ispeed;
> -		termios.c_ospeed = uport->cons->ospeed;
> -
> -		/*
> -		 * If that's unset, use the tty termios setting.
> -		 */
> -		if (port->tty && termios.c_cflag == 0)
> -			termios = port->tty->termios;
> -
> -		if (console_suspend_enabled)
> -			uart_change_pm(state, UART_PM_STATE_ON);
> -		uport->ops->set_termios(uport, &termios, NULL);
> -		if (!console_suspend_enabled && uport->ops->start_rx)
> -			uport->ops->start_rx(uport);
> -		if (console_suspend_enabled)
> -			console_start(uport->cons);
> -	}
> -
>  	if (tty_port_suspended(port)) {
>  		const struct uart_ops *ops = uport->ops;
>  		int ret;
> @@ -2471,6 +2444,33 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
>  		tty_port_set_suspended(port, false);
>  	}
>  
> +	/*
> +	 * Re-enable the console device after suspending.
> +	 */
> +	if (uart_console(uport)) {
> +		/*
> +		 * First try to use the console cflag setting.
> +		 */
> +		memset(&termios, 0, sizeof(struct ktermios));
> +		termios.c_cflag = uport->cons->cflag;
> +		termios.c_ispeed = uport->cons->ispeed;
> +		termios.c_ospeed = uport->cons->ospeed;
> +
> +		/*
> +		 * If that's unset, use the tty termios setting.
> +		 */
> +		if (port->tty && termios.c_cflag == 0)
> +			termios = port->tty->termios;
> +
> +		if (console_suspend_enabled)
> +			uart_change_pm(state, UART_PM_STATE_ON);
> +		uport->ops->set_termios(uport, &termios, NULL);
> +		if (!console_suspend_enabled && uport->ops->start_rx)
> +			uport->ops->start_rx(uport);
> +		if (console_suspend_enabled)
> +			console_start(uport->cons);
> +	}
> +
>  	mutex_unlock(&port->mutex);
>  
>  	return 0;
> 

To me it looks the whole function is too messed up to fix anything this 
easily. I'd start with splitting the two large ifs block so that the 
ordering makes sense:

- set_termios / uart_change_line_settings is called only once
- rx and tx is started only after set_termios
- failure path (the one doing uart_shutdown) logic is reverse + gotoed


-- 
 i.