Re: [PATCH 2 1/9] serial: core: move RS485 configuration tasks from drivers into core

[Lino Sanfilippo]

Hi,

On 17.02.22 at 12:33, Lukas Wunner wrote:
> On Wed, Feb 16, 2022 at 01:17:55AM +0100, Lino Sanfilippo wrote:
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -1282,8 +1282,26 @@ static int uart_set_rs485_config(struct uart_port *port,
>>  	if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user)))
>>  		return -EFAULT;
>>
>> +	/* 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;
>> +	}
>
> The policy you're enforcing here is:  If settings are nonsensical,
> always default to active-high polarity.
>
> However some drivers currently enforce a completely different policy:
> E.g. with 8250_lpc18xx.c, if SER_RS485_RTS_ON_SEND is set, use active-high
> (and fix up SER_RS485_RTS_AFTER_SEND), else use active-low polarity.
> This yields a different result compared to your policy in case both bits
> are cleared.
>> Similarly, sc16is7xx.c defaults to active-low if SER_RS485_RTS_AFTER_SEND
> is set, else active-high polarity.  This yields a different result compared
> to your policy in case both bits are set.
>
> You risk breaking existing user space applications with this change
> if those applications specify nonsensical polarity settings.
>

Ok, but IMHO this is a very small risk. I cannot imagine that there
are many (or any at all) userspace applications that do not
specify any RTS setting and then rely on a driver specific fallback
implementation. I would not like to remove the RTS check from
uart_set_rs485_config() only because of that.

>
> I happen to have created a similar commit to this one a month ago
> and I came to the conclusion that all one can do is offer a library
> function uart_sanitize_rs485_mode() which drivers may elect to call
> if that helper's policy is identical to what they're doing now:
>
> https://github.com/l1k/linux/commit/637984111e42
>>
>> +
>> +	rs485.delay_rts_before_send = min_t(unsigned int,
>> +					    rs485.delay_rts_before_send,
>> +					    SER_RS485_MAX_RTS_DELAY);
>> +	rs485.delay_rts_after_send = min_t(unsigned int,
>> +					   rs485.delay_rts_after_send,
>> +					   SER_RS485_MAX_RTS_DELAY);
>
> Nonsensical delays may not only be passed in from user space via ioctl(),
> but through the device tree.  That's another reason to use a library
> function:  It can be called both from the ioctl() as well as after (or in)
> uart_get_rs485_mode().
>

The idea of my patch set is actually to provide a common behavior for the
RS485 configuration by userspace similar to what uart_get_rs485_mode()
provides for the configuration by device tree.

However with the solution you propose sanity checks for userspace
configuration are still up to each single driver and thus can vary
from driver to driver or not be implemented at all. I dont think that
this is the better approach in the long term.


>
>> +	/* Return clean padding area to userspace */
>> +	memset(rs485.padding, 0, sizeof(rs485.padding));
>
> Unlike the polarity and delay handling, this one makes sense.
>
> Thanks,
>
> Lukas
>

Regards,
Lino