Re: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs

[Kalle Valo <kvalo@kernel.org>]

David Laight <David.Laight@ACULAB.COM> writes:

> From: Martin Blumenstingl
>> Sent: 04 January 2023 15:30
>> 
>> Hi Ping-Ke, Hi David,
>> 
>> On Sun, Jan 1, 2023 at 2:09 PM Ping-Ke Shih <pkshih@realtek.com> wrote:
>> [...]
>> > Yes, it should not use bit filed. Instead, use a __le16 for all fields, such as
>> I think this can be done in a separate patch.
>> My v2 of this patch has reduced these changes to a minimum, see [0]
>> 
>> [...]
>> > struct rtw8821ce_efuse {
>> >    ...
>> >    u8 data1;       // offset 0x100
>> >    __le16 data2;   // offset 0x101-0x102
>> >    ...
>> > } __packed;
>> >
>> > Without __packed, compiler could has pad between data1 and data2,
>> > and then get wrong result.
>> My understanding is that this is the reason why we need __packed.
>
> True, but does it really have to look like that?
> I can't find that version (I don't have a net_next tree).
> Possibly it should be 'u8 data2[2];'
>
> Most hardware definitions align everything.
>
> What you may want to do is add compile-time asserts for the
> sizes of the structures.
>
> Remember that if you have 16/32 bit fields in packed structures
> on some architectures the compile has to generate code that does
> byte loads and shifts.
>
> The 'misaligned' property is lost when you take the address - so
> you can easily generate a fault.
>
> Adding __packed to a struct is a sledgehammer you really shouldn't need.

Avoiding use of __packed is news to me, but is this really a safe rule?
Most of the wireless engineers are no compiler experts (myself included)
so I'm worried. For example, in ath10k and ath11k I try to use __packed
for all structs which are accessing hardware or firmware just to make
sure that the compiler is not changing anything.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches