[Intel-wired-lan] [next-queue] fm10k: explain why we need __packed on some TLV structures

[Intel-wired-lan] [next-queue] fm10k: explain why we need __packed on some TLV structures

[Jacob Keller]

Add an explanatory comment about the __packed attribute on these
structures due to TLV data layout requirements.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/fm10k/fm10k_pf.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pf.h b/drivers/net/ethernet/intel/fm10k/fm10k_pf.h
index a8fc512a2416..661a4062b756 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 specifically to TLV mailbox
+ * messages, and must not have gaps between their values. They must line up
+ * correctly to the TLV definition.
+ */
+
 struct fm10k_mac_update {
 	__le32	mac_lower;
 	__le16	mac_upper;
-- 
2.6.1.264.gbab76a9

[Intel-wired-lan] [next-queue] fm10k: explain why we need __packed on some TLV structures

[Alexander Duyck]

On 11/03/2015 11:32 AM, Jacob Keller wrote:
> Add an explanatory comment about the __packed attribute on these
> structures due to TLV data layout requirements.
>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>   drivers/net/ethernet/intel/fm10k/fm10k_pf.h | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pf.h b/drivers/net/ethernet/intel/fm10k/fm10k_pf.h
> index a8fc512a2416..661a4062b756 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 specifically to TLV mailbox
> + * messages, and must not have gaps between their values. They must line up
> + * correctly to the TLV definition.
> + */
> +
>   struct fm10k_mac_update {
>   	__le32	mac_lower;
>   	__le16	mac_upper;

Really I don't think this tells the story of why the packed attribute is 
needed.  From what I can tell there aren't any holes in the structures 
themselves.  It looks like the issue is the padding on the end.  From 
what I can tell only the only message that really needs the packed 
attribute it is the 1588 message because it uses __le64 values and as 
such the actual size will vary by 4 bytes between 64b and 32b systems.

- Alex

[Intel-wired-lan] [next-queue] fm10k: explain why we need __packed on some TLV structures

[Keller, Jacob E]

On Tue, 2015-11-03 at 13:47 -0800, Alexander Duyck wrote:
> On 11/03/2015 11:32 AM, Jacob Keller wrote:
> > Add an explanatory comment about the __packed attribute on these
> > structures due to TLV data layout requirements.
> > 
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > ---
> >   drivers/net/ethernet/intel/fm10k/fm10k_pf.h | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pf.h
> > b/drivers/net/ethernet/intel/fm10k/fm10k_pf.h
> > index a8fc512a2416..661a4062b756 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 specifically to TLV
> > mailbox
> > + * messages, and must not have gaps between their values. They
> > must line up
> > + * correctly to the TLV definition.
> > + */
> > +
> >   struct fm10k_mac_update {
> >   	__le32	mac_lower;
> >   	__le16	mac_upper;
> 
> Really I don't think this tells the story of why the packed attribute
> is 
> needed.  From what I can tell there aren't any holes in the
> structures 
> themselves.  It looks like the issue is the padding on the end.  From
> what I can tell only the only message that really needs the packed 
> attribute it is the 1588 message because it uses __le64 values and as
> such the actual size will vary by 4 bytes between 64b and 32b
> systems.
> 

I know at least one of the structures broke the mailbox due to sizing,
because there were gaps in how the structure was laid out in memory,
when being copied into the TLV. It doesn't just add space at the end,
we had a huge bug when this was originally added due to sizing issues.
I think this comment explains the reasoning even if it doesn't
necessarily required for these particular structures, we're better off
being safe by specifying __packed for these structures given their
usage.

Regards,
Jake

> - Alex
> 
> 

[Intel-wired-lan] [next-queue] fm10k: explain why we need __packed on some TLV structures

[Alexander Duyck]

On 11/03/2015 01:55 PM, Keller, Jacob E wrote:
> On Tue, 2015-11-03 at 13:47 -0800, Alexander Duyck wrote:
>> On 11/03/2015 11:32 AM, Jacob Keller wrote:
>>> Add an explanatory comment about the __packed attribute on these
>>> structures due to TLV data layout requirements.
>>>
>>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>>> ---
>>>    drivers/net/ethernet/intel/fm10k/fm10k_pf.h | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pf.h
>>> b/drivers/net/ethernet/intel/fm10k/fm10k_pf.h
>>> index a8fc512a2416..661a4062b756 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 specifically to TLV
>>> mailbox
>>> + * messages, and must not have gaps between their values. They
>>> must line up
>>> + * correctly to the TLV definition.
>>> + */
>>> +
>>>    struct fm10k_mac_update {
>>>    	__le32	mac_lower;
>>>    	__le16	mac_upper;
>> Really I don't think this tells the story of why the packed attribute
>> is
>> needed.  From what I can tell there aren't any holes in the
>> structures
>> themselves.  It looks like the issue is the padding on the end.  From
>> what I can tell only the only message that really needs the packed
>> attribute it is the 1588 message because it uses __le64 values and as
>> such the actual size will vary by 4 bytes between 64b and 32b
>> systems.
>>
> I know at least one of the structures broke the mailbox due to sizing,
> because there were gaps in how the structure was laid out in memory,
> when being copied into the TLV. It doesn't just add space at the end,
> we had a huge bug when this was originally added due to sizing issues.
> I think this comment explains the reasoning even if it doesn't
> necessarily required for these particular structures, we're better off
> being safe by specifying __packed for these structures given their
> usage.

Actually my concern is that __packed can make things less safe. 
Specifically the TLV code as it currently is assumes structures are 
aligned by at least 32 bits.  Using the __packed attribute potentially 
throws that out the window and only 8b aligns the structure.  For now 
things are safe because all of the structures are 32b aligned at least.

What you are probably looking for is something more like 
"__attribute__((packed, aligned(4))" instead of just straight packed.  
That way if any new fields are added you would get the proper alignment 
in terms of size since the TLV code assumes a minimum of 32b alignment 
for structures.

- Alex

[Intel-wired-lan] [next-queue] fm10k: explain why we need __packed on some TLV structures

[Keller, Jacob E]

On Tue, 2015-11-03 at 14:40 -0800, Alexander Duyck wrote:
> On 11/03/2015 01:55 PM, Keller, Jacob E wrote:
> > On Tue, 2015-11-03 at 13:47 -0800, Alexander Duyck wrote:
> > > On 11/03/2015 11:32 AM, Jacob Keller wrote:
> > > > Add an explanatory comment about the __packed attribute on
> > > > these
> > > > structures due to TLV data layout requirements.
> > > > 
> > > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > > > ---
> > > >    drivers/net/ethernet/intel/fm10k/fm10k_pf.h | 5 +++++
> > > >    1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pf.h
> > > > b/drivers/net/ethernet/intel/fm10k/fm10k_pf.h
> > > > index a8fc512a2416..661a4062b756 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 specifically to
> > > > TLV
> > > > mailbox
> > > > + * messages, and must not have gaps between their values. They
> > > > must line up
> > > > + * correctly to the TLV definition.
> > > > + */
> > > > +
> > > >    struct fm10k_mac_update {
> > > >    	__le32	mac_lower;
> > > >    	__le16	mac_upper;
> > > Really I don't think this tells the story of why the packed
> > > attribute
> > > is
> > > needed.  From what I can tell there aren't any holes in the
> > > structures
> > > themselves.  It looks like the issue is the padding on the end. 
> > >  From
> > > what I can tell only the only message that really needs the
> > > packed
> > > attribute it is the 1588 message because it uses __le64 values
> > > and as
> > > such the actual size will vary by 4 bytes between 64b and 32b
> > > systems.
> > > 
> > I know at least one of the structures broke the mailbox due to
> > sizing,
> > because there were gaps in how the structure was laid out in
> > memory,
> > when being copied into the TLV. It doesn't just add space at the
> > end,
> > we had a huge bug when this was originally added due to sizing
> > issues.
> > I think this comment explains the reasoning even if it doesn't
> > necessarily required for these particular structures, we're better
> > off
> > being safe by specifying __packed for these structures given their
> > usage.
> 
> Actually my concern is that __packed can make things less safe. 
> Specifically the TLV code as it currently is assumes structures are 
> aligned by at least 32 bits.  Using the __packed attribute
> potentially 
> throws that out the window and only 8b aligns the structure.  For now
> things are safe because all of the structures are 32b aligned at
> least.
> 
> What you are probably looking for is something more like 
> "__attribute__((packed, aligned(4))" instead of just straight packed.
>   
> That way if any new fields are added you would get the proper
> alignment 
> in terms of size since the TLV code assumes a minimum of 32b
> alignment 
> for structures.
> 
> - Alex
> 

Hmm. It does appear to be doing chunks of the structure in 4byte
alignment, so that's correct. I thought it was different but a quick
glance at the TLV code suggests you are correct.

I'll work up that change.

Regards,
Jake