Re: [PATCH v4 tty-next 2/2] serial: 8250: add driver for NI UARTs

[Brenda Streiff <brenda.streiff@ni.com>]

Apologies for the delay in response. I have your other requested
changes in a pending v5 patchset, but answering this question took
some digging (and I got bounced between projects in the intervening
time)

On 5/8/23 03:01, Ilpo Järvinen wrote:
> On Fri, 5 May 2023, Brenda Streiff wrote:

>> +static const struct serial_rs485 ni16550_rs485_supported = {
>> +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RTS_AFTER_SEND,
>> +	/*
>> +	 * delay_rts_* and RX_DURING_TX are not supported.
>> +	 *
>> +	 * RTS_{ON,AFTER}_SEND are supported, but ignored; the transceiver
>> +	 * is connected in only one way and we don't need userspace to tell
>> +	 * us, but want to retain compatibility with applications that do.
> 
> This is a bit odd statement. If application wants to specify those flags,
> they will be ignored (cleared) by serial core without triggering an error
> even when driver does not claim to support them.
> 
> Or are you saying some application sets these flags and fails if it
> doesn't get the same flags back? 

Yes, this is the case.

There's an unfortunate amount of historical baggage on our side here.

The hardware supports four modes:
   Wire Mode              | Tx enabled?        | Rx enabled?
   -----------------------|--------------------|------------------------
   NI16650_PCR_RS422      | Always             | Always
   NI16650_PCR_ECHO_RS485 | If DTR asserted    | Always
   NI16650_PCR_DTR_RS485  | If DTR asserted    | Disabled when TX enabled
   NI16650_PCR_AUTO_RS485 | If data in TX FIFO | Disabled when TX enabled

These four hardware modes map directly to "wire mode" settings in higher
level NI software APIs, such as NI-VISA's VI_ATTR_ASRL_WIRE_MODE
property [1] on other operating systems, and our drivers for those OSes
just present those settings directly in their ABI. Linux already had
support for RS485, but when the driver was first authored for Linux [2]
(and then kept out-of-tree for a decade), it was done so with an
(erroneous) understanding of the RS485 flags:

   Wire Mode                struct serial_rs485::flags
   -----------------------  --------------------
   NI16650_PCR_RS422:       0
   NI16650_PCR_ECHO_RS485:  SER_RS485_ENABLED|SER_RS485_RX_DURING_TX
   NI16650_PCR_DTR_RS485:   SER_RS485_ENABLED
   NI16650_PCR_AUTO_RS485:  SER_RS485_ENABLED|SER_RS485_RTS_ON_SEND

The "RS485 serial_rs485 sanitization" patchset [3] ended up breaking us
and forced us to reevaluate it, because now some of those states were
invalid (a driver cannot now simultaneously support not having
RTS_ON_SEND at the same time as supporting having RTS_ON_SEND).

Ultimately we made the decision that we only care about the RS422 and
AUTO_RS485 modes, but now we have a slightly different problem, in that
there's a bunch of userspace middleware that is still coded to expect
all four modes; so if we remove support for the SER_RS485_RTS_ON_SEND
flag, even though we don't need it, we end up with:
- middleware sets SER_RS485_ENABLED|SER_RS485_RTS_ON_SEND
- kernel coerces this into SER_RS485_ENABLED
- middleware reads back the value, sees the flags are different, and
   assumes that the mode isn't properly supported and errors.

So we need to "support" SER_RS485_RTS_ON_SEND, even though we don't
require the presence or absence of the flag to direct any hardware
configuration change, in order to avoid this coercion happening.

Ideally we would have a userspace more tolerant of the kernel now
coercing flags differently than it used to, but unfortunately we're
stuck dealing with it for a while.

If there's a better way you'd prefer this be handled, let me know.
We're trying to walk the line between "the driver needs to present
the ABI correctly to userspace" and "our userspace relied on it having
been incorrectly implemented in the past".

[1] https://www.ni.com/docs/en-US/bundle/ni-visa/page/ni-visa/vi_attr_asrl_wire_mode.html
[2] https://github.com/ni/linux/blob/nilrt_pub/14.0/3.10/drivers/tty/serial/8250/8250_ni16550.c#L96
[3] https://lore.kernel.org/all/20220606100433.13793-1-ilpo.jarvinen@linux.intel.com/