[PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"

[PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"

[Su Bao Cheng]

This reverts commit f45709df7731ad36306a28a3e1af7309d55c35f5.

The `serial8250_do_set_mctrl` not only used by userspace ioctl but
also the kernel itself.

During tty_open, the uart_port_startup sets the MCR to 0, and then use
set_mctrl to restore the MCR, so at this time, the MCR read does not
reflect the desired value.

Signed-off-by: Su Bao Cheng <baocheng.su@siemens.com>
---
 drivers/tty/serial/8250/8250_port.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 66374704747e..40736e460956 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2024,13 +2024,6 @@ void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	struct uart_8250_port *up = up_to_u8250p(port);
 	unsigned char mcr;
 
-	if (port->rs485.flags & SER_RS485_ENABLED) {
-		if (serial8250_in_MCR(up) & UART_MCR_RTS)
-			mctrl |= TIOCM_RTS;
-		else
-			mctrl &= ~TIOCM_RTS;
-	}
-
 	mcr = serial8250_TIOCM_to_MCR(mctrl);
 
 	mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr;
-- 
2.25.1

Re: [PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"

[Lukas Wunner]

On Wed, Oct 27, 2021 at 07:16:44PM +0800, Su Bao Cheng wrote:
> This reverts commit f45709df7731ad36306a28a3e1af7309d55c35f5.
> 
> The `serial8250_do_set_mctrl` not only used by userspace ioctl but
> also the kernel itself.
> 
> During tty_open, the uart_port_startup sets the MCR to 0, and then use
> set_mctrl to restore the MCR, so at this time, the MCR read does not
> reflect the desired value.

I don't quite follow.  Where is uart_port_startup() setting the MCR to 0?
Are you referring to the call to uart_port_dtr_rts()?  That function should
set RTS correctly according to RS485 state, so I don't see where any
breakage may occur.

What is the user-visible issue that you seek to fix with the revert?

Thanks,

Lukas

> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 66374704747e..40736e460956 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2024,13 +2024,6 @@ void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  	struct uart_8250_port *up = up_to_u8250p(port);
>  	unsigned char mcr;
>  
> -	if (port->rs485.flags & SER_RS485_ENABLED) {
> -		if (serial8250_in_MCR(up) & UART_MCR_RTS)
> -			mctrl |= TIOCM_RTS;
> -		else
> -			mctrl &= ~TIOCM_RTS;
> -	}
> -
>  	mcr = serial8250_TIOCM_to_MCR(mctrl);
>  
>  	mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr;
> -- 
> 2.25.1

Re: [PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"

[Su Bao Cheng]

On 2021/10/27 下午7:39, Lukas Wunner wrote:
> On Wed, Oct 27, 2021 at 07:16:44PM +0800, Su Bao Cheng wrote:
>> This reverts commit f45709df7731ad36306a28a3e1af7309d55c35f5.
>>
>> The `serial8250_do_set_mctrl` not only used by userspace ioctl but
>> also the kernel itself.
>>
>> During tty_open, the uart_port_startup sets the MCR to 0, and then use
>> set_mctrl to restore the MCR, so at this time, the MCR read does not
>> reflect the desired value.
> 
> I don't quite follow.  Where is uart_port_startup() setting the MCR to 0?
> Are you referring to the call to uart_port_dtr_rts()?  That function should
> set RTS correctly according to RS485 state, so I don't see where any
> breakage may occur.
>
> What is the user-visible issue that you seek to fix with the revert?
> 

Sorry for the late response, the company exchange server does not work
for me at this moment, so I have to use the private email instead.

The issue is observed on omap8250 hardware (CPU: AM6548). the use case
is RS485 half-duplex (2 wire mode), in this mode the RTS pin is used to
control the direction and is software controller via the MCR[1]
register. The problem is that the RS485 transmitting is OK, but the
receiving is not working. Similar issue also exists for the RS422, i.e.
the 4-wire full-duplex mode of RS485, but this time the TX does not
work, RX is fine.

The MCR is set to 0 at this line within uart_port_startup():
	retval = uport->ops->startup(uport);

On omap8250, the startup() points to omap_8250_startup(), within it:
	up->mcr = 0;

For software controlled RTS pin of RS485 half-duplex, when not in the
transmitting, the MCR[1] should be constant to indicate the current
direction is receiving. This is set in serial8250_em485_stop_tx().

So after this point of setting the MCR to 0, this up->mcr register
mirror does not reflect the actual desired value anymore. Further
checking against it leads to false result.

Another possible fix could be, instead of setting the mcr to 0 blindly,
one could check if the current operating mode is RS485 half-duplex, and
if so, mask the MCR[1], so that this bit is not changed. Because the
MCR[1] will be changed to the correct value before TX in
serial8250_em485_start_tx(), this change would not impact the transmitting.

> Thanks,
> 
> Lukas
> 
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index 66374704747e..40736e460956 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -2024,13 +2024,6 @@ void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
>>  	struct uart_8250_port *up = up_to_u8250p(port);
>>  	unsigned char mcr;
>>  
>> -	if (port->rs485.flags & SER_RS485_ENABLED) {
>> -		if (serial8250_in_MCR(up) & UART_MCR_RTS)
>> -			mctrl |= TIOCM_RTS;
>> -		else
>> -			mctrl &= ~TIOCM_RTS;
>> -	}
>> -
>>  	mcr = serial8250_TIOCM_to_MCR(mctrl);
>>  
>>  	mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr;
>> -- 
>> 2.25.1

Re: [PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"

[Jan Kiszka]

On 12.11.21 07:14, Su Bao Cheng wrote:
> On 2021/10/27 下午7:39, Lukas Wunner wrote:
>> On Wed, Oct 27, 2021 at 07:16:44PM +0800, Su Bao Cheng wrote:
>>> This reverts commit f45709df7731ad36306a28a3e1af7309d55c35f5.
>>>
>>> The `serial8250_do_set_mctrl` not only used by userspace ioctl but
>>> also the kernel itself.
>>>
>>> During tty_open, the uart_port_startup sets the MCR to 0, and then use
>>> set_mctrl to restore the MCR, so at this time, the MCR read does not
>>> reflect the desired value.
>>
>> I don't quite follow.  Where is uart_port_startup() setting the MCR to 0?
>> Are you referring to the call to uart_port_dtr_rts()?  That function should
>> set RTS correctly according to RS485 state, so I don't see where any
>> breakage may occur.
>>
>> What is the user-visible issue that you seek to fix with the revert?
>>
> 
> Sorry for the late response, the company exchange server does not work
> for me at this moment, so I have to use the private email instead.
> 
> The issue is observed on omap8250 hardware (CPU: AM6548). the use case
> is RS485 half-duplex (2 wire mode), in this mode the RTS pin is used to
> control the direction and is software controller via the MCR[1]
> register. The problem is that the RS485 transmitting is OK, but the
> receiving is not working. Similar issue also exists for the RS422, i.e.
> the 4-wire full-duplex mode of RS485, but this time the TX does not
> work, RX is fine.
> 
> The MCR is set to 0 at this line within uart_port_startup():
> 	retval = uport->ops->startup(uport);
> 
> On omap8250, the startup() points to omap_8250_startup(), within it:
> 	up->mcr = 0;
> 
> For software controlled RTS pin of RS485 half-duplex, when not in the
> transmitting, the MCR[1] should be constant to indicate the current
> direction is receiving. This is set in serial8250_em485_stop_tx().
> 
> So after this point of setting the MCR to 0, this up->mcr register
> mirror does not reflect the actual desired value anymore. Further
> checking against it leads to false result.
> 
> Another possible fix could be, instead of setting the mcr to 0 blindly,
> one could check if the current operating mode is RS485 half-duplex, and
> if so, mask the MCR[1], so that this bit is not changed. Because the
> MCR[1] will be changed to the correct value before TX in
> serial8250_em485_start_tx(), this change would not impact the transmitting.
> 

From this description, it seems like we have a rather fundamental
regression here. Was RS485-half-duplex / RS422 tested after this change,
Lukas?

A revert is just a workaround, I would say. But unless we have a quick
idea for the proper fix, it may be the option for stable at least.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"

[Jan Kiszka]

On 19.11.21 09:00, Jan Kiszka wrote:
> On 12.11.21 07:14, Su Bao Cheng wrote:
>> On 2021/10/27 下午7:39, Lukas Wunner wrote:
>>> On Wed, Oct 27, 2021 at 07:16:44PM +0800, Su Bao Cheng wrote:
>>>> This reverts commit f45709df7731ad36306a28a3e1af7309d55c35f5.
>>>>
>>>> The `serial8250_do_set_mctrl` not only used by userspace ioctl but
>>>> also the kernel itself.
>>>>
>>>> During tty_open, the uart_port_startup sets the MCR to 0, and then use
>>>> set_mctrl to restore the MCR, so at this time, the MCR read does not
>>>> reflect the desired value.
>>>
>>> I don't quite follow.  Where is uart_port_startup() setting the MCR to 0?
>>> Are you referring to the call to uart_port_dtr_rts()?  That function should
>>> set RTS correctly according to RS485 state, so I don't see where any
>>> breakage may occur.
>>>
>>> What is the user-visible issue that you seek to fix with the revert?
>>>
>>
>> Sorry for the late response, the company exchange server does not work
>> for me at this moment, so I have to use the private email instead.
>>
>> The issue is observed on omap8250 hardware (CPU: AM6548). the use case
>> is RS485 half-duplex (2 wire mode), in this mode the RTS pin is used to
>> control the direction and is software controller via the MCR[1]
>> register. The problem is that the RS485 transmitting is OK, but the
>> receiving is not working. Similar issue also exists for the RS422, i.e.
>> the 4-wire full-duplex mode of RS485, but this time the TX does not
>> work, RX is fine.
>>
>> The MCR is set to 0 at this line within uart_port_startup():
>> 	retval = uport->ops->startup(uport);
>>
>> On omap8250, the startup() points to omap_8250_startup(), within it:
>> 	up->mcr = 0;
>>
>> For software controlled RTS pin of RS485 half-duplex, when not in the
>> transmitting, the MCR[1] should be constant to indicate the current
>> direction is receiving. This is set in serial8250_em485_stop_tx().
>>
>> So after this point of setting the MCR to 0, this up->mcr register
>> mirror does not reflect the actual desired value anymore. Further
>> checking against it leads to false result.
>>
>> Another possible fix could be, instead of setting the mcr to 0 blindly,
>> one could check if the current operating mode is RS485 half-duplex, and
>> if so, mask the MCR[1], so that this bit is not changed. Because the
>> MCR[1] will be changed to the correct value before TX in
>> serial8250_em485_start_tx(), this change would not impact the transmitting.
>>
> 
> From this description, it seems like we have a rather fundamental
> regression here. Was RS485-half-duplex / RS422 tested after this change,
> Lukas?
> 
> A revert is just a workaround, I would say. But unless we have a quick
> idea for the proper fix, it may be the option for stable at least.
> 
> Jan
> 

Digging a bit deeper: One issue of the original patch was definitely
that it tried to sanitized mctrl inside serial8250_do_set_mctrl, rather
than serial8250_set_mctrl. That caused alternative set_mctrl handler to
become out of sync /wrt mctrl. Just look at omap8250_set_mctrl, how it
will work on the unsanitized value.

That may or may not yet explain all issues Baocheng is seeing with the
patch.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"

[Lukas Wunner]

On Fri, Nov 12, 2021 at 02:14:11PM +0800, Su Bao Cheng wrote:
> The issue is observed on omap8250 hardware (CPU: AM6548). the use case
> is RS485 half-duplex (2 wire mode), in this mode the RTS pin is used to
> control the direction and is software controller via the MCR[1]
> register. The problem is that the RS485 transmitting is OK, but the
> receiving is not working.

That means the RTS pin is always asserted and never deasserted.
I don't see how that could be related to the patch in question.
It's rather an indication that RS-485 is not enabled on the port.


> The MCR is set to 0 at this line within uart_port_startup():
> 	retval = uport->ops->startup(uport);
> 
> On omap8250, the startup() points to omap_8250_startup(), within it:
> 	up->mcr = 0;
> 
> For software controlled RTS pin of RS485 half-duplex, when not in the
> transmitting, the MCR[1] should be constant to indicate the current
> direction is receiving. This is set in serial8250_em485_stop_tx().
> 
> So after this point of setting the MCR to 0, this up->mcr register
> mirror does not reflect the actual desired value anymore. Further
> checking against it leads to false result.

And?  up->mcr is not used by the patch.  It retrieves the
current value of the RTS bit from the MCR register instead
of using the cached value in up->mcr.  And all it seeks to
achieve is *preserve* the current value of the RTS bit.
So I don't see why the cached value up->mcr is relevant here.

Thanks,

Lukas

Re: [PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"

[Lukas Wunner]

On Fri, Nov 19, 2021 at 09:43:12AM +0100, Jan Kiszka wrote:
> Digging a bit deeper: One issue of the original patch was definitely
> that it tried to sanitized mctrl inside serial8250_do_set_mctrl, rather
> than serial8250_set_mctrl. That caused alternative set_mctrl handler to
> become out of sync /wrt mctrl. Just look at omap8250_set_mctrl, how it
> will work on the unsanitized value.

All omap8250_set_mctrl does is enable or disable autoRTS if autoRTS is
used.  Are you using autoRTS on this port?  That's one of several pieces
of information that I'm missing here to understand what's going on.

Thanks,

Lukas

Re: [PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"

[Lukas Wunner]

On Fri, Nov 12, 2021 at 02:14:11PM +0800, Su Bao Cheng wrote:
> On 2021/10/27 7:39, Lukas Wunner wrote:
> > On Wed, Oct 27, 2021 at 07:16:44PM +0800, Su Bao Cheng wrote:
> > > During tty_open, the uart_port_startup sets the MCR to 0, and then use
> > > set_mctrl to restore the MCR, so at this time, the MCR read does not
> > > reflect the desired value.

So only the *initial* value of MCR[1] is wrong and prevents receiving.
But once you've sent some data, RTS is deasserted correctly and you can
receive again.  Did I understand that correctly?


> The MCR is set to 0 at this line within uart_port_startup():
> 	retval = uport->ops->startup(uport);
> 
> On omap8250, the startup() points to omap_8250_startup(), within it:
> 	up->mcr = 0;
> 
> For software controlled RTS pin of RS485 half-duplex, when not in the
> transmitting, the MCR[1] should be constant to indicate the current
> direction is receiving. This is set in serial8250_em485_stop_tx().

I'm missing an important piece of information here:  Are you using
inverse polarity for RTS?  Normally MCR[1] should be 1 to transmit
and 0 to receive, per the figure on page 8734 of this document:

https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf

Thus, setting up->mcr = 0 should be perfectly fine because it results
in RTS being deasserted, so the transceiver is in receive mode.

I suspect that you're using inverse polarity for RTS, so the desired
initial value of MCR[1] is 1 in your case.  Is that correct?

You write above that "the MCR[1] should be constant to indicate the
current direction is receiving".  That sentence is missing the desired
value, i.e. should MCR[1] be constant 1 or constant 0?

I suspect that the incorrect value in MCR[1] is evaluated by
omap8250_set_mctrl() via this call stack:
  omap_8250_set_termios()
    omap8250_restore_regs()
      up->port.ops->set_mctrl()

Could you confirm this please by inserting a dump_stack() in
omap8250_set_mctrl()?

I would also like to know if you have set UPSTAT_AUTORTS on the port
by enabling hardware flow-control (CRTSCTS) on the tty.  That would
cause omap8250_set_mctrl to fiddle with UART_EFR_RTS bit and I think
the user-visible result is that the transceiver is switched to
transmit mode when the "RX FIFO HALT trigger level" is reached.
We should probably stop it from doing that.

Thanks,

Lukas

Re: [PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"

[Jan Kiszka]

On 20.11.21 18:18, Lukas Wunner wrote:
> On Fri, Nov 12, 2021 at 02:14:11PM +0800, Su Bao Cheng wrote:
>> On 2021/10/27 7:39, Lukas Wunner wrote:
>>> On Wed, Oct 27, 2021 at 07:16:44PM +0800, Su Bao Cheng wrote:
>>>> During tty_open, the uart_port_startup sets the MCR to 0, and then use
>>>> set_mctrl to restore the MCR, so at this time, the MCR read does not
>>>> reflect the desired value.
> 
> So only the *initial* value of MCR[1] is wrong and prevents receiving.
> But once you've sent some data, RTS is deasserted correctly and you can
> receive again.  Did I understand that correctly?
> 
> 
>> The MCR is set to 0 at this line within uart_port_startup():
>> 	retval = uport->ops->startup(uport);
>>
>> On omap8250, the startup() points to omap_8250_startup(), within it:
>> 	up->mcr = 0;
>>
>> For software controlled RTS pin of RS485 half-duplex, when not in the
>> transmitting, the MCR[1] should be constant to indicate the current
>> direction is receiving. This is set in serial8250_em485_stop_tx().
> 
> I'm missing an important piece of information here:  Are you using
> inverse polarity for RTS?  Normally MCR[1] should be 1 to transmit
> and 0 to receive, per the figure on page 8734 of this document:
> 
> https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
> 
> Thus, setting up->mcr = 0 should be perfectly fine because it results
> in RTS being deasserted, so the transceiver is in receive mode.
> 
> I suspect that you're using inverse polarity for RTS, so the desired
> initial value of MCR[1] is 1 in your case.  Is that correct?
> 
> You write above that "the MCR[1] should be constant to indicate the
> current direction is receiving".  That sentence is missing the desired
> value, i.e. should MCR[1] be constant 1 or constant 0?
> 
> I suspect that the incorrect value in MCR[1] is evaluated by
> omap8250_set_mctrl() via this call stack:
>   omap_8250_set_termios()
>     omap8250_restore_regs()
>       up->port.ops->set_mctrl()
> 
> Could you confirm this please by inserting a dump_stack() in
> omap8250_set_mctrl()?
> 
> I would also like to know if you have set UPSTAT_AUTORTS on the port
> by enabling hardware flow-control (CRTSCTS) on the tty.  That would
> cause omap8250_set_mctrl to fiddle with UART_EFR_RTS bit and I think
> the user-visible result is that the transceiver is switched to
> transmit mode when the "RX FIFO HALT trigger level" is reached.
> We should probably stop it from doing that.
> 

Meanwhile reproduced myself, and now I believe your patch is broken in
ignoring the internal call path to serial8250_set_mctrl, coming from
uart_port_dtr_rts:

[  257.923335] uart_port_dtr_rts: rs485_on 1, RTS_after_send 1, raise 1
[   25.411508] mcr = 1 (was 0)
[  257.932631] CPU: 0 PID: 457 Comm: cat Not tainted 5.16.0-rc1+ #190
[  257.938803] Hardware name: SIMATIC IOT2050 Basic (DT)
[  257.943843] Call trace:
[  257.946280]  dump_backtrace+0x0/0x1ac
[  257.949948]  show_stack+0x18/0x70
[  257.953260]  dump_stack_lvl+0x68/0x84
[  257.956920]  dump_stack+0x18/0x34
[  257.960231]  serial8250_do_set_mctrl+0x184/0x190
[  257.964847]  omap8250_set_mctrl+0x24/0xd0
[  257.968855]  serial8250_set_mctrl+0x18/0x34
[  257.973033]  uart_port_dtr_rts+0xc0/0x160
[  257.977036]  uart_dtr_rts+0x64/0xdc
[  257.980519]  tty_port_block_til_ready+0x1fc/0x33c
[  257.985219]  tty_port_open+0xc4/0x250
[  257.988877]  uart_open+0x1c/0x30
[  257.992102]  tty_open+0x140/0x61c
[  257.995417]  chrdev_open+0xc0/0x230
[  257.998904]  do_dentry_open+0x12c/0x3a0
[  258.002737]  vfs_open+0x30/0x3c
[  258.005877]  path_openat+0x864/0xd30
[  258.009447]  do_filp_open+0x80/0x130
[  258.013018]  do_sys_openat2+0xb4/0x16c
[  258.016763]  __arm64_sys_openat+0x64/0xb0
[  258.020769]  invoke_syscall+0x48/0x114
[  258.024515]  el0_svc_common.constprop.0+0x44/0xec
[  258.029214]  do_el0_svc+0x24/0x90
[  258.032525]  el0_svc+0x20/0x60
[  258.035577]  el0t_64_sync_handler+0xe8/0xf0
[  258.039755]  el0t_64_sync+0x1a0/0x1a4

This case is not triggered by userspace setting a custom RTS value but
by the uart-internal machinery selecting it based on the rs485 mode,
among other things. That path must not be intercepted and made
conditional using the current MCR state but has to write the request
value *as is*. I think that is not even an omap-specific regression,
it's generic.

If you want to sanitize mctrl, you must do that at the entry point from
userspace, probably in uart_set_termios (but I didn't dig very deep into
that). So, reverting this commit still seems like the best option to me.

I see the point of filtering TIOCMSET, but that would have to be done
differently.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"

[Lukas Wunner]

On Sun, Nov 21, 2021 at 10:00:51AM +0100, Jan Kiszka wrote:
> Meanwhile reproduced myself, and now I believe your patch is broken in
> ignoring the internal call path to serial8250_set_mctrl, coming from
> uart_port_dtr_rts:
[...]
> This case is not triggered by userspace setting a custom RTS value but
> by the uart-internal machinery selecting it based on the rs485 mode,
> among other things. That path must not be intercepted and made
> conditional using the current MCR state but has to write the request
> value *as is*.

Thanks for the analysis and sorry for the breakage.  I'm proposing the
fix below.  Let me know if that works for you.

However I believe that omap_8250_startup() should be amended to not set
up->mcr = 0 unconditionally.  Rather, it should set the RTS bit if rs485
is enabled and RTS polarity is inverted (as seems to be the case on your
product).  Right now, even with the fix below you'll see a brief glitch
wherein RTS is asserted (so the transceiver's driver is enabled) and
immediately deasserted when opening the port.  This may disturb the
communication of other devices on the bus.  Do you agree?  If so, I can
prepare a separate fix for that.  Note that we may have never noticed
that without f45709df7731, so... ;)

Thanks,

Lukas

-- >8 --
Subject: [PATCH] serial: 8250: Fix RTS modem control while in rs485 mode

Commit f45709df7731 ("serial: 8250: Don't touch RTS modem control while
in rs485 mode") sought to prevent user space from interfering with rs485
communication by ignoring a TIOCMSET ioctl() which changes RTS polarity.

It did so in serial8250_do_set_mctrl(), which turns out to be too deep
in the call stack:  When a uart_port is opened, RTS polarity is set by
the rs485-aware function uart_port_dtr_rts().  It calls down to
serial8250_do_set_mctrl() and that particular RTS polarity change should
*not* be ignored.

The user-visible result is that on 8250_omap ports which use rs485 with
inverse polarity (RTS bit in MCR register is 1 to receive, 0 to send),
a newly opened port initially sets up RTS for sending instead of
receiving.  That's because omap_8250_startup() sets the cached value
up->mcr to 0 and omap_8250_restore_regs() subsequently writes it to the
MCR register.  Due to the commit, serial8250_do_set_mctrl() preserves
that incorrect register value:

do_sys_openat2
  do_filp_open
    path_openat
      vfs_open
        do_dentry_open
	  chrdev_open
	    tty_open
	      uart_open
	        tty_port_open
		  uart_port_activate
		    uart_startup
		      uart_port_startup
		        serial8250_startup
			  omap_8250_startup # up->mcr = 0
			uart_change_speed
			  serial8250_set_termios
			    omap_8250_set_termios
			      omap_8250_restore_regs
			        serial8250_out_MCR # up->mcr written
		  tty_port_block_til_ready
		    uart_dtr_rts
		      uart_port_dtr_rts
		        serial8250_set_mctrl
			  omap8250_set_mctrl
			    serial8250_do_set_mctrl # mcr[1] = 1 ignored

Fix by intercepting RTS changes from user space in uart_tiocmset()
instead.

Fixes: f45709df7731 ("serial: 8250: Don't touch RTS modem control while in rs485 mode")
Reported-by: Su Bao Cheng <baocheng.su@siemens.com>
Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Chao Zeng <chao.zeng@siemens.com>
Cc: stable@vger.kernel.org # v5.7+
---
 drivers/tty/serial/8250/8250_port.c | 7 -------
 drivers/tty/serial/serial_core.c    | 5 +++++
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 5775cbff8f6e..46e2079ad1aa 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2024,13 +2024,6 @@ void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	struct uart_8250_port *up = up_to_u8250p(port);
 	unsigned char mcr;
 
-	if (port->rs485.flags & SER_RS485_ENABLED) {
-		if (serial8250_in_MCR(up) & UART_MCR_RTS)
-			mctrl |= TIOCM_RTS;
-		else
-			mctrl &= ~TIOCM_RTS;
-	}
-
 	mcr = serial8250_TIOCM_to_MCR(mctrl);
 
 	mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr;
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 1e738f265eea..6a38e9d7b87a 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1075,6 +1075,11 @@ uart_tiocmset(struct tty_struct *tty, unsigned int set, unsigned int clear)
 		goto out;
 
 	if (!tty_io_error(tty)) {
+		if (uport->rs485.flags & SER_RS485_ENABLED) {
+			set &= ~TIOCM_RTS;
+			clear &= ~TIOCM_RTS;
+		}
+
 		uart_update_mctrl(uport, set, clear);
 		ret = 0;
 	}
-- 
2.33.0

Re: [PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"

[Su Bao Cheng]

On 11/22/21 1:43 AM, Lukas Wunner wrote:
> On Sun, Nov 21, 2021 at 10:00:51AM +0100, Jan Kiszka wrote:
>> Meanwhile reproduced myself, and now I believe your patch is broken in
>> ignoring the internal call path to serial8250_set_mctrl, coming from
>> uart_port_dtr_rts:
> [...]
>> This case is not triggered by userspace setting a custom RTS value but
>> by the uart-internal machinery selecting it based on the rs485 mode,
>> among other things. That path must not be intercepted and made
>> conditional using the current MCR state but has to write the request
>> value *as is*.
> 
> Thanks for the analysis and sorry for the breakage.  I'm proposing the
> fix below.  Let me know if that works for you.
> 
> However I believe that omap_8250_startup() should be amended to not set
> up->mcr = 0 unconditionally.  Rather, it should set the RTS bit if rs485
> is enabled and RTS polarity is inverted (as seems to be the case on your
> product).  Right now, even with the fix below you'll see a brief glitch
> wherein RTS is asserted (so the transceiver's driver is enabled) and
> immediately deasserted when opening the port.  This may disturb the
> communication of other devices on the bus.  Do you agree?  If so, I can
> prepare a separate fix for that.  Note that we may have never noticed
> that without f45709df7731, so... ;)
> 
> Thanks,
> 
> Lukas
> 

The new patch works on our setup.

Thanks,

Baocheng Su

> -- >8 --
> Subject: [PATCH] serial: 8250: Fix RTS modem control while in rs485 mode
> 
> Commit f45709df7731 ("serial: 8250: Don't touch RTS modem control while
> in rs485 mode") sought to prevent user space from interfering with rs485
> communication by ignoring a TIOCMSET ioctl() which changes RTS polarity.
> 
> It did so in serial8250_do_set_mctrl(), which turns out to be too deep
> in the call stack:  When a uart_port is opened, RTS polarity is set by
> the rs485-aware function uart_port_dtr_rts().  It calls down to
> serial8250_do_set_mctrl() and that particular RTS polarity change should
> *not* be ignored.
> 
> The user-visible result is that on 8250_omap ports which use rs485 with
> inverse polarity (RTS bit in MCR register is 1 to receive, 0 to send),
> a newly opened port initially sets up RTS for sending instead of
> receiving.  That's because omap_8250_startup() sets the cached value
> up->mcr to 0 and omap_8250_restore_regs() subsequently writes it to the
> MCR register.  Due to the commit, serial8250_do_set_mctrl() preserves
> that incorrect register value:
> 
> do_sys_openat2
>   do_filp_open
>     path_openat
>       vfs_open
>         do_dentry_open
> 	  chrdev_open
> 	    tty_open
> 	      uart_open
> 	        tty_port_open
> 		  uart_port_activate
> 		    uart_startup
> 		      uart_port_startup
> 		        serial8250_startup
> 			  omap_8250_startup # up->mcr = 0
> 			uart_change_speed
> 			  serial8250_set_termios
> 			    omap_8250_set_termios
> 			      omap_8250_restore_regs
> 			        serial8250_out_MCR # up->mcr written
> 		  tty_port_block_til_ready
> 		    uart_dtr_rts
> 		      uart_port_dtr_rts
> 		        serial8250_set_mctrl
> 			  omap8250_set_mctrl
> 			    serial8250_do_set_mctrl # mcr[1] = 1 ignored
> 
> Fix by intercepting RTS changes from user space in uart_tiocmset()
> instead.
> 
> Fixes: f45709df7731 ("serial: 8250: Don't touch RTS modem control while in rs485 mode")
> Reported-by: Su Bao Cheng <baocheng.su@siemens.com>
> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Chao Zeng <chao.zeng@siemens.com>
> Cc: stable@vger.kernel.org # v5.7+
> ---
>  drivers/tty/serial/8250/8250_port.c | 7 -------
>  drivers/tty/serial/serial_core.c    | 5 +++++
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 5775cbff8f6e..46e2079ad1aa 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2024,13 +2024,6 @@ void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  	struct uart_8250_port *up = up_to_u8250p(port);
>  	unsigned char mcr;
>  
> -	if (port->rs485.flags & SER_RS485_ENABLED) {
> -		if (serial8250_in_MCR(up) & UART_MCR_RTS)
> -			mctrl |= TIOCM_RTS;
> -		else
> -			mctrl &= ~TIOCM_RTS;
> -	}
> -
>  	mcr = serial8250_TIOCM_to_MCR(mctrl);
>  
>  	mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr;
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 1e738f265eea..6a38e9d7b87a 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1075,6 +1075,11 @@ uart_tiocmset(struct tty_struct *tty, unsigned int set, unsigned int clear)
>  		goto out;
>  
>  	if (!tty_io_error(tty)) {
> +		if (uport->rs485.flags & SER_RS485_ENABLED) {
> +			set &= ~TIOCM_RTS;
> +			clear &= ~TIOCM_RTS;
> +		}
> +
>  		uart_update_mctrl(uport, set, clear);
>  		ret = 0;
>  	}
> 

Re: [PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"

[Lukas Wunner]

On Mon, Nov 22, 2021 at 05:01:07PM +0800, Su Bao Cheng wrote:
> The new patch works on our setup.

Thanks for testing.

Actually AM65 supports RS-485 in hardware, so the software emulation
which is currently used isn't necessary.

Below is a tentative, compile-tested only patch to add support for that.
Would you mind giving it a spin and see if it works?  Unfortunately I
can't test it myself.  I do have an AM64 EVM on my desk, but it lacks
an on-board RS-485 transceiver.  (Only the AM65 EVM has one.)

If the RTS pin is controlled by hardware, the delays on assertion and
deassertion of RTS are fixed (cannot be configured).  Also I'm not sure
whether full-duplex (RS-422) works correctly and whether you'll see
your own echo when sending in RS-485 mode.

Thanks!

Lukas

-- >8 --
Subject: [PATCH] serial: 8250: 8250_omap: Support native rs485

Recent TI Sitara SoCs such as AM64/AM65 have gained the ability to
automatically assert RTS when data is transmitted, obviating the need
to emulate this functionality in software.

The feature is controlled through new DIR_EN and DIR_POL bits in the
Mode Definition Register 3.  For details see page 8783 and 8890 of the
AM65 TRM:  https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Su Bao Cheng <baocheng.su@siemens.com>
Cc: Vignesh Raghavendra <vigneshr@ti.com>
Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
Cc: Nishanth Menon <nm@ti.com>
---
 drivers/tty/serial/8250/8250_omap.c | 73 +++++++++++++++++++++++++++--
 1 file changed, 69 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 0fea7bde25ea..46e6c8cb2841 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -44,6 +44,7 @@
 #define	UART_HAS_EFR2			BIT(4)
 #define UART_HAS_RHR_IT_DIS		BIT(5)
 #define UART_RX_TIMEOUT_QUIRK		BIT(6)
+#define UART_HAS_NATIVE_RS485		BIT(7)
 
 #define OMAP_UART_FCR_RX_TRIG		6
 #define OMAP_UART_FCR_TX_TRIG		4
@@ -101,6 +102,11 @@
 #define UART_OMAP_IER2			0x1B
 #define UART_OMAP_IER2_RHR_IT_DIS	BIT(2)
 
+/* Mode Definition Register 3 */
+#define UART_OMAP_MDR3			0x20
+#define UART_OMAP_MDR3_DIR_POL		BIT(3)
+#define UART_OMAP_MDR3_DIR_EN		BIT(4)
+
 /* Enhanced features register 2 */
 #define UART_OMAP_EFR2			0x23
 #define UART_OMAP_EFR2_TIMEOUT_BEHAVE	BIT(6)
@@ -807,6 +813,60 @@ static void omap_8250_unthrottle(struct uart_port *port)
 	pm_runtime_put_autosuspend(port->dev);
 }
 
+static int omap8250_rs485_config(struct uart_port *port,
+				 struct serial_rs485 *rs485)
+{
+	struct uart_8250_port *up = up_to_u8250p(port);
+	struct omap8250_priv *priv = port->private_data;
+	unsigned int baud;
+	u32 reg;
+
+	/* pick sane settings if the user hasn't */
+	if (!!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
+		!!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
+		rs485->flags |= SER_RS485_RTS_ON_SEND;
+		rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
+	}
+
+	/*
+	 * There is a fixed delay of 3 bit clock cycles after the TX shift
+	 * register is going empty to allow time for the stop bit to transition
+	 * through the transceiver before direction is changed to receive.
+	 */
+	if (priv->quot) {
+		if (priv->mdr1 & UART_OMAP_MDR1_16X_MODE)
+			baud = port->uartclk / (16 * priv->quot);
+		else
+			baud = port->uartclk / (13 * priv->quot);
+		rs485->delay_rts_after_send = 3 * MSEC_PER_SEC / baud;
+	} else {
+		rs485->delay_rts_after_send = 0;
+	}
+	rs485->delay_rts_before_send = 0;
+
+	memset(rs485->padding, 0, sizeof(rs485->padding));
+	port->rs485 = *rs485;
+
+	gpiod_set_value(port->rs485_term_gpio,
+			rs485->flags & SER_RS485_TERMINATE_BUS);
+
+	reg = serial_in(up, UART_OMAP_MDR3);
+
+	if (rs485->flags & SER_RS485_ENABLED)
+		reg |= UART_OMAP_MDR3_DIR_EN;
+	else
+		reg &= ~UART_OMAP_MDR3_DIR_EN;
+
+	if (rs485->flags & SER_RS485_RTS_ON_SEND)
+		reg |= UART_OMAP_MDR3_DIR_POL;
+	else
+		reg &= ~UART_OMAP_MDR3_DIR_POL;
+
+	serial_out(up, UART_OMAP_MDR3, reg);
+
+	return 0;
+}
+
 #ifdef CONFIG_SERIAL_8250_DMA
 static int omap_8250_rx_dma(struct uart_8250_port *p);
 
@@ -1259,7 +1319,7 @@ static struct omap8250_dma_params am33xx_dma = {
 static struct omap8250_platdata am654_platdata = {
 	.dma_params	= &am654_dma,
 	.habit		= UART_HAS_EFR2 | UART_HAS_RHR_IT_DIS |
-			  UART_RX_TIMEOUT_QUIRK,
+			  UART_RX_TIMEOUT_QUIRK | UART_HAS_NATIVE_RS485,
 };
 
 static struct omap8250_platdata am33xx_platdata = {
@@ -1352,9 +1412,6 @@ static int omap8250_probe(struct platform_device *pdev)
 	up.port.shutdown = omap_8250_shutdown;
 	up.port.throttle = omap_8250_throttle;
 	up.port.unthrottle = omap_8250_unthrottle;
-	up.port.rs485_config = serial8250_em485_config;
-	up.rs485_start_tx = serial8250_em485_start_tx;
-	up.rs485_stop_tx = serial8250_em485_stop_tx;
 	up.port.has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE);
 
 	ret = of_alias_get_id(np, "serial");
@@ -1393,6 +1450,14 @@ static int omap8250_probe(struct platform_device *pdev)
 			 DEFAULT_CLK_SPEED);
 	}
 
+	if (priv->habit & UART_HAS_NATIVE_RS485) {
+		up.port.rs485_config = omap8250_rs485_config;
+	} else {
+		up.port.rs485_config = serial8250_em485_config;
+		up.rs485_start_tx = serial8250_em485_start_tx;
+		up.rs485_stop_tx = serial8250_em485_stop_tx;
+	}
+
 	priv->latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
 	priv->calc_latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
 	cpu_latency_qos_add_request(&priv->pm_qos_request, priv->latency);
-- 
2.33.0

Re: [PATCH] Revert "serial: 8250: Don't touch RTS modem control while in rs485 mode"

[Lukas Wunner]

On Sun, Nov 21, 2021 at 10:00:51AM +0100, Jan Kiszka wrote:
> Meanwhile reproduced myself, and now I believe your patch is broken in
> ignoring the internal call path to serial8250_set_mctrl, coming from
> uart_port_dtr_rts:
> 
> [  257.923335] uart_port_dtr_rts: rs485_on 1, RTS_after_send 1, raise 1
> [   25.411508] mcr = 1 (was 0)
> [  257.932631] CPU: 0 PID: 457 Comm: cat Not tainted 5.16.0-rc1+ #190
> [  257.938803] Hardware name: SIMATIC IOT2050 Basic (DT)
> [  257.943843] Call trace:
> [  257.946280]  dump_backtrace+0x0/0x1ac
> [  257.949948]  show_stack+0x18/0x70
> [  257.953260]  dump_stack_lvl+0x68/0x84
> [  257.956920]  dump_stack+0x18/0x34
> [  257.960231]  serial8250_do_set_mctrl+0x184/0x190
> [  257.964847]  omap8250_set_mctrl+0x24/0xd0
> [  257.968855]  serial8250_set_mctrl+0x18/0x34
> [  257.973033]  uart_port_dtr_rts+0xc0/0x160

Are you using a custom patch which is not in mainline to fix the behavior
of uart_port_dtr_rts() if rs485 is enabled?

In the dmesg output above, why does the timestamp jump from 257.923335
back to 25.411508?

After extensive debugging and staring at the code for a couple of days
I'm under the impression commit a6845e1e1b78 is broken and sets RTS
exactly the wrong way.  The commit failed to appreciate that with RS-485,
there's a single negation of RTS (MCR contains the complement of the
desired signal level) whereas with RS-232 there's a double negation
(the RS-232 transceiver negates the resulting signal level yet again).
So, "setting RTS" and "clearing RTS" is the inverse for RS-232 vis-a-vis
RS-485.

With the code in mainline broken, yet it apparently not being broken in
your tree (going by your debug output above), only explanation I can
think of is you've got a custom fix in your tree.  If you do, why haven't
you upstreamed it?

Thanks,

Lukas