[PATCH v8 0/6] Add RS485 9th bit addressing mode support to DW UART

[PATCH v8 0/6] Add RS485 9th bit addressing mode support to DW UART

[Ilpo Järvinen]

This patchset adds RS-485 9th bit addressing mode support to the DW
UART driver and the necessary serial core bits to handle it. The
addressing mode is configured through ->rs485_config() as was requested
during the review of the earlier versions. The line configuration
related ADDRB is still kept in ktermios->c_cflag to be able to take
account the extra addressing bit while calculating timing, etc. but it
is set/cleared by ->rs485_config().

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-api@vger.kernel.org
Cc: linux-arch@vger.kernel.org

v7 -> v8:
- Use anonymous union/struct in serial_rs485 to create "v2" of it
- Remove a stray newline change
- Reorder local var declarations
- Put ktermios param before serial_rs485 for rs485_config

v6 -> v7:
- Fixed typos in documentation & comment
- Changes lsr typing from unsigned int to u16

v5 -> v6:
- Reorder remaining patches
- LSR changes are simpler due to helper added by LSR fix series
- Depend on rs485_struct sanitization on catching much of invalid config
- In order to be able to alter ADDRB in termios .c_cflag within
  .rs485_config(), take termios_rwsem and pass ktermios to it.
- Moved addressing mode setup entirely into .rs485_config()
- Use ndelay() instead of udelay() (uart_port->frame_time is in nsecs)


Ilpo Järvinen (6):
  serial: 8250: make saved LSR larger
  serial: 8250: create lsr_save_mask
  serial: 8250_lpss: Use 32-bit reads
  serial: take termios_rwsem for ->rs485_config() & pass termios as
    param
  serial: Support for RS-485 multipoint addresses
  serial: 8250_dwlib: Support for 9th bit multipoint addressing

 Documentation/driver-api/serial/driver.rst    |   2 +
 .../driver-api/serial/serial-rs485.rst        |  26 ++++-
 drivers/tty/serial/8250/8250.h                |   9 +-
 drivers/tty/serial/8250/8250_core.c           |   4 +
 drivers/tty/serial/8250/8250_dw.c             |   2 +-
 drivers/tty/serial/8250/8250_dwlib.c          | 105 +++++++++++++++++-
 drivers/tty/serial/8250/8250_exar.c           |  11 +-
 drivers/tty/serial/8250/8250_fintek.c         |   2 +-
 drivers/tty/serial/8250/8250_fsl.c            |   2 +-
 drivers/tty/serial/8250/8250_ingenic.c        |   2 +-
 drivers/tty/serial/8250/8250_lpc18xx.c        |   2 +-
 drivers/tty/serial/8250/8250_lpss.c           |   2 +-
 drivers/tty/serial/8250/8250_omap.c           |   7 +-
 drivers/tty/serial/8250/8250_pci.c            |   2 +-
 drivers/tty/serial/8250/8250_port.c           |  20 ++--
 drivers/tty/serial/amba-pl011.c               |   2 +-
 drivers/tty/serial/ar933x_uart.c              |   2 +-
 drivers/tty/serial/atmel_serial.c             |   2 +-
 drivers/tty/serial/fsl_lpuart.c               |   4 +-
 drivers/tty/serial/imx.c                      |   2 +-
 drivers/tty/serial/max310x.c                  |   2 +-
 drivers/tty/serial/mcf.c                      |   3 +-
 drivers/tty/serial/omap-serial.c              |   3 +-
 drivers/tty/serial/sc16is7xx.c                |   2 +-
 drivers/tty/serial/serial_core.c              |  28 ++++-
 drivers/tty/serial/stm32-usart.c              |   2 +-
 drivers/tty/tty_ioctl.c                       |   4 +
 include/linux/serial_8250.h                   |   7 +-
 include/linux/serial_core.h                   |   1 +
 include/uapi/asm-generic/termbits-common.h    |   1 +
 include/uapi/linux/serial.h                   |  20 +++-
 31 files changed, 230 insertions(+), 53 deletions(-)

-- 
2.30.2

[PATCH v8 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.

The changes to serial_rs485 struct were test built with a few traps to
detect mislayouting on archs lkp/0day builts for (all went fine):
  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)));

[*] 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.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

---
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
---
 Documentation/driver-api/serial/driver.rst    |  2 ++
 .../driver-api/serial/serial-rs485.rst        | 26 ++++++++++++++++++-
 drivers/tty/serial/serial_core.c              | 14 +++++++++-
 drivers/tty/tty_ioctl.c                       |  4 +++
 include/uapi/asm-generic/termbits-common.h    |  1 +
 include/uapi/linux/serial.h                   | 20 ++++++++++++--
 6 files changed, 63 insertions(+), 4 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 44c3785445e3..414099e3262a 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;
 }
 
@@ -1349,7 +1360,8 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
 	rs485->flags &= supported_flags;
 
 	/* Return clean padding area to userspace */
-	memset(rs485->padding, 0, sizeof(rs485->padding));
+	memset(rs485->padding0, 0, sizeof(rs485->padding0));
+	memset(rs485->padding1, 0, sizeof(rs485->padding1));
 }
 
 int uart_rs485_config(struct uart_port *port)
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..c4c75fd7037e 100644
--- a/include/uapi/linux/serial.h
+++ b/include/uapi/linux/serial.h
@@ -126,10 +126,26 @@ 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 .. */
+	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];
+		};
+	};
 };
 
 /*
-- 
2.30.2

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

[Andy Shevchenko]

On Mon, Jun 20, 2022 at 09:40:29AM +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.

> The changes to serial_rs485 struct were test built with a few traps to
> detect mislayouting on archs lkp/0day builts for (all went fine):
>   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)));

You may add static_asserts() for the above mentioned cases.

> [*] 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.

...

> -	__u32	padding[5];		/* Memory is cheap, new structs
> -					   are a royal PITA .. */
> +	union {
> +		/* v1 */
> +		__u32	padding[5];		/* Memory is cheap, new structs are a pain */
> +
> +		/* v2 (adds addressing mode fields) */

How user space will inform a kernel that it's trying v2?

Usually when we have a union, it should be accompanied with the enum or version
or something to tell which part of it is in use. I can imagine that in this case
it's implied by the IOCTL parameters that never should be used on a garbage.

Either add a commit message / UAPI comment or add a version field or ...?

> +		struct {
> +			__u8	addr_recv;
> +			__u8	addr_dest;
> +			__u8	padding0[2];
> +			__u32	padding1[4];
> +		};
> +	};

-- 
With Best Regards,
Andy Shevchenko

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

[Ilpo Järvinen]

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

On Mon, 20 Jun 2022, Andy Shevchenko wrote:

> On Mon, Jun 20, 2022 at 09:40:29AM +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.
> 
> > The changes to serial_rs485 struct were test built with a few traps to
> > detect mislayouting on archs lkp/0day builts for (all went fine):
> >   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)));
> 
> You may add static_asserts() for the above mentioned cases.

I'll add into the end of serial_core.h but in a cleaned up form
using offsetof(). Those above look rather ugly :-).

> > [*] 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.
> 
> ...
> 
> > -	__u32	padding[5];		/* Memory is cheap, new structs
> > -					   are a royal PITA .. */
> > +	union {
> > +		/* v1 */
> > +		__u32	padding[5];		/* Memory is cheap, new structs are a pain */
> > +
> > +		/* v2 (adds addressing mode fields) */
> 
> How user space will inform a kernel that it's trying v2?
>
> Usually when we have a union, it should be accompanied with the enum or version
> or something to tell which part of it is in use. I can imagine that in this case
> it's implied by the IOCTL parameters that never should be used on a garbage.
> 
> Either add a commit message / UAPI comment or add a version field or ...?
> 
> > +		struct {
> > +			__u8	addr_recv;
> > +			__u8	addr_dest;

The flags in .flags indicate when these two new fields are in use. Do you 
think I need something beyond that. Maybe I should remove those comments 
so they don't mislead you to think it's a "version" for real?


-- 
 i.

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

[Andy Shevchenko]

On Mon, Jun 20, 2022 at 02:26:17PM +0300, Ilpo Järvinen wrote:
> On Mon, 20 Jun 2022, Andy Shevchenko wrote:
> > On Mon, Jun 20, 2022 at 09:40:29AM +0300, Ilpo Järvinen wrote:

...

> > > The changes to serial_rs485 struct were test built with a few traps to
> > > detect mislayouting on archs lkp/0day builts for (all went fine):
> > >   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)));
> > 
> > You may add static_asserts() for the above mentioned cases.
> 
> I'll add into the end of serial_core.h but in a cleaned up form
> using offsetof(). Those above look rather ugly :-).

Agree!

...

> > > -	__u32	padding[5];		/* Memory is cheap, new structs
> > > -					   are a royal PITA .. */
> > > +	union {
> > > +		/* v1 */
> > > +		__u32	padding[5];		/* Memory is cheap, new structs are a pain */
> > > +
> > > +		/* v2 (adds addressing mode fields) */
> > 
> > How user space will inform a kernel that it's trying v2?
> >
> > Usually when we have a union, it should be accompanied with the enum or version
> > or something to tell which part of it is in use. I can imagine that in this case
> > it's implied by the IOCTL parameters that never should be used on a garbage.
> > 
> > Either add a commit message / UAPI comment or add a version field or ...?
> > 
> > > +		struct {
> > > +			__u8	addr_recv;
> > > +			__u8	addr_dest;
> 
> The flags in .flags indicate when these two new fields are in use. Do you 
> think I need something beyond that. Maybe I should remove those comments 
> so they don't mislead you to think it's a "version" for real?

Yes, either drop this versioning, or replace with a comment on top of a union
like:

	/* The fields are defined by flags */

-- 
With Best Regards,
Andy Shevchenko