staging: vboxvideo: Remove chekpatch issues

This change was suggested by checkpath.pl.

CHECK: Prefer kernel type 'u32' over 'uint32_t'

Signed-off-by Shayenne da Luz Moura <[email protected]>

---
  drivers/staging/vboxvideo/vbox_mode.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/vboxvideo/vbox_mode.c +1 -1

index 79836c8fb909..8a1b117990b8 100644
@@ -311,7 +311,7 @@ static int vbox_crtc_mode_set(struct drm_crtc *crtc,
  static int vbox_crtc_page_flip(struct drm_crtc *crtc,
  			       struct drm_framebuffer *fb,
  			       struct drm_pending_vblank_event *event,
-			       uint32_t page_flip_flags,
+			       u32 page_flip_flags,
  			       struct drm_modeset_acquire_ctx *ctx)
  {
  	struct vbox_private *vbox = crtc->dev->dev_private;

This change was suggested by checkpath.pl. Use unsigned int with bitfield allocate only one bit to the boolean variable.

CHECK: Avoid using bool structure members because of possible alignment issues

Signed-off-by Shayenne da Luz Moura <[email protected]>

---
drivers/staging/vboxvideo/vbox_drv.h | 14 +++++++-------
drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-

2 files changed, 8 insertions(+), 8 deletions(-)

drivers/staging/vboxvideo/vbox_drv.h +0 -0

index 594f84272957..7d3e329a6b1c 100644

Quoted 18 lines · Shayenne da Luz Moura

This change was suggested by checkpath.pl. Use unsigned int with bitfield allocate only one bit to the boolean variable.

CHECK: Avoid using bool structure members because of possible alignment issues

Signed-off-by Shayenne da Luz Moura <[email protected]>

---
drivers/staging/vboxvideo/vbox_drv.h | 14 +++++++-------
drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-

2 files changed, 8 insertions(+), 8 deletions(-)

drivers/staging/vboxvideo/vbox_drv.h +0 -0

index 594f84272957..7d3e329a6b1c 100644

drivers/staging/vboxvideo/vbox_drv.h struct vbox_private { · line 87

Quoted 8 lines

drivers/staging/vboxvideo/vbox_drv.h +1 -1

@@ -81,7 +81,7 @@ struct vbox_private {
 	u8 __iomem *vbva_buffers;
 	struct gen_pool *guest_pool;
 	struct vbva_buf_ctx *vbva_info;
-	bool any_pitch;
+	unsigned int any_pitch:1;
 	u32 num_crtcs;
 	/** Amount of available VRAM, including space used for buffers. */
 	u32 full_vram_size;

Using bitfields for booleans in these cases is less efficient than just using "regular" booleans for two reasons:

1. It will use the same amount of space. Due to alignment requirements, the compiler can't squeeze in anything into the 7 bits that are now "free". Each member, unless it's another bitfield, must start at a whole byte.

2. This is actually less efficient (slower) for the compiler to work with. The smallest granularity we have to access memory is 1 byte; we can't set individual bits directly in memory. For the original code, the assembly for 'vbox_private.any_pitch = true' would look something like this:

	movl   $0x1,-0x10(%rsp)

As you can see, the compiler can directly write into the variable. However, when we switch to using bitfields, the compiler must preserve the original value of the other 7 bits, so it must first read them from memory, manipulate the value and write it back. The assembly would look something like this:

	movzbl -0x10(%rsp),%eax
	or     $0x1,%eax
	mov    %al,-0x10(%rsp)

Which is less efficient than what was previously happening.

[Adding Joe Perches]

Quoted 50 lines · Sasha Levin

Quoted 18 lines · Shayenne da Luz Moura

This change was suggested by checkpath.pl. Use unsigned int with bitfield allocate only one bit to the boolean variable.

CHECK: Avoid using bool structure members because of possible alignment issues

Signed-off-by Shayenne da Luz Moura <[email protected]>

---
drivers/staging/vboxvideo/vbox_drv.h | 14 +++++++-------
drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-

2 files changed, 8 insertions(+), 8 deletions(-)

drivers/staging/vboxvideo/vbox_drv.h +0 -0

index 594f84272957..7d3e329a6b1c 100644

drivers/staging/vboxvideo/vbox_drv.h struct vbox_private { · line 87

Quoted 8 lines

drivers/staging/vboxvideo/vbox_drv.h +1 -1

@@ -81,7 +81,7 @@ struct vbox_private {
 	u8 __iomem *vbva_buffers;
 	struct gen_pool *guest_pool;
 	struct vbva_buf_ctx *vbva_info;
-	bool any_pitch;
+	unsigned int any_pitch:1;
 	u32 num_crtcs;
 	/** Amount of available VRAM, including space used for buffers. */
 	u32 full_vram_size;

Using bitfields for booleans in these cases is less efficient than just using "regular" booleans for two reasons:

1. It will use the same amount of space. Due to alignment requirements, the compiler can't squeeze in anything into the 7 bits that are now "free". Each member, unless it's another bitfield, must start at a whole byte.

2. This is actually less efficient (slower) for the compiler to work with. The smallest granularity we have to access memory is 1 byte; we can't set individual bits directly in memory. For the original code, the assembly for 'vbox_private.any_pitch = true' would look something like this:

	movl   $0x1,-0x10(%rsp)

As you can see, the compiler can directly write into the variable. However, when we switch to using bitfields, the compiler must preserve the original value of the other 7 bits, so it must first read them from memory, manipulate the value and write it back. The assembly would look something like this:

Quoted 5 lines

	movzbl -0x10(%rsp),%eax
	or     $0x1,%eax
	mov    %al,-0x10(%rsp)

Which is less efficient than what was previously happening.

Maybe checkpatch could be more precise about what kind of bools should be changed?

Quoted 35 lines · Julia Lawall

[Adding Joe Perches]

Quoted 31 lines · Sasha Levin

Quoted 18 lines · Shayenne da Luz Moura

This change was suggested by checkpath.pl. Use unsigned int with bitfield allocate only one bit to the boolean variable.

CHECK: Avoid using bool structure members because of possible alignment issues

Signed-off-by Shayenne da Luz Moura <[email protected]>

---
drivers/staging/vboxvideo/vbox_drv.h | 14 +++++++-------
drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-

2 files changed, 8 insertions(+), 8 deletions(-)

drivers/staging/vboxvideo/vbox_drv.h +0 -0

index 594f84272957..7d3e329a6b1c 100644

drivers/staging/vboxvideo/vbox_drv.h struct vbox_private { · line 87

Quoted 8 lines

drivers/staging/vboxvideo/vbox_drv.h +1 -1

@@ -81,7 +81,7 @@ struct vbox_private {
 	u8 __iomem *vbva_buffers;
 	struct gen_pool *guest_pool;
 	struct vbva_buf_ctx *vbva_info;
-	bool any_pitch;
+	unsigned int any_pitch:1;
 	u32 num_crtcs;
 	/** Amount of available VRAM, including space used for buffers. */
 	u32 full_vram_size;

Using bitfields for booleans in these cases is less efficient than just using "regular" booleans for two reasons:

Quoted 4 lines

Quoted 4 lines

1. It will use the same amount of space. Due to alignment requirements, the compiler can't squeeze in anything into the 7 bits that are now "free". Each member, unless it's another bitfield, must start at a whole byte.

Quoted 19 lines

Quoted 19 lines

2. This is actually less efficient (slower) for the compiler to work with. The smallest granularity we have to access memory is 1 byte; we can't set individual bits directly in memory. For the original code, the assembly for 'vbox_private.any_pitch = true' would look something like this:

	movl   $0x1,-0x10(%rsp)

As you can see, the compiler can directly write into the variable. However, when we switch to using bitfields, the compiler must preserve the original value of the other 7 bits, so it must first read them from memory, manipulate the value and write it back. The assembly would look something like this:

	movzbl -0x10(%rsp),%eax
	or     $0x1,%eax
	mov    %al,-0x10(%rsp)

Quoted 4 lines

Quoted 1 line

Which is less efficient than what was previously happening.

Maybe checkpatch could be more precise about what kind of bools should be changed?

Probably so, what verbiage would you suggest?

Also, any conversion from bool to int would
have to take care than any assigment uses !!
where appropriate.

Quoted 17 lines · Shayenne da Luz Moura

This change was suggested by checkpath.pl. Use unsigned int with bitfield allocate only one bit to the boolean variable.

CHECK: Avoid using bool structure members because of possible alignment issues

Signed-off-by Shayenne da Luz Moura <[email protected]>

---
drivers/staging/vboxvideo/vbox_drv.h | 14 +++++++-------
drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-

2 files changed, 8 insertions(+), 8 deletions(-)

drivers/staging/vboxvideo/vbox_drv.h +0 -0

index 594f84272957..7d3e329a6b1c 100644

drivers/staging/vboxvideo/vbox_drv.h struct vbox_private { · line 87

Quoted 8 lines

drivers/staging/vboxvideo/vbox_drv.h +1 -1

@@ -81,7 +81,7 @@ struct vbox_private {
 	u8 __iomem *vbva_buffers;
 	struct gen_pool *guest_pool;
 	struct vbva_buf_ctx *vbva_info;
-	bool any_pitch;
+	unsigned int any_pitch:1;
 	u32 num_crtcs;
 	/** Amount of available VRAM, including space used for buffers. */
 	u32 full_vram_size;

Using bitfields for booleans in these cases is less efficient than just using "regular" booleans for two reasons:

1. It will use the same amount of space. Due to alignment requirements, the compiler can't squeeze in anything into the 7 bits that are now "free". Each member, unless it's another bitfield, must start at a whole byte.

The "possible alignement issues" in CHECK report is difficult to figure out by just doing a glance analysis. :)

Linus also suggested to use bool as the base type i.e., bool x:1 but again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.

Quoted 5 lines

The "possible alignement issues" in CHECK report is difficult to figure out by just doing a glance analysis. :)

Linus also suggested to use bool as the base type i.e., bool x:1 but again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.

If bool x:1 has the size of bool, then wouldn't int x:1 have the size of int? But my little experiments suggest that the size is the smallest that fits the requested bits and alignment chosen by the compiler, regardless of the type.

bool x:1 has the advantage that anything that is not 0 is considered true.

So for bool x:1, x = 4 is true, while for int x:1, x = 4 is false.

But the :1 adds instructions, so at least for only one bool, where little space is saved, it is probably not worth it.

Quoted 10 lines · Julia Lawall

Quoted 5 lines

The "possible alignement issues" in CHECK report is difficult to figure out by just doing a glance analysis. :)

Linus also suggested to use bool as the base type i.e., bool x:1 but again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.

If bool x:1 has the size of bool, then wouldn't int x:1 have the size of int? But my little experiments suggest that the size is the smallest that fits the requested bits and alignment chosen by the compiler, regardless of the type.

Yes, correct!
And we can't use sizeof on bitfields *directly*, nor reference it using a pointer.

It can be applied only when these bitfields are wrapped in a structure.

Testing:

#include <stdio.h>
#include <stdbool.h>

struct S {
bool a:1;
bool b:1;
bool c:1;
bool d:1;
};

int main(void)

{
    printf("%zu\n", sizeof(struct S));
}

Output: 1

If I change all bool to unsigned int, output is: *4*.

So, conclusion is compiler doesn't squeeze the size less than native size of the datatype i.e., if we changed all members to unsigned int:1,
total width = 4 bits
padding = 4 bits

Therefore, total size should have been = 1 byte! But since sizeof(unsigned int) == 4, it can't be squeezed to less than it.

Well, int x:1 can either have 0..1 or -1..0 range due implementation defined behavior as I said in the previous reply.

If you really want to consider negative values, then make it explicit using signed int x:1 which make range guaranteed to be -1..0

Quoted 46 lines · Julia Lawall

Quoted 10 lines

Quoted 5 lines

The "possible alignement issues" in CHECK report is difficult to figure out by just doing a glance analysis. :)

Linus also suggested to use bool as the base type i.e., bool x:1 but again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.

If bool x:1 has the size of bool, then wouldn't int x:1 have the size of int? But my little experiments suggest that the size is the smallest that fits the requested bits and alignment chosen by the compiler, regardless of the type.

Yes, correct!
And we can't use sizeof on bitfields *directly*, nor reference it using a pointer.

It can be applied only when these bitfields are wrapped in a structure.

Testing:

#include <stdio.h>
#include <stdbool.h>

struct S {
bool a:1;
bool b:1;
bool c:1;
bool d:1;
};

int main(void)

{
    printf("%zu\n", sizeof(struct S));
}

Output: 1

If I change all bool to unsigned int, output is: *4*.

So, conclusion is compiler doesn't squeeze the size less than native size of the datatype i.e., if we changed all members to unsigned int:1,
total width = 4 bits
padding = 4 bits

Quoted 3 lines

Therefore, total size should have been = 1 byte! But since sizeof(unsigned int) == 4, it can't be squeezed to less than it.

This conclusion does not seem to be correct, if you try the following program. I get 4 for everything, meaning that the four unsigned int bits are getting squeezed into one byte when it is convenient.

#include <stdio.h>
#include <stdbool.h>

struct S1 {
bool a:1;
bool b:1;
bool c:1;
bool d:1;
char a1;
char a2;
char a3;
};

struct S2 {
unsigned int a:1;
unsigned int b:1;
unsigned int c:1;
unsigned int d:1;
char a1;
char a2;
char a3;
};

int main(void)

{
    printf("%zu\n", sizeof(struct S1));
    printf("%zu\n", sizeof(struct S2));
    printf("%zu\n", sizeof(unsigned int));
}

Quoted 3 lines

Well, int x:1 can either have 0..1 or -1..0 range due implementation defined behavior as I said in the previous reply.

Quoted 2 lines

If you really want to consider negative values, then make it explicit using signed int x:1 which make range guaranteed to be -1..0

The code wants booleans, not negative values.

julia

Hi,

Quoted 88 lines · Himanshu Jha

Quoted 46 lines · Julia Lawall

Quoted 10 lines

Quoted 5 lines

The "possible alignement issues" in CHECK report is difficult to figure out by just doing a glance analysis. :)

Linus also suggested to use bool as the base type i.e., bool x:1 but again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.

If bool x:1 has the size of bool, then wouldn't int x:1 have the size of int? But my little experiments suggest that the size is the smallest that fits the requested bits and alignment chosen by the compiler, regardless of the type.

Yes, correct!
And we can't use sizeof on bitfields *directly*, nor reference it using a pointer.

It can be applied only when these bitfields are wrapped in a structure.

Testing:

#include <stdio.h>
#include <stdbool.h>

struct S {
bool a:1;
bool b:1;
bool c:1;
bool d:1;
};

int main(void)

{
    printf("%zu\n", sizeof(struct S));
}

Output: 1

If I change all bool to unsigned int, output is: *4*.

So, conclusion is compiler doesn't squeeze the size less than native size of the datatype i.e., if we changed all members to unsigned int:1,
total width = 4 bits
padding = 4 bits

Quoted 3 lines

Therefore, total size should have been = 1 byte! But since sizeof(unsigned int) == 4, it can't be squeezed to less than it.

This conclusion does not seem to be correct, if you try the following program. I get 4 for everything, meaning that the four unsigned int bits are getting squeezed into one byte when it is convenient.

#include <stdio.h>
#include <stdbool.h>

struct S1 {
bool a:1;
bool b:1;
bool c:1;
bool d:1;
char a1;
char a2;
char a3;
};

struct S2 {
unsigned int a:1;
unsigned int b:1;
unsigned int c:1;
unsigned int d:1;
char a1;
char a2;
char a3;
};

int main(void)

{
    printf("%zu\n", sizeof(struct S1));
    printf("%zu\n", sizeof(struct S2));
    printf("%zu\n", sizeof(unsigned int));
}

Quoted 3 lines

Well, int x:1 can either have 0..1 or -1..0 range due implementation defined behavior as I said in the previous reply.

Quoted 6 lines

Quoted 2 lines

If you really want to consider negative values, then make it explicit using signed int x:1 which make range guaranteed to be -1..0

The code wants booleans, not negative values.

julia

Thank you all for the discussion!

However, I think I do not understand the conclusion.

It means that the best way is to use only boolean instead of use unsigned int with bitfield? I mean specifically in the case of my patch, where there are some boolean variables are mixed with other variables types.

Quoted 97 lines · Shayenne Moura

Hi,

Quoted 88 lines · Himanshu Jha

Quoted 46 lines · Julia Lawall

Quoted 10 lines

Quoted 5 lines

The "possible alignement issues" in CHECK report is difficult to figure out by just doing a glance analysis. :)

Linus also suggested to use bool as the base type i.e., bool x:1 but again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.

If bool x:1 has the size of bool, then wouldn't int x:1 have the size of int? But my little experiments suggest that the size is the smallest that fits the requested bits and alignment chosen by the compiler, regardless of the type.

Yes, correct!
And we can't use sizeof on bitfields *directly*, nor reference it using a pointer.

It can be applied only when these bitfields are wrapped in a structure.

Testing:

#include <stdio.h>
#include <stdbool.h>

struct S {
bool a:1;
bool b:1;
bool c:1;
bool d:1;
};

int main(void)

{
    printf("%zu\n", sizeof(struct S));
}

Output: 1

If I change all bool to unsigned int, output is: *4*.

So, conclusion is compiler doesn't squeeze the size less than native size of the datatype i.e., if we changed all members to unsigned int:1,
total width = 4 bits
padding = 4 bits

Quoted 3 lines

Therefore, total size should have been = 1 byte! But since sizeof(unsigned int) == 4, it can't be squeezed to less than it.

This conclusion does not seem to be correct, if you try the following program. I get 4 for everything, meaning that the four unsigned int bits are getting squeezed into one byte when it is convenient.

#include <stdio.h>
#include <stdbool.h>

struct S1 {
bool a:1;
bool b:1;
bool c:1;
bool d:1;
char a1;
char a2;
char a3;
};

struct S2 {
unsigned int a:1;
unsigned int b:1;
unsigned int c:1;
unsigned int d:1;
char a1;
char a2;
char a3;
};

int main(void)

{
    printf("%zu\n", sizeof(struct S1));
    printf("%zu\n", sizeof(struct S2));
    printf("%zu\n", sizeof(unsigned int));
}

Quoted 3 lines

Well, int x:1 can either have 0..1 or -1..0 range due implementation defined behavior as I said in the previous reply.

Quoted 6 lines

Quoted 2 lines

If you really want to consider negative values, then make it explicit using signed int x:1 which make range guaranteed to be -1..0

The code wants booleans, not negative values.

julia

Quoted 7 lines

Thank you all for the discussion!

However, I think I do not understand the conclusion.

It means that the best way is to use only boolean instead of use unsigned int with bitfield? I mean specifically in the case of my patch, where there are some boolean variables are mixed with other variables types.

To my recollection, your code had a bool with larger types on either side. In that case, I think bool is fine. The compiler it likely to align those larger typed values such that the field with the bool type will get more than one byte no matter what type you use. If there are several fields with very small types adjacent, there might be some benefit to thinking about what the type should be.

julia