[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87r0w2fvgz.fsf@kernel.org>
Date: Tue, 10 Jan 2023 14:02:52 +0200
From: Kalle Valo <kvalo@...nel.org>
To: David Laight <David.Laight@...LAB.COM>
Cc: 'Martin Blumenstingl' <martin.blumenstingl@...glemail.com>,
Ping-Ke Shih <pkshih@...ltek.com>,
"linux-wireless\@vger.kernel.org" <linux-wireless@...r.kernel.org>,
"tehuang\@realtek.com" <tehuang@...ltek.com>,
"s.hauer\@pengutronix.de" <s.hauer@...gutronix.de>,
"tony0620emma\@gmail.com" <tony0620emma@...il.com>,
"netdev\@vger.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs
David Laight <David.Laight@...LAB.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@...ltek.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
Powered by blists - more mailing lists