Re: [PATCH v5 06/10] serial: General support for multipoint addresses

Greg KH
2022-04-26T12:56:54Z

On Tue, Apr 26, 2022 at 03:24:44PM +0300, Ilpo Järvinen wrote:
> Add generic support for serial multipoint addressing. Two new ioctls
> are added. TIOCSADDR is used to indicate the destination/receive
> address. TIOCGADDR returns the current address in use. The driver
> should implement set_addr and get_addr to support addressing mode.
>
> Adjust ADDRB clearing to happen only if driver does not provide
> set_addr (=the driver doesn't support address mode).
>
> This change is necessary for supporting devices with RS485 multipoint
> addressing [*]. A following patch in the patch series adds support for
> Synopsys Designware UART capable for 9th bit addressing mode. In this
> mode, 9th bit is used to indicate an address (byte) within the
> communication line. The 9th bit addressing mode is selected using ADDRB
> introduced by the previous patch.
>
> Transmit addresses / receiver filter are specified by setting the flags
> SER_ADDR_DEST and/or SER_ADDR_RECV. When the user supplies the transmit
> address, in the 9bit addressing mode it is sent out immediately with
> the 9th bit set to 1. After that, the subsequent normal data bytes are
> sent with 9th bit as 0 and they are intended to the device with the
> given address. It is up to receiver to enforce the filter using
> SER_ADDR_RECV. When userspace has supplied the receive address, the
> driver is expected to handle the matching of the address and only data
> with that address is forwarded to the user. Both SER_ADDR_DEST and
> SER_ADDR_RECV can be given at the same time in a single call if the
> addresses are the same.
>
> The user can clear the receive filter with SER_ADDR_RECV_CLEAR.
>
> [*] 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: linux-api@vger.kernel.org
> Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> Cc: Matt Turner <mattst88@gmail.com>
> Cc: linux-alpha@vger.kernel.org
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: linux-mips@vger.kernel.org
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: linux-parisc@vger.kernel.org
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
> Cc: Rich Felker <dalias@libc.org>
> Cc: linux-sh@vger.kernel.org
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: sparclinux@vger.kernel.org
> Cc: Chris Zankel <chris@zankel.net>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Cc: linux-xtensa@linux-xtensa.org
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-arch@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> .../driver-api/serial/serial-rs485.rst | 23 ++++++-
> arch/alpha/include/uapi/asm/ioctls.h | 3 +
> arch/mips/include/uapi/asm/ioctls.h | 3 +
> arch/parisc/include/uapi/asm/ioctls.h | 3 +
> arch/powerpc/include/uapi/asm/ioctls.h | 3 +
> arch/sh/include/uapi/asm/ioctls.h | 3 +
> arch/sparc/include/uapi/asm/ioctls.h | 3 +
> arch/xtensa/include/uapi/asm/ioctls.h | 3 +
> drivers/tty/serial/8250/8250_core.c | 2 +
> drivers/tty/serial/serial_core.c | 62 ++++++++++++++++++-
> drivers/tty/tty_io.c | 2 +
> include/linux/serial_core.h | 6 ++
> include/uapi/asm-generic/ioctls.h | 3 +
> include/uapi/linux/serial.h | 8 +++
> 14 files changed, 125 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/driver-api/serial/serial-rs485.rst b/Documentation/driver-api/serial/serial-rs485.rst
> index 6bc824f948f9..2f45f007fa5b 100644
> --- a/Documentation/driver-api/serial/serial-rs485.rst
> +++ b/Documentation/driver-api/serial/serial-rs485.rst
> @@ -95,7 +95,28 @@ RS485 Serial Communications
> /* Error handling. See errno. */
> }
>
> -5. References
> +5. Multipoint Addressing
> +========================
> +
> + The Linux kernel provides serial_addr structure to handle addressing within
> + multipoint serial communications line such as RS485. 9th bit addressiong mode
> + is enabled by adding ADDRB flag in termios c_cflag.
> +
> + Serial core calls device specific set/get_addr in response to TIOCSADDR and
> + TIOCGADDR ioctls with a pointer to serial_addr. Destination and receive
> + address can be specified using serial_addr flags field. Receive address may
> + also be cleared using flags. Once an 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.
> +
> + Address flags:
> + - SER_ADDR_RECV: Receive (filter) address.
> + - SER_ADDR_RECV_CLEAR: Clear receive filter (only for TIOCSADDR).
> + - SER_ADDR_DEST: Destination address.
> +
> + Note: not all devices supporting RS485 support multipoint addressing.
> +
> +6. References
> =============
>
> [1] include/uapi/linux/serial.h
> diff --git a/arch/alpha/include/uapi/asm/ioctls.h b/arch/alpha/include/uapi/asm/ioctls.h
> index 971311605288..500cab3e1d6b 100644
> --- a/arch/alpha/include/uapi/asm/ioctls.h
> +++ b/arch/alpha/include/uapi/asm/ioctls.h
> @@ -125,4 +125,7 @@
> #define TIOCMIWAIT 0x545C /* wait for a change on serial input line(s) */
> #define TIOCGICOUNT 0x545D /* read serial port inline interrupt counts */
>
> +#define TIOCSADDR _IOWR('T', 0x63, struct serial_addr)
> +#define TIOCGADDR _IOWR('T', 0x64, struct serial_addr)
> +
> #endif /* _ASM_ALPHA_IOCTLS_H */
> diff --git a/arch/mips/include/uapi/asm/ioctls.h b/arch/mips/include/uapi/asm/ioctls.h
> index 16aa8a766aec..3859dc46857e 100644
> --- a/arch/mips/include/uapi/asm/ioctls.h
> +++ b/arch/mips/include/uapi/asm/ioctls.h
> @@ -96,6 +96,9 @@
> #define TIOCGISO7816 _IOR('T', 0x42, struct serial_iso7816)
> #define TIOCSISO7816 _IOWR('T', 0x43, struct serial_iso7816)
>
> +#define TIOCSADDR _IOWR('T', 0x63, struct serial_addr)
> +#define TIOCGADDR _IOWR('T', 0x64, struct serial_addr)
> +
> /* I hope the range from 0x5480 on is free ... */
> #define TIOCSCTTY 0x5480 /* become controlling tty */
> #define TIOCGSOFTCAR 0x5481
> diff --git a/arch/parisc/include/uapi/asm/ioctls.h b/arch/parisc/include/uapi/asm/ioctls.h
> index 82d1148c6379..62337743db64 100644
> --- a/arch/parisc/include/uapi/asm/ioctls.h
> +++ b/arch/parisc/include/uapi/asm/ioctls.h
> @@ -86,6 +86,9 @@
> #define TIOCSTOP 0x5462
> #define TIOCSLTC 0x5462
>
> +#define TIOCSADDR _IOWR('T', 0x63, struct serial_addr)
> +#define TIOCGADDR _IOWR('T', 0x64, struct serial_addr)
> +
> /* Used for packet mode */
> #define TIOCPKT_DATA 0
> #define TIOCPKT_FLUSHREAD 1
> diff --git a/arch/powerpc/include/uapi/asm/ioctls.h b/arch/powerpc/include/uapi/asm/ioctls.h
> index 2c145da3b774..84fd69ac366a 100644
> --- a/arch/powerpc/include/uapi/asm/ioctls.h
> +++ b/arch/powerpc/include/uapi/asm/ioctls.h
> @@ -120,4 +120,7 @@
> #define TIOCMIWAIT 0x545C /* wait for a change on serial input line(s) */
> #define TIOCGICOUNT 0x545D /* read serial port inline interrupt counts */
>
> +#define TIOCSADDR _IOWR('T', 0x63, struct serial_addr)
> +#define TIOCGADDR _IOWR('T', 0x64, struct serial_addr)
> +
> #endif /* _ASM_POWERPC_IOCTLS_H */
> diff --git a/arch/sh/include/uapi/asm/ioctls.h b/arch/sh/include/uapi/asm/ioctls.h
> index 11866d4f60e1..f82966b7dba2 100644
> --- a/arch/sh/include/uapi/asm/ioctls.h
> +++ b/arch/sh/include/uapi/asm/ioctls.h
> @@ -113,4 +113,7 @@
> #define TIOCMIWAIT _IO('T', 92) /* 0x545C */ /* wait for a change on serial input line(s) */
> #define TIOCGICOUNT 0x545D /* read serial port inline interrupt counts */
>
> +#define TIOCSADDR _IOWR('T', 0x63, struct serial_addr)
> +#define TIOCGADDR _IOWR('T', 0x64, struct serial_addr)
> +
> #endif /* __ASM_SH_IOCTLS_H */
> diff --git a/arch/sparc/include/uapi/asm/ioctls.h b/arch/sparc/include/uapi/asm/ioctls.h
> index 7fd2f5873c9e..e44624c67c79 100644
> --- a/arch/sparc/include/uapi/asm/ioctls.h
> +++ b/arch/sparc/include/uapi/asm/ioctls.h
> @@ -125,6 +125,9 @@
> #define TIOCMIWAIT 0x545C /* Wait for change on serial input line(s) */
> #define TIOCGICOUNT 0x545D /* Read serial port inline interrupt counts */
>
> +#define TIOCSADDR _IOWR('T', 0x63, struct serial_addr)
> +#define TIOCGADDR _IOWR('T', 0x64, struct serial_addr)
> +
> /* Kernel definitions */
>
> /* Used for packet mode */
> diff --git a/arch/xtensa/include/uapi/asm/ioctls.h b/arch/xtensa/include/uapi/asm/ioctls.h
> index 6d4a87296c95..759ca9377f2a 100644
> --- a/arch/xtensa/include/uapi/asm/ioctls.h
> +++ b/arch/xtensa/include/uapi/asm/ioctls.h
> @@ -127,4 +127,7 @@
> #define TIOCMIWAIT _IO('T', 92) /* wait for a change on serial input line(s) */
> #define TIOCGICOUNT 0x545D /* read serial port inline interrupt counts */
>
> +#define TIOCSADDR _IOWR('T', 0x63, struct serial_addr)
> +#define TIOCGADDR _IOWR('T', 0x64, struct serial_addr)
> +
> #endif /* _XTENSA_IOCTLS_H */
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 01d30f6ed8fb..f67bc3b76f65 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1008,6 +1008,8 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
> uart->port.rs485 = up->port.rs485;
> uart->rs485_start_tx = up->rs485_start_tx;
> uart->rs485_stop_tx = up->rs485_stop_tx;
> + uart->port.set_addr = up->port.set_addr;
> + uart->port.get_addr = up->port.get_addr;
> uart->dma = up->dma;
>
> /* Take tx_loadsz from fifosize if it wasn't set separately */
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 0f397e67eeb0..5762987bf176 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1397,6 +1397,56 @@ static int uart_set_iso7816_config(struct uart_port *port,
> return 0;
> }
>
> +static int uart_set_addr(struct uart_port *port,
> + struct serial_addr __user *serial_addr_user)
> +{
> + struct serial_addr addr;
> + unsigned long flags;
> + int ret;
> +
> + if (!port->set_addr)
> + return -ENOTTY;
> +
> + if (copy_from_user(&addr, serial_addr_user, sizeof(*serial_addr_user)))
> + return -EFAULT;
> +
> + spin_lock_irqsave(&port->lock, flags);
> + ret = port->set_addr(port, &addr);
> + spin_unlock_irqrestore(&port->lock, flags);
> + if (ret)
> + return ret;
> +
> + if (copy_to_user(serial_addr_user, &addr, sizeof(addr)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static int uart_get_addr(struct uart_port *port,
> + struct serial_addr __user *serial_addr_user)
> +{
> + struct serial_addr addr;
> + unsigned long flags;
> + int ret;
> +
> + if (!port->get_addr)
> + return -ENOTTY;
> +
> + if (copy_from_user(&addr, serial_addr_user, sizeof(*serial_addr_user)))
> + return -EFAULT;
> +
> + spin_lock_irqsave(&port->lock, flags);
> + ret = port->get_addr(port, &addr);
> + spin_unlock_irqrestore(&port->lock, flags);
> + if (ret)
> + return ret;
> +
> + if (copy_to_user(serial_addr_user, &addr, sizeof(addr)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> /*
> * Called via sys_ioctl. We can use spin_lock_irq() here.
> */
> @@ -1474,6 +1524,15 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
> case TIOCGISO7816:
> ret = uart_get_iso7816_config(state->uart_port, uarg);
> break;
> +
> + case TIOCSADDR:
> + ret = uart_set_addr(uport, uarg);
> + break;
> +
> + case TIOCGADDR:
> + ret = uart_get_addr(uport, uarg);
> + break;
> +
> default:
> if (uport->ops->ioctl)
> ret = uport->ops->ioctl(uport, cmd, arg);
> @@ -1540,7 +1599,8 @@ static void uart_set_termios(struct tty_struct *tty,
> goto out;
> }
>
> - tty->termios.c_cflag &= ~ADDRB;
> + if (!uport->set_addr)
> + tty->termios.c_cflag &= ~ADDRB;
>
> uart_change_speed(tty, state, old_termios);
> /* reload cflag from termios; port driver may have overridden flags */
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 8fec1d8648f5..ef22b95e9c79 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2885,6 +2885,8 @@ static long tty_compat_ioctl(struct file *file, unsigned int cmd,
> case TIOCSERGETLSR:
> case TIOCGRS485:
> case TIOCSRS485:
> + case TIOCSADDR:
> + case TIOCGADDR:
> #ifdef TIOCGETP
> case TIOCGETP:
> case TIOCSETP:
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index cbd5070bc87f..2dd4c4d01c59 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -135,6 +135,12 @@ struct uart_port {
> struct serial_rs485 *rs485);
> int (*iso7816_config)(struct uart_port *,
> struct serial_iso7816 *iso7816);
> +
> + int (*set_addr)(struct uart_port *p,
> + struct serial_addr *addr);
> + int (*get_addr)(struct uart_port *p,
> + struct serial_addr *addr);
> +
> unsigned int irq; /* irq number */
> unsigned long irqflags; /* irq flags */
> unsigned int uartclk; /* base uart clock */
> diff --git a/include/uapi/asm-generic/ioctls.h b/include/uapi/asm-generic/ioctls.h
> index cdc9f4ca8c27..689743366091 100644
> --- a/include/uapi/asm-generic/ioctls.h
> +++ b/include/uapi/asm-generic/ioctls.h
> @@ -106,6 +106,9 @@
> # define FIOQSIZE 0x5460
> #endif
>
> +#define TIOCSADDR _IOWR('T', 0x63, struct serial_addr)
> +#define TIOCGADDR _IOWR('T', 0x64, struct serial_addr)
> +
> /* Used for packet mode */
> #define TIOCPKT_DATA 0
> #define TIOCPKT_FLUSHREAD 1
> diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> index fa6b16e5fdd8..8cb785ea7087 100644
> --- a/include/uapi/linux/serial.h
> +++ b/include/uapi/linux/serial.h
> @@ -149,4 +149,12 @@ struct serial_iso7816 {
> __u32 reserved[5];
> };
>
> +struct serial_addr {
> + __u32 flags;
> +#define SER_ADDR_RECV (1 << 0)
> +#define SER_ADDR_RECV_CLEAR (1 << 1)
> +#define SER_ADDR_DEST (1 << 2)

You never check for invalid flags being sent to the kernel, which means
this api can never change in the future to add new flags :(

And what about struct serial_rs485? Shouldn't that be used here
instead? Why do we need a new ioctl and structure?

thanks,

greg k-h