[V3] drivers/tty/serial/qcom-geni-serial: Do stop_rx in suspend path

[V3] drivers/tty/serial/qcom-geni-serial: Do stop_rx in suspend path

[Vijaya Krishna Nivarthi]

V1 patch contained additional unrelated changes in vicinity
a) replace hardcoded flags with macros
b) perform voting before calling resume_port in resume sequence
c) consequent to b, swap the order in suspend sequence as well
These will be take up in later patches after this one lands.
Thank you.

Vijaya Krishna Nivarthi (1):
  drivers/tty/serial/qcom-geni-serial: Do stop_rx in suspend path for
    console if console_suspend is disabled

 drivers/tty/serial/qcom_geni_serial.c | 4 ++++
 1 file changed, 4 insertions(+)

-- 

[V3] drivers/tty/serial/qcom-geni-serial: Do stop_rx in suspend path for console if console_suspend is disabled

[Vijaya Krishna Nivarthi]

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.

Re: [V3] drivers/tty/serial/qcom-geni-serial: Do stop_rx in suspend path for console if console_suspend is disabled

[Jiri Slaby]

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

Re: [V3] drivers/tty/serial/qcom-geni-serial: Do stop_rx in suspend path for console if console_suspend is disabled

[Vijaya Krishna Nivarthi]

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.

Re: [V3] drivers/tty/serial/qcom-geni-serial: Do stop_rx in suspend path for console if console_suspend is disabled

[Greg KH]

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