On Thu, 2019-01-31 at 13:52 +0200, Kalle Valo wrote: > Tony Chuang <yhchuang@xxxxxxxxxxx> writes: > > > > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > > > > > > > +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. Well, it *is* strictly stronger as a build-time check, so that makes sense? 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. johannes
