[v4,1/3] serial: imx: set_termios(): do not enable autoRTS if RTS is unset
diff mbox series

Message ID 1563526074-20399-2-git-send-email-sorganov@gmail.com
State New
Headers show
Series
  • serial: imx: fix RTS and RTS/CTS handling
Related show

Commit Message

Sergey Organov July 19, 2019, 8:47 a.m. UTC
set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
cleared. Added corresponding check in imx_uart_rts_auto() to fix this.

Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Uwe Kleine-König July 19, 2019, 9:11 a.m. UTC | #1
On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
> set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
> cleared. Added corresponding check in imx_uart_rts_auto() to fix this.

This is not understandable unless you read the reference manual.

In terms understandable without manual, this patch does:

	Don't let the receiver hardware control the RTS output if it was
	requested to be inactive.

> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  drivers/tty/serial/imx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 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?

Best regards
Uwe
Sergey Organov July 19, 2019, 12:18 p.m. UTC | #2
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

> On Fri, Jul 19, 2019 at 11:47:52AM +0300, Sergey Organov wrote:
>> set_termios() shouldn't set UCR2_CTSC bit if UCR2_CTS (=TIOCM_RTS) is
>> cleared. Added corresponding check in imx_uart_rts_auto() to fix this.
>
> This is not understandable unless you read the reference manual.
>
> In terms understandable without manual, this patch does:
>
> 	Don't let the receiver hardware control the RTS output if it was
> 	requested to be inactive.

I'll fix it, thanks!

>
>> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
>> Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  drivers/tty/serial/imx.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> 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.

Alternatively, look at v3 of the patches, but you basically already did.
It's that the first part of v3 patch series has been already accepted
upstream, and this is the rest of those patches.

Thanks!

-- Sergey
Uwe Kleine-König July 19, 2019, 2:31 p.m. UTC | #3
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.

Best regards
Uwe
Sergey Organov July 19, 2019, 3:13 p.m. UTC | #4
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?

-- Sergey
Uwe Kleine-König July 19, 2019, 8:19 p.m. UTC | #5
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 :-|

So you convinced me that you are right and if you update the commit log
as agreed already before and even add a comment in imx_uart_rts_auto
along the lines of

	/*
	 * Only let the receiver control RTS output if we were not
	 * requested to have RTS inactive (which then should take
	 * precedence).
	 */

you can have my Ack.

Best regards
Uwe
Sergey Organov July 22, 2019, 7:42 a.m. UTC | #6
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 :-)

> So you convinced me that you are right and if you update the commit log
> as agreed already before and even add a comment in imx_uart_rts_auto
> along the lines of
>
> 	/*
> 	 * Only let the receiver control RTS output if we were not
> 	 * requested to have RTS inactive (which then should take
> 	 * precedence).
> 	 */
>
> you can have my Ack.

OK, will do

Thanks!

-- Sergey
Uwe Kleine-König July 22, 2019, 7:51 a.m. UTC | #7
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?

Best regards
Uwe
Sergey Organov July 22, 2019, 9:20 a.m. UTC | #8
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.

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
Uwe Kleine-König July 22, 2019, 9:46 a.m. UTC | #9
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.

Best regards
Uwe
Russell King - ARM Linux admin July 22, 2019, 9:57 a.m. UTC | #10
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.
Uwe Kleine-König July 22, 2019, 10:04 a.m. UTC | #11
Hello Russell,

On Mon, Jul 22, 2019 at 10:57:21AM +0100, Russell King - ARM Linux admin wrote:
> 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.

Ack, I included this condition in a later mail already.

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

<sarcasm>With that reasoning we can drop RTS driving completely. Let's
just assert it unconditionally.</sarcam>

Right, deasserting RTS doesn't help against long receive FIFOs or
senders that react slowly (or not at all), but still it's better than
nothing, isn't it?

Best regards
Uwe
Russell King - ARM Linux admin July 22, 2019, 10:17 a.m. UTC | #12
On Mon, Jul 22, 2019 at 12:04:14PM +0200, Uwe Kleine-König wrote:
> Hello Russell,
> 
> On Mon, Jul 22, 2019 at 10:57:21AM +0100, Russell King - ARM Linux admin wrote:
> > 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.
> 
> <sarcasm>With that reasoning we can drop RTS driving completely. Let's
> just assert it unconditionally.</sarcam>

Please, I'm being serious.

> Right, deasserting RTS doesn't help against long receive FIFOs or
> senders that react slowly (or not at all), but still it's better than
> nothing, isn't it?

Not really.

In the normal use case of RTS, RTS doesn't get deasserted when there is
no buffer space available, but when the available buffer space reaches
a low-threshold.  Buffer space remains to allow the sender to react to
the change of RTS state.

In the case you are promoting, which is to deassert RTS and then
immediately start changing the port settings, you are not giving any
chance for the sender to react to that state change.  You could even
be mid-way through receiving a character from the remote end - and
at 75 baud, a single character lasts around 133ms.

Even if you wait 133ms, that doesn't mean that the remote end has
finished sending.  If the remote end has a 16 byte FIFO, you'd need
to wait about 2.2 seconds.  At faster baud rates, of course the
delay gets shorter.

Just deasserting RTS just before changing the port settings gains
very little protection.  You need a character-period based delay as
well.

If we do start adding delays, it means that changing the baud rate
for a port set to 75 baud starts taking ages to complete if CRTSCTS
is enabled, irrespective of the settings change mode that was
requested by userspace.

However, adding delays is likely to screw up various userspace
applications, such as those that do need to change baud rate at
specific points in their protocol.
Sergey Organov July 22, 2019, 1:54 p.m. UTC | #13
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.

In this particular case, application is free to deassert RTS before it
decides to change communication parameters, if it actually needs to, and
thus kernel is wrong level for doing this.

Best Regards,

-- Sergey
Uwe Kleine-König July 22, 2019, 4:47 p.m. UTC | #14
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

Patch
diff mbox series

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;
 }
 
 /* called with port.lock taken and irqs off */