Re: [PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Bart Van Assche <bart.vanassche@sandisk.com>]

On 05/31/2016 10:30 AM, Leon Romanovsky wrote:
> On Tue, May 31, 2016 at 11:13:06AM -0600, Jason Gunthorpe wrote:
>> On Tue, May 31, 2016 at 08:35:10AM -0700, Bart Van Assche wrote:
>>> On 05/30/16 03:09, Max Gurtovoy wrote:
>>>> ib_device_cap_flags 64-bit expansion caused a possible caps overlapping
>>>> (depending on machine endianness) and made consumers read wrong device
>>>> capabilities. For example IB_DEVICE_SG_GAPS_REG was falsely read by the
>>>> iser driver causing it to use a non-existing capability. Fix this by
>>>> casting ib_device_cap_flags enumerations to ULL.
>>>>
>>>> [ ... ]
>>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>>>> [ ... ]
>>>> enum ib_device_cap_flags {
>>>>       [ ... ]
>>>> 	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
>>>>      [ ... ]
>>>> };
>>>
>>> How can this patch make a difference? The presence of any constant
>>> in an enum that does not fit in a 32-bit integer makes an enum 64
>>> bits wide. In other words, all the changes from "1" into "1ULL" in
>>> this patch do not have
>>
>> The expressions are evaluated before the enum type is decided, the
>> enum type has no impact on the type of the expressions.
>
> It is machine/compiler dependent.
>
> Bart,
> Can you share your source of C-standard?
>
> This link [1] states in chapter "6.7.2.2 Enumeration specifiers"
>
> "Each enumerated type shall be compatible with char, a signed integer type,
> or an unsigned integer type. The choice of type is implementation-defined (110),
> but shall be capable of representing the values of all the members of the enumeration.
> The enumerated type is incomplete until after the } that terminates the list of enumerator
> declarations."
>
> And the footnote (110):
> "An implementation **may** delay the choice of which integer type until all enumeration
> constants have been seen."
>
> [1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

Let me rephrase my question. Before and after this patch 
IB_DEVICE_SG_GAPS_REG is defined as 1ULL << 32, so how can this patch 
make a difference? And if the issue is that some compilers choose a 
32-bit integer for ib_device_cap_flags and others a 64-bit integer, 
shouldn't ib_device_cap_flags be converted from an enum into a series of 
#defines? Or is the issue rather that some compilers choose the enum 
size depending on the size of the first enumeration constant? Anyway, I 
think the patch description needs to be clarified.

Bart.