DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 6788B60AD7
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Tony Chuang <yhchuang@realtek.com>
Cc:     Brian Norris <briannorris@chromium.org>,
        "johannes\@sipsolutions.net" <johannes@sipsolutions.net>,
        "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>
Date:   Thu, 31 Jan 2019 13:52:28 +0200
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D179ABC2@RTITMBSVM04.realtek.com.tw>
        (Tony Chuang's message of "Thu, 31 Jan 2019 11:36:35 +0000")
Message-ID: <87ef8tjb7n.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

Tony Chuang <yhchuang@realtek.com> writes:

>> From: Brian Norris [mailto:briannorris@chromium.org]
>>=20
>> > +static inline void
>> > +rtw_write32s_mask(struct rtw_dev *rtwdev, u32 addr, u32 mask, u32 dat=
a)
>> > +{
>> > +	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 time --
>> structure offsets, constants, etc. This is usually (always?) a constant,
>> 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 error=
: 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 deleted
>> enough calls to rtw_write32s_mask() it came down to this one:
>>=20
>>         rtw_write32s_mask(rtwdev, REG_RFEINV, BIT(11) | BIT(10) | 0x3f,
>> 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.
>
> Yeah, you're right. I think we should turn it into macro.

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.

--=20
Kalle Valo