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

On Thu, 16 Jun 2022, Jiri Slaby wrote:

On 16. 06. 22, 7:04, Ilpo Järvinen wrote:

quoted

On Wed, 15 Jun 2022, Andy Shevchenko wrote:

quoted

On Wed, Jun 15, 2022 at 03:48:28PM +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.

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

quoted

quoted

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