| Message ID | 20180828174146.29894-2-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: make field unsigned | expand |
Quoting Lucas De Marchi (2018-08-28 18:41:46) > Document it like a real struct for ease of copy and paste, remove > comment of C99 compatibility and document that in some cases the first 2 I do recall that we couldn't use either C99 or class due to userspace compatibility... The essence is that we need a reminder that we can't assume the relaxed nature of kcc here. -Chris
On Tue, Aug 28, 2018 at 10:41:46AM -0700, Lucas De Marchi wrote: > Document it like a real struct for ease of copy and paste, remove > comment of C99 compatibility and document that in some cases the first 2 > fields can be u16. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > include/drm/i915_pciids.h | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h > index 754ce4b10129..0c2cc43f916c 100644 > --- a/include/drm/i915_pciids.h > +++ b/include/drm/i915_pciids.h > @@ -26,14 +26,16 @@ > #define _I915_PCIIDS_H > > /* > - * A pci_device_id struct { > - * __u32 vendor, device; > - * __u32 subvendor, subdevice; > - * __u32 class, class_mask; > - * kernel_ulong_t driver_data; > + * These macros can be used with a struct declared like this: > + * > + * struct pci_device_id { > + * __u32 vendor, device; > + * __u32 subvendor, subdevice; > + * __u32 class, class_mask; > + * kernel_ulong_t driver_data; > * }; > - * Don't use C99 here because "class" is reserved and we want to > - * give userspace flexibility. > + * > + * First two fields may be __u16 if PCI_DEVICE_ANY is not used PCI_DEVICE_ANY undefined? Also you can surely use u16 just fine as long as you're careful when comparing with ~0? > */ > #define INTEL_VGA_DEVICE(id, info) { \ > 0x8086, id, \ > -- > 2.17.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Aug 28, 2018 at 07:05:46PM +0100, Chris Wilson wrote: > Quoting Lucas De Marchi (2018-08-28 18:41:46) > > Document it like a real struct for ease of copy and paste, remove > > comment of C99 compatibility and document that in some cases the first 2 > > I do recall that we couldn't use either C99 or class due to userspace you can't actually use a c++ compiler. For C it works with any of -std=c99, gnu99, c11, gnu11, c17, gnu17. Tested with both gcc and clang. I've never heard of class being a reserved keyword and section 6.4.5 of said standard doesn't list it neither. Here the struct definition is in a *comment*... i.e. the user will copy and paste somewhere else and probably change __u16 to uint16_t in userspace. If he's building with g++, he can name the field to something else. If it was something we were defining in this header than I would agree with you... to retain compatibility with c++, not c99. Lucas De Marchi > compatibility... The essence is that we need a reminder that we can't > assume the relaxed nature of kcc here. > -Chris
On Tue, Aug 28, 2018 at 09:06:15PM +0300, Ville Syrjälä wrote: > On Tue, Aug 28, 2018 at 10:41:46AM -0700, Lucas De Marchi wrote: > > Document it like a real struct for ease of copy and paste, remove > > comment of C99 compatibility and document that in some cases the first 2 > > fields can be u16. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > --- > > include/drm/i915_pciids.h | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h > > index 754ce4b10129..0c2cc43f916c 100644 > > --- a/include/drm/i915_pciids.h > > +++ b/include/drm/i915_pciids.h > > @@ -26,14 +26,16 @@ > > #define _I915_PCIIDS_H > > > > /* > > - * A pci_device_id struct { > > - * __u32 vendor, device; > > - * __u32 subvendor, subdevice; > > - * __u32 class, class_mask; > > - * kernel_ulong_t driver_data; > > + * These macros can be used with a struct declared like this: > > + * > > + * struct pci_device_id { > > + * __u32 vendor, device; > > + * __u32 subvendor, subdevice; > > + * __u32 class, class_mask; > > + * kernel_ulong_t driver_data; > > * }; > > - * Don't use C99 here because "class" is reserved and we want to > > - * give userspace flexibility. > > + * > > + * First two fields may be __u16 if PCI_DEVICE_ANY is not used > > PCI_DEVICE_ANY undefined? this should be PCI_ANY_ID, but now I'm not sure if I should even mention it since it's defined somewhere that's private to kernel. > > Also you can surely use u16 just fine as long as you're careful when > comparing with ~0? nops, since ~0u is unsigned int (~0 being int before patch 1), it will overflow and cause a warning due to -Woverflow. This is what happens in igt if I do what you said: ../intel/i915_pciids.h:40:2: warning: conversion from ‘unsigned int’ to ‘short unsigned int’ changes value from ‘4294967295’ to ‘65535’ [-Woverflow] ~0u, ~0u, \ ^ Lucas De Marchi > > > */ > > #define INTEL_VGA_DEVICE(id, info) { \ > > 0x8086, id, \ > > -- > > 2.17.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
On Tue, 28 Aug 2018, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > On Tue, Aug 28, 2018 at 07:05:46PM +0100, Chris Wilson wrote: >> Quoting Lucas De Marchi (2018-08-28 18:41:46) >> > Document it like a real struct for ease of copy and paste, remove >> > comment of C99 compatibility and document that in some cases the first 2 >> >> I do recall that we couldn't use either C99 or class due to userspace > > you can't actually use a c++ compiler. > > For C it works with any of -std=c99, gnu99, c11, gnu11, c17, gnu17. > Tested with both gcc and clang. I've never heard of class being a > reserved keyword and section 6.4.5 of said standard doesn't list it > neither. > > Here the struct definition is in a *comment*... i.e. the user will copy > and paste somewhere else and probably change __u16 to uint16_t in > userspace. If he's building with g++, he can name the field to something > else. > > If it was something we were defining in this header than I would agree > with you... to retain compatibility with c++, not c99. I always thought the comment told you not to use designated initializers (introduced in C99), which would both impose a minimum C version requirement as well as bring .class = foo in the code, which requires more than just renaming the field with C++. BR, Jani. > > Lucas De Marchi > >> compatibility... The essence is that we need a reminder that we can't >> assume the relaxed nature of kcc here. >> -Chris > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/include/drm/i915_pciids.h b/include/drm/i915_pciids.h index 754ce4b10129..0c2cc43f916c 100644 --- a/include/drm/i915_pciids.h +++ b/include/drm/i915_pciids.h @@ -26,14 +26,16 @@ #define _I915_PCIIDS_H /* - * A pci_device_id struct { - * __u32 vendor, device; - * __u32 subvendor, subdevice; - * __u32 class, class_mask; - * kernel_ulong_t driver_data; + * These macros can be used with a struct declared like this: + * + * struct pci_device_id { + * __u32 vendor, device; + * __u32 subvendor, subdevice; + * __u32 class, class_mask; + * kernel_ulong_t driver_data; * }; - * Don't use C99 here because "class" is reserved and we want to - * give userspace flexibility. + * + * First two fields may be __u16 if PCI_DEVICE_ANY is not used */ #define INTEL_VGA_DEVICE(id, info) { \ 0x8086, id, \
Document it like a real struct for ease of copy and paste, remove comment of C99 compatibility and document that in some cases the first 2 fields can be u16. Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- include/drm/i915_pciids.h | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)