On Mon, 20 Jun 2022, Andy Shevchenko wrote:

On Mon, Jun 20, 2022 at 09:40:29AM +0300, Ilpo Järvinen wrote:

quoted

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.

quoted

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 :-).

quoted

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

...

quoted

-	__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 ...?

quoted

+		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.