bluetooth.h: __ variants of u8 and friends are not neccessary inside kernel

PATCH Superseded Pavel Machek 5 participants 16 messages Oct 3, 2016 – Oct 6, 2016

Subsystems (4)

Networking Network drivers Bluetooth drivers Bluetooth

lore

Share & tools

lore

Pavel Machek

author

2016-10-03 08:56 UTC #0 lore raw

PATCH 1/1 bluetooth.h: __ variants of u8 and friends are not neccessary inside kernel

bluetooth.h is not part of user API, so __ variants are not neccessary here.

Signed-off-by Pavel Machek <[email protected]>

include/net/bluetooth/bluetooth.h +18 ~~-17~~

index bfd1590..aea0371 100644
@@ -60,8 +60,8 @@
 
 #define BT_SECURITY	4
 struct bt_security {
-	__u8 level;
-	__u8 key_size;
+	u8 level;
+	u8 key_size;
 };
 #define BT_SECURITY_SDP		0
 #define BT_SECURITY_LOW		1

@@ -78,7 +78,7 @@ struct bt_security {
 
 #define BT_POWER	9
 struct bt_power {
-	__u8 force_active;
+	u8 force_active;
 };
 #define BT_POWER_FORCE_ACTIVE_OFF 0
 #define BT_POWER_FORCE_ACTIVE_ON  1

@@ -112,7 +112,7 @@ struct bt_power {
 
 #define BT_VOICE		11
 struct bt_voice {
-	__u16 setting;
+	u16 setting;
 };
 
 #define BT_VOICE_TRANSPARENT			0x0003

@@ -188,7 +188,7 @@ static inline const char *state_to_string(int state)
 
 /* BD Address */
 typedef struct {
-	__u8 b[6];
+	u8 b[6];
 } __packed bdaddr_t;
 
 /* BD Address type */

@@ -196,7 +196,7 @@ typedef struct {
 #define BDADDR_LE_PUBLIC	0x01
 #define BDADDR_LE_RANDOM	0x02
 
-static inline bool bdaddr_type_is_valid(__u8 type)
+static inline bool bdaddr_type_is_valid(u8 type)
 {
 	switch (type) {
 	case BDADDR_BREDR:

@@ -208,7 +208,7 @@ static inline bool bdaddr_type_is_valid(__u8 type)
 	return false;
 }
 
-static inline bool bdaddr_type_is_le(__u8 type)
+static inline bool bdaddr_type_is_le(u8 type)
 {
 	switch (type) {
 	case BDADDR_LE_PUBLIC:

@@ -278,15 +278,16 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock);
 
 /* Skb helpers */
 struct l2cap_ctrl {
-	__u8	sframe:1,
+	u8	sframe:1,
 		poll:1,
 		final:1,
 		fcs:1,
 		sar:2,
 		super:2;
-	__u16	reqseq;
-	__u16	txseq;
-	__u8	retries;
+
+	u16	reqseq;
+	u16	txseq;
+	u8	retries;
 	__le16  psm;
 	bdaddr_t bdaddr;
 	struct l2cap_chan *chan;

@@ -302,7 +303,7 @@ typedef void (*hci_req_complete_skb_t)(struct hci_dev *hdev, u8 status,
 #define HCI_REQ_SKB	BIT(1)
 
 struct hci_ctrl {
-	__u16 opcode;
+	u16 opcode;
 	u8 req_flags;
 	u8 req_event;
 	union {

@@ -312,10 +313,10 @@ struct hci_ctrl {
 };
 
 struct bt_skb_cb {
-	__u8 pkt_type;
-	__u8 force_active;
-	__u16 expect;
-	__u8 incoming:1;
+	u8 pkt_type;
+	u8 force_active;
+	u16 expect;
+	u8 incoming:1;
 	union {
 		struct l2cap_ctrl l2cap;
 		struct hci_ctrl hci;

@@ -365,7 +366,7 @@ out:
 	return NULL;
 }
 
-int bt_to_errno(__u16 code);
+int bt_to_errno(u16 code);
 
 void hci_sock_set_flag(struct sock *sk, int nr);
 void hci_sock_clear_flag(struct sock *sk, int nr);

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Joe Perches

2016-10-05 17:53 UTC #2 lore raw

Quoted 8 lines · Marcel Holtmann

Hi Pavel,
include/net/bluetooth/bluetooth.h
Quoted 6 lines
bluetooth.h is not part of user API, so __ variants are not neccessary here.
Signed-off-by Pavel Machek <[email protected]>
include/net/bluetooth/bluetooth.h +0 -0

[]

Quoted 4 lines

Quoted 4 lines
struct bt_skb_cb {
include/net/bluetooth/bluetooth.h +0 -3
-	__u8 pkt_type;
-	__u8 force_active;
-	__u16 expect;

Quoted 8 lines

include/net/bluetooth/bluetooth.h +4 -1

-	__u8 incoming:1;
+	u8 pkt_type;
+	u8 force_active;
+	u16 expect;
+	u8 incoming:1;

	union {
		struct l2cap_ctrl l2cap;
		struct hci_ctrl hci;

trivia:

It's generally faster to use bool instead of u8 foo:1;

Pavel Machek

author

2016-10-05 19:11 UTC #3 lore raw

Quoted 25 lines · Joe Perches

Quoted 8 lines · Marcel Holtmann
Hi Pavel,
include/net/bluetooth/bluetooth.h
Quoted 6 lines
bluetooth.h is not part of user API, so __ variants are not neccessary here.
Signed-off-by Pavel Machek <[email protected]>
include/net/bluetooth/bluetooth.h +0 -0
[]
Quoted 4 lines
Quoted 4 lines
struct bt_skb_cb {
include/net/bluetooth/bluetooth.h +0 -3
-	__u8 pkt_type;
-	__u8 force_active;
-	__u16 expect;
Quoted 8 lines
Quoted 8 lines
include/net/bluetooth/bluetooth.h +4 -1
-	__u8 incoming:1;
+	u8 pkt_type;
+	u8 force_active;
+	u16 expect;
+	u8 incoming:1;
	union {
		struct l2cap_ctrl l2cap;
		struct hci_ctrl hci;
trivia:

Quoted 1 line

It's generally faster to use bool instead of u8 foo:1;

Ok, but I'm not changing that in this patch.

(And actually, bool will take a lot more memory, right?)
Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Joe Perches

2016-10-05 19:15 UTC #4 lore raw

Quoted 28 lines · Pavel Machek

Quoted 27 lines · Joe Perches
Quoted 8 lines · Marcel Holtmann
Hi Pavel,
include/net/bluetooth/bluetooth.h
Quoted 6 lines
bluetooth.h is not part of user API, so __ variants are not neccessary here.
Signed-off-by Pavel Machek <[email protected]>
include/net/bluetooth/bluetooth.h +0 -0
[]
Quoted 4 lines
Quoted 4 lines
struct bt_skb_cb {
include/net/bluetooth/bluetooth.h +0 -3
-	__u8 pkt_type;
-	__u8 force_active;
-	__u16 expect;
Quoted 8 lines
Quoted 8 lines
include/net/bluetooth/bluetooth.h +4 -1
-	__u8 incoming:1;
+	u8 pkt_type;
+	u8 force_active;
+	u16 expect;
+	u8 incoming:1;
	union {
		struct l2cap_ctrl l2cap;
		struct hci_ctrl hci;
trivia:

Quoted 4 lines

Quoted 1 line
It's generally faster to use bool instead of u8 foo:1;
Ok, but I'm not changing that in this patch. (And actually, bool will take a lot more memory, right?)

No worries, and bool is the same size as u8.

Quoted 1 line

Pavel Machek

author

2016-10-05 22:13 UTC #5 lore raw

Quoted 29 lines · Joe Perches

Quoted 28 lines · Pavel Machek
Quoted 27 lines · Joe Perches
Quoted 8 lines · Marcel Holtmann
Hi Pavel,
include/net/bluetooth/bluetooth.h
Quoted 6 lines
bluetooth.h is not part of user API, so __ variants are not neccessary here.
Signed-off-by Pavel Machek <[email protected]>
include/net/bluetooth/bluetooth.h +0 -0
[]
Quoted 4 lines
Quoted 4 lines
struct bt_skb_cb {
include/net/bluetooth/bluetooth.h +0 -3
-	__u8 pkt_type;
-	__u8 force_active;
-	__u16 expect;
Quoted 8 lines
Quoted 8 lines
include/net/bluetooth/bluetooth.h +4 -1
-	__u8 incoming:1;
+	u8 pkt_type;
+	u8 force_active;
+	u16 expect;
+	u8 incoming:1;
	union {
		struct l2cap_ctrl l2cap;
		struct hci_ctrl hci;
trivia:

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
It's generally faster to use bool instead of u8 foo:1;
Ok, but I'm not changing that in this patch. (And actually, bool will take a lot more memory, right?)
No worries, and bool is the same size as u8.

Exactly what I'm talking about :-). One byte vs. one bit, right?

Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Joe Perches

2016-10-05 22:28 UTC #6 lore raw

Quoted 3 lines · Pavel Machek

Quoted 2 lines · Joe Perches
Quoted 1 line · Pavel Machek
On Wed 2016-10-05 10:53:16, Joe Perches wrote:

[]

Quoted 6 lines

Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
trivia:
It's generally faster to use bool instead of u8 foo:1;
Ok, but I'm not changing that in this patch. (And actually, bool will take a lot more memory, right?)
No worries, and bool is the same size as u8.
Exactly what I'm talking about :-). One byte vs. one bit, right?

Memory isn't bit addressable.
So it's the same byte, it just doesn't use a read/modify/write operation to update a value.

Pavel Machek

author

2016-10-06 07:02 UTC #7 lore raw

Quoted 7 lines · Joe Perches

Quoted 3 lines · Pavel Machek
Quoted 2 lines · Joe Perches
Quoted 1 line · Pavel Machek
On Wed 2016-10-05 10:53:16, Joe Perches wrote:
[]
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
trivia:
It's generally faster to use bool instead of u8 foo:1;

Quoted 8 lines

Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
Ok, but I'm not changing that in this patch. (And actually, bool will take a lot more memory, right?)
No worries, and bool is the same size as u8.
Exactly what I'm talking about :-). One byte vs. one bit, right?
Memory isn't bit addressable.
So it's the same byte, it just doesn't use a read/modify/write operation to update a value.

I believe you are wrong. bit addressability does not matter, cpu can definitely get the bit values.

u8 foo:1;
u8 bar:1;
u8 baz:1;

should take 1 byte, where

bool foo, bar, baz;

will take more like 3.

Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Joe Perches

2016-10-06 07:07 UTC #8 lore raw

Quoted 7 lines · Pavel Machek

I believe you are wrong. bit addressability does not matter, cpu can definitely get the bit values.
u8 foo:1;
u8 bar:1;
u8 baz:1;

Quoted 5 lines

should take 1 byte, where
bool foo, bar, baz;
will take more like 3.

Definitely true.

There is only one single bitfield foo here though
so what you wrote doesn't apply.

Johan Hedberg

2016-10-06 08:38 UTC #9 lore raw

Hi,

Quoted 10 lines · Joe Perches

Quoted 9 lines · Pavel Machek
I believe you are wrong. bit addressability does not matter, cpu can definitely get the bit values.
u8 foo:1;
u8 bar:1;
u8 baz:1;
should take 1 byte, where

Quoted 8 lines

Quoted 3 lines
bool foo, bar, baz;
will take more like 3.
Definitely true.
There is only one single bitfield foo here though
so what you wrote doesn't apply.

What's in the tree is a left-over from times when there were multiple bit fields in this struct. By the time others were removed and there was only one left no-one has apparently bothered to update it to a bool or single u8.

Johan

David Laight

2016-10-06 09:41 UTC #10 lore raw

In reply to Joe Perches

From: Of Joe Perches
...

Quoted 1 line

No worries, and bool is the same size as u8.

That is not guaranteed at all.
One of the ARM ABI defined bool to be the size of int.

David

Joe Perches

2016-10-06 11:38 UTC #11 lore raw

Quoted 4 lines · David Laight

From: Joe Perches
Quoted 1 line
No worries, and bool is the same ,size as u8.
That is not guaranteed at all.
One of the ARM ABI defined bool to be the size of int.

Really? What kernel has sizeof(_Bool) != 1 ?

David Laight

2016-10-06 13:00 UTC #12 lore raw

From: Joe Perches

Quoted 8 lines

Sent: 06 October 2016 12:39
Quoted 4 lines · David Laight
From: Joe Perches
Quoted 1 line
No worries, and bool is the same ,size as u8.
That is not guaranteed at all.
One of the ARM ABI defined bool to be the size of int.
=20
Really? What kernel has sizeof(_Bool) !=3D 1 ?

Probably none, but I know systems have used larger bool. I found this:

Quoted 6 lines

with egcs-2.90.29 980515 (egcs-1.0.3 release) on alphaev56-dec-osf4.0d
bool =3D 8
short =3D 2
int =3D 4=20
long =3D 8

I'm pretty sure something newer than an old alpha ABI used 4 byte bool.

David

Joe Perches

2016-10-06 15:41 UTC #13 lore raw

Quoted 15 lines · David Laight

From: Joe Perches
Quoted 7 lines
Sent: 06 October 2016 12:39
Quoted 4 lines · David Laight
From: Joe Perches
Quoted 1 line
No worries, and bool is the same ,size as u8.
That is not guaranteed at all.
One of the ARM ABI defined bool to be the size of int.
Really? What kernel has sizeof(_Bool) != 1 ?
Probably none, but I know systems have used larger bool. I found this:
Quoted 5 lines
with egcs-2.90.29 980515 (egcs-1.0.3 release) on alphaev56-dec-osf4.0d
bool = 8
short = 2
int = 4
long = 8

It's likely there are probably DSPs and old TOPS-20/CDC-6400 systems where sizeof(u16) isn't 2 as well.

I think linux isn't likely to be ported successfully to those platforms.

No matter. If bool isn't desired because some future expansion to this is likely and memory needs to be conserved, fine, use a bitfield.

It can be slower than bool because it can be RMW.

cheers, Joe

Pavel Machek

author

2016-10-05 20:51 UTC #14 lore raw

PATCH v2 1/1 bluetooth.h: __ variants of u8 and friends are not neccessary inside kernel

In reply to Pavel Machek

bluetooth.h is not part of user API, so __ variants are not neccessary here.

Signed-off-by Pavel Machek <[email protected]>

---
v2: not touching stuff that Marcel does not want touched, as it will become API later.

include/net/bluetooth/bluetooth.h +13 ~~-12~~

index bfd1590..aea0371 100644
@@ -196,7 +196,7 @@ typedef struct {
 #define BDADDR_LE_PUBLIC	0x01
 #define BDADDR_LE_RANDOM	0x02
 
-static inline bool bdaddr_type_is_valid(__u8 type)
+static inline bool bdaddr_type_is_valid(u8 type)
 {
 	switch (type) {
 	case BDADDR_BREDR:

@@ -208,7 +208,7 @@ static inline bool bdaddr_type_is_valid(__u8 type)
 	return false;
 }
 
-static inline bool bdaddr_type_is_le(__u8 type)
+static inline bool bdaddr_type_is_le(u8 type)
 {
 	switch (type) {
 	case BDADDR_LE_PUBLIC:

@@ -278,15 +278,16 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock);
 
 /* Skb helpers */
 struct l2cap_ctrl {
-	__u8	sframe:1,
+	u8	sframe:1,
 		poll:1,
 		final:1,
 		fcs:1,
 		sar:2,
 		super:2;
-	__u16	reqseq;
-	__u16	txseq;
-	__u8	retries;
+
+	u16	reqseq;
+	u16	txseq;
+	u8	retries;
 	__le16  psm;
 	bdaddr_t bdaddr;
 	struct l2cap_chan *chan;

@@ -302,7 +303,7 @@ typedef void (*hci_req_complete_skb_t)(struct hci_dev *hdev, u8 status,
 #define HCI_REQ_SKB	BIT(1)
 
 struct hci_ctrl {
-	__u16 opcode;
+	u16 opcode;
 	u8 req_flags;
 	u8 req_event;
 	union {

@@ -312,10 +313,10 @@ struct hci_ctrl {
 };
 
 struct bt_skb_cb {
-	__u8 pkt_type;
-	__u8 force_active;
-	__u16 expect;
-	__u8 incoming:1;
+	u8 pkt_type;
+	u8 force_active;
+	u16 expect;
+	u8 incoming:1;
 	union {
 		struct l2cap_ctrl l2cap;
 		struct hci_ctrl hci;

@@ -365,7 +366,7 @@ out:
 	return NULL;
 }
 
-int bt_to_errno(__u16 code);
+int bt_to_errno(u16 code);
 
 void hci_sock_set_flag(struct sock *sk, int nr);
 void hci_sock_clear_flag(struct sock *sk, int nr);

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Marcel Holtmann

maintainer

2016-10-05 21:54 UTC #15 lore raw

Hi Pavel,

Quoted 8 lines

bluetooth.h is not part of user API, so __ variants are not neccessary here.
Signed-off-by Pavel Machek <[email protected]>
---
v2: not touching stuff that Marcel does not want touched, as it will become API later.

patch has been applied to bluetooth-next tree.

Regards

Marcel