On Thu, 2019-01-31 at 13:52 +0200, Kalle Valo wrote:
> Tony Chuang <[email protected]> writes:
>
> > > From: Brian Norris [mailto:[email protected]]
> > >
> > > > +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
