For the case of console_suspend disabled, if back to back suspend/resume
test is executed, at the end of test, sometimes console would appear to
be frozen not responding to input. This would happen because, for
console_suspend disabled, suspend/resume routines only turn resources
off/on but don't do a port close/open.
As a result, during resume, some rx transactions come in before system is
ready, malfunction of rx happens in turn resulting in console appearing
to be stuck.

Do a stop_rx in suspend sequence to prevent this. start_rx is already
present in resume sequence as part of call to set_termios which does a
stop_rx/start_rx.

Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>
---
v3: swapped the order of conditions to be more human readable
v2: restricted patch to contain only stop_rx in suspend sequence
v1: intial patch contained 2 additional unrelated changes in vicinity
---
 drivers/tty/serial/qcom_geni_serial.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 1543a60..53723d2 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1481,6 +1481,10 @@ static int __maybe_unused qcom_geni_serial_sys_suspend(struct device *dev)
 	struct uart_port *uport = &port->uport;
 	struct qcom_geni_private_data *private_data = uport->private_data;
 
+	/* do a stop_rx here, start_rx is handled in uart_resume_port by call to setermios */
+	if (uart_console(uport) && !console_suspend_enabled)
+		uport->ops->stop_rx(uport);
+
 	/*
 	 * This is done so we can hit the lowest possible state in suspend
 	 * even with no_console_suspend
-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.

On 07. 04. 22, 9:25, Vijaya Krishna Nivarthi wrote:
> For the case of console_suspend disabled, if back to back suspend/resume
> test is executed, at the end of test, sometimes console would appear to
> be frozen not responding to input. This would happen because, for
> console_suspend disabled, suspend/resume routines only turn resources
> off/on but don't do a port close/open.
> As a result, during resume, some rx transactions come in before system is
> ready, malfunction of rx happens in turn resulting in console appearing
> to be stuck.
> 
> Do a stop_rx in suspend sequence to prevent this. start_rx is already
> present in resume sequence as part of call to set_termios which does a
> stop_rx/start_rx.

So why is it OK for every other driver? Should uart_suspend_port() be 
fixed instead?

> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>
> ---
> v3: swapped the order of conditions to be more human readable
> v2: restricted patch to contain only stop_rx in suspend sequence
> v1: intial patch contained 2 additional unrelated changes in vicinity
> ---
>   drivers/tty/serial/qcom_geni_serial.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 1543a60..53723d2 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1481,6 +1481,10 @@ static int __maybe_unused qcom_geni_serial_sys_suspend(struct device *dev)
>   	struct uart_port *uport = &port->uport;
>   	struct qcom_geni_private_data *private_data = uport->private_data;
>   
> +	/* do a stop_rx here, start_rx is handled in uart_resume_port by call to setermios */
> +	if (uart_console(uport) && !console_suspend_enabled)
> +		uport->ops->stop_rx(uport);
> +
>   	/*
>   	 * This is done so we can hit the lowest possible state in suspend
>   	 * even with no_console_suspend

thanks,
-- 
js
suse labs

On 4/7/2022 1:21 PM, Jiri Slaby wrote:
> On 07. 04. 22, 9:25, Vijaya Krishna Nivarthi wrote:
>> For the case of console_suspend disabled, if back to back suspend/resume
>> test is executed, at the end of test, sometimes console would appear to
>> be frozen not responding to input. This would happen because, for
>> console_suspend disabled, suspend/resume routines only turn resources
>> off/on but don't do a port close/open.
>> As a result, during resume, some rx transactions come in before 
>> system is
>> ready, malfunction of rx happens in turn resulting in console appearing
>> to be stuck.
>>
>> Do a stop_rx in suspend sequence to prevent this. start_rx is already
>> present in resume sequence as part of call to set_termios which does a
>> stop_rx/start_rx.
>
> So why is it OK for every other driver? Should uart_suspend_port() be 
> fixed instead?

For qcom driver we know that set_termios() call in uart_suspend_port() 
will recover with a call to start_rx.
However that may not be the case with other drivers.

We can move stop_rx to uart_suspend_port() and additionally have a 
start_rx in uart_resume_port()
Please let know if such a change would be ok.
Thank you.

On Fri, Apr 08, 2022 at 11:45:11AM +0530, Vijaya Krishna Nivarthi wrote:
> 
> On 4/7/2022 1:21 PM, Jiri Slaby wrote:
> > On 07. 04. 22, 9:25, Vijaya Krishna Nivarthi wrote:
> > > For the case of console_suspend disabled, if back to back suspend/resume
> > > test is executed, at the end of test, sometimes console would appear to
> > > be frozen not responding to input. This would happen because, for
> > > console_suspend disabled, suspend/resume routines only turn resources
> > > off/on but don't do a port close/open.
> > > As a result, during resume, some rx transactions come in before
> > > system is
> > > ready, malfunction of rx happens in turn resulting in console appearing
> > > to be stuck.
> > > 
> > > Do a stop_rx in suspend sequence to prevent this. start_rx is already
> > > present in resume sequence as part of call to set_termios which does a
> > > stop_rx/start_rx.
> > 
> > So why is it OK for every other driver? Should uart_suspend_port() be
> > fixed instead?
> 
> For qcom driver we know that set_termios() call in uart_suspend_port() will
> recover with a call to start_rx.
> However that may not be the case with other drivers.
> 
> We can move stop_rx to uart_suspend_port() and additionally have a start_rx
> in uart_resume_port()
> Please let know if such a change would be ok.

This should not be something that each individual driver has to do,
please fix it for everyone.

thanks,

greg k-h