Linux kernel tolerates C++ style comments these days. Actually, the
SPDX License tags for .c files start with //.
On the other hand, uapi headers are written in more strict C, where
the C++ comment style is forbidden.
Signed-off-by: Masahiro Yamada <[email protected]>
---
include/uapi/linux/dvb/audio.h | 2 +-
include/uapi/linux/dvb/osd.h | 170 ++++++++++++++++++++-------------
2 files changed, 103 insertions(+), 69 deletions(-)
diff --git a/include/uapi/linux/dvb/audio.h b/include/uapi/linux/dvb/audio.h
index afeae063e640..977bed135e22 100644
--- a/include/uapi/linux/dvb/audio.h
+++ b/include/uapi/linux/dvb/audio.h
@@ -52,7 +52,7 @@ typedef enum {
typedef struct audio_mixer {
unsigned int volume_left;
unsigned int volume_right;
- // what else do we need? bass, pass-through, ...
+ /* what else do we need? bass, pass-through, ... */
} audio_mixer_t;
diff --git a/include/uapi/linux/dvb/osd.h b/include/uapi/linux/dvb/osd.h
index e163508b9ae8..723824ce3622 100644
--- a/include/uapi/linux/dvb/osd.h
+++ b/include/uapi/linux/dvb/osd.h
@@ -28,74 +28,108 @@
#include <linux/compiler.h>
typedef enum {
- // All functions return -2 on "not open"
- OSD_Close=1, // ()
- // Disables OSD and releases the buffers
- // returns 0 on success
- OSD_Open, // (x0,y0,x1,y1,BitPerPixel[2/4/8](color&0x0F),mix[0..15](color&0xF0))
- // Opens OSD with this size and bit depth
- // returns 0 on success, -1 on DRAM allocation error, -2 on "already open"
- OSD_Show, // ()
- // enables OSD mode
- // returns 0 on success
- OSD_Hide, // ()
- // disables OSD mode
- // returns 0 on success
- OSD_Clear, // ()
- // Sets all pixel to color 0
- // returns 0 on success
- OSD_Fill, // (color)
- // Sets all pixel to color <col>
- // returns 0 on success
- OSD_SetColor, // (color,R{x0},G{y0},B{x1},opacity{y1})
- // set palette entry <num> to <r,g,b>, <mix> and <trans> apply
- // R,G,B: 0..255
- // R=Red, G=Green, B=Blue
- // opacity=0: pixel opacity 0% (only video pixel shows)
- // opacity=1..254: pixel opacity as specified in header
- // opacity=255: pixel opacity 100% (only OSD pixel shows)
- // returns 0 on success, -1 on error
- OSD_SetPalette, // (firstcolor{color},lastcolor{x0},data)
- // Set a number of entries in the palette
- // sets the entries "firstcolor" through "lastcolor" from the array "data"
- // data has 4 byte for each color:
- // R,G,B, and a opacity value: 0->transparent, 1..254->mix, 255->pixel
- OSD_SetTrans, // (transparency{color})
- // Sets transparency of mixed pixel (0..15)
- // returns 0 on success
- OSD_SetPixel, // (x0,y0,color)
- // sets pixel <x>,<y> to color number <col>
- // returns 0 on success, -1 on error
- OSD_GetPixel, // (x0,y0)
- // returns color number of pixel <x>,<y>, or -1
- OSD_SetRow, // (x0,y0,x1,data)
- // fills pixels x0,y through x1,y with the content of data[]
- // returns 0 on success, -1 on clipping all pixel (no pixel drawn)
- OSD_SetBlock, // (x0,y0,x1,y1,increment{color},data)
- // fills pixels x0,y0 through x1,y1 with the content of data[]
- // inc contains the width of one line in the data block,
- // inc<=0 uses blockwidth as linewidth
- // returns 0 on success, -1 on clipping all pixel
- OSD_FillRow, // (x0,y0,x1,color)
- // fills pixels x0,y through x1,y with the color <col>
- // returns 0 on success, -1 on clipping all pixel
- OSD_FillBlock, // (x0,y0,x1,y1,color)
- // fills pixels x0,y0 through x1,y1 with the color <col>
- // returns 0 on success, -1 on clipping all pixel
- OSD_Line, // (x0,y0,x1,y1,color)
- // draw a line from x0,y0 to x1,y1 with the color <col>
- // returns 0 on success
- OSD_Query, // (x0,y0,x1,y1,xasp{color}}), yasp=11
- // fills parameters with the picture dimensions and the pixel aspect ratio
- // returns 0 on success
- OSD_Test, // ()
- // draws a test picture. for debugging purposes only
- // returns 0 on success
-// TODO: remove "test" in final version
- OSD_Text, // (x0,y0,size,color,text)
- OSD_SetWindow, // (x0) set window with number 0<x0<8 as current
- OSD_MoveWindow, // move current window to (x0, y0)
- OSD_OpenRaw, // Open other types of OSD windows
+ /* All functions return -2 on "not open" */
+ OSD_Close=1, /* () */
+ /*
+ * Disables OSD and releases the buffers
+ * returns 0 on success
+ */
+ OSD_Open, /* (x0,y0,x1,y1,BitPerPixel[2/4/8](color&0x0F),mix[0..15](color&0xF0)) */
+ /*
+ * Opens OSD with this size and bit depth
+ * returns 0 on success, -1 on DRAM allocation error, -2 on "already open"
+ */
+ OSD_Show, /* () */
+ /*
+ * enables OSD mode
+ * returns 0 on success
+ */
+ OSD_Hide, /* () */
+ /*
+ * disables OSD mode
+ * returns 0 on success
+ */
+ OSD_Clear, /* () */
+ /*
+ * Sets all pixel to color 0
+ * returns 0 on success
+ */
+ OSD_Fill, /* (color) */
+ /*
+ * Sets all pixel to color <col>
+ * returns 0 on success
+ */
+ OSD_SetColor, /* (color,R{x0},G{y0},B{x1},opacity{y1}) */
+ /*
+ * set palette entry <num> to <r,g,b>, <mix> and <trans> apply
+ * R,G,B: 0..255
+ * R=Red, G=Green, B=Blue
+ * opacity=0: pixel opacity 0% (only video pixel shows)
+ * opacity=1..254: pixel opacity as specified in header
+ * opacity=255: pixel opacity 100% (only OSD pixel shows)
+ * returns 0 on success, -1 on error
+ */
+ OSD_SetPalette, /* (firstcolor{color},lastcolor{x0},data) */
+ /*
+ * Set a number of entries in the palette
+ * sets the entries "firstcolor" through "lastcolor" from the array "data"
+ * data has 4 byte for each color:
+ * R,G,B, and a opacity value: 0->transparent, 1..254->mix, 255->pixel
+ */
+ OSD_SetTrans, /* (transparency{color}) */
+ /*
+ * Sets transparency of mixed pixel (0..15)
+ * returns 0 on success
+ */
+ OSD_SetPixel, /* (x0,y0,color) */
+ /*
+ * sets pixel <x>,<y> to color number <col>
+ * returns 0 on success, -1 on error
+ */
+ OSD_GetPixel, /* (x0,y0) */
+ /* returns color number of pixel <x>,<y>, or -1 */
+ OSD_SetRow, /* (x0,y0,x1,data) */
+ /*
+ * fills pixels x0,y through x1,y with the content of data[]
+ * returns 0 on success, -1 on clipping all pixel (no pixel drawn)
+ */
+ OSD_SetBlock, /* (x0,y0,x1,y1,increment{color},data) */
+ /*
+ * fills pixels x0,y0 through x1,y1 with the content of data[]
+ * inc contains the width of one line in the data block,
+ * inc<=0 uses blockwidth as linewidth
+ * returns 0 on success, -1 on clipping all pixel
+ */
+ OSD_FillRow, /* (x0,y0,x1,color) */
+ /*
+ * fills pixels x0,y through x1,y with the color <col>
+ * returns 0 on success, -1 on clipping all pixel
+ */
+ OSD_FillBlock, /* (x0,y0,x1,y1,color) */
+ /*
+ * fills pixels x0,y0 through x1,y1 with the color <col>
+ * returns 0 on success, -1 on clipping all pixel
+ */
+ OSD_Line, /* (x0,y0,x1,y1,color) */
+ /*
+ * draw a line from x0,y0 to x1,y1 with the color <col>
+ * returns 0 on success
+ */
+ OSD_Query, /* (x0,y0,x1,y1,xasp{color}}), yasp=11 */
+ /*
+ * fills parameters with the picture dimensions and the pixel aspect ratio
+ * returns 0 on success
+ */
+ OSD_Test, /* () */
+ /*
+ * draws a test picture. for debugging purposes only
+ * returns 0 on success
+ * TODO: remove "test" in final version
+ */
+ OSD_Text, /* (x0,y0,size,color,text) */
+ OSD_SetWindow, /* (x0) set window with number 0<x0<8 as current */
+ OSD_MoveWindow, /* move current window to (x0, y0) */
+ OSD_OpenRaw, /* Open other types of OSD windows */
} OSD_Command;
typedef struct osd_cmd_s {
--
2.17.1
On Tue, 2019-06-04 at 20:13 +0900, Masahiro Yamada wrote:
> On the other hand, uapi headers are written in more strict C, where
> the C++ comment style is forbidden.
Is this a real problem for any toolchain?
On Tue, Jun 4, 2019 at 8:24 PM Joe Perches <[email protected]> wrote:
>
> On Tue, 2019-06-04 at 20:13 +0900, Masahiro Yamada wrote:
> > On the other hand, uapi headers are written in more strict C, where
> > the C++ comment style is forbidden.
>
> Is this a real problem for any toolchain?
I was waiting for this comment!
Which standard should UAPI headers follow?
Is it defined somewhere?
If there is no rule, is it up to subsystem maintainers?
We have a certain of unknowledge in user-space,
I do not know it it is a real problem.
Actually, this patch is related to this thread:
https://lkml.org/lkml/2019/5/22/1441
Thomas and you agreed
// should be avoided for SPDX tags in UAPI headers.
So, I just thought C99 was forbidden for user-space.
If C89/C90 is already fantasy,
let's clearly say "Kernel requires C99 for user-space",
and use // everywhere for SPDX tags?
--
Best Regards
Masahiro Yamada
On Tue, Jun 4, 2019 at 1:23 PM Joe Perches <[email protected]> wrote:
>
> On Tue, 2019-06-04 at 20:13 +0900, Masahiro Yamada wrote:
> > On the other hand, uapi headers are written in more strict C, where
> > the C++ comment style is forbidden.
>
> Is this a real problem for any toolchain?
There is likely some code that is built with -Wpedandic -Werror --std=c89
or similar. Since glibc allows this combination for its own headers, it seems
best to also allow it in kernel headers that may be included by libc headers
or by applications, at least where it does not hurt.
Realistically though, we probably assume c99 or gnu89 in user space
headers anyway, since there is no 'long long' in earlier standards.
Arnd
On Tue, 2019-06-04 at 20:48 +0900, Masahiro Yamada wrote:
> On Tue, Jun 4, 2019 at 8:24 PM Joe Perches <[email protected]> wrote:
> > On Tue, 2019-06-04 at 20:13 +0900, Masahiro Yamada wrote:
> > > On the other hand, uapi headers are written in more strict C, where
> > > the C++ comment style is forbidden.
> >
> > Is this a real problem for any toolchain?
>
> I was waiting for this comment!
>
> Which standard should UAPI headers follow?
> Is it defined somewhere?
>
> If there is no rule, is it up to subsystem maintainers?
>
> We have a certain of unknowledge in user-space,
> I do not know it it is a real problem.
>
> Actually, this patch is related to this thread:
> https://lkml.org/lkml/2019/5/22/1441
>
> Thomas and you agreed
> // should be avoided for SPDX tags in UAPI headers.
If it's really a generic issue, I think there are more
uses of // comments in uapi files.
$ git grep '//' include/uapi/ | grep -vP '(http://|https://|ftp:/)' | wc -l
101
> So, I just thought C99 was forbidden for user-space.
No idea, I just believe if it's really a problem
it likely would have been reported already.
> If C89/C90 is already fantasy,
> let's clearly say "Kernel requires C99 for user-space",
> and use // everywhere for SPDX tags?
OK by me.
I have a checkpatch patch waiting to submit to remove the
requirement to use the /* */ comment style in .h files.
The docs need to be updated too.
cheers, Joe
On Tue, Jun 4, 2019 at 8:55 PM Arnd Bergmann <[email protected]> wrote:
>
> On Tue, Jun 4, 2019 at 1:23 PM Joe Perches <[email protected]> wrote:
> >
> > On Tue, 2019-06-04 at 20:13 +0900, Masahiro Yamada wrote:
> > > On the other hand, uapi headers are written in more strict C, where
> > > the C++ comment style is forbidden.
> >
> > Is this a real problem for any toolchain?
>
> There is likely some code that is built with -Wpedandic -Werror --std=c89
> or similar. Since glibc allows this combination for its own headers, it seems
> best to also allow it in kernel headers that may be included by libc headers
> or by applications, at least where it does not hurt.
>
> Realistically though, we probably assume c99 or gnu89 in user space
> headers anyway, since there is no 'long long' in earlier standards.
>
> Arnd
In fact, I detected this issue by the following patch:
https://patchwork.kernel.org/patch/10974669/
When I worked on it, I wondered which
c-dialect flags should be used.
This code:
> # Unlike the kernel space, uapi headers are written in more strict C.
> # - Forbid C++ style comments
> # - Use '__inline', '__asm__' instead of 'inline', 'asm'
> #
> # -std=c90 (equivalent to -ansi) catches the violation of those.
> # We cannot go as far as adding -Wpedantic since it emits too many warnings.
> #
> # REVISIT: re-consider the proper set of compiler flags for uapi compile-test.
>
> UAPI_CFLAGS := -std=c90 -Wpedantic -Wall -Werror=implicit-function-declaration
Even "-std=c99 -Wpedantic" emits lots of warnings.
I noticed one more thing.
There are two ways to define fixed-width type.
[1] #include <linux/types.h>, __u8, __u16, __u32, __u64
vs
[2] #include <stdint.h>, uint8_t, uint16_t, uint32_t, uint64_t
Both are used in UAPI headers.
IIRC, <stdint.h> was standardized by C99.
So, we have already relied on C99 in user-space too.
--
Best Regards
Masahiro Yamada
On Tue, Jun 4, 2019 at 9:48 PM Masahiro Yamada
<[email protected]> wrote:
>
> On Tue, Jun 4, 2019 at 8:55 PM Arnd Bergmann <[email protected]> wrote:
> >
> > On Tue, Jun 4, 2019 at 1:23 PM Joe Perches <[email protected]> wrote:
> > >
> > > On Tue, 2019-06-04 at 20:13 +0900, Masahiro Yamada wrote:
> > > > On the other hand, uapi headers are written in more strict C, where
> > > > the C++ comment style is forbidden.
> > >
> > > Is this a real problem for any toolchain?
> >
> > There is likely some code that is built with -Wpedandic -Werror --std=c89
> > or similar. Since glibc allows this combination for its own headers, it seems
> > best to also allow it in kernel headers that may be included by libc headers
> > or by applications, at least where it does not hurt.
> >
> > Realistically though, we probably assume c99 or gnu89 in user space
> > headers anyway, since there is no 'long long' in earlier standards.
> >
> > Arnd
>
> In fact, I detected this issue by the following patch:
> https://patchwork.kernel.org/patch/10974669/
>
> When I worked on it, I wondered which
> c-dialect flags should be used.
>
> This code:
>
> > # Unlike the kernel space, uapi headers are written in more strict C.
> > # - Forbid C++ style comments
> > # - Use '__inline', '__asm__' instead of 'inline', 'asm'
> > #
> > # -std=c90 (equivalent to -ansi) catches the violation of those.
> > # We cannot go as far as adding -Wpedantic since it emits too many warnings.
> > #
> > # REVISIT: re-consider the proper set of compiler flags for uapi compile-test.
> >
> > UAPI_CFLAGS := -std=c90 -Wpedantic -Wall -Werror=implicit-function-declaration
I got rid of -Wpedantic in the submitted patch.
Sorry if I confused you.
--
Best Regards
Masahiro Yamada
On Tue, Jun 04, 2019 at 09:48:12PM +0900, Masahiro Yamada wrote:
> On Tue, Jun 4, 2019 at 8:55 PM Arnd Bergmann <[email protected]> wrote:
> >
> > On Tue, Jun 4, 2019 at 1:23 PM Joe Perches <[email protected]> wrote:
> > >
> > > On Tue, 2019-06-04 at 20:13 +0900, Masahiro Yamada wrote:
> > > > On the other hand, uapi headers are written in more strict C, where
> > > > the C++ comment style is forbidden.
> > >
> > > Is this a real problem for any toolchain?
> >
> > There is likely some code that is built with -Wpedandic -Werror --std=c89
> > or similar. Since glibc allows this combination for its own headers, it seems
> > best to also allow it in kernel headers that may be included by libc headers
> > or by applications, at least where it does not hurt.
> >
> > Realistically though, we probably assume c99 or gnu89 in user space
> > headers anyway, since there is no 'long long' in earlier standards.
> >
> > Arnd
>
> In fact, I detected this issue by the following patch:
> https://patchwork.kernel.org/patch/10974669/
>
> When I worked on it, I wondered which
> c-dialect flags should be used.
>
> This code:
>
> > # Unlike the kernel space, uapi headers are written in more strict C.
> > # - Forbid C++ style comments
> > # - Use '__inline', '__asm__' instead of 'inline', 'asm'
> > #
> > # -std=c90 (equivalent to -ansi) catches the violation of those.
> > # We cannot go as far as adding -Wpedantic since it emits too many warnings.
> > #
> > # REVISIT: re-consider the proper set of compiler flags for uapi compile-test.
> >
> > UAPI_CFLAGS := -std=c90 -Wpedantic -Wall -Werror=implicit-function-declaration
>
> Even "-std=c99 -Wpedantic" emits lots of warnings.
>
>
>
> I noticed one more thing.
>
> There are two ways to define fixed-width type.
>
> [1] #include <linux/types.h>, __u8, __u16, __u32, __u64
>
> vs
>
> [2] #include <stdint.h>, uint8_t, uint16_t, uint32_t, uint64_t
>
>
> Both are used in UAPI headers.
> IIRC, <stdint.h> was standardized by C99.
>
> So, we have already relied on C99 in user-space too.
Just because we have relied on it in the past, does not mean we need to
keep relying on it. I have had numerous complaints over the years from
libc authors that our uapi headers are _NOT_ able to be directly
consumed by them. They all end up having to fix things up and include
local "sanitized" copies.
So any work we can do here to make them more sane and work properly
everywhere is a good thing, as right now, they are broken.
thanks,
greg k-h
Em Tue, 4 Jun 2019 20:13:34 +0900
Masahiro Yamada <[email protected]> escreveu:
> Linux kernel tolerates C++ style comments these days. Actually, the
> SPDX License tags for .c files start with //.
>
> On the other hand, uapi headers are written in more strict C, where
> the C++ comment style is forbidden.
I've seen that the current comments are more a discussion about
c++ style against "normal c" comments.
I don't have any strong opinion about using or not c++ style comments
on headers.
From my side, though, I would prefer to use // for SPDX everywhere,
as using it on C, but avoiding on headers doesn't seem a good idea
to me, as people can easily mess with it, but that's just my two
cents.
-
Yet, with respect to those headers:
include/uapi/linux/dvb/video.h
include/uapi/linux/dvb/osd.h
include/uapi/linux/dvb/audio.h
They belong to an old deprecated API, with just two drivers using it, with
was meant to control MPEG decoder hardware:
1) the av7110 DVB legacy driver - it is for a hardware developed on the
previous millennium and with is not manufactured anymore for a long time.
We didn't remove it yet just because we don't have any strong reason why
doing it;
2) by the V4L2 ivtv driver (with is also for a legacy driver) - with
doesn't support Digital TV. This was a first approach to control its
internal MPEG decoder and/or encoder. This was replaced several years
ago by another API designed to work with MPEG encoders/decoders,
using a more generic approach. The deprecated API is there just for
the sake of not breaking anything on userspace.
So, if you're working on it just to sanitize the headers, that's
fine, but please don't use them on any future project.
Regards,
Mauro
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
>
> include/uapi/linux/dvb/audio.h | 2 +-
> include/uapi/linux/dvb/osd.h | 170 ++++++++++++++++++++-------------
> 2 files changed, 103 insertions(+), 69 deletions(-)
>
> diff --git a/include/uapi/linux/dvb/audio.h b/include/uapi/linux/dvb/audio.h
> index afeae063e640..977bed135e22 100644
> --- a/include/uapi/linux/dvb/audio.h
> +++ b/include/uapi/linux/dvb/audio.h
> @@ -52,7 +52,7 @@ typedef enum {
> typedef struct audio_mixer {
> unsigned int volume_left;
> unsigned int volume_right;
> - // what else do we need? bass, pass-through, ...
> + /* what else do we need? bass, pass-through, ... */
> } audio_mixer_t;
>
>
> diff --git a/include/uapi/linux/dvb/osd.h b/include/uapi/linux/dvb/osd.h
> index e163508b9ae8..723824ce3622 100644
> --- a/include/uapi/linux/dvb/osd.h
> +++ b/include/uapi/linux/dvb/osd.h
> @@ -28,74 +28,108 @@
> #include <linux/compiler.h>
>
> typedef enum {
> - // All functions return -2 on "not open"
> - OSD_Close=1, // ()
> - // Disables OSD and releases the buffers
> - // returns 0 on success
> - OSD_Open, // (x0,y0,x1,y1,BitPerPixel[2/4/8](color&0x0F),mix[0..15](color&0xF0))
> - // Opens OSD with this size and bit depth
> - // returns 0 on success, -1 on DRAM allocation error, -2 on "already open"
> - OSD_Show, // ()
> - // enables OSD mode
> - // returns 0 on success
> - OSD_Hide, // ()
> - // disables OSD mode
> - // returns 0 on success
> - OSD_Clear, // ()
> - // Sets all pixel to color 0
> - // returns 0 on success
> - OSD_Fill, // (color)
> - // Sets all pixel to color <col>
> - // returns 0 on success
> - OSD_SetColor, // (color,R{x0},G{y0},B{x1},opacity{y1})
> - // set palette entry <num> to <r,g,b>, <mix> and <trans> apply
> - // R,G,B: 0..255
> - // R=Red, G=Green, B=Blue
> - // opacity=0: pixel opacity 0% (only video pixel shows)
> - // opacity=1..254: pixel opacity as specified in header
> - // opacity=255: pixel opacity 100% (only OSD pixel shows)
> - // returns 0 on success, -1 on error
> - OSD_SetPalette, // (firstcolor{color},lastcolor{x0},data)
> - // Set a number of entries in the palette
> - // sets the entries "firstcolor" through "lastcolor" from the array "data"
> - // data has 4 byte for each color:
> - // R,G,B, and a opacity value: 0->transparent, 1..254->mix, 255->pixel
> - OSD_SetTrans, // (transparency{color})
> - // Sets transparency of mixed pixel (0..15)
> - // returns 0 on success
> - OSD_SetPixel, // (x0,y0,color)
> - // sets pixel <x>,<y> to color number <col>
> - // returns 0 on success, -1 on error
> - OSD_GetPixel, // (x0,y0)
> - // returns color number of pixel <x>,<y>, or -1
> - OSD_SetRow, // (x0,y0,x1,data)
> - // fills pixels x0,y through x1,y with the content of data[]
> - // returns 0 on success, -1 on clipping all pixel (no pixel drawn)
> - OSD_SetBlock, // (x0,y0,x1,y1,increment{color},data)
> - // fills pixels x0,y0 through x1,y1 with the content of data[]
> - // inc contains the width of one line in the data block,
> - // inc<=0 uses blockwidth as linewidth
> - // returns 0 on success, -1 on clipping all pixel
> - OSD_FillRow, // (x0,y0,x1,color)
> - // fills pixels x0,y through x1,y with the color <col>
> - // returns 0 on success, -1 on clipping all pixel
> - OSD_FillBlock, // (x0,y0,x1,y1,color)
> - // fills pixels x0,y0 through x1,y1 with the color <col>
> - // returns 0 on success, -1 on clipping all pixel
> - OSD_Line, // (x0,y0,x1,y1,color)
> - // draw a line from x0,y0 to x1,y1 with the color <col>
> - // returns 0 on success
> - OSD_Query, // (x0,y0,x1,y1,xasp{color}}), yasp=11
> - // fills parameters with the picture dimensions and the pixel aspect ratio
> - // returns 0 on success
> - OSD_Test, // ()
> - // draws a test picture. for debugging purposes only
> - // returns 0 on success
> -// TODO: remove "test" in final version
> - OSD_Text, // (x0,y0,size,color,text)
> - OSD_SetWindow, // (x0) set window with number 0<x0<8 as current
> - OSD_MoveWindow, // move current window to (x0, y0)
> - OSD_OpenRaw, // Open other types of OSD windows
> + /* All functions return -2 on "not open" */
> + OSD_Close=1, /* () */
> + /*
> + * Disables OSD and releases the buffers
> + * returns 0 on success
> + */
> + OSD_Open, /* (x0,y0,x1,y1,BitPerPixel[2/4/8](color&0x0F),mix[0..15](color&0xF0)) */
> + /*
> + * Opens OSD with this size and bit depth
> + * returns 0 on success, -1 on DRAM allocation error, -2 on "already open"
> + */
> + OSD_Show, /* () */
> + /*
> + * enables OSD mode
> + * returns 0 on success
> + */
> + OSD_Hide, /* () */
> + /*
> + * disables OSD mode
> + * returns 0 on success
> + */
> + OSD_Clear, /* () */
> + /*
> + * Sets all pixel to color 0
> + * returns 0 on success
> + */
> + OSD_Fill, /* (color) */
> + /*
> + * Sets all pixel to color <col>
> + * returns 0 on success
> + */
> + OSD_SetColor, /* (color,R{x0},G{y0},B{x1},opacity{y1}) */
> + /*
> + * set palette entry <num> to <r,g,b>, <mix> and <trans> apply
> + * R,G,B: 0..255
> + * R=Red, G=Green, B=Blue
> + * opacity=0: pixel opacity 0% (only video pixel shows)
> + * opacity=1..254: pixel opacity as specified in header
> + * opacity=255: pixel opacity 100% (only OSD pixel shows)
> + * returns 0 on success, -1 on error
> + */
> + OSD_SetPalette, /* (firstcolor{color},lastcolor{x0},data) */
> + /*
> + * Set a number of entries in the palette
> + * sets the entries "firstcolor" through "lastcolor" from the array "data"
> + * data has 4 byte for each color:
> + * R,G,B, and a opacity value: 0->transparent, 1..254->mix, 255->pixel
> + */
> + OSD_SetTrans, /* (transparency{color}) */
> + /*
> + * Sets transparency of mixed pixel (0..15)
> + * returns 0 on success
> + */
> + OSD_SetPixel, /* (x0,y0,color) */
> + /*
> + * sets pixel <x>,<y> to color number <col>
> + * returns 0 on success, -1 on error
> + */
> + OSD_GetPixel, /* (x0,y0) */
> + /* returns color number of pixel <x>,<y>, or -1 */
> + OSD_SetRow, /* (x0,y0,x1,data) */
> + /*
> + * fills pixels x0,y through x1,y with the content of data[]
> + * returns 0 on success, -1 on clipping all pixel (no pixel drawn)
> + */
> + OSD_SetBlock, /* (x0,y0,x1,y1,increment{color},data) */
> + /*
> + * fills pixels x0,y0 through x1,y1 with the content of data[]
> + * inc contains the width of one line in the data block,
> + * inc<=0 uses blockwidth as linewidth
> + * returns 0 on success, -1 on clipping all pixel
> + */
> + OSD_FillRow, /* (x0,y0,x1,color) */
> + /*
> + * fills pixels x0,y through x1,y with the color <col>
> + * returns 0 on success, -1 on clipping all pixel
> + */
> + OSD_FillBlock, /* (x0,y0,x1,y1,color) */
> + /*
> + * fills pixels x0,y0 through x1,y1 with the color <col>
> + * returns 0 on success, -1 on clipping all pixel
> + */
> + OSD_Line, /* (x0,y0,x1,y1,color) */
> + /*
> + * draw a line from x0,y0 to x1,y1 with the color <col>
> + * returns 0 on success
> + */
> + OSD_Query, /* (x0,y0,x1,y1,xasp{color}}), yasp=11 */
> + /*
> + * fills parameters with the picture dimensions and the pixel aspect ratio
> + * returns 0 on success
> + */
> + OSD_Test, /* () */
> + /*
> + * draws a test picture. for debugging purposes only
> + * returns 0 on success
> + * TODO: remove "test" in final version
> + */
> + OSD_Text, /* (x0,y0,size,color,text) */
> + OSD_SetWindow, /* (x0) set window with number 0<x0<8 as current */
> + OSD_MoveWindow, /* move current window to (x0, y0) */
> + OSD_OpenRaw, /* Open other types of OSD windows */
> } OSD_Command;
>
> typedef struct osd_cmd_s {
Thanks,
Mauro
On Tue, Jun 4, 2019 at 10:44 PM Greg KH <[email protected]> wrote:
>
> On Tue, Jun 04, 2019 at 09:48:12PM +0900, Masahiro Yamada wrote:
> > On Tue, Jun 4, 2019 at 8:55 PM Arnd Bergmann <[email protected]> wrote:
> > >
> > > On Tue, Jun 4, 2019 at 1:23 PM Joe Perches <[email protected]> wrote:
> > > >
> > > > On Tue, 2019-06-04 at 20:13 +0900, Masahiro Yamada wrote:
> > > > > On the other hand, uapi headers are written in more strict C, where
> > > > > the C++ comment style is forbidden.
> > > >
> > > > Is this a real problem for any toolchain?
> > >
> > > There is likely some code that is built with -Wpedandic -Werror --std=c89
> > > or similar. Since glibc allows this combination for its own headers, it seems
> > > best to also allow it in kernel headers that may be included by libc headers
> > > or by applications, at least where it does not hurt.
> > >
> > > Realistically though, we probably assume c99 or gnu89 in user space
> > > headers anyway, since there is no 'long long' in earlier standards.
> > >
> > > Arnd
> >
> > In fact, I detected this issue by the following patch:
> > https://patchwork.kernel.org/patch/10974669/
> >
> > When I worked on it, I wondered which
> > c-dialect flags should be used.
> >
> > This code:
> >
> > > # Unlike the kernel space, uapi headers are written in more strict C.
> > > # - Forbid C++ style comments
> > > # - Use '__inline', '__asm__' instead of 'inline', 'asm'
> > > #
> > > # -std=c90 (equivalent to -ansi) catches the violation of those.
> > > # We cannot go as far as adding -Wpedantic since it emits too many warnings.
> > > #
> > > # REVISIT: re-consider the proper set of compiler flags for uapi compile-test.
> > >
> > > UAPI_CFLAGS := -std=c90 -Wpedantic -Wall -Werror=implicit-function-declaration
> >
> > Even "-std=c99 -Wpedantic" emits lots of warnings.
> >
> >
> >
> > I noticed one more thing.
> >
> > There are two ways to define fixed-width type.
> >
> > [1] #include <linux/types.h>, __u8, __u16, __u32, __u64
> >
> > vs
> >
> > [2] #include <stdint.h>, uint8_t, uint16_t, uint32_t, uint64_t
> >
> >
> > Both are used in UAPI headers.
> > IIRC, <stdint.h> was standardized by C99.
> >
> > So, we have already relied on C99 in user-space too.
>
> Just because we have relied on it in the past, does not mean we need to
> keep relying on it. I have had numerous complaints over the years from
> libc authors that our uapi headers are _NOT_ able to be directly
> consumed by them. They all end up having to fix things up and include
> local "sanitized" copies.
>
> So any work we can do here to make them more sane and work properly
> everywhere is a good thing, as right now, they are broken.
Maybe, we should document UAPI header coding guideline.
Without To-Don't list,
people will do anything.
--
Best Regards
Masahiro Yamada
On Tue, Jun 4, 2019 at 5:28 PM Masahiro Yamada
<[email protected]> wrote:
> On Tue, Jun 4, 2019 at 10:44 PM Greg KH <[email protected]> wrote:
> > On Tue, Jun 04, 2019 at 09:48:12PM +0900, Masahiro Yamada wrote:
> > > On Tue, Jun 4, 2019 at 8:55 PM Arnd Bergmann <[email protected]> wrote:
> > > > # Unlike the kernel space, uapi headers are written in more strict C.
> > > > # -std=c90 (equivalent to -ansi) catches the violation of those.
> > > > # We cannot go as far as adding -Wpedantic since it emits too many warnings.
At least with clang, we might be able to be more specific about which
warnings to add or not to add.
> > >
> > > There are two ways to define fixed-width type.
> > >
> > > [1] #include <linux/types.h>, __u8, __u16, __u32, __u64
> > >
> > > vs
> > >
> > > [2] #include <stdint.h>, uint8_t, uint16_t, uint32_t, uint64_t
> > >
> > >
> > > Both are used in UAPI headers.
> > > IIRC, <stdint.h> was standardized by C99.
> > >
> > > So, we have already relied on C99 in user-space too.
A related problem is that using the stdint.h types requires
including stdint.h first, but the C library requires that including
one standard header does not include another one recursively.
So if sys/socket.h includes linux/socket.h, that must not include
stdint.h or any other header file that does so.
> > Just because we have relied on it in the past, does not mean we need to
> > keep relying on it. I have had numerous complaints over the years from
> > libc authors that our uapi headers are _NOT_ able to be directly
> > consumed by them. They all end up having to fix things up and include
> > local "sanitized" copies.
Yes, and this is getting worse with 64-bit time_t as we now get conflicting
definitions of timespec, timeval and derived types. We probably need to
change a lot of the common headers that conflict with libc definitions
and come up with a better way of exposing the interfaces there.
Similarly, a header that may get included by libc should not define any
data structures with members that may conflict with a user space macro
name. E.g.
struct foo {
__u32 bar;
};
uses the correct type, but if an application contains
#define bar __read_bar()
#include <linux/foo.h>
then it will get a compile failure. Not sure what we can do about this,
but we might need a form of classification of headers into those
that can be included by libc and must follow very strict rules,
as opposed to those headers that are specific to a driver or subsystem
that will not be included unless some application specifically needs the
symbols in that header to talk to the kernel.
> > So any work we can do here to make them more sane and work properly
> > everywhere is a good thing, as right now, they are broken.
>
>
> Maybe, we should document UAPI header coding guideline.
>
> Without To-Don't list,
> people will do anything.
This also came up recently with the discussion on how to define
data structures in a portable way that avoids not only the identifier
conflicts but also differences in size or alignment of member types.
Arnd
On Wed, Jun 5, 2019 at 3:21 AM Arnd Bergmann <[email protected]> wrote:
> > > >
> > > > There are two ways to define fixed-width type.
> > > >
> > > > [1] #include <linux/types.h>, __u8, __u16, __u32, __u64
> > > >
> > > > vs
> > > >
> > > > [2] #include <stdint.h>, uint8_t, uint16_t, uint32_t, uint64_t
> > > >
> > > >
> > > > Both are used in UAPI headers.
> > > > IIRC, <stdint.h> was standardized by C99.
> > > >
> > > > So, we have already relied on C99 in user-space too.
>
> A related problem is that using the stdint.h types requires
> including stdint.h first, but the C library requires that including
> one standard header does not include another one recursively.
>
> So if sys/socket.h includes linux/socket.h, that must not include
> stdint.h or any other header file that does so.
This means we cannot reliably use uint{8,16,32,64}_t in UAPI headers.
[1] If we include <stdint.h> from linux/foo.h
If sys/foo.h includes <linux/foo.h> and <stdint.h>,
it violates the C library requirement.
[2] If we do not include <stdint.h> from linux/foo.h
If sys/foo.h includes <linux/foo.h>, but not <stdint.h>,
we get 'unknown type name' errors.
--
Best Regards
Masahiro Yamada
On Wed, Jun 05, 2019 at 01:10:41PM +0900, Masahiro Yamada wrote:
> On Wed, Jun 5, 2019 at 3:21 AM Arnd Bergmann <[email protected]> wrote:
> > > > >
> > > > > There are two ways to define fixed-width type.
> > > > >
> > > > > [1] #include <linux/types.h>, __u8, __u16, __u32, __u64
> > > > >
> > > > > vs
> > > > >
> > > > > [2] #include <stdint.h>, uint8_t, uint16_t, uint32_t, uint64_t
> > > > >
> > > > >
> > > > > Both are used in UAPI headers.
> > > > > IIRC, <stdint.h> was standardized by C99.
> > > > >
> > > > > So, we have already relied on C99 in user-space too.
> >
> > A related problem is that using the stdint.h types requires
> > including stdint.h first, but the C library requires that including
> > one standard header does not include another one recursively.
> >
> > So if sys/socket.h includes linux/socket.h, that must not include
> > stdint.h or any other header file that does so.
>
>
> This means we cannot reliably use uint{8,16,32,64}_t in UAPI headers.
We should not be doing that as they are in the userspace "namespace" of
variables, not in the kernel namespace. We've been over this many times
in the past :(
> [1] If we include <stdint.h> from linux/foo.h
>
> If sys/foo.h includes <linux/foo.h> and <stdint.h>,
> it violates the C library requirement.
>
>
> [2] If we do not include <stdint.h> from linux/foo.h
>
> If sys/foo.h includes <linux/foo.h>, but not <stdint.h>,
> we get 'unknown type name' errors.
We need to just use the proper __u{8,16,32,64} variable types instead,
that is exactly what they are there for.
thanks,
greg k-h
On Wed, 2019-06-05 at 07:10 +0200, Greg KH wrote:
> On Wed, Jun 05, 2019 at 01:10:41PM +0900, Masahiro Yamada wrote:
> > On Wed, Jun 5, 2019 at 3:21 AM Arnd Bergmann <[email protected]> wrote:
[]
> > This means we cannot reliably use uint{8,16,32,64}_t in UAPI headers.
>
> We should not be doing that as they are in the userspace "namespace" of
> variables, not in the kernel namespace. We've been over this many times
> in the past :(
Just not very successfully...
$ git grep -w -P 'u?_?int(?:8|16|32|64)_t' include/uapi | wc -l
342
$ git grep -w -P --name-only 'u?_?int(?:8|16|32|64)_t' include/uapi | wc -l
13
Documentation helps a bit, checkpatch helps as well.
Maintainer knowledge and vigilance probably helps the most.
On Tue, Jun 04, 2019 at 10:22:05PM -0700, Joe Perches wrote:
> On Wed, 2019-06-05 at 07:10 +0200, Greg KH wrote:
> > On Wed, Jun 05, 2019 at 01:10:41PM +0900, Masahiro Yamada wrote:
> > > On Wed, Jun 5, 2019 at 3:21 AM Arnd Bergmann <[email protected]> wrote:
> []
> > > This means we cannot reliably use uint{8,16,32,64}_t in UAPI headers.
> >
> > We should not be doing that as they are in the userspace "namespace" of
> > variables, not in the kernel namespace. We've been over this many times
> > in the past :(
>
> Just not very successfully...
>
> $ git grep -w -P 'u?_?int(?:8|16|32|64)_t' include/uapi | wc -l
> 342
>
> $ git grep -w -P --name-only 'u?_?int(?:8|16|32|64)_t' include/uapi | wc -l
> 13
>
> Documentation helps a bit, checkpatch helps as well.
> Maintainer knowledge and vigilance probably helps the most.
Yes, it's not been a dedicated effort at all :(
But it needs to be resolved, if we want people to actually use our
kernel headers easily.
thanks,
greg k-h
On Wed, Jun 5, 2019 at 3:03 PM Greg KH <[email protected]> wrote:
>
> On Tue, Jun 04, 2019 at 10:22:05PM -0700, Joe Perches wrote:
> > On Wed, 2019-06-05 at 07:10 +0200, Greg KH wrote:
> > > On Wed, Jun 05, 2019 at 01:10:41PM +0900, Masahiro Yamada wrote:
> > > > On Wed, Jun 5, 2019 at 3:21 AM Arnd Bergmann <[email protected]> wrote:
> > []
> > > > This means we cannot reliably use uint{8,16,32,64}_t in UAPI headers.
> > >
> > > We should not be doing that as they are in the userspace "namespace" of
> > > variables, not in the kernel namespace. We've been over this many times
> > > in the past :(
> >
> > Just not very successfully...
> >
> > $ git grep -w -P 'u?_?int(?:8|16|32|64)_t' include/uapi | wc -l
> > 342
> >
> > $ git grep -w -P --name-only 'u?_?int(?:8|16|32|64)_t' include/uapi | wc -l
> > 13
> >
> > Documentation helps a bit, checkpatch helps as well.
> > Maintainer knowledge and vigilance probably helps the most.
>
> Yes, it's not been a dedicated effort at all :(
I am proposing this series.
https://lkml.org/lkml/2019/6/4/1379
When CONFIG_UAPI_HEADER_TEST=y,
UAPI headers are compile-tested.
0-day bot tests allmodconfig, which enables CONFIG_UAPI_HEADER_TEST,
so new buggy headers will be blocked.
It will take some time to eliminate existing bugs.
I just started with low-hanging fruits:
https://lore.kernel.org/patchwork/patch/1083711/
https://lore.kernel.org/patchwork/patch/1084123/
Anyway, having a document will be really nice.
Not all maintainers understand the detail.
Having some evidence in Documentation/
will help the review process move smoothly.
> But it needs to be resolved, if we want people to actually use our
> kernel headers easily.
>
> thanks,
>
> greg k-h
--
Best Regards
Masahiro Yamada
Em Tue, 04 Jun 2019 22:22:05 -0700
Joe Perches <[email protected]> escreveu:
> On Wed, 2019-06-05 at 07:10 +0200, Greg KH wrote:
> > On Wed, Jun 05, 2019 at 01:10:41PM +0900, Masahiro Yamada wrote:
> > > On Wed, Jun 5, 2019 at 3:21 AM Arnd Bergmann <[email protected]> wrote:
> []
> > > This means we cannot reliably use uint{8,16,32,64}_t in UAPI headers.
> >
> > We should not be doing that as they are in the userspace "namespace" of
> > variables, not in the kernel namespace. We've been over this many times
> > in the past :(
>
> Just not very successfully...
>
> $ git grep -w -P 'u?_?int(?:8|16|32|64)_t' include/uapi | wc -l
> 342
> $ git grep -w -P --name-only 'u?_?int(?:8|16|32|64)_t' include/uapi | wc -l
> 13
Out of curiosity, I decided to check those occurrences...
About half of those 13 files are false-positives:
- bpf.h, pps.h and amdgpu_drm.h use those int types only inside comments;
- drm.h and coda.h have their own typedefs for those int types;
- vmwgfx_drm.h includes drm.h (which has the typedefs).
So, only 6 headers actually use posix types in a way that it seems that
it would require including stdint.h:
- include/uapi/linux/fuse.h
This one explicitly includes stdint.h if !__KERNEL__
- include/uapi/linux/netfilter_bridge/ebtables.h,
include/uapi/linux/sctp.h,
include/uapi/scsi/scsi_netlink.h and
include/uapi/scsi/scsi_netlink_fc.h
They include linux/types.h unconditionally, relying on
scripts/headers_install.sh to remove it;
- include/uapi/scsi/scsi_bsg_fc.h
It doesn't include anything. In other words, it assumes that the c file
would include either linux/types.h or stdint.h.
---
Not saying that this is a good idea, but, as we have already a script that
it is meant to sanitize uAPI header files when installing them
(scripts/headers_install.sh), one could modify it (or convert to perl/python)
in a way that it would be doing something like[1]:
sed -E
...
-e 's,//(.*),/* \1 */,'
[1] the actual rule should be more complex than that, in order to avoid
replacing // inside /**/ comments.
Thanks,
Mauro
On Wed, 2019-06-05 at 07:14 -0300, Mauro Carvalho Chehab wrote:
> Em Tue, 04 Jun 2019 22:22:05 -0700
> Joe Perches <[email protected]> escreveu:
>
> > On Wed, 2019-06-05 at 07:10 +0200, Greg KH wrote:
> > > On Wed, Jun 05, 2019 at 01:10:41PM +0900, Masahiro Yamada wrote:
> > > > On Wed, Jun 5, 2019 at 3:21 AM Arnd Bergmann <[email protected]> wrote:
> > []
> > > > This means we cannot reliably use uint{8,16,32,64}_t in UAPI headers.
> > >
> > > We should not be doing that as they are in the userspace "namespace" of
> > > variables, not in the kernel namespace. We've been over this many times
> > > in the past :(
> >
> > Just not very successfully...
> >
> > $ git grep -w -P 'u?_?int(?:8|16|32|64)_t' include/uapi | wc -l
> > 342
> > $ git grep -w -P --name-only 'u?_?int(?:8|16|32|64)_t' include/uapi | wc -l
> > 13
>
> Out of curiosity, I decided to check those occurrences...
>
> About half of those 13 files are false-positives:
>
> - bpf.h, pps.h and amdgpu_drm.h use those int types only inside comments;
> - drm.h and coda.h have their own typedefs for those int types;
> - vmwgfx_drm.h includes drm.h (which has the typedefs).
>
> So, only 6 headers actually use posix types in a way that it seems that
> it would require including stdint.h:
>
> - include/uapi/linux/fuse.h
>
> This one explicitly includes stdint.h if !__KERNEL__
>
> - include/uapi/linux/netfilter_bridge/ebtables.h,
> include/uapi/linux/sctp.h,
> include/uapi/scsi/scsi_netlink.h and
> include/uapi/scsi/scsi_netlink_fc.h
>
> They include linux/types.h unconditionally, relying on
> scripts/headers_install.sh to remove it;
>
> - include/uapi/scsi/scsi_bsg_fc.h
>
> It doesn't include anything. In other words, it assumes that the c file
> would include either linux/types.h or stdint.h.
>
> ---
>
> Not saying that this is a good idea, but, as we have already a script that
> it is meant to sanitize uAPI header files when installing them
> (scripts/headers_install.sh), one could modify it (or convert to perl/python)
> in a way that it would be doing something like[1]:
>
> sed -E
> ...
> -e 's,//(.*),/* \1 */,'
>
> [1] the actual rule should be more complex than that, in order to avoid
> replacing // inside /**/ comments.
Perhaps a checkpatch change too:
The first block updates unsigned only bitfields
The second tests uapi definitions and suggests "__<kernel_types"
---
scripts/checkpatch.pl | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c33c5002f190..afc4bb05a987 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3747,7 +3747,7 @@ sub process {
}
# check for declarations of signed or unsigned without int
- while ($line =~ m{\b($Declare)\s*(?!char\b|short\b|int\b|long\b)\s*($Ident)?\s*[=,;\[\)\(]}g) {
+ while ($line =~ m{\b($Declare)\s*(?!char\b|short\b|int\b|long\b)\s*($Ident)?\s*[=,;:\[\)\(]}g) {
my $type = $1;
my $var = $2;
$var = "" if (!defined $var);
@@ -5905,10 +5905,10 @@ sub process {
"Using weak declarations can have unintended link defects\n" . $herecurr);
}
-# check for c99 types like uint8_t used outside of uapi/ and tools/
- if ($realfile !~ m@\binclude/uapi/@ &&
- $realfile !~ m@\btools/@ &&
- $line =~ /\b($Declare)\s*$Ident\s*[=;,\[]/) {
+# check for c99 types like uint8_t used outside of tools/
+# for uapi, suggest using __<types>, otherwise use <types> like s8, u32, etc...
+ if ($realfile !~ m@\btools/@ &&
+ $line =~ /\b($Declare)\s*$Ident\s*[=,;:\[]/) {
my $type = $1;
if ($type =~ /\b($typeC99Typedefs)\b/) {
$type = $1;
@@ -5916,6 +5916,9 @@ sub process {
$kernel_type = 's' if ($type =~ /^_*[si]/);
$type =~ /(\d+)/;
$kernel_type .= $1;
+ if ($realfile =~ m@\binclude/uapi/@) {
+ $kernel_type = "__" . $kernel_type;
+ }
if (CHK("PREFER_KERNEL_TYPES",
"Prefer kernel type '$kernel_type' over '$type'\n" . $herecurr) &&
$fix) {
Hi Joe,
On Thu, Jun 6, 2019 at 2:06 AM Joe Perches <[email protected]> wrote:
> Perhaps a checkpatch change too:
>
> The first block updates unsigned only bitfields
> The second tests uapi definitions and suggests "__<kernel_types"
Good.
In addition,
"warn if __u8, __u16, __u32, __u64 are used outside of uapi/"
Lots of kernel-space headers use __u{8,16,32,64} instead of u{8,16,32,64}
just because developers often miss to understand when to use
the underscore-prefixed types.
--
Best Regards
Masahiro Yamada
On Sun, 2019-06-09 at 16:14 +0900, Masahiro Yamada wrote:
> Hi Joe,
>
> On Thu, Jun 6, 2019 at 2:06 AM Joe Perches <[email protected]> wrote:
> > Perhaps a checkpatch change too:
> >
> > The first block updates unsigned only bitfields
> > The second tests uapi definitions and suggests "__<kernel_types"
>
> Good.
>
> In addition,
>
> "warn if __u8, __u16, __u32, __u64 are used outside of uapi/"
>
> Lots of kernel-space headers use __u{8,16,32,64} instead of u{8,16,32,64}
> just because developers often miss to understand when to use
> the underscore-prefixed types.
The problem there is that checkpatch can't know if the
__<uapi_type> being used is for an actual uapi use or not.
coccinelle could be much better at that.
On Sun, Jun 9, 2019 at 8:57 PM Joe Perches <[email protected]> wrote:
>
> On Sun, 2019-06-09 at 16:14 +0900, Masahiro Yamada wrote:
> > Hi Joe,
> >
> > On Thu, Jun 6, 2019 at 2:06 AM Joe Perches <[email protected]> wrote:
> > > Perhaps a checkpatch change too:
> > >
> > > The first block updates unsigned only bitfields
> > > The second tests uapi definitions and suggests "__<kernel_types"
> >
> > Good.
> >
> > In addition,
> >
> > "warn if __u8, __u16, __u32, __u64 are used outside of uapi/"
> >
> > Lots of kernel-space headers use __u{8,16,32,64} instead of u{8,16,32,64}
> > just because developers often miss to understand when to use
> > the underscore-prefixed types.
>
> The problem there is that checkpatch can't know if the
> __<uapi_type> being used is for an actual uapi use or not.
>
> coccinelle could be much better at that.
Why?
u{8,16,32,64} are _exactly_ the same as __u{8,16,32,64}.
See include/asm-generic/int-ll64.h
We just use __u{8,16,32,64} for user-space
to avoid identifier name conflict,
but we do not have reason to do so for kernel-space.
--
Best Regards
Masahiro Yamada
On Sun, 2019-06-09 at 22:08 +0900, Masahiro Yamada wrote:
> On Sun, Jun 9, 2019 at 8:57 PM Joe Perches <[email protected]> wrote:
> > On Sun, 2019-06-09 at 16:14 +0900, Masahiro Yamada wrote:
> > > Hi Joe,
> > >
> > > On Thu, Jun 6, 2019 at 2:06 AM Joe Perches <[email protected]> wrote:
> > > > Perhaps a checkpatch change too:
> > > >
> > > > The first block updates unsigned only bitfields
> > > > The second tests uapi definitions and suggests "__<kernel_types"
> > >
> > > Good.
> > >
> > > In addition,
> > >
> > > "warn if __u8, __u16, __u32, __u64 are used outside of uapi/"
> > >
> > > Lots of kernel-space headers use __u{8,16,32,64} instead of u{8,16,32,64}
> > > just because developers often miss to understand when to use
> > > the underscore-prefixed types.
> >
> > The problem there is that checkpatch can't know if the
> > __<uapi_type> being used is for an actual uapi use or not.
> >
> > coccinelle could be much better at that.
>
> Why?
Perhaps it's (somewhat) bad form to have a __uapi type in a
structure, include that structure in a driver for something
like a copy_to/from_user, and map the __<uapi_type> to a non
underscore prefixed <kernel_type>
For instance
struct flat_binder_object in drivers/android/binder.c
How is checkpatch supposed to know that __u32 flags is
inappropriate?
On Sun, Jun 9, 2019 at 10:40 PM Joe Perches <[email protected]> wrote:
>
> On Sun, 2019-06-09 at 22:08 +0900, Masahiro Yamada wrote:
> > On Sun, Jun 9, 2019 at 8:57 PM Joe Perches <[email protected]> wrote:
> > > On Sun, 2019-06-09 at 16:14 +0900, Masahiro Yamada wrote:
> > > > Hi Joe,
> > > >
> > > > On Thu, Jun 6, 2019 at 2:06 AM Joe Perches <[email protected]> wrote:
> > > > > Perhaps a checkpatch change too:
> > > > >
> > > > > The first block updates unsigned only bitfields
> > > > > The second tests uapi definitions and suggests "__<kernel_types"
> > > >
> > > > Good.
> > > >
> > > > In addition,
> > > >
> > > > "warn if __u8, __u16, __u32, __u64 are used outside of uapi/"
> > > >
> > > > Lots of kernel-space headers use __u{8,16,32,64} instead of u{8,16,32,64}
> > > > just because developers often miss to understand when to use
> > > > the underscore-prefixed types.
> > >
> > > The problem there is that checkpatch can't know if the
> > > __<uapi_type> being used is for an actual uapi use or not.
> > >
> > > coccinelle could be much better at that.
> >
> > Why?
>
>
> Perhaps it's (somewhat) bad form to have a __uapi type in a
> structure, include that structure in a driver for something
> like a copy_to/from_user, and map the __<uapi_type> to a non
> underscore prefixed <kernel_type>
Linus Torvalds wrote 'sparse' to check this.
Any attempt to distinguish the address-space
by the presence of double-underscore-prefixes is pointless.
This is already checked by __kernel / __user.
It is absolutely correct to assign __u32 to u32, and vice versa.
If you think the following patch is wrong, please tell me why:
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 748ac489ef7e..24c1b73d9fbd 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1132,7 +1132,7 @@ static struct binder_node *binder_init_node_ilocked(
struct binder_node *node;
binder_uintptr_t ptr = fp ? fp->binder : 0;
binder_uintptr_t cookie = fp ? fp->cookie : 0;
- __u32 flags = fp ? fp->flags : 0;
+ u32 flags = fp ? fp->flags : 0;
assert_spin_locked(&proc->inner_lock);
@@ -4918,7 +4918,7 @@ static int
binder_ioctl_get_node_info_for_ref(struct binder_proc *proc,
{
struct binder_node *node;
struct binder_context *context = proc->context;
- __u32 handle = info->handle;
+ u32 handle = info->handle;
if (info->strong_count || info->weak_count || info->reserved1 ||
info->reserved2 || info->reserved3) {
>
> For instance
>
> struct flat_binder_object in drivers/android/binder.c
>
> How is checkpatch supposed to know that __u32 flags is
> inappropriate?
>
>
--
Best Regards
Masahiro Yamada
Hi!
> On Thu, Jun 6, 2019 at 2:06 AM Joe Perches <[email protected]> wrote:
> > Perhaps a checkpatch change too:
> >
> > The first block updates unsigned only bitfields
> > The second tests uapi definitions and suggests "__<kernel_types"
>
> Good.
>
> In addition,
>
> "warn if __u8, __u16, __u32, __u64 are used outside of uapi/"
>
> Lots of kernel-space headers use __u{8,16,32,64} instead of u{8,16,32,64}
> just because developers often miss to understand when to use
> the underscore-prefixed types.
I have seen it even in the .c files in kernel....
And you want same handling for __s64 (etc) variants.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> > > In fact, I detected this issue by the following patch:
> > > https://patchwork.kernel.org/patch/10974669/
> > >
> > > When I worked on it, I wondered which
> > > c-dialect flags should be used.
> > >
> > > This code:
> > >
> > > > # Unlike the kernel space, uapi headers are written in more strict C.
> > > > # - Forbid C++ style comments
> > > > # - Use '__inline', '__asm__' instead of 'inline', 'asm'
> > > > #
> > > > # -std=c90 (equivalent to -ansi) catches the violation of those.
> > > > # We cannot go as far as adding -Wpedantic since it emits too many warnings.
> > > > #
> > > > # REVISIT: re-consider the proper set of compiler flags for uapi compile-test.
> > > >
> > > > UAPI_CFLAGS := -std=c90 -Wpedantic -Wall -Werror=implicit-function-declaration
> > >
> > > Even "-std=c99 -Wpedantic" emits lots of warnings.
> > >
> > >
> > >
> > > I noticed one more thing.
> > >
> > > There are two ways to define fixed-width type.
> > >
> > > [1] #include <linux/types.h>, __u8, __u16, __u32, __u64
> > >
> > > vs
> > >
> > > [2] #include <stdint.h>, uint8_t, uint16_t, uint32_t, uint64_t
> > >
> > >
> > > Both are used in UAPI headers.
> > > IIRC, <stdint.h> was standardized by C99.
> > >
> > > So, we have already relied on C99 in user-space too.
> >
> > Just because we have relied on it in the past, does not mean we need to
> > keep relying on it. I have had numerous complaints over the years from
> > libc authors that our uapi headers are _NOT_ able to be directly
> > consumed by them. They all end up having to fix things up and include
> > local "sanitized" copies.
> >
> > So any work we can do here to make them more sane and work properly
> > everywhere is a good thing, as right now, they are broken.
>
> Maybe, we should document UAPI header coding guideline.
> Without To-Don't list,
> people will do anything.
Even better would be a (compile-time) test that checks for problems in the headers...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html