On Mon, Jul 22, 2019 at 12:20:02PM +0300, Sergey Organov wrote:
> Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> writes:
>
> > On Mon, Jul 22, 2019 at 10:42:57AM +0300, Sergey Organov wrote:
> >> Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> writes:
> >>
> >> > On Fri, Jul 19, 2019 at 06:13:52PM +0300, Sergey Organov wrote:
> >> >> Hello Uwe,
> >> >>
> >> >> Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> writes:
> >> >> > Hello Sergey,
> >> >> >
> >> >> > On Fri, Jul 19, 2019 at 03:18:13PM +0300, Sergey Organov wrote:
> >> >> >> Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> 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.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
