Re: [PATCH 02/14] can: rx-fifo: Increase MB size limit from 32 to 64

[David Jander <david@protonic.nl>]

On Fri, 07 Nov 2014 09:40:36 +0100
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 11/06/2014 05:20 PM, David Jander wrote:
> >>> Btw, there is a subtle bug in the line you linked to:
> >>>
> >>>> #define GENMASK_ULL(h, l)       (((U64_C(1) << ((h) - (l) + 1)) - 1) <<
> >>>> (l))
> >>>
> >>> The _ULL in the name of the macro seems to hint to "unsigned long long",
> >>> while the implementation seems to assume that it means U64_C. This is
> >>> obviously wrong, although for most platforms that are currently capable
> >>> of running Linux, those are probably equivalent...
> >>
> >> #define U64_C(x) x ## ULL
> > 
> > This line:
> > 
> > http://lxr.free-electrons.com/source/include/asm-generic/int-ll64.h#L4
> > 
> > ...seems to indicate, that it may not always be the case :-)
> > AFAIK, C99 specifies that "long long" should at _least_ have 64 bit length,
> > but says nothing about when it may be larger.
> > OTOH, I have yet to find a machine/compiler combination that defines "long
> > long" as more than 64bit... at least not running Linux.
> > A correct definition should be trivial though:
> > 
> > #define GENMASK_ULL(h, l)       (((1ULL << ((h) - (l) + 1)) - 1) << (l))
> 
> That's what it does:
> 
> $ cat << EOF | gcc -xc -E -
> #define U64_C(x) x ## ULL
> #define GENMASK_ULL(h, l)   (((U64_C(1) << ((h) - (l) + 1)) - 1) << (l))
> GENMASK_ULL(foo, bar)
> EOF
> 
> Results in:
> (((1ULL << ((foo) - (bar) + 1)) - 1) << (bar))

Yes of course! I am not questioning that.
The problem is that although right now asm-generic/int-ll64.h is the only
place where U64_C is defined, this may not be the case in the future. In the
same way as in C99 an "unsigned long" can be either 32bit or 64bit wide, there
is also no guarantee that an "unsigned long long" will always be 64bits. If
tomorrow we add support for another arch in Linux that happens to define
"unsigned long long" as 128bit for example, on that platform we will have
another version arch/foo/include/asm/int-ll64.h that defines...

#define U64_C(x) x ## UL

On that platform one would assume that something called "GENMASK_ULL" is
supposed to support 128bit masks, which is not true because the definition is
wrong. Of course you can say that in that case one should also re-define
GENMASK_ULL, but why do that when fixing the current definition (replace
U64_C(1) with 1ULL) would always be correct? The point is, that in "C" mixing
type definitions between C-types (int, long, long long, char, etc...) with
fixed-size types (int32_t, int64_t, int8_t, etc...) is semantically wrong,
platform-dependent and should be avoided. That's what stdint.h is for in
user-land.
Btw, on some 16-bit DSP processors, the C-type "char" is 16-bit wide... how
many issues do you think this can cause if one is not careful with type
naming? Not that I intend to run Linux on such a DSP, but you get the idea...

Now, can we please get back on-topic ;-)

I am going to use this macro (although it is semantically slightly broken) in
the next version.

Best regards,

-- 
David Jander
Protonic Holland.