RE: [EXT] Re: [net PATCH] octeontx2-af: Fix marking couple of structure as __packed

From: Suman Ghosh
Date: Wed Dec 20 2023 - 08:05:14 EST


>>>
>>> However that may not be true across all compilers etc. Also all the
>>> other structures are __packed. Makes sense.
>>
>> Or not - maybe all the __packed should be removed instead!
>>
>> Unless these structures (or any others) appear in 'messages' which get
>> transferred between systems they really shouldn't be __packed.
>> And a 93 byte 'message' with all those fields seems rather odd.
>>
>> The above breakdown seems to imply everything is 'unsigned char'
>> so the __packed makes no difference.
>>
>> Using __packed requires the compiler generate byte loads/store with
>> shifts (etc) on many architectures and should really be avoided unless
>> it is absolutely needed for binary compatibility.
>>
>> Even then if the problem is a 64bit field that only needs to be 32bit
>> aligned (as is common for some compat32 code) then the 64bit fields
>> should be marked as being 32bit aligned.
>>
>> David
>>
>Right. Typically packed is only required when dealing with something
>where the exact binary layout matters (i.e. copying to/from hardware or
>across systems in such a way that the layout might change with different
>compilers/arch).
>
>If that isn't how this structure is used, then ya, removing __packed
>seems reasonable. And at least for one system I see no difference in the
>actual generated layout, making __packed redundant.
>
>However, its not clear to me at a glance how this structure is used and
>whether it really is copied between places where binary compatibility is
>a requirement.
>
>> -
>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
>> MK1 1PT, UK Registration No: 1397386 (Wales)
[Suman] Yes, these structures are copied from firmware. It is needed to inform kernel about some parsing information required by hardware. That is the reason structures are packed and these two were missed.