| Message ID | 1563526074-20399-2-git-send-email-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | serial: imx: fix RTS and RTS/CTS handling | expand |
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
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
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
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
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
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
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
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
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
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.
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
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.
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
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
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 */