Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Kalle Valo <kvalo@kernel.org>
To:     David Laight <David.Laight@ACULAB.COM>
Cc:     'Martin Blumenstingl' <martin.blumenstingl@googlemail.com>,
        Ping-Ke Shih <pkshih@realtek.com>,
        "linux-wireless\@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        "tehuang\@realtek.com" <tehuang@realtek.com>,
        "s.hauer\@pengutronix.de" <s.hauer@pengutronix.de>,
        "tony0620emma\@gmail.com" <tony0620emma@gmail.com>,
        "netdev\@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/4] rtw88: Add packed attribute to the eFuse structs
References: <20221228133547.633797-1-martin.blumenstingl@googlemail.com>
        <20221228133547.633797-2-martin.blumenstingl@googlemail.com>
        <92eb7dfa8b7d447e966a2751e174b642@realtek.com>
        <87da8c82dec749dc826b5a1b4c4238aa@AcuMS.aculab.com>
        <eee17e2f4e44a2f38021a839dc39fedc1c1a4141.camel@realtek.com>
        <a86893f11fe64930897473a38226a9a8@AcuMS.aculab.com>
        <5c0c77240e7ddfdffbd771ee7e50d36ef3af9c84.camel@realtek.com>
        <CAFBinCC+1jGJx1McnBY+kr3RTQ-UpxW6JYNpHzStUTredDuCug@mail.gmail.com>
        <ec6a0988f3f943128e0122d50959185a@AcuMS.aculab.com>
Date:   Tue, 10 Jan 2023 14:02:52 +0200
In-Reply-To: <ec6a0988f3f943128e0122d50959185a@AcuMS.aculab.com> (David
        Laight's message of "Wed, 4 Jan 2023 15:53:17 +0000")
Message-ID: <87r0w2fvgz.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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