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 <[email protected]>
---
Changes in v2:
- Revise the logic that checks whether RS422/RS485 are enabled
simultaneously.
v1: https://lore.kernel.org/all/[email protected]/
---
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)
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;
--
2.34.1
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 <[email protected]>
>
> ---
> Changes in v2:
> - Revise the logic that checks whether RS422/RS485 are enabled
> simultaneously.
>
> v1: https://lore.kernel.org/all/[email protected]/
>
> ---
> 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?
> + 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)
Why the extra blank line before this?
And why isn't it using the proper BIT() type macro instead (yeah, the
others are not, I understand...)
Also, what userspace code is going to use this? How is it tested?
thanks,
greg k-h
On Wed, Nov 01, 2023 at 07:49:48AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Nov 01, 2023 at 02:44:04PM +0800, Crescent CY Hsieh wrote:
> > 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)
>
> Why the extra blank line before this?
The extra blank line is for the clarity, to seperate RS422 flag from
RS485 flags.
> Also, what userspace code is going to use this? How is it tested?
This flag could be used when user tries to switch serial interface into
RS422, just like the original flag "SER_RS485_ENABLED" can also be used to
switch serial interface into RS485 with some RS485 configurations.
---
Sincerely,
Crescent CY Hsieh
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 <[email protected]>
>
> ---
> Changes in v2:
> - Revise the logic that checks whether RS422/RS485 are enabled
> simultaneously.
>
> v1: https://lore.kernel.org/all/[email protected]/
>
> ---
> 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 <[email protected]>
>>
>> ---
>> Changes in v2:
>> - Revise the logic that checks whether RS422/RS485 are enabled
>> simultaneously.
>>
>> v1: https://lore.kernel.org/all/[email protected]/
>>
>> ---
>> 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