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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

On Mon, Jul 22, 2019 at 04:54:28PM +0300, Sergey Organov wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> 
> > On Mon, Jul 22, 2019 at 12:20:02PM +0300, Sergey Organov wrote:
> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> >> 
> >> > 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?
> >> 
> >> I don't think it's a good idea to change RTS state over .set_terimios,
> >> as it doesn't in fact solve anything (notice that the other end should
> >> also change baud rate accordingly), and changes visible state (even if
> >> temporarily) that it was not asked to change, that could in turn lead to
> >> utter surprises.
> >
> > It should for sure not be done in imx-uart specific code. But I think
> > that deasserting RTS before calling .set_termios (iff rtscts is enabled)
> > is a sane thing to do for generic code. I don't want to motivate the
> > other side to send data while I reconfigure my receiver. Yes, this is a
> > visible change, but IMHO a good one.
> >
> >> Correct changing of baud rates, bits, etc., could only be implemented at
> >> communication protocol level (read: application level), and there are
> >> all the tools in the kernel to do it right, provided driver does not do
> >> what it was not asked to do.
> >
> > Hmm, deasserting RTS while not being ready helps here. Otherwise the
> > communication partner that sends first after both agreed to change the
> > baud rate might start doing that before the receiver on the other end is
> > done. When RTS is deasserted this race window is at least smaller.
> 
> In general, it's a wrong idea to do in the kernel what could be done as
> efficiently at application level.

I agree a bit here. If the resulting behaviour when relying on userspace
might be wrong, the kernel should be free to fix (or smooth) it. (Even a
WARN_ON_ONCE would be fine in my eyes.) But I don't care enough to
discuss this further. The behaviour with the patch under discussion is
better than before, so let's settle it with that.

Best regards
Uwe

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