| Message ID | 20231101064404.45711-1-crescentcy.hsieh@moxa.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] tty: serial: Add RS422 flag to struct serial_rs485 | expand |
Hi, On 01.11.23 07:44, Crescent CY Hsieh wrote: > Add "SER_RS422_ENABLED" flag within struct serial_rs485, so that serial > port can switching interface into RS422 if supported by using ioctl > command "TIOCSRS485". > > In case of interfaces confliction, add checks within s/interfaces confliction/interface conflicts ? > uart_sanitize_serial_rs485() such that only one of RS422/RS485 is set. > > Signed-off-by: Crescent CY Hsieh <crescentcy.hsieh@moxa.com> > > --- > Changes in v2: > - Revise the logic that checks whether RS422/RS485 are enabled > simultaneously. > > v1: https://lore.kernel.org/all/20231030053632.5109-1-crescentcy.hsieh@moxa.com/ > > --- > drivers/tty/serial/serial_core.c | 19 +++++++++++++++++-- > include/uapi/linux/serial.h | 4 ++++ > 2 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > index 831d03361..54a104c52 100644 > --- a/drivers/tty/serial/serial_core.c > +++ b/drivers/tty/serial/serial_core.c > @@ -1305,7 +1305,7 @@ static int uart_get_icount(struct tty_struct *tty, > > #define SER_RS485_LEGACY_FLAGS (SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | \ > SER_RS485_RTS_AFTER_SEND | SER_RS485_RX_DURING_TX | \ > - SER_RS485_TERMINATE_BUS) > + SER_RS485_TERMINATE_BUS | SER_RS422_ENABLED) > How can this be a legacy flag when you just introduced it? > static int uart_check_rs485_flags(struct uart_port *port, struct serial_rs485 *rs485) > { > @@ -1371,11 +1371,26 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4 > { > u32 supported_flags = port->rs485_supported.flags; > > - if (!(rs485->flags & SER_RS485_ENABLED)) { > + if (!(rs485->flags & (SER_RS485_ENABLED | SER_RS422_ENABLED))) { > memset(rs485, 0, sizeof(*rs485)); > return; > } > > + /* Pick sane setting if the user enables both interfaces */> + if (rs485->flags & SER_RS485_ENABLED && rs485->flags & SER_RS422_ENABLED) { > + dev_warn_ratelimited(port->dev, > + "%s (%d): Invalid serial interface setting, using RS485 instead\n", > + port->name, port->line); > + rs485->flags &= ~SER_RS422_ENABLED; > + } > + > + /* Clear other bits and return if RS422 is enabled */ > + if (rs485->flags & SER_RS422_ENABLED) {> + memset(rs485, 0, sizeof(*rs485)); Why are all flags cleared but SER_RS422_ENABLED? > + rs485->flags |= SER_RS422_ENABLED; > + return; > + } What about all the other code places that check for SER_RS485_ENABLED? For example uart_update_mctrl(), uart_suspend_port() and uart_resume_port() check this flag to decide whether to set the modem control lines or not. Should this not also apply to SER_RS422_ENABLED? Maybe it would be better to change the meaning of the flag: Instead of being a substitution for SER_RS485_ENABLED, it could be used to mark a special mode. So if both SER_RS485_ENABLED and SER_RS485_MODE_RS422 are set it would mean that we have RS422. Regards, Lino > /* Pick sane settings if the user hasn't */ > if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) && > !(rs485->flags & SER_RS485_RTS_ON_SEND) == > diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h > index 53bc1af67..427609fd5 100644 > --- a/include/uapi/linux/serial.h > +++ b/include/uapi/linux/serial.h > @@ -137,6 +137,8 @@ struct serial_icounter_struct { > * * %SER_RS485_ADDRB - Enable RS485 addressing mode. > * * %SER_RS485_ADDR_RECV - Receive address filter (enables @addr_recv). Requires %SER_RS485_ADDRB. > * * %SER_RS485_ADDR_DEST - Destination address (enables @addr_dest). Requires %SER_RS485_ADDRB. > + * > + * * %SER_RS422_ENABLED - RS422 enabled. > */ > struct serial_rs485 { > __u32 flags; > @@ -149,6 +151,8 @@ struct serial_rs485 { > #define SER_RS485_ADDR_RECV (1 << 7) > #define SER_RS485_ADDR_DEST (1 << 8) > > +#define SER_RS422_ENABLED (1 << 9) > + > __u32 delay_rts_before_send; > __u32 delay_rts_after_send; >
Hi, On 01.11.23 07:49, Greg Kroah-Hartman wrote: > On Wed, Nov 01, 2023 at 02:44:04PM +0800, Crescent CY Hsieh wrote: >> Add "SER_RS422_ENABLED" flag within struct serial_rs485, so that serial >> port can switching interface into RS422 if supported by using ioctl >> command "TIOCSRS485". >> >> In case of interfaces confliction, add checks within >> uart_sanitize_serial_rs485() such that only one of RS422/RS485 is set. >> >> Signed-off-by: Crescent CY Hsieh <crescentcy.hsieh@moxa.com> >> >> --- >> Changes in v2: >> - Revise the logic that checks whether RS422/RS485 are enabled >> simultaneously. >> >> v1: https://lore.kernel.org/all/20231030053632.5109-1-crescentcy.hsieh@moxa.com/ >> >> --- >> drivers/tty/serial/serial_core.c | 19 +++++++++++++++++-- >> include/uapi/linux/serial.h | 4 ++++ >> 2 files changed, 21 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c >> index 831d03361..54a104c52 100644 >> --- a/drivers/tty/serial/serial_core.c >> +++ b/drivers/tty/serial/serial_core.c >> @@ -1305,7 +1305,7 @@ static int uart_get_icount(struct tty_struct *tty, >> >> #define SER_RS485_LEGACY_FLAGS (SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | \ >> SER_RS485_RTS_AFTER_SEND | SER_RS485_RX_DURING_TX | \ >> - SER_RS485_TERMINATE_BUS) >> + SER_RS485_TERMINATE_BUS | SER_RS422_ENABLED) > > A new flag is "legacy"? > >> >> static int uart_check_rs485_flags(struct uart_port *port, struct serial_rs485 *rs485) >> { >> @@ -1371,11 +1371,26 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4 >> { >> u32 supported_flags = port->rs485_supported.flags; >> >> - if (!(rs485->flags & SER_RS485_ENABLED)) { >> + if (!(rs485->flags & (SER_RS485_ENABLED | SER_RS422_ENABLED))) { >> memset(rs485, 0, sizeof(*rs485)); >> return; >> } >> >> + /* Pick sane setting if the user enables both interfaces */ >> + if (rs485->flags & SER_RS485_ENABLED && rs485->flags & SER_RS422_ENABLED) { >> + dev_warn_ratelimited(port->dev, >> + "%s (%d): Invalid serial interface setting, using RS485 instead\n", >> + port->name, port->line); > > Why is this ratelimited? What would cause lots of repeats of this? > > uart_sanitize_serial_rs485() is called when userspace sets the RS485 configuration via TIOCSRS485. So warnings in this function are ratelimited to prevent userspace from spamming the kernel log. Regards, Lino
On Sat, Nov 04, 2023 at 08:53:18PM +0100, Lino Sanfilippo wrote: > On 01.11.23 07:44, Crescent CY Hsieh wrote: > > @@ -1371,11 +1371,26 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4 > > { > > u32 supported_flags = port->rs485_supported.flags; > > > > - if (!(rs485->flags & SER_RS485_ENABLED)) { > > + if (!(rs485->flags & (SER_RS485_ENABLED | SER_RS422_ENABLED))) { > > memset(rs485, 0, sizeof(*rs485)); > > return; > > } > > > > + /* Pick sane setting if the user enables both interfaces */ > > + if (rs485->flags & SER_RS485_ENABLED && rs485->flags & SER_RS422_ENABLED) { > > + dev_warn_ratelimited(port->dev, > > + "%s (%d): Invalid serial interface setting, using RS485 instead\n", > > + port->name, port->line); > > + rs485->flags &= ~SER_RS422_ENABLED; > > + } > > + > > + /* Clear other bits and return if RS422 is enabled */ > > + if (rs485->flags & SER_RS422_ENABLED) { > > + memset(rs485, 0, sizeof(*rs485)); > > Why are all flags cleared but SER_RS422_ENABLED? IMO, RS422 and RS485 are distinct serial interfaces. Therefore, when RS422 is enabled, the other RS485 flags should be cleared, and vice versa. > > + rs485->flags |= SER_RS422_ENABLED; > > + return; > > + } > > What about all the other code places that check for SER_RS485_ENABLED? > For example uart_update_mctrl(), uart_suspend_port() and uart_resume_port() check this flag > to decide whether to set the modem control lines or not. Should this not also apply to > SER_RS422_ENABLED? After reviewing the code in serial_core.c, there are actually some codes that check for "SER_RS485_ENABLED" flag before setting the modem control lines. It also appears that these codes can only be done when disabling RS485. So yes, I will apply "SER_RS422_ENABLED" flag to these locations in the next patch. > > Maybe it would be better to change the meaning of the flag: Instead of being a substitution for > SER_RS485_ENABLED, it could be used to mark a special mode. > So if both SER_RS485_ENABLED and SER_RS485_MODE_RS422 are set it would mean that we have RS422. RS422 is not a mode of RS485, so I think using two flags to represent them is much more reasonable, even though they are both included in the "struct serial_rs485". --- Sincerely, Crescent CY Hsieh
Hi, On 06.11.23 08:19, Crescent CY Hsieh wrote: > On Sat, Nov 04, 2023 at 08:53:18PM +0100, Lino Sanfilippo wrote: >> On 01.11.23 07:44, Crescent CY Hsieh wrote: >>> @@ -1371,11 +1371,26 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4 >>> { >>> u32 supported_flags = port->rs485_supported.flags; >>> >>> - if (!(rs485->flags & SER_RS485_ENABLED)) { >>> + if (!(rs485->flags & (SER_RS485_ENABLED | SER_RS422_ENABLED))) { >>> memset(rs485, 0, sizeof(*rs485)); >>> return; >>> } >>> >>> + /* Pick sane setting if the user enables both interfaces */ >>> + if (rs485->flags & SER_RS485_ENABLED && rs485->flags & SER_RS422_ENABLED) { >>> + dev_warn_ratelimited(port->dev, >>> + "%s (%d): Invalid serial interface setting, using RS485 instead\n", >>> + port->name, port->line); >>> + rs485->flags &= ~SER_RS422_ENABLED; >>> + } >>> + >>> + /* Clear other bits and return if RS422 is enabled */ >>> + if (rs485->flags & SER_RS422_ENABLED) { >>> + memset(rs485, 0, sizeof(*rs485)); >> >> Why are all flags cleared but SER_RS422_ENABLED? > > IMO, RS422 and RS485 are distinct serial interfaces. Therefore, when > RS422 is enabled, the other RS485 flags should be cleared, and vice > versa. > >>> + rs485->flags |= SER_RS422_ENABLED; >>> + return; >>> + } >> >> What about all the other code places that check for SER_RS485_ENABLED? >> For example uart_update_mctrl(), uart_suspend_port() and uart_resume_port() check this flag >> to decide whether to set the modem control lines or not. Should this not also apply to >> SER_RS422_ENABLED? > > After reviewing the code in serial_core.c, there are actually some codes > that check for "SER_RS485_ENABLED" flag before setting the modem control > lines. > > It also appears that these codes can only be done when disabling RS485. > > So yes, I will apply "SER_RS422_ENABLED" flag to these locations in the > next patch. > >> >> Maybe it would be better to change the meaning of the flag: Instead of being a substitution for >> SER_RS485_ENABLED, it could be used to mark a special mode. >> So if both SER_RS485_ENABLED and SER_RS485_MODE_RS422 are set it would mean that we have RS422. > > RS422 is not a mode of RS485, so I think using two flags to represent > them is much more reasonable, even though they are both included in the > "struct serial_rs485". Yes, RS422 is not a mode of RS485, but you are already using the rs485 (and not a rs422) structure. And treating RS422 as a different mode in the existing code would make things much easier and keep the code clean. For example you would not have to alter all the code places that check for SER_RS485_ENABLED. Also SER_RS485_ENABLED and SER_RS422_ENABLED would have the exact same effect, so why use two different flags, when the effect is the same? Regards, Lino
On Mon, Nov 06, 2023 at 03:43:49PM +0100, Lino Sanfilippo wrote: > On 06.11.23 08:19, Crescent CY Hsieh wrote: > > On Sat, Nov 04, 2023 at 08:53:18PM +0100, Lino Sanfilippo wrote: > > > > > > Maybe it would be better to change the meaning of the flag: Instead of being a substitution for > > > SER_RS485_ENABLED, it could be used to mark a special mode. > > > So if both SER_RS485_ENABLED and SER_RS485_MODE_RS422 are set it would mean that we have RS422. > > > > RS422 is not a mode of RS485, so I think using two flags to represent > > them is much more reasonable, even though they are both included in the > > "struct serial_rs485". > > Yes, RS422 is not a mode of RS485, but you are already using the rs485 (and not a rs422) structure. > And treating RS422 as a different mode in the existing code would make things much easier and keep the code > clean. For example you would not have to alter all the code places that check for SER_RS485_ENABLED. > Also SER_RS485_ENABLED and SER_RS422_ENABLED would have the exact same effect, so why use two > different flags, when the effect is the same? Agree, by treating RS422 as a mode, it would make things easier. However, I think, eventually, RS422 might add some configuration flags and should be distinguished from RS485 (Perhaps by adding RS422 structure or revising the name of RS485 structure...) But this should be a future work and require more discussion. Anyway, I will see RS422 as a mode in the next patch. Thanks for the suggestion. --- Sincerely, Crescent CY Hsieh
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index 831d03361..54a104c52 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -1305,7 +1305,7 @@ static int uart_get_icount(struct tty_struct *tty, #define SER_RS485_LEGACY_FLAGS (SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | \ SER_RS485_RTS_AFTER_SEND | SER_RS485_RX_DURING_TX | \ - SER_RS485_TERMINATE_BUS) + SER_RS485_TERMINATE_BUS | SER_RS422_ENABLED) static int uart_check_rs485_flags(struct uart_port *port, struct serial_rs485 *rs485) { @@ -1371,11 +1371,26 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4 { u32 supported_flags = port->rs485_supported.flags; - if (!(rs485->flags & SER_RS485_ENABLED)) { + if (!(rs485->flags & (SER_RS485_ENABLED | SER_RS422_ENABLED))) { memset(rs485, 0, sizeof(*rs485)); return; } + /* Pick sane setting if the user enables both interfaces */ + if (rs485->flags & SER_RS485_ENABLED && rs485->flags & SER_RS422_ENABLED) { + dev_warn_ratelimited(port->dev, + "%s (%d): Invalid serial interface setting, using RS485 instead\n", + port->name, port->line); + rs485->flags &= ~SER_RS422_ENABLED; + } + + /* Clear other bits and return if RS422 is enabled */ + if (rs485->flags & SER_RS422_ENABLED) { + memset(rs485, 0, sizeof(*rs485)); + rs485->flags |= SER_RS422_ENABLED; + return; + } + /* Pick sane settings if the user hasn't */ if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) && !(rs485->flags & SER_RS485_RTS_ON_SEND) == diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h index 53bc1af67..427609fd5 100644 --- a/include/uapi/linux/serial.h +++ b/include/uapi/linux/serial.h @@ -137,6 +137,8 @@ struct serial_icounter_struct { * * %SER_RS485_ADDRB - Enable RS485 addressing mode. * * %SER_RS485_ADDR_RECV - Receive address filter (enables @addr_recv). Requires %SER_RS485_ADDRB. * * %SER_RS485_ADDR_DEST - Destination address (enables @addr_dest). Requires %SER_RS485_ADDRB. + * + * * %SER_RS422_ENABLED - RS422 enabled. */ struct serial_rs485 { __u32 flags; @@ -149,6 +151,8 @@ struct serial_rs485 { #define SER_RS485_ADDR_RECV (1 << 7) #define SER_RS485_ADDR_DEST (1 << 8) +#define SER_RS422_ENABLED (1 << 9) + __u32 delay_rts_before_send; __u32 delay_rts_after_send;
Add "SER_RS422_ENABLED" flag within struct serial_rs485, so that serial port can switching interface into RS422 if supported by using ioctl command "TIOCSRS485". In case of interfaces confliction, add checks within uart_sanitize_serial_rs485() such that only one of RS422/RS485 is set. Signed-off-by: Crescent CY Hsieh <crescentcy.hsieh@moxa.com> --- Changes in v2: - Revise the logic that checks whether RS422/RS485 are enabled simultaneously. v1: https://lore.kernel.org/all/20231030053632.5109-1-crescentcy.hsieh@moxa.com/ --- drivers/tty/serial/serial_core.c | 19 +++++++++++++++++-- include/uapi/linux/serial.h | 4 ++++ 2 files changed, 21 insertions(+), 2 deletions(-)