Re: [PATCH v2] byteswap: try to avoid __builtin_constant_p gcc bug

[Arnd Bergmann]

On Wednesday, June 22, 2016 11:24:50 AM CEST Tomas Winkler wrote:
> On Tue, Jun 21, 2016 at 12:02 PM, Tomas Winkler <tomasw@xxxxxxxxx> wrote:
> > On Tue, May 3, 2016 at 9:36 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> >> On Monday 02 May 2016 16:32:25 Andrew Morton wrote:

> >>  #ifdef __HAVE_BUILTIN_BSWAP32__
> >> -#define __swab32(x) __builtin_bswap32((__u32)(x))
> >> +#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
> >>  #else
> >>  #define __swab32(x)                            \
> >>         (__builtin_constant_p((__u32)(x)) ?     \
> >
> >>
> >
> > I wonder if this doesn't break switch statement that requires a
> > constant expression, there few cases like this over the kernel.
> >
> > switch(val) {
> > case cpu_to_le32(IXGBE_RXDADV_STAT_FCSTAT_FCPRSP):
> >
> > http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c#L458
> >
> 
> I'm asking because sparse and checkpatch doesn't agree on that ping
> sparse issues
> 'error: bad constant expression'
> When changing to __constant_cpu_to_le32 sparse is happy but
> checkpatch.ps is complaining
> __constant_cpu_to_le32 should be cpu_to_le32
> 

That is an interesting problem, as both seem to be reasonable:
sparse probably doesn't understand __builtin_constant_p() enough,
while avoiding __constant_cpu_to_le32() is a good recommendation
in general.

How many instances of this do you see in the kernel? If ixgbe is the
only one, I'd just move the byteswap up into the switch statement:

	switch (le32_to_cpu(val)) {
	case IXGBE_RXDADV_STAT_FCSTAT_FCPRSP:

which may cost one or two cycles for the non-constant byteswap,
but is also easier to read than the current code.

	Arnd