Johannes Berg <johannes@xxxxxxxxxxxxxxxx> writes: > 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? 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. -- Kalle Valo
