[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 at 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
> 
>