[PATCH v7 5/6] serial: Support for RS-485 multipoint addresses

[PATCH v7 5/6] serial: Support for RS-485 multipoint addresses

[Ilpo Järvinen]

Add support for RS-485 multipoint addressing using 9th bit [*]. The
addressing mode is configured through .rs485_config().

ADDRB in termios indicates 9th bit addressing mode is enabled. In this
mode, 9th bit is used to indicate an address (byte) within the
communication line. ADDRB can only be enabled/disabled through
.rs485_config() that is also responsible for setting the destination and
receiver (filter) addresses.

[*] Technically, RS485 is just an electronic spec and does not itself
specify the 9th bit addressing mode but 9th bit seems at least
"semi-standard" way to do addressing with RS485.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: linux-api@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 Documentation/driver-api/serial/driver.rst    |  2 ++
 .../driver-api/serial/serial-rs485.rst        | 26 ++++++++++++++++++-
 drivers/tty/serial/serial_core.c              | 11 ++++++++
 drivers/tty/tty_ioctl.c                       |  4 +++
 include/uapi/asm-generic/termbits-common.h    |  1 +
 include/uapi/linux/serial.h                   | 12 +++++++--
 6 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/Documentation/driver-api/serial/driver.rst b/Documentation/driver-api/serial/driver.rst
index 7ef83fd3917b..35fb3866bb3d 100644
--- a/Documentation/driver-api/serial/driver.rst
+++ b/Documentation/driver-api/serial/driver.rst
@@ -261,6 +261,8 @@ hardware.
 			- parity enable
 		PARODD
 			- odd parity (when PARENB is in force)
+		ADDRB
+			- address bit (changed through .rs485_config()).
 		CREAD
 			- enable reception of characters (if not set,
 			  still receive characters from the port, but
diff --git a/Documentation/driver-api/serial/serial-rs485.rst b/Documentation/driver-api/serial/serial-rs485.rst
index 00b5d333acba..6ebad75c74ed 100644
--- a/Documentation/driver-api/serial/serial-rs485.rst
+++ b/Documentation/driver-api/serial/serial-rs485.rst
@@ -99,7 +99,31 @@ RS485 Serial Communications
 		/* Error handling. See errno. */
 	}
 
-5. References
+5. Multipoint Addressing
+========================
+
+   The Linux kernel provides addressing mode for multipoint RS-485 serial
+   communications line. The addressing mode is enabled with SER_RS485_ADDRB
+   flag in serial_rs485. Struct serial_rs485 has two additional flags and
+   fields for enabling receive and destination addresses.
+
+   Address mode flags:
+	- SER_RS485_ADDRB: Enabled addressing mode (sets also ADDRB in termios).
+	- SER_RS485_ADDR_RECV: Receive (filter) address enabled.
+	- SER_RS485_ADDR_DEST: Set destination address.
+
+   Address fields (enabled with corresponding SER_RS485_ADDR_* flag):
+	- addr_recv: Receive address.
+	- addr_dest: Destination address.
+
+   Once a receive address is set, the communication can occur only with the
+   particular device and other peers are filtered out. It is left up to the
+   receiver side to enforce the filtering. Receive address will be cleared
+   if SER_RS485_ADDR_RECV is not set.
+
+   Note: not all devices supporting RS485 support multipoint addressing.
+
+6. References
 =============
 
  [1]	include/uapi/linux/serial.h
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 76bb1b77b06e..b2bce1696540 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1294,6 +1294,17 @@ static int uart_check_rs485_flags(struct uart_port *port, struct serial_rs485 *r
 	if (flags & ~port->rs485_supported->flags)
 		return -EINVAL;
 
+	/* Asking for address w/o addressing mode? */
+	if (!(rs485->flags & SER_RS485_ADDRB) &&
+	    (rs485->flags & (SER_RS485_ADDR_RECV|SER_RS485_ADDR_DEST)))
+		return -EINVAL;
+
+	/* Address given but not enabled? */
+	if (!(rs485->flags & SER_RS485_ADDR_RECV) && rs485->addr_recv)
+		return -EINVAL;
+	if (!(rs485->flags & SER_RS485_ADDR_DEST) && rs485->addr_dest)
+		return -EINVAL;
+
 	return 0;
 }
 
diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index adae687f654b..ed253f2337a7 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -319,6 +319,8 @@ unsigned char tty_get_frame_size(unsigned int cflag)
 		bits++;
 	if (cflag & PARENB)
 		bits++;
+	if (cflag & ADDRB)
+		bits++;
 
 	return bits;
 }
@@ -353,6 +355,8 @@ int tty_set_termios(struct tty_struct *tty, struct ktermios *new_termios)
 	old_termios = tty->termios;
 	tty->termios = *new_termios;
 	unset_locked_termios(tty, &old_termios);
+	/* Reset any ADDRB changes, ADDRB is changed through .rs485_config() */
+	tty->termios.c_cflag ^= (tty->termios.c_cflag ^ old_termios.c_cflag) & ADDRB;
 
 	if (tty->ops->set_termios)
 		tty->ops->set_termios(tty, &old_termios);
diff --git a/include/uapi/asm-generic/termbits-common.h b/include/uapi/asm-generic/termbits-common.h
index 4d084fe8def5..4a6a79f28b21 100644
--- a/include/uapi/asm-generic/termbits-common.h
+++ b/include/uapi/asm-generic/termbits-common.h
@@ -46,6 +46,7 @@ typedef unsigned int	speed_t;
 #define EXTA		B19200
 #define EXTB		B38400
 
+#define ADDRB		0x20000000	/* address bit */
 #define CMSPAR		0x40000000	/* mark or space (stick) parity */
 #define CRTSCTS		0x80000000	/* flow control */
 
diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
index fa6b16e5fdd8..9df86f0a3d46 100644
--- a/include/uapi/linux/serial.h
+++ b/include/uapi/linux/serial.h
@@ -126,10 +126,18 @@ struct serial_rs485 {
 #define SER_RS485_TERMINATE_BUS		(1 << 5)	/* Enable bus
 							   termination
 							   (if supported) */
+
+/* RS-485 addressing mode */
+#define SER_RS485_ADDRB			(1 << 6)	/* Enable addressing mode */
+#define SER_RS485_ADDR_RECV		(1 << 7)	/* Receive address filter */
+#define SER_RS485_ADDR_DEST		(1 << 8)	/* Destination address */
+
 	__u32	delay_rts_before_send;	/* Delay before send (milliseconds) */
 	__u32	delay_rts_after_send;	/* Delay after send (milliseconds) */
-	__u32	padding[5];		/* Memory is cheap, new structs
-					   are a royal PITA .. */
+	__u8	addr_recv;
+	__u8	addr_dest;
+	__u8	padding[2 + 4 * sizeof(__u32)];		/* Memory is cheap, new structs
+							 * are a royal PITA .. */
 };
 
 /*
-- 
2.30.2

Re: [PATCH v7 5/6] serial: Support for RS-485 multipoint addresses

[Andy Shevchenko]

On Wed, Jun 15, 2022 at 03:48:28PM +0300, Ilpo Järvinen wrote:
> Add support for RS-485 multipoint addressing using 9th bit [*]. The
> addressing mode is configured through .rs485_config().
> 
> ADDRB in termios indicates 9th bit addressing mode is enabled. In this
> mode, 9th bit is used to indicate an address (byte) within the
> communication line. ADDRB can only be enabled/disabled through
> .rs485_config() that is also responsible for setting the destination and
> receiver (filter) addresses.
> 
> [*] Technically, RS485 is just an electronic spec and does not itself
> specify the 9th bit addressing mode but 9th bit seems at least
> "semi-standard" way to do addressing with RS485.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: linux-api@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arch@vger.kernel.org

Hmm... In order to reduce commit messages you can move these Cc:s after the
cutter line ('---').

...

> -	__u32	padding[5];		/* Memory is cheap, new structs
> -					   are a royal PITA .. */
> +	__u8	addr_recv;
> +	__u8	addr_dest;
> +	__u8	padding[2 + 4 * sizeof(__u32)];		/* Memory is cheap, new structs
> +							 * are a royal PITA .. */

I'm not sure it's an equivalent. I would leave u32 members  untouched, so
something like

	__u8	addr_recv;
	__u8	addr_dest;
	__u8	padding0[2];		/* Memory is cheap, new structs
	__u32	padding1[4];		 * are a royal PITA .. */

And repeating about `pahole` tool which may be useful here to check for ABI
potential changes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 5/6] serial: Support for RS-485 multipoint addresses

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2668 bytes --]

On Wed, 15 Jun 2022, Andy Shevchenko wrote:

> On Wed, Jun 15, 2022 at 03:48:28PM +0300, Ilpo Järvinen wrote:
> > Add support for RS-485 multipoint addressing using 9th bit [*]. The
> > addressing mode is configured through .rs485_config().
> > 
> > ADDRB in termios indicates 9th bit addressing mode is enabled. In this
> > mode, 9th bit is used to indicate an address (byte) within the
> > communication line. ADDRB can only be enabled/disabled through
> > .rs485_config() that is also responsible for setting the destination and
> > receiver (filter) addresses.
> > 
> > [*] Technically, RS485 is just an electronic spec and does not itself
> > specify the 9th bit addressing mode but 9th bit seems at least
> > "semi-standard" way to do addressing with RS485.
> > 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Jonathan Corbet <corbet@lwn.net>
> > Cc: linux-api@vger.kernel.org
> > Cc: linux-doc@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux-arch@vger.kernel.org
> 
> Hmm... In order to reduce commit messages you can move these Cc:s after the
> cutter line ('---').

Ok, although the toolchain I use didn't support preserving --- content
so I had to create hack to preserve them, hopefully nothing backfires due 
to the hack. :-)

> > -	__u32	padding[5];		/* Memory is cheap, new structs
> > -					   are a royal PITA .. */
> > +	__u8	addr_recv;
> > +	__u8	addr_dest;
> > +	__u8	padding[2 + 4 * sizeof(__u32)];		/* Memory is cheap, new structs
> > +							 * are a royal PITA .. */
> 
> I'm not sure it's an equivalent. I would leave u32 members  untouched, so
> something like
> 
> 	__u8	addr_recv;
> 	__u8	addr_dest;
> 	__u8	padding0[2];		/* Memory is cheap, new structs
> 	__u32	padding1[4];		 * are a royal PITA .. */
> 
> And repeating about `pahole` tool which may be useful here to check for ABI
> potential changes.

I cannot take __u32 padding[] away like that, this is an uapi header. Or 
do you mean I should create anonymous union? ...I'm skeptical that can be 
pulled off w/o breaking user-space compile in some circumstances. Anon 
unions were only introduced by C11 but is it ok to rely on C11 in uapi/ 
headers?

Even making padding smaller has some unwanted consequences if somebody is 
clearing just .padding. In retrospect, having padding as a direct member 
doesn't seem a good idea. That padding[5] should have been within an union 
right from the start to make this easily extendable.

Maybe create a copy of that struct under another name which is just equal 
sized, that would give more freedom on member naming. But can I change 
ioctl's param type to another struct (in _IOR/_IOWR) w/o breaking 
something?

-- 
 i.

Re: [PATCH v7 5/6] serial: Support for RS-485 multipoint addresses

[Jiri Slaby]

On 16. 06. 22, 7:04, Ilpo Järvinen wrote:
> On Wed, 15 Jun 2022, Andy Shevchenko wrote:
> 
>> On Wed, Jun 15, 2022 at 03:48:28PM +0300, Ilpo Järvinen wrote:
>>> Add support for RS-485 multipoint addressing using 9th bit [*]. The
>>> addressing mode is configured through .rs485_config().
>>>
>>> ADDRB in termios indicates 9th bit addressing mode is enabled. In this
>>> mode, 9th bit is used to indicate an address (byte) within the
>>> communication line. ADDRB can only be enabled/disabled through
>>> .rs485_config() that is also responsible for setting the destination and
>>> receiver (filter) addresses.
>>>
>>> [*] Technically, RS485 is just an electronic spec and does not itself
>>> specify the 9th bit addressing mode but 9th bit seems at least
>>> "semi-standard" way to do addressing with RS485.
>>>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Cc: Jonathan Corbet <corbet@lwn.net>
>>> Cc: linux-api@vger.kernel.org
>>> Cc: linux-doc@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Cc: linux-arch@vger.kernel.org
>>
>> Hmm... In order to reduce commit messages you can move these Cc:s after the
>> cutter line ('---').
> 
> Ok, although the toolchain I use didn't support preserving --- content
> so I had to create hack to preserve them, hopefully nothing backfires due
> to the hack. :-)
> 
>>> -	__u32	padding[5];		/* Memory is cheap, new structs
>>> -					   are a royal PITA .. */
>>> +	__u8	addr_recv;
>>> +	__u8	addr_dest;
>>> +	__u8	padding[2 + 4 * sizeof(__u32)];		/* Memory is cheap, new structs
>>> +							 * are a royal PITA .. */
>>
>> I'm not sure it's an equivalent. I would leave u32 members  untouched, so
>> something like
>>
>> 	__u8	addr_recv;
>> 	__u8	addr_dest;
>> 	__u8	padding0[2];		/* Memory is cheap, new structs
>> 	__u32	padding1[4];		 * are a royal PITA .. */
>>
>> And repeating about `pahole` tool which may be useful here to check for ABI
>> potential changes.
> 
> I cannot take __u32 padding[] away like that, this is an uapi header.

Yeah, but it's padding after all. I would personally break it for 
example as Andy suggests (if pahole shows no differences in size on both 
32/64 bit) and wait if something breaks. To be honest, I'd not expect 
anyone to touch it. And if someone does, we would fix it somehow and 
they should too...

thanks,
-- 
js
suse labs

Re: [PATCH v7 5/6] serial: Support for RS-485 multipoint addresses

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3500 bytes --]

On Thu, 16 Jun 2022, Jiri Slaby wrote:

> On 16. 06. 22, 7:04, Ilpo Järvinen wrote:
> > On Wed, 15 Jun 2022, Andy Shevchenko wrote:
> > 
> > > On Wed, Jun 15, 2022 at 03:48:28PM +0300, Ilpo Järvinen wrote:
> > > > Add support for RS-485 multipoint addressing using 9th bit [*]. The
> > > > addressing mode is configured through .rs485_config().
> > > > 
> > > > ADDRB in termios indicates 9th bit addressing mode is enabled. In this
> > > > mode, 9th bit is used to indicate an address (byte) within the
> > > > communication line. ADDRB can only be enabled/disabled through
> > > > .rs485_config() that is also responsible for setting the destination and
> > > > receiver (filter) addresses.
> > > > 
> > > > [*] Technically, RS485 is just an electronic spec and does not itself
> > > > specify the 9th bit addressing mode but 9th bit seems at least
> > > > "semi-standard" way to do addressing with RS485.
> > > > 
> > > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > > Cc: Jonathan Corbet <corbet@lwn.net>
> > > > Cc: linux-api@vger.kernel.org
> > > > Cc: linux-doc@vger.kernel.org
> > > > Cc: linux-kernel@vger.kernel.org
> > > > Cc: linux-arch@vger.kernel.org
> > > 
> > > Hmm... In order to reduce commit messages you can move these Cc:s after
> > > the
> > > cutter line ('---').
> > 
> > Ok, although the toolchain I use didn't support preserving --- content
> > so I had to create hack to preserve them, hopefully nothing backfires due
> > to the hack. :-)
> > 
> > > > -	__u32	padding[5];		/* Memory is cheap, new structs
> > > > -					   are a royal PITA .. */
> > > > +	__u8	addr_recv;
> > > > +	__u8	addr_dest;
> > > > +	__u8	padding[2 + 4 * sizeof(__u32)];		/* Memory is cheap,
> > > > new structs
> > > > +							 * are a royal PITA ..
> > > > */
> > > 
> > > I'm not sure it's an equivalent. I would leave u32 members  untouched, so
> > > something like
> > > 
> > > 	__u8	addr_recv;
> > > 	__u8	addr_dest;
> > > 	__u8	padding0[2];		/* Memory is cheap, new structs
> > > 	__u32	padding1[4];		 * are a royal PITA .. */
> > > 
> > > And repeating about `pahole` tool which may be useful here to check for
> > > ABI
> > > potential changes.
> > 
> > I cannot take __u32 padding[] away like that, this is an uapi header.
> 
> Yeah, but it's padding after all. I would personally break it for example as
> Andy suggests (if pahole shows no differences in size on both 32/64 bit) and
> wait if something breaks. To be honest, I'd not expect anyone to touch it. And
> if someone does, we would fix it somehow and they should too...

I realized there are plenty of anonymous unions already in include/uapi/ 
so I think I can keep padding[5] too:

        union {
                /* v1 */
                __u32   padding[5];             /* Memory is cheap, new structs are a pain */

                /* v2 (adds addressing mode fields) */
                struct {
                        __u8    addr_recv;
                        __u8    addr_dest;
                        __u8    padding0[2];
                        __u32   padding1[4];
                };
        };

I'll just skip manual pahole step and add a few BUILD_BUG_ON()s and use 
our build bot to do a quick check over all archs it builds for, that gives 
much better confidence on it being ok:

	BUILD_BUG_ON(((&rs485.delay_rts_after_send) + 1) != &rs485.padding[0]);
	BUILD_BUG_ON(&rs485.padding[1] != &rs485.padding1[0]);
	BUILD_BUG_ON(sizeof(rs485) != ((u8 *)(&rs485.padding[4]) - ((u8 *)&rs485.flags) + sizeof(__u32)));


-- 
 i.