* [PATCH 1/2] tty: serial: imx: add lock for registers save/restore @ 2018-08-09 10:06 Anson Huang 2018-08-09 10:06 ` [PATCH 2/2] tty: serial: imx: add pinctrl sleep/default mode switch for suspend Anson Huang 2018-09-04 20:32 ` [PATCH 1/2] tty: serial: imx: add lock for registers save/restore Uwe Kleine-König 0 siblings, 2 replies; 6+ messages in thread From: Anson Huang @ 2018-08-09 10:06 UTC (permalink / raw) To: gregkh, jslaby, linux-serial, linux-kernel; +Cc: Linux-imx 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] tty: serial: imx: add pinctrl sleep/default mode switch for suspend 2018-08-09 10:06 [PATCH 1/2] tty: serial: imx: add lock for registers save/restore Anson Huang @ 2018-08-09 10:06 ` Anson Huang 2018-09-03 8:51 ` Anson Huang 2018-09-04 20:33 ` Uwe Kleine-König 2018-09-04 20:32 ` [PATCH 1/2] tty: serial: imx: add lock for registers save/restore Uwe Kleine-König 1 sibling, 2 replies; 6+ messages in thread From: Anson Huang @ 2018-08-09 10:06 UTC (permalink / raw) To: gregkh, jslaby, linux-serial, linux-kernel; +Cc: Linux-imx 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH 2/2] tty: serial: imx: add pinctrl sleep/default mode switch for suspend 2018-08-09 10:06 ` [PATCH 2/2] tty: serial: imx: add pinctrl sleep/default mode switch for suspend Anson Huang @ 2018-09-03 8:51 ` Anson Huang 2018-09-04 20:33 ` Uwe Kleine-König 1 sibling, 0 replies; 6+ messages in thread From: Anson Huang @ 2018-09-03 8:51 UTC (permalink / raw) To: gregkh, jslaby, linux-serial, linux-kernel; +Cc: dl-linux-imx 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] tty: serial: imx: add pinctrl sleep/default mode switch for suspend 2018-08-09 10:06 ` [PATCH 2/2] tty: serial: imx: add pinctrl sleep/default mode switch for suspend Anson Huang 2018-09-03 8:51 ` Anson Huang @ 2018-09-04 20:33 ` Uwe Kleine-König 1 sibling, 0 replies; 6+ messages in thread From: Uwe Kleine-König @ 2018-09-04 20:33 UTC (permalink / raw) To: Anson Huang; +Cc: gregkh, jslaby, linux-serial, linux-kernel, Linux-imx 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/ | ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] tty: serial: imx: add lock for registers save/restore 2018-08-09 10:06 [PATCH 1/2] tty: serial: imx: add lock for registers save/restore Anson Huang 2018-08-09 10:06 ` [PATCH 2/2] tty: serial: imx: add pinctrl sleep/default mode switch for suspend Anson Huang @ 2018-09-04 20:32 ` Uwe Kleine-König 2018-09-05 1:20 ` Anson Huang 1 sibling, 1 reply; 6+ messages in thread From: Uwe Kleine-König @ 2018-09-04 20:32 UTC (permalink / raw) To: Anson Huang; +Cc: gregkh, jslaby, linux-serial, linux-kernel, Linux-imx 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/ | ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 1/2] tty: serial: imx: add lock for registers save/restore 2018-09-04 20:32 ` [PATCH 1/2] tty: serial: imx: add lock for registers save/restore Uwe Kleine-König @ 2018-09-05 1:20 ` Anson Huang 0 siblings, 0 replies; 6+ messages in thread From: Anson Huang @ 2018-09-05 1:20 UTC (permalink / raw) To: Uwe Kleine-König Cc: gregkh, jslaby, linux-serial, linux-kernel, dl-linux-imx 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&data=02%7C01%7Canson.huang%40nxp.com%7C74 > a28e347934451a59bf08d612a57f8c%7C686ea1d3bc2b4c6fa92cd99c5c301635 > %7C0%7C0%7C636716899353510311&sdata=lByZDd%2BbQitjEf%2FaiN3 > e26El3ErT0fnmvaKCqckF7Qc%3D&reserved=0 | ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-09-05 1:20 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-09 10:06 [PATCH 1/2] tty: serial: imx: add lock for registers save/restore Anson Huang 2018-08-09 10:06 ` [PATCH 2/2] tty: serial: imx: add pinctrl sleep/default mode switch for suspend Anson Huang 2018-09-03 8:51 ` Anson Huang 2018-09-04 20:33 ` Uwe Kleine-König 2018-09-04 20:32 ` [PATCH 1/2] tty: serial: imx: add lock for registers save/restore Uwe Kleine-König 2018-09-05 1:20 ` Anson Huang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).