linux-kernel - RE: [PATCH] tty: serial: imx: disable UCR4_OREN in .stop

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <AS8PR04MB8404219D85CCB5B743EA378492609@AS8PR04MB8404.eurprd04.prod.outlook.com>
Date:   Tue, 23 Nov 2021 09:05:07 +0000
From:   Sherry Sun <sherry.sun@....com>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "jirislaby@...nel.org" <jirislaby@...nel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH] tty: serial: imx: disable UCR4_OREN in .stop_rx()

Hi Uwe,

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
> Sent: 2021年11月23日 16:43
> To: Sherry Sun <sherry.sun@....com>
> Cc: gregkh@...uxfoundation.org; jirislaby@...nel.org; linux-
> serial@...r.kernel.org; linux-kernel@...r.kernel.org; dl-linux-imx <linux-
> imx@....com>
> Subject: Re: [PATCH] tty: serial: imx: disable UCR4_OREN in .stop_rx()
> 
> Hello again,
> 
> On Tue, Nov 23, 2021 at 08:51:22AM +0100, Uwe Kleine-König wrote:
> > On Tue, Nov 23, 2021 at 03:04:37PM +0800, Sherry Sun wrote:
> > > From: Fugang Duan <fugang.duan@....com>
> > >
> > > Disable UCR4_OREN bit in .stop_rx() to avoid endless interrupt
> > > happen while tty port is closing.
> > >
> > > Signed-off-by: Fugang Duan <fugang.duan@....com>
> > > Signed-off-by: Sherry Sun <sherry.sun@....com>
> > > ---
> > >  drivers/tty/serial/imx.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > index fb75e3e0d828..9ecaf051249d 100644
> > > --- a/drivers/tty/serial/imx.c
> > > +++ b/drivers/tty/serial/imx.c
> > > @@ -486,18 +486,21 @@ static void imx_uart_stop_tx(struct uart_port
> > > *port)  static void imx_uart_stop_rx(struct uart_port *port)  {
> > >  	struct imx_port *sport = (struct imx_port *)port;
> > > -	u32 ucr1, ucr2;
> > > +	u32 ucr1, ucr2, ucr4;
> > >
> > >  	ucr1 = imx_uart_readl(sport, UCR1);
> > >  	ucr2 = imx_uart_readl(sport, UCR2);
> > > +	ucr4 = imx_uart_readl(sport, UCR4);
> > >
> > >  	if (sport->dma_is_enabled) {
> > >  		ucr1 &= ~(UCR1_RXDMAEN | UCR1_ATDMAEN);
> > >  	} else {
> > >  		ucr1 &= ~UCR1_RRDYEN;
> > >  		ucr2 &= ~UCR2_ATEN;
> > > +		ucr4 &= ~UCR4_OREN;
> > >  	}
> > >  	imx_uart_writel(sport, ucr1, UCR1);
> > > +	imx_uart_writel(sport, ucr4, UCR4);
> >
> > I don't understand this yet, but I guess this is just a matter of
> > improving the commit log. How does the endless irq happen?

To be frankly, this patch is an old local patch, I also don't know the history about it.
But I checked the code logic is fine, the UCR4_OREN should be disabled when stop rx.

> >
> > ... some time later after thinking and reading the driver source ...
> >
> > Hmm, when the OR event is triggered the handler does
> >
> > 	imx_uart_writel(sport, USR2_ORE, USR2);
> >
> > and so clears the event. I would expect that this should be good
> > enough to prevent an irq storm even if the receiver is disabled.
> > Doesn't acking work with the receiver disabled?
> 
> One thing I forgot to mention:
> 
> If imx_uart_stop_rx() now clears UCR4_OREN, clearing in
> imx_uart_shutdown() should probably be dropped.

You are right, with this patch, clear the UCR4_OREN in imx_uart_shutdown is not necessary any more, I will remove it and send V2. Thank!

Best regards
Sherry

> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |