Re: [PATCH v3 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
From: David Miller <davem@davemloft.net>
Date: 2021-04-09 00:41:31
Also in:
linux-hyperv, lkml
From: Dexuan Cui <decui@microsoft.com> Date: Fri, 9 Apr 2021 00:24:51 +0000
quoted
From: David Miller <davem@davemloft.net> Sent: Thursday, April 8, 2021 4:46 PM ...quoted
+struct gdma_msg_hdr { + u32 hdr_type; + u32 msg_type; + u16 msg_version; + u16 hwc_msg_id; + u32 msg_size; +} __packed; + +struct gdma_dev_id { + union { + struct { + u16 type; + u16 instance; + }; + + u32 as_uint32; + }; +} __packed;Please don't use __packed unless absolutely necessary. It generates suboptimal code (byte at a time accesses etc.) and for many of these you don't even need it.In the driver code, all the structs/unions marked by __packed are used to talk with the hardware, so I think __packed is necessary here?
It actually isan't in many cases, check with and without the __packed directive and see if anything chasnges.
quoted hunk ↗ jump to hunk
Do you think if it's better if we remove all the __packed, and add static_assert(sizeof(struct XXX) == YYY) instead? e.g.@@ -105,7 +105,8 @@ struct gdma_msg_hdr { u16 msg_version; u16 hwc_msg_id; u32 msg_size; -} __packed; +}; +static_assert(sizeof(struct gdma_msg_hdr) == 16);
This won't make sure the structure member offsets are what you expect. I think you'll have to go through the structures one-by-one by hand to figure out which ones really require the __packed attribute and which do not. Thank you.