Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <30fb1e3e-c130-b738-3a09-da525ae97de1@ni.com>
Date:   Fri, 23 Jun 2023 13:18:49 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH v4 tty-next 2/2] serial: 8250: add driver for NI UARTs
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=c3=a4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc:     Gratian Crisan <gratian.crisan@ni.com>,
        Jason Smith <jason.smith@ni.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        linux-serial <linux-serial@vger.kernel.org>,
        devicetree@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>
References: <20230505213850.829639-1-brenda.streiff@ni.com>
 <20230505213850.829639-3-brenda.streiff@ni.com>
 <4585cc23-aa14-7916-11e5-3d3de3153d80@linux.intel.com>
From:   Brenda Streiff <brenda.streiff@ni.com>
In-Reply-To: <4585cc23-aa14-7916-11e5-3d3de3153d80@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?amJpa08wd1dMM0h4c3BBTVlyWWhNSVZsaHRYcXdOYW9sZkJOeXFhVTVsK2lJ?=
 =?utf-8?B?dDhqYkdHTnlhY0hNaWkyalA1cWhMbnZrRWxQUExCcDRlaG1jQ0VkajUyTTFq?=
 =?utf-8?B?TStzZTVYMjh5cFBwUXFNbngvZW14VlM3Rk9raGhZTU9DZVo4VmVyZXFaWW1O?=
 =?utf-8?B?MEJOd2tncVp6ZEhUdm5CUzJKemEwcXRnU3Nsd2gyb0pyWkJVbytGbTU3TitK?=
 =?utf-8?B?SGw2blFGV3g3SExrS2FSeTQ1aWxRL1hsZ2VIVWI4Smt6TjJ4RCsvaTV5ODNi?=
 =?utf-8?B?TVl5d0JLRGFqNzFUWjc0WXpFSUFmVlI2QWNVTGR4K0E0SUFISVdMa3ZYUk16?=
 =?utf-8?B?QWtTbGorMzA2TVkyT2JrZktQZEgxTmp2QmI2R2UwTGQ2QnFRQllrUlp3UXRr?=
 =?utf-8?B?aVgvUTFkb25DNUNsWGpEdjc2bnhuQ2UrY2lxb0QxdWJDTnorMzJiLzBlZzJH?=
 =?utf-8?B?UmFFNVNEYnFOaDJIMkpTZWxEVmN5QkJ4VEtmTXAvR1d1UGptOGRjV1dNVDhU?=
 =?utf-8?B?eGlVNGVXeCs1QWU2TmJPMDVGSXdaM0h5UG9Vc0tUQ0dGZnI2N2hHbGl5TTJY?=
 =?utf-8?B?WXRRWnd2SzhZRFRvYWRraXNVVjl1RHpaSG9vOXlScmhMcnJqaHkrVlJsdGF1?=
 =?utf-8?B?SU1iQVMzZXByL1pYaUt5Q3A0akovRTBJcXVNMlhZZk5MeEQrTCtYNGo5RzdW?=
 =?utf-8?B?cVZaaDcwZzhaQlExU1pETTFhS2NrdmpYbUxyOWZTWVI3Zy9PdW5WZXVtalJC?=
 =?utf-8?B?dysvOUZIVWd2RXhpR1NlVS9lMkxYNmhpbC93aDhzZytHbExkSkwvbk5MQ3Zm?=
 =?utf-8?B?OGgvclFBeUJQWGdMcmJtamRuSDBtSEdrSVdVeU9MNEFyNHN1Y3ZlQlE1WjFZ?=
 =?utf-8?B?Q3ljY3JFNkd4Y2huVzI3VnBlL1h5d29Kc0R6RGhVQmNVSHE5VjVNZTFkQUlQ?=
 =?utf-8?B?bm5KSUlYSzZtSkpzenZpcXYvL2c2OC9ONXljeUROQkx4YXZNWjNUUDNQRnhQ?=
 =?utf-8?B?SlNDRGxhOXlEREd3UEtta0lhNlFpS0c0VDVVbzI2dlBLa0grazRoZ01zNi8r?=
 =?utf-8?B?cHVIR0FHUW5jQTZzSXVHR0tYTGVrbkdvRW1sVW5vMGlsYm1VdjZXOGZJQTNN?=
 =?utf-8?B?RjhhY2M4d0IyWUJNK1JVZWNJTVVJbXB2Qm5PVU5rMXhKQ2gxZlAxZWM2WE9m?=
 =?utf-8?B?Q2t1ZlRCQnMvOEY5VWZZYW9vSENGQjlVUUpuUW42K3JhdTVYQmVFZEZqZGNw?=
 =?utf-8?B?LzZ6Z200RVdRNEw5NXA4a0sxYXF6SFFlWXE5a2loY0pGVU5Vcy9xS3VlK0kr?=
 =?utf-8?B?djQzTVFXVkNDMUt3VEtUN2U2S0pJUUQrcmErdEhYUFZnVkdlY2JqVTRQMFU5?=
 =?utf-8?B?VnRzV1EveWxJYVY0SjUwNEg3THRmaVc4bGFnbGF6WUVMVnl5MjdqQjI5c3Bl?=
 =?utf-8?B?STA4eE4zU3ZTNGNCa0hreEpaRUxuSVI2OEU2cTZxMW93cWlZNGk5aVdjbk9p?=
 =?utf-8?B?UStDTTNoWFZGTnpISmhHNHdQZ3hNbGtyYTh4VVVpV3dZc05yUFY3U2luVzF3?=
 =?utf-8?B?S2kvL3dTWG4weGhkZGtrR0c1L0p3MWlMMzlaaHFNVFl2WTlmWFFSUWozUmN3?=
 =?utf-8?B?bXJWK2VIR3c4UCtCQ1g3dWtZT0JNRVpYVlRVZ3RobXgxYlpDdWVzOXlCdHFm?=
 =?utf-8?B?VlpXT1dzM3VxVG9YYUpheWxpT2paR2N5dmpicm9zTkgweDJFTEtzV01sa0x1?=
 =?utf-8?B?OHg2VlNhY2plbkZRMGZaVmtZL2REeUdKTEpGemtaVk90Q3RBTmMyRXU1bFZQ?=
 =?utf-8?B?Um9wU0Roa0hWUkRTYjI5ek5veTVxSlpncmZqRjQ1UDd2WXZscHg3bmdoNk1z?=
 =?utf-8?B?SVZjMldoSVQ5TmZ6aGQ4YXVpcjFKMGNWS1liZ1RCT2ZnOTAwRXNka3pCTzdB?=
 =?utf-8?B?S0NzcFpTVjdkNlVLazhVODdPb0JSMUpzTFNBNzlwRmlLY1h3RDNCbWtHVito?=
 =?utf-8?B?QURySVF5d3N5Ykx0bTlOMXhDOURBVkk1TXRSczlGWkM4M0tGTVNaWTRYaHRM?=
 =?utf-8?B?YUhSYnFLSjFOLzg0ZlFhZUxteFAwYzBtcnpFM0srUUZQTnUwb3ZHbTcwWFBG?=
 =?utf-8?B?Y3NHWXJUandEaXZsTzBhdlBDQWM1TmVmT1JMZUJoZGFZZVBlcHBQdVNHZ1lz?=
 =?utf-8?B?RUE9PQ==?=
X-OriginatorOrg: ni.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 408ebd07-4b53-4bec-4040-08db74164c3a
X-MS-Exchange-CrossTenant-AuthSource: SN6PR04MB4973.namprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Jun 2023 18:18:52.1197
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 87ba1f9a-44cd-43a6-b008-6fdb45a5204e
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: erteAXWWGuoRdNG0wLxi5kXxcqyMUkuUBEtsKiOQLw9bsYN0w4QExWyd18MhzCtxy9j5ti+I27GXqSd3XA4YNQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO6PR04MB7492
X-Proofpoint-ORIG-GUID: sEWu-TwM41a7cnRSIksqXoYP-IMhSwJk
X-Proofpoint-GUID: sEWu-TwM41a7cnRSIksqXoYP-IMhSwJk
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-06-23_10,2023-06-22_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=outbound_policy_notspam policy=outbound_policy score=30 clxscore=1011
 impostorscore=0 malwarescore=0 spamscore=0 mlxscore=0 mlxlogscore=999
 suspectscore=0 phishscore=0 adultscore=0 bulkscore=0 priorityscore=1501
 lowpriorityscore=0 classifier=spam adjust=30 reason=mlx scancount=1
 engine=8.12.0-2305260000 definitions=main-2306230163
X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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/