Re: [PATCH] bitfield: avoid gcc-8 -Wint-in-bool-context warning

[Arnd Bergmann]

On Tue, Aug 14, 2018 at 12:04 PM David Laight <David.Laight@xxxxxxxxxx> wrote:
>
> From: Johannes Berg
> > Sent: 14 August 2018 08:57
> ...
> > > How about fixing the root cause
> > > in drivers/net/wireless/intel/iwlwifi/fw/api/rx.h ?
> > >
> > >
> > > #define IWL_RX_HE_PHY_SIBG_SYM_OR_USER_NUM_MASK   0x1e00000000ULL
> > >
> > >
> > > enum iwl_rx_he_phy looks really strange.
> >
> > Why? I don't think this is a problem, the enum is used here to get
> > constants so that we can also have documentation for them. That's a
> > common and accepted technique.
>
> It would be much more useful to indicate where the values are used.
> Such a field/parameter could (probably) have the type of the enum.
> But, at some point, the compiler might start barfing at that at well.

I think the compiler warning here only happens because one uses
a compile-time constant expression that is not a numeric literal value
into a boolean operator. That doesn't mean that there is something
wrong with the enum in particular, or that enums cause a lot of
issues elsewhere.

I would also argue that generally speaking the BUILD_BUG_ON_MSG()
should try to either produce the specific build failure it was designed
for, or not produce any output at all, rather than something
that is more confusing to developers. If we want to enforce
passing in number literals here, we should make that an explicit
check, or otherwise allow any compile-time constant values.

> There are also a whole load of crappy __packed in that header file.
> There might be one or two 64bit items on 32bit boundaries but
> that can be solved without using __packed.

Agreed, this likely causes problems on architectures without unaligned
load/store instructions that end up doing byte accesses to the descriptor
fields, which in turn can confuse the hardware, and can become very
slow when they live in dma_alloc_coherent() memory. That looks
like a completely unrelated issue though.

      Arnd