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

[Leon Romanovsky <leon@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2731 bytes --]

On Tue, May 31, 2016 at 11:05:00AM -0700, Bart Van Assche wrote:
> 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.

I understand from the description/standards that first enum declares
type.

> 
> Bart.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]