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@xxxxxxxxxxxxxx>
> Sent: Wednesday, September 5, 2018 4:32 AM
> To: Anson Huang <anson.huang@xxxxxxx>
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; jslaby@xxxxxxxx;
> linux-serial@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>
> 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@xxxxxxx>
> > ---
> >  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  |