Re: [PATCH v4 09/13] rtw88: chip files

[Kalle Valo <kvalo@codeaurora.org>]

Tony Chuang <yhchuang@realtek.com> writes:

>> From: Brian Norris [mailto:briannorris@chromium.org]
>> 
>> > +static inline void
>> > +rtw_write32s_mask(struct rtw_dev *rtwdev, u32 addr, u32 mask, u32 data)
>> > +{
>> > +	BUILD_BUG_ON(addr < 0xC00 || addr >= 0xD00);
>> 
>> 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.
>> 
>> Anyway...this is failing confusingly for me when I try to build:
>> 
>> drivers/net/wireless/realtek/rtw88/rtw8822b.c: In function
>> ‘rtw_write32s_mask’:
>> drivers/net/wireless/realtek/rtw88/rtw8822b.c:230:176: error: call to
>> ‘__compiletime_assert_230’ declared with attribute error: BUILD_BUG_ON
>> failed: addr < 0xC00 || addr >= 0xD00
>>   BUILD_BUG_ON(addr < 0xC00 || addr >= 0xD00);
>> 
>> ^
>> 
>> 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:
>> 
>>         rtw_write32s_mask(rtwdev, REG_RFEINV, BIT(11) | BIT(10) | 0x3f,
>> 0x0);
>> 
>> which doesn't really make sense, as that's a value of 0xcbc.
>> 
>> 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.

-- 
Kalle Valo