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

[Matthias Kaehlcke]

On Mon, Mar 28, 2022 at 11:47:24PM +0530, Vijaya Krishna Nivarthi wrote:
> [Why]
> 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.
> 
> [How]

Please drop the [Why] / [How] 'tags'

> 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.
> Additionally other changes have been made at same place
> a) replace the hardcoded flags with macros
> b) perform voting before calling resume_port in resume sequence
> c) consequently, swap the order in suspend sequence

This patch is short, but IMO it still does too many things at once which
aren't all directly related. At the very least the change from hardcoded
flags to macros should be in a separate patch. If the ICC voting order
isn't direcly related with the console_suspend issue then I'd also suggest
to split it out into its own patch.

> Signed-off-by: Vijaya Krishna Nivarthi <quic_vnivarth@xxxxxxxxxxx>
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index aedc388..37d064f 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -19,6 +19,7 @@
>  #include <linux/slab.h>
>  #include <linux/tty.h>
>  #include <linux/tty_flip.h>
> +#include <dt-bindings/interconnect/qcom,icc.h>
>  
>  /* UART specific GENI registers */
>  #define SE_UART_LOOPBACK_CFG		0x22c
> @@ -1477,34 +1478,38 @@ static int qcom_geni_serial_remove(struct platform_device *pdev)
>  
>  static int __maybe_unused qcom_geni_serial_sys_suspend(struct device *dev)
>  {
> +	int ret;
>  	struct qcom_geni_serial_port *port = dev_get_drvdata(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 (!console_suspend_enabled && uart_console(uport))
> +		uport->ops->stop_rx(uport);
> +
>  	/*
>  	 * This is done so we can hit the lowest possible state in suspend
>  	 * even with no_console_suspend
>  	 */
> +	ret = uart_suspend_port(private_data->drv, uport);
>  	if (uart_console(uport)) {
> -		geni_icc_set_tag(&port->se, 0x3);
> +		geni_icc_set_tag(&port->se, QCOM_ICC_TAG_ACTIVE_ONLY);
>  		geni_icc_set_bw(&port->se);
>  	}
> -	return uart_suspend_port(private_data->drv, uport);
> +	return ret;
>  }
>  
>  static int __maybe_unused qcom_geni_serial_sys_resume(struct device *dev)
>  {
> -	int ret;
>  	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
>  	struct uart_port *uport = &port->uport;
>  	struct qcom_geni_private_data *private_data = uport->private_data;
>  
> -	ret = uart_resume_port(private_data->drv, uport);
>  	if (uart_console(uport)) {
> -		geni_icc_set_tag(&port->se, 0x7);
> +		geni_icc_set_tag(&port->se, QCOM_ICC_TAG_ALWAYS);
>  		geni_icc_set_bw(&port->se);
>  	}
> -	return ret;
> +	return uart_resume_port(private_data->drv, uport);
>  }
>  
>  static const struct dev_pm_ops qcom_geni_serial_pm_ops = {
> -- 
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.
>