[Intel-wired-lan] [next-queue PATCH v3] fm10k: correctly pack TLV structures and explain reasoning

[Intel-wired-lan] [next-queue PATCH v3] fm10k: correctly pack TLV structures and explain reasoning

[Jacob Keller]

The TLV format for little endian structures is actually 4 byte aligned
copy. To this end, we need to add an additional __aligned(4) marker
along with __packed to ensure that these structures are actually 4 byte
aligned and packed correctly. Use of just __packed will not work as this
will result in 1byte alignment which is incorrect. Add a comment
explaining the reasoning behind why these structures need the special
treatment.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
- v3
* use __aligned(4) instead of __attribute__(aligned(4))

Note: this patch replaces both TLV patches currently on the queue as it
looks like Jeff had forgotten to remove the earlier one when the subject
changed.

 drivers/net/ethernet/intel/fm10k/fm10k_pf.h | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pf.h b/drivers/net/ethernet/intel/fm10k/fm10k_pf.h
index a8fc512a2416..337859260b9b 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_pf.h
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_pf.h
@@ -74,6 +74,11 @@ enum fm10k_pf_tlv_attr_id_v1 {
 #define FM10K_MSG_UPDATE_PVID_PVID_SHIFT	16
 #define FM10K_MSG_UPDATE_PVID_PVID_SIZE		16
 
+/* The following data structures are overlayed directly onto TLV mailbox
+ * messages, and must not break 4 byte alignment. Ensure the structures line
+ * up correctly as per their TLV definition.
+ */
+
 struct fm10k_mac_update {
 	__le32	mac_lower;
 	__le16	mac_upper;
@@ -81,26 +86,26 @@ struct fm10k_mac_update {
 	__le16	glort;
 	u8	flags;
 	u8	action;
-} __packed;
+} __aligned(4) __packed;
 
 struct fm10k_global_table_data {
 	__le32	used;
 	__le32	avail;
-} __packed;
+} __aligned(4) __packed;
 
 struct fm10k_swapi_error {
 	__le32				status;
 	struct fm10k_global_table_data	mac;
 	struct fm10k_global_table_data	nexthop;
 	struct fm10k_global_table_data	ffu;
-} __packed;
+} __aligned(4) __packed;
 
 struct fm10k_swapi_1588_timestamp {
 	__le64 egress;
 	__le64 ingress;
 	__le16 dglort;
 	__le16 sglort;
-} __packed;
+} __aligned(4) __packed;
 
 s32 fm10k_msg_lport_map_pf(struct fm10k_hw *, u32 **, struct fm10k_mbx_info *);
 extern const struct fm10k_tlv_attr fm10k_lport_map_msg_attr[];
-- 
2.6.1.264.gbab76a9

[Intel-wired-lan] [next-queue PATCH v3] fm10k: correctly pack TLV structures and explain reasoning

[Jeff Kirsher]

On Mon, 2015-11-09 at 14:04 -0800, Jacob Keller wrote:
> The TLV format for little endian structures is actually 4 byte
> aligned
> copy. To this end, we need to add an additional __aligned(4) marker
> along with __packed to ensure that these structures are actually 4
> byte
> aligned and packed correctly. Use of just __packed will not work as
> this
> will result in 1byte alignment which is incorrect. Add a comment
> explaining the reasoning behind why these structures need the special
> treatment.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> - v3
> * use __aligned(4) instead of __attribute__(aligned(4))
> 
> Note: this patch replaces both TLV patches currently on the queue as
> it
> looks like Jeff had forgotten to remove the earlier one when the
> subject
> changed.

Nope, I just applied the last submitted version of this patch. ?Might
be helpful if the version actually incremented when you re-submit a
patch. ?This is the third version of "v3", so techincally this should
have been v5. :-)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20151110/7cf5337d/attachment.asc>

[Intel-wired-lan] [next-queue PATCH v3] fm10k: correctly pack TLV structures and explain reasoning

[Keller, Jacob E]

On Tue, 2015-11-10 at 08:27 -0800, Jeff Kirsher wrote:
> On Mon, 2015-11-09 at 14:04 -0800, Jacob Keller wrote:
> > The TLV format for little endian structures is actually 4 byte
> > aligned
> > copy. To this end, we need to add an additional __aligned(4) marker
> > along with __packed to ensure that these structures are actually 4
> > byte
> > aligned and packed correctly. Use of just __packed will not work as
> > this
> > will result in 1byte alignment which is incorrect. Add a comment
> > explaining the reasoning behind why these structures need the
> > special
> > treatment.
> > 
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > ---
> > - v3
> > * use __aligned(4) instead of __attribute__(aligned(4))
> > 
> > Note: this patch replaces both TLV patches currently on the queue
> > as
> > it
> > looks like Jeff had forgotten to remove the earlier one when the
> > subject
> > changed.
> 
> Nope, I just applied the last submitted version of this patch. ?Might
> be helpful if the version actually incremented when you re-submit a
> patch. ?This is the third version of "v3", so techincally this should
> have been v5. :-)

At one point there were two different versions on the queue somehow.

It is difficult to remember the version because git doesn't make it
easy to keep track of the version. I'm currently working on how to
integrate git-notes into my workflow for this sort of thing.

Regards,
Jake

[Intel-wired-lan] [next-queue PATCH v3] fm10k: correctly pack TLV structures and explain reasoning

[Singh, Krishneil K]

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Keller, Jacob E
Sent: Tuesday, November 10, 2015 9:38 AM
To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; intel-wired-lan at lists.osuosl.org
Subject: Re: [Intel-wired-lan] [next-queue PATCH v3] fm10k: correctly pack TLV structures and explain reasoning

Tested-by: Krishneil Singh <Krishneil.k.singh@intel.com>