DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 0120060867
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     Tony Chuang <yhchuang@realtek.com>,
        Brian Norris <briannorris@chromium.org>,
        "Larry.Finger\@lwfinger.net" <Larry.Finger@lwfinger.net>,
        Pkshih <pkshih@realtek.com>, Andy Huang <tehuang@realtek.com>,
        "sgruszka\@redhat.com" <sgruszka@redhat.com>,
        "linux-wireless\@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH v4 09/13] rtw88: chip files
References: <1548820940-15237-1-git-send-email-yhchuang@realtek.com>
        <1548820940-15237-10-git-send-email-yhchuang@realtek.com>
        <20190130194453.GA156750@google.com>
        <F7CD281DE3E379468C6D07993EA72F84D179ABC2@RTITMBSVM04.realtek.com.tw>
        <87ef8tjb7n.fsf@kamboji.qca.qualcomm.com>
        <e1e062f20e61e61e1428802436248782be08ea7d.camel@sipsolutions.net>
Date:   Thu, 31 Jan 2019 15:40:13 +0200
In-Reply-To: <e1e062f20e61e61e1428802436248782be08ea7d.camel@sipsolutions.net>
        (Johannes Berg's message of "Thu, 31 Jan 2019 12:55:15 +0100")
Message-ID: <87a7jhj682.fsf@kamboji.qca.qualcomm.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Johannes Berg <johannes@sipsolutions.net> writes:

> On Thu, 2019-01-31 at 13:52 +0200, Kalle Valo wrote:
>> Tony Chuang <yhchuang@realtek.com> writes:
>>=20
>> > > From: Brian Norris [mailto:briannorris@chromium.org]
>> > >=20
>> > > > +static inline void
>> > > > +rtw_write32s_mask(struct rtw_dev *rtwdev, u32 addr, u32 mask, u32=
 data)
>> > > > +{
>> > > > +	BUILD_BUG_ON(addr < 0xC00 || addr >=3D 0xD00);
>> > >=20
>> > > This seems to be a non-traditional use of BUILD_BUG_ON(). Normally, I
>> > > see this used for stuff that's guaranteed to be known at compile tim=
e --
>> > > structure offsets, constants, etc. This is usually (always?) a const=
ant,
>> > > but it passes through a function parameter, so I'm not sure if that's
>> > > really guaranteed.
>> > >=20
>> > > Anyway...this is failing confusingly for me when I try to build:
>> > >=20
>> > > drivers/net/wireless/realtek/rtw88/rtw8822b.c: In function
>> > > =E2=80=98rtw_write32s_mask=E2=80=99:
>> > > drivers/net/wireless/realtek/rtw88/rtw8822b.c:230:176: error: call to
>> > > =E2=80=98__compiletime_assert_230=E2=80=99 declared with attribute e=
rror: BUILD_BUG_ON
>> > > failed: addr < 0xC00 || addr >=3D 0xD00
>> > >   BUILD_BUG_ON(addr < 0xC00 || addr >=3D 0xD00);
>> > >=20
>> > > ^
>> > >=20
>> > > I tried to pinpoint which call yielded this, and I think once I dele=
ted
>> > > enough calls to rtw_write32s_mask() it came down to this one:
>> > >=20
>> > >         rtw_write32s_mask(rtwdev, REG_RFEINV, BIT(11) | BIT(10) | 0x=
3f,
>> > > 0x0);
>> > >=20
>> > > which doesn't really make sense, as that's a value of 0xcbc.
>> > >=20
>> > > What I really think it comes down to is that you can't guarantee
>> > > rtw_write32s_mask() will get inlined, and so BUILD_BUG_ON() may not =
know
>> > > what to do with it.
>> >=20
>> > Yeah, you're right. I think we should turn it into macro.
>>=20
>> Does this really need to be a build time check? Like Brian said, this is
>> not really common use of BUILD_BUG_ON(). I would just change it to
>> WARN_ON_ONCE() or a ratelimited warning message so that we don't to have
>> an ugly macro.
>
> Well, it *is* strictly stronger as a build-time check, so that makes
> sense?

Sure, it's a lot stronger check. But IMHO this isn't that important (or
difficult to spot) that it would need to be a compile time check and a
runtime check would suffice as well.

> I'd probably still suggest doing something like this:
>
> static inline void _rtw_write32s_mask(...)
> {
>   ... as before w/o BUILD_BUG_ON ...
> }
> #define rtw_write32s_mask(bla, bla) do { \
>    BUILD_BUG_ON(...); \
>    _rtw_write32s_mask(...); \
> } while (0)
>
> so you also get the type checks etc. from having actual function args.

Yes, this would be a perfect compromise.

--=20
Kalle Valo