[PATCH 1/2] tty: serial: imx: add lock for registers save/restore

[PATCH 1/2] tty: serial: imx: add lock for registers save/restore

[Anson Huang]

In noirq suspend/resume stage with no_console_suspend enabled,
.imx_console_write() may be called to print out log_buf
message by .printk(), so there will be race condition between
.imx_console_write() and .serial_imx_save/restore_context(),
need to add lock to protect the registers save/restore operations.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/tty/serial/imx.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 239c0fa..a09ccef 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2376,9 +2376,12 @@ static int imx_uart_remove(struct platform_device *pdev)
 
 static void imx_uart_restore_context(struct imx_port *sport)
 {
+	unsigned long flags;
+
 	if (!sport->context_saved)
 		return;
 
+	spin_lock_irqsave(&sport->port.lock, flags);
 	imx_uart_writel(sport, sport->saved_reg[4], UFCR);
 	imx_uart_writel(sport, sport->saved_reg[5], UESC);
 	imx_uart_writel(sport, sport->saved_reg[6], UTIM);
@@ -2390,11 +2393,15 @@ static void imx_uart_restore_context(struct imx_port *sport)
 	imx_uart_writel(sport, sport->saved_reg[2], UCR3);
 	imx_uart_writel(sport, sport->saved_reg[3], UCR4);
 	sport->context_saved = false;
+	spin_unlock_irqrestore(&sport->port.lock, flags);
 }
 
 static void imx_uart_save_context(struct imx_port *sport)
 {
+	unsigned long flags;
+
 	/* Save necessary regs */
+	spin_lock_irqsave(&sport->port.lock, flags);
 	sport->saved_reg[0] = imx_uart_readl(sport, UCR1);
 	sport->saved_reg[1] = imx_uart_readl(sport, UCR2);
 	sport->saved_reg[2] = imx_uart_readl(sport, UCR3);
@@ -2406,6 +2413,7 @@ static void imx_uart_save_context(struct imx_port *sport)
 	sport->saved_reg[8] = imx_uart_readl(sport, UBMR);
 	sport->saved_reg[9] = imx_uart_readl(sport, IMX21_UTS);
 	sport->context_saved = true;
+	spin_unlock_irqrestore(&sport->port.lock, flags);
 }
 
 static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
-- 
2.7.4

[PATCH 2/2] tty: serial: imx: add pinctrl sleep/default mode switch for suspend

[Anson Huang]

On some i.MX SoCs' low power mode, UART iomux settings
may be lost, need to add pinctrl sleep/default mode switch
during suspend/resume to make sure UART iomux settings are
correct after resume.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/tty/serial/imx.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index a09ccef..c280d43 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -24,6 +24,7 @@
 #include <linux/serial.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/rational.h>
 #include <linux/slab.h>
 #include <linux/of.h>
@@ -2447,6 +2448,8 @@ static int imx_uart_suspend_noirq(struct device *dev)
 
 	clk_disable(sport->clk_ipg);
 
+	pinctrl_pm_select_sleep_state(dev);
+
 	return 0;
 }
 
@@ -2455,6 +2458,8 @@ static int imx_uart_resume_noirq(struct device *dev)
 	struct imx_port *sport = dev_get_drvdata(dev);
 	int ret;
 
+	pinctrl_pm_select_default_state(dev);
+
 	ret = clk_enable(sport->clk_ipg);
 	if (ret)
 		return ret;
-- 
2.7.4

RE: [PATCH 2/2] tty: serial: imx: add pinctrl sleep/default mode switch for suspend

[Anson Huang]

Gentle Ping...

Anson Huang
Best Regards!


> -----Original Message-----
> From: Anson Huang
> Sent: Thursday, August 9, 2018 6:06 PM
> To: gregkh@linuxfoundation.org; jslaby@suse.com;
> linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: [PATCH 2/2] tty: serial: imx: add pinctrl sleep/default mode switch for
> suspend
> 
> On some i.MX SoCs' low power mode, UART iomux settings may be lost, need
> to add pinctrl sleep/default mode switch during suspend/resume to make sure
> UART iomux settings are correct after resume.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/tty/serial/imx.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index
> a09ccef..c280d43 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -24,6 +24,7 @@
>  #include <linux/serial.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/rational.h>
>  #include <linux/slab.h>
>  #include <linux/of.h>
> @@ -2447,6 +2448,8 @@ static int imx_uart_suspend_noirq(struct device
> *dev)
> 
>  	clk_disable(sport->clk_ipg);
> 
> +	pinctrl_pm_select_sleep_state(dev);
> +
>  	return 0;
>  }
> 
> @@ -2455,6 +2458,8 @@ static int imx_uart_resume_noirq(struct device
> *dev)
>  	struct imx_port *sport = dev_get_drvdata(dev);
>  	int ret;
> 
> +	pinctrl_pm_select_default_state(dev);
> +
>  	ret = clk_enable(sport->clk_ipg);
>  	if (ret)
>  		return ret;
> --
> 2.7.4

Re: [PATCH 1/2] tty: serial: imx: add lock for registers save/restore

[Uwe Kleine-König]

Hello,

On Thu, Aug 09, 2018 at 06:06:08PM +0800, Anson Huang wrote:
> In noirq suspend/resume stage with no_console_suspend enabled,
> .imx_console_write() may be called to print out log_buf
> message by .printk(), so there will be race condition between
> .imx_console_write() and .serial_imx_save/restore_context(),
> need to add lock to protect the registers save/restore operations.

The function names changes some time ago. Also I'd drop the leading dots
in the names which I would understand as members in a struct.

Is this a theoretical issue, or did you see actual breakage?

> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/tty/serial/imx.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 239c0fa..a09ccef 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -2376,9 +2376,12 @@ static int imx_uart_remove(struct platform_device *pdev)
>  
>  static void imx_uart_restore_context(struct imx_port *sport)
>  {
> +	unsigned long flags;
> +
>  	if (!sport->context_saved)
>  		return;

Is it safe to check .context_saved without holding the lock?

> +	spin_lock_irqsave(&sport->port.lock, flags);
>  	imx_uart_writel(sport, sport->saved_reg[4], UFCR);
>  	imx_uart_writel(sport, sport->saved_reg[5], UESC);
>  	imx_uart_writel(sport, sport->saved_reg[6], UTIM);
> @@ -2390,11 +2393,15 @@ static void imx_uart_restore_context(struct imx_port *sport)
>  	imx_uart_writel(sport, sport->saved_reg[2], UCR3);
>  	imx_uart_writel(sport, sport->saved_reg[3], UCR4);
>  	sport->context_saved = false;
> +	spin_unlock_irqrestore(&sport->port.lock, flags);
>  }
>  
>  static void imx_uart_save_context(struct imx_port *sport)
>  {
> +	unsigned long flags;
> +
>  	/* Save necessary regs */
> +	spin_lock_irqsave(&sport->port.lock, flags);
>  	sport->saved_reg[0] = imx_uart_readl(sport, UCR1);
>  	sport->saved_reg[1] = imx_uart_readl(sport, UCR2);
>  	sport->saved_reg[2] = imx_uart_readl(sport, UCR3);
> @@ -2406,6 +2413,7 @@ static void imx_uart_save_context(struct imx_port *sport)
>  	sport->saved_reg[8] = imx_uart_readl(sport, UBMR);
>  	sport->saved_reg[9] = imx_uart_readl(sport, IMX21_UTS);
>  	sport->context_saved = true;
> +	spin_unlock_irqrestore(&sport->port.lock, flags);
>  }
>  
>  static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/2] tty: serial: imx: add pinctrl sleep/default mode switch for suspend

[Uwe Kleine-König]

On Thu, Aug 09, 2018 at 06:06:09PM +0800, Anson Huang wrote:
> On some i.MX SoCs' low power mode, UART iomux settings

Would be nice to point out specific affected SoCs.

> may be lost, need to add pinctrl sleep/default mode switch
> during suspend/resume to make sure UART iomux settings are
> correct after resume.

Otherwise the change looks fine.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH 1/2] tty: serial: imx: add lock for registers save/restore

[Anson Huang]

Hi, Uwe

Anson Huang
Best Regards!


> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: Wednesday, September 5, 2018 4:32 AM
> To: Anson Huang <anson.huang@nxp.com>
> Cc: gregkh@linuxfoundation.org; jslaby@suse.com;
> linux-serial@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH 1/2] tty: serial: imx: add lock for registers save/restore
> 
> Hello,
> 
> On Thu, Aug 09, 2018 at 06:06:08PM +0800, Anson Huang wrote:
> > In noirq suspend/resume stage with no_console_suspend enabled,
> > .imx_console_write() may be called to print out log_buf message by
> > .printk(), so there will be race condition between
> > .imx_console_write() and .serial_imx_save/restore_context(),
> > need to add lock to protect the registers save/restore operations.
> 
> The function names changes some time ago. Also I'd drop the leading dots in
> the names which I would understand as members in a struct.
> 
> Is this a theoretical issue, or did you see actual breakage?
 
I will update the function name and remove the dots.

And we did see actual breakage during stress test, although the reproduce rate
is NOT that high, but the issue came out and fixed by this patch. 

> 
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  drivers/tty/serial/imx.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index
> > 239c0fa..a09ccef 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -2376,9 +2376,12 @@ static int imx_uart_remove(struct
> > platform_device *pdev)
> >
> >  static void imx_uart_restore_context(struct imx_port *sport)  {
> > +	unsigned long flags;
> > +
> >  	if (!sport->context_saved)
> >  		return;
> 
> Is it safe to check .context_saved without holding the lock?
 
Ah, my mistake, will put the check inside the lock. Please help
review V2 patch set, thanks.

Anson.

> 
> > +	spin_lock_irqsave(&sport->port.lock, flags);
> >  	imx_uart_writel(sport, sport->saved_reg[4], UFCR);
> >  	imx_uart_writel(sport, sport->saved_reg[5], UESC);
> >  	imx_uart_writel(sport, sport->saved_reg[6], UTIM); @@ -2390,11
> > +2393,15 @@ static void imx_uart_restore_context(struct imx_port *sport)
> >  	imx_uart_writel(sport, sport->saved_reg[2], UCR3);
> >  	imx_uart_writel(sport, sport->saved_reg[3], UCR4);
> >  	sport->context_saved = false;
> > +	spin_unlock_irqrestore(&sport->port.lock, flags);
> >  }
> >
> >  static void imx_uart_save_context(struct imx_port *sport)  {
> > +	unsigned long flags;
> > +
> >  	/* Save necessary regs */
> > +	spin_lock_irqsave(&sport->port.lock, flags);
> >  	sport->saved_reg[0] = imx_uart_readl(sport, UCR1);
> >  	sport->saved_reg[1] = imx_uart_readl(sport, UCR2);
> >  	sport->saved_reg[2] = imx_uart_readl(sport, UCR3); @@ -2406,6
> > +2413,7 @@ static void imx_uart_save_context(struct imx_port *sport)
> >  	sport->saved_reg[8] = imx_uart_readl(sport, UBMR);
> >  	sport->saved_reg[9] = imx_uart_readl(sport, IMX21_UTS);
> >  	sport->context_saved = true;
> > +	spin_unlock_irqrestore(&sport->port.lock, flags);
> >  }
> >
> >  static void imx_uart_enable_wakeup(struct imx_port *sport, bool on)
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7C74
> a28e347934451a59bf08d612a57f8c%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636716899353510311&amp;sdata=lByZDd%2BbQitjEf%2FaiN3
> e26El3ErT0fnmvaKCqckF7Qc%3D&amp;reserved=0  |