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.
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.
-- Sergey
