| Message ID | 20220216001803.637-2-LinoSanfilippo@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [2,1/9] serial: core: move RS485 configuration tasks from drivers into core | expand |
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
On Wed, Feb 16, 2022 at 01:17:55AM +0100, Lino Sanfilippo wrote: > Several drivers that support setting the RS485 configuration via userspace > implement one or more of the following tasks: > > - in case of an invalid RTS configuration (both RTS after send and RTS on > send set or both unset) fall back to enable RTS on send and disable RTS > after send > > - nullify the padding field of the returned serial_rs485 struct > > - copy the configuration into the uart port struct > > - limit RTS delays to 100 ms > > Move these tasks into the serial core to make them generic and to provide > a consistent behaviour among all drivers. > > Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> > --- > drivers/tty/serial/serial_core.c | 18 ++++++++++++++++++ > include/uapi/linux/serial.h | 3 +++ > 2 files changed, 21 insertions(+) > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > index 846192a7b4bf..a4f7e847d414 100644 > --- 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; > + } > + > + 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); > + /* Return clean padding area to userspace */ > + memset(rs485.padding, 0, sizeof(rs485.padding)); > + > spin_lock_irqsave(&port->lock, flags); > ret = port->rs485_config(port, &rs485); > + if (!ret) > + port->rs485 = rs485; > spin_unlock_irqrestore(&port->lock, flags); > if (ret) > return ret; > diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h > index fa6b16e5fdd8..859045a53231 100644 > --- a/include/uapi/linux/serial.h > +++ b/include/uapi/linux/serial.h > @@ -128,6 +128,9 @@ struct serial_rs485 { > (if supported) */ > __u32 delay_rts_before_send; /* Delay before send (milliseconds) */ > __u32 delay_rts_after_send; /* Delay after send (milliseconds) */ > +#define SER_RS485_MAX_RTS_DELAY 100 /* Max time with active > + RTS before/after > + data sent (msecs) */ Why is this a userspace value now? What can userspace do with this number? Once we add this, it's fixed for forever. thanks, greg k-h
Hi, On 21.02.22 at 19:39, Greg KH wrote: >> diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h >> index fa6b16e5fdd8..859045a53231 100644 >> --- a/include/uapi/linux/serial.h >> +++ b/include/uapi/linux/serial.h >> @@ -128,6 +128,9 @@ struct serial_rs485 { >> (if supported) */ >> __u32 delay_rts_before_send; /* Delay before send (milliseconds) */ >> __u32 delay_rts_after_send; /* Delay after send (milliseconds) */ >> +#define SER_RS485_MAX_RTS_DELAY 100 /* Max time with active >> + RTS before/after >> + data sent (msecs) */ > > Why is this a userspace value now? What can userspace do with this > number? Once we add this, it's fixed for forever. > > thanks, > > greg k-h > Ok, I think we do not really need to expose it to userspace, since we clamp the delay anyway if it is too big. I will correct this in the next patch version. Regards, Lino
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index 846192a7b4bf..a4f7e847d414 100644 --- 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; + } + + 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); + /* Return clean padding area to userspace */ + memset(rs485.padding, 0, sizeof(rs485.padding)); + spin_lock_irqsave(&port->lock, flags); ret = port->rs485_config(port, &rs485); + if (!ret) + port->rs485 = rs485; spin_unlock_irqrestore(&port->lock, flags); if (ret) return ret; diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h index fa6b16e5fdd8..859045a53231 100644 --- a/include/uapi/linux/serial.h +++ b/include/uapi/linux/serial.h @@ -128,6 +128,9 @@ struct serial_rs485 { (if supported) */ __u32 delay_rts_before_send; /* Delay before send (milliseconds) */ __u32 delay_rts_after_send; /* Delay after send (milliseconds) */ +#define SER_RS485_MAX_RTS_DELAY 100 /* Max time with active + RTS before/after + data sent (msecs) */ __u32 padding[5]; /* Memory is cheap, new structs are a royal PITA .. */ };
Several drivers that support setting the RS485 configuration via userspace implement one or more of the following tasks: - in case of an invalid RTS configuration (both RTS after send and RTS on send set or both unset) fall back to enable RTS on send and disable RTS after send - nullify the padding field of the returned serial_rs485 struct - copy the configuration into the uart port struct - limit RTS delays to 100 ms Move these tasks into the serial core to make them generic and to provide a consistent behaviour among all drivers. Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> --- drivers/tty/serial/serial_core.c | 18 ++++++++++++++++++ include/uapi/linux/serial.h | 3 +++ 2 files changed, 21 insertions(+) base-commit: 802d00bd774b77fe132e9e83462b96fb9919411c