Re: [PATCH v4 1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Mon, Jul 22, 2019 at 09:51:07AM +0200, Uwe Kleine-König wrote:
> On Mon, Jul 22, 2019 at 10:42:57AM +0300, Sergey Organov wrote:
> > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > 
> > > On Fri, Jul 19, 2019 at 06:13:52PM +0300, Sergey Organov wrote:
> > >> Hello Uwe,
> > >> 
> > >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > >> > Hello Sergey,
> > >> >
> > >> > On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
> > >> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > >> >> > On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
> > >> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > >> >> >> index 57d6e6b..95d7984 100644
> > >> >> >> --- a/drivers/tty/serial/imx.c
> > >> >> >> +++ b/drivers/tty/serial/imx.c
> > >> >> >> @@ -405,7 +405,8 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
> > >> >> >>  /* called with port.lock taken and irqs caller dependent */
> > >> >> >>  static void imx_uart_rts_auto(struct imx_port *sport, u32 *ucr2)
> > >> >> >>  {
> > >> >> >> -	*ucr2 |= UCR2_CTSC;
> > >> >> >> +	if (*ucr2 & UCR2_CTS)
> > >> >> >> +		*ucr2 |= UCR2_CTSC;
> > >> >> >
> > >> >> > I think this patch is wrong or the commit log is insufficient.
> > >> >> > imx_uart_rts_auto() has only a single caller and there ucr2 & UCR2_CTS is
> > >> >> > never true. And CTSC isn't restored anywhere, is it?
> > >> >> 
> > >> >> This is rebase to 'tty-next' branch, and you need to look at it in that
> > >> >> context. There, ucr2 & UCR2_CTS does already make sense, due to previous
> > >> >> fix that is already there.
> > >> >
> > >> > I looked at 57d6e6b which is the file you patched. And there
> > >> > imx_uart_rts_auto is only ever called with *ucr2 not having UCR2_CTS.
> > >> >
> > >> > If you still think I'm wrong, please improve the commit log
> > >> > accordingly.
> > >> 
> > >> I still think you are wrong, but I don't know how to improve commit log.
> > >> 
> > >> To check it once again, I just did:
> > >> 
> > >> $ git show 57d6e6b > imx.c
> > >> 
> > >> There, in imx_uart_set_termios(), I see:
> > >> 
> > >> 1569:	old_ucr2 = imx_uart_readl(sport, UCR2);
> > >> 1570:	ucr2 = old_ucr2 & (UCR2_TXEN | UCR2_RXEN | UCR2_ATEN | UCR2_CTS);
> > >> 
> > >> Here, current UCR2 value is read into 'old_ucr2' and then its /current/
> > >> UCR2_CTS bit is copied into 'ucr2' (along with 3 other bits).
> > >> 
> > >> Then, later in the same function:
> > >> 
> > >> 1591:		imx_uart_rts_auto(sport, &ucr2);
> > >> 
> > >> is called that can check /current/ state of UCR2_CTS bit in '*ucr2'.
> > >> 
> > >> That's what the patch does, checks this bit.
> > >> 
> > >> Sorry, I fail to see how you came to conclusion that "*ucr2 not having
> > >> UCR2_CTS". Do we maybe still read different versions of the file?
> > >
> > > No, it's just that I failed to see that UCR2_CTS is in the set of bits
> > > that are retained even when looking twice :-|
> > 
> > Ah, that one... How familiar :-)
> 
> I thought again a bit over the weekend about this. I wonder if it's
> correct to keep RTS active while going through .set_termios. Shouldn't
> it maybe always be inactive to prevent the other side sending data while
> we are changing the baud rate?

Only if CRTSCTS is enabled, and then there are a lot of serial drivers
to fix.

However, RTS is not guaranteed to stop the remote end sending characters
as soon as it is deasserted - 16550 relies on software noticing that
CTS has changed, and even then it may have a FIFO full of characters
already queued to be sent that will still be sent.

So, disabling RTS just before changing the baud doesn't give any
guarantees that a character won't be in the process of being received
while we're changing the baud rate, which means disabling it doesn't
actually gain us anything.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel