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

[Jan Kiszka <jan.kiszka@siemens.com>]

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