Message-ID: <e1e062f20e61e61e1428802436248782be08ea7d.camel@sipsolutions.net>
Subject: Re: [PATCH v4 09/13] rtw88: chip files
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Kalle Valo <kvalo@codeaurora.org>,
        Tony Chuang <yhchuang@realtek.com>
Cc:     Brian Norris <briannorris@chromium.org>,
        "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>
Date:   Thu, 31 Jan 2019 12:55:15 +0100
In-Reply-To: <87ef8tjb7n.fsf@kamboji.qca.qualcomm.com>
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>
         <87ef8tjb7n.fsf@kamboji.qca.qualcomm.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Thu, 2019-01-31 at 13:52 +0200, Kalle Valo wrote:
> 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.

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