[PATCH 1/2] Bluetooth: Use defined link key size

[PATCH 1/2] Bluetooth: Use defined link key size

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Remove magic number with defined link key size. This is especially
useful for calculation other keys which are 2 * key_size, etc that
makes usage of magic numbers too much.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/bluetooth.h |    2 ++
 include/net/bluetooth/hci.h       |    4 ++--
 include/net/bluetooth/hci_core.h  |    2 +-
 net/bluetooth/hci_core.c          |    2 +-
 net/bluetooth/hci_event.c         |    2 +-
 net/bluetooth/mgmt.c              |    2 +-
 6 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 961669b..4abedfe 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -68,6 +68,8 @@ struct bt_security {
 #define BT_SECURITY_MEDIUM	2
 #define BT_SECURITY_HIGH	3
 
+#define BT_SEC_LINK_KEY_SIZE		16
+
 #define BT_DEFER_SETUP	7
 
 #define BT_FLUSHABLE	8
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 66a7b57..6ab8d0b 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -371,7 +371,7 @@ struct hci_cp_reject_conn_req {
 #define HCI_OP_LINK_KEY_REPLY		0x040b
 struct hci_cp_link_key_reply {
 	bdaddr_t bdaddr;
-	__u8     link_key[16];
+	__u8     link_key[BT_SEC_LINK_KEY_SIZE];
 } __packed;
 
 #define HCI_OP_LINK_KEY_NEG_REPLY	0x040c
@@ -1048,7 +1048,7 @@ struct hci_ev_link_key_req {
 #define HCI_EV_LINK_KEY_NOTIFY		0x18
 struct hci_ev_link_key_notify {
 	bdaddr_t bdaddr;
-	__u8     link_key[16];
+	__u8     link_key[BT_SEC_LINK_KEY_SIZE];
 	__u8     key_type;
 } __packed;
 
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 9fc7728..20b7d6b 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -105,7 +105,7 @@ struct link_key {
 	struct list_head list;
 	bdaddr_t bdaddr;
 	u8 type;
-	u8 val[16];
+	u8 val[BT_SEC_LINK_KEY_SIZE];
 	u8 pin_len;
 };
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d0a960d..6bd562b 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1291,7 +1291,7 @@ int hci_add_link_key(struct hci_dev *hdev, struct hci_conn *conn, int new_key,
 	}
 
 	bacpy(&key->bdaddr, bdaddr);
-	memcpy(key->val, val, 16);
+	memcpy(key->val, val, BT_SEC_LINK_KEY_SIZE);
 	key->pin_len = pin_len;
 
 	if (type == HCI_LK_CHANGED_COMBINATION)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 6c2d7cc..df96f10 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2739,7 +2739,7 @@ static inline void hci_link_key_request_evt(struct hci_dev *hdev,
 	}
 
 	bacpy(&cp.bdaddr, &ev->bdaddr);
-	memcpy(cp.link_key, key->val, 16);
+	memcpy(cp.link_key, key->val, BT_SEC_LINK_KEY_SIZE);
 
 	hci_send_cmd(hdev, HCI_OP_LINK_KEY_REPLY, sizeof(cp), &cp);
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 6a7e926..08e7211 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2955,7 +2955,7 @@ int mgmt_new_link_key(struct hci_dev *hdev, struct link_key *key,
 	bacpy(&ev.key.addr.bdaddr, &key->bdaddr);
 	ev.key.addr.type = BDADDR_BREDR;
 	ev.key.type = key->type;
-	memcpy(ev.key.val, key->val, 16);
+	memcpy(ev.key.val, key->val, BT_SEC_LINK_KEY_SIZE);
 	ev.key.pin_len = key->pin_len;
 
 	return mgmt_event(MGMT_EV_NEW_LINK_KEY, hdev, &ev, sizeof(ev), NULL);
-- 
1.7.9.5

[PATCH 2/2] Bluetooth: Define L2CAP conf continuation flag

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Define Continuation flag which the only flag used from Flags field
in L2CAP Configuration Request and Response.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/l2cap.h |    4 ++++
 net/bluetooth/l2cap_core.c    |   12 ++++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 452fcc4..8fdfaca 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -271,6 +271,10 @@ struct l2cap_conf_rsp {
 #define L2CAP_CONF_PENDING	0x0004
 #define L2CAP_CONF_EFS_REJECT	0x0005
 
+/* conf req/rsp continuation flag */
+#define L2CAP_CONF_FLAG_CONT_CLEARED	0
+#define L2CAP_CONF_FLAG_CONT_SET	1
+
 struct l2cap_conf_opt {
 	__u8       type;
 	__u8       len;
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index d9f215f..65e56d5 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2544,7 +2544,7 @@ done:
 	}
 
 	req->dcid  = cpu_to_le16(chan->dcid);
-	req->flags = cpu_to_le16(0);
+	req->flags = cpu_to_le16(L2CAP_CONF_FLAG_CONT_CLEARED);
 
 	return ptr - data;
 }
@@ -2764,7 +2764,7 @@ done:
 	}
 	rsp->scid   = cpu_to_le16(chan->dcid);
 	rsp->result = cpu_to_le16(result);
-	rsp->flags  = cpu_to_le16(0x0000);
+	rsp->flags  = cpu_to_le16(L2CAP_CONF_FLAG_CONT_CLEARED);
 
 	return ptr - data;
 }
@@ -2863,7 +2863,7 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len, voi
 	}
 
 	req->dcid   = cpu_to_le16(chan->dcid);
-	req->flags  = cpu_to_le16(0x0000);
+	req->flags  = cpu_to_le16(L2CAP_CONF_FLAG_CONT_CLEARED);
 
 	return ptr - data;
 }
@@ -3218,11 +3218,11 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	memcpy(chan->conf_req + chan->conf_len, req->data, len);
 	chan->conf_len += len;
 
-	if (flags & 0x0001) {
+	if (flags & L2CAP_CONF_FLAG_CONT_SET) {
 		/* Incomplete config. Send empty response. */
 		l2cap_send_cmd(conn, cmd->ident, L2CAP_CONF_RSP,
 				l2cap_build_conf_rsp(chan, rsp,
-					L2CAP_CONF_SUCCESS, 0x0001), rsp);
+					L2CAP_CONF_SUCCESS, flags), rsp);
 		goto unlock;
 	}
 
@@ -3369,7 +3369,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 		goto done;
 	}
 
-	if (flags & 0x01)
+	if (flags & L2CAP_CONF_FLAG_CONT_SET)
 		goto done;
 
 	set_bit(CONF_INPUT_DONE, &chan->conf_state);
-- 
1.7.9.5

Re: [PATCH 1/2] Bluetooth: Use defined link key size

[Johan Hedberg]

Hi Andrei,

On Fri, May 18, 2012, Andrei Emeltchenko wrote:
> Remove magic number with defined link key size. This is especially
> useful for calculation other keys which are 2 * key_size, etc that
> makes usage of magic numbers too much.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  include/net/bluetooth/bluetooth.h |    2 ++
>  include/net/bluetooth/hci.h       |    4 ++--
>  include/net/bluetooth/hci_core.h  |    2 +-
>  net/bluetooth/hci_core.c          |    2 +-
>  net/bluetooth/hci_event.c         |    2 +-
>  net/bluetooth/mgmt.c              |    2 +-
>  6 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 961669b..4abedfe 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -68,6 +68,8 @@ struct bt_security {
>  #define BT_SECURITY_MEDIUM	2
>  #define BT_SECURITY_HIGH	3
>  
> +#define BT_SEC_LINK_KEY_SIZE		16

The patch seems ok'ish to me but I'd remove the _SEC part from the
define name since it doesn't really buy you anything. Also, I'm a bit
confused by the commit message. What other keys are "2 * key_size"?
There are no such occurrences in the patch itself so you should at least
give some examples in the commit message.

Johan

Re: [PATCH 1/2] Bluetooth: Use defined link key size

[Marcel Holtmann]

Hi Andrei,

> Remove magic number with defined link key size. This is especially
> useful for calculation other keys which are 2 * key_size, etc that
> makes usage of magic numbers too much.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  include/net/bluetooth/bluetooth.h |    2 ++
>  include/net/bluetooth/hci.h       |    4 ++--
>  include/net/bluetooth/hci_core.h  |    2 +-
>  net/bluetooth/hci_core.c          |    2 +-
>  net/bluetooth/hci_event.c         |    2 +-
>  net/bluetooth/mgmt.c              |    2 +-
>  6 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 961669b..4abedfe 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -68,6 +68,8 @@ struct bt_security {
>  #define BT_SECURITY_MEDIUM	2
>  #define BT_SECURITY_HIGH	3
>  
> +#define BT_SEC_LINK_KEY_SIZE		16
> +

where is the _SEC_ part of this coming from? Any reason for it? And why
is it defined in the middle of socket options? Seems the absolute wrong
place for this.

>  #define BT_DEFER_SETUP	7
>  
>  #define BT_FLUSHABLE	8
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 66a7b57..6ab8d0b 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -371,7 +371,7 @@ struct hci_cp_reject_conn_req {
>  #define HCI_OP_LINK_KEY_REPLY		0x040b
>  struct hci_cp_link_key_reply {
>  	bdaddr_t bdaddr;
> -	__u8     link_key[16];
> +	__u8     link_key[BT_SEC_LINK_KEY_SIZE];
>  } __packed;

Putting it in hci.h with HCI_LINK_KEY_SIZE seems more reasonable. Or do
you have other plans for this constant?

Regards

Marcel

Re: [PATCH 2/2] Bluetooth: Define L2CAP conf continuation flag

[Marcel Holtmann]

Hi Andrei,

> Define Continuation flag which the only flag used from Flags field
> in L2CAP Configuration Request and Response.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  include/net/bluetooth/l2cap.h |    4 ++++
>  net/bluetooth/l2cap_core.c    |   12 ++++++------
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 452fcc4..8fdfaca 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -271,6 +271,10 @@ struct l2cap_conf_rsp {
>  #define L2CAP_CONF_PENDING	0x0004
>  #define L2CAP_CONF_EFS_REJECT	0x0005
>  
> +/* conf req/rsp continuation flag */
> +#define L2CAP_CONF_FLAG_CONT_CLEARED	0
> +#define L2CAP_CONF_FLAG_CONT_SET	1
> +

this is misleading. Even while the only defined flag is currently the
continuation flag, it is clearly not limited to it.

I am fine with introducing L2CAP_CONF_FLAG_CONTINUATION or
L2CAP_CONF_FLAG_CONT, but we need to keep the no flags set part to 0 or
something else.

>  struct l2cap_conf_opt {
>  	__u8       type;
>  	__u8       len;
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index d9f215f..65e56d5 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -2544,7 +2544,7 @@ done:
>  	}
>  
>  	req->dcid  = cpu_to_le16(chan->dcid);
> -	req->flags = cpu_to_le16(0);
> +	req->flags = cpu_to_le16(L2CAP_CONF_FLAG_CONT_CLEARED);

And as a side note, this should be __constant_cpu_to_le16(0).
 
>  	return ptr - data;
>  }
> @@ -2764,7 +2764,7 @@ done:
>  	}
>  	rsp->scid   = cpu_to_le16(chan->dcid);
>  	rsp->result = cpu_to_le16(result);
> -	rsp->flags  = cpu_to_le16(0x0000);
> +	rsp->flags  = cpu_to_le16(L2CAP_CONF_FLAG_CONT_CLEARED);

Same here, __constant_cpu_to_le16(0) is the right call.
 
>  	return ptr - data;
>  }
> @@ -2863,7 +2863,7 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len, voi
>  	}
>  
>  	req->dcid   = cpu_to_le16(chan->dcid);
> -	req->flags  = cpu_to_le16(0x0000);
> +	req->flags  = cpu_to_le16(L2CAP_CONF_FLAG_CONT_CLEARED);

And here as well.

No idea on how we kept missing to change this all the time.

>  
>  	return ptr - data;
>  }
> @@ -3218,11 +3218,11 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>  	memcpy(chan->conf_req + chan->conf_len, req->data, len);
>  	chan->conf_len += len;
>  
> -	if (flags & 0x0001) {
> +	if (flags & L2CAP_CONF_FLAG_CONT_SET) {
>  		/* Incomplete config. Send empty response. */
>  		l2cap_send_cmd(conn, cmd->ident, L2CAP_CONF_RSP,
>  				l2cap_build_conf_rsp(chan, rsp,
> -					L2CAP_CONF_SUCCESS, 0x0001), rsp);
> +					L2CAP_CONF_SUCCESS, flags), rsp);

Here you are actually changing behavior. If flags ever has more than one
option, you are returning the other flags as well.

We might better restructure the code a little bit to make it more
readable.

>  		goto unlock;
>  	}
>  
> @@ -3369,7 +3369,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>  		goto done;
>  	}
>  
> -	if (flags & 0x01)
> +	if (flags & L2CAP_CONF_FLAG_CONT_SET)
>  		goto done;
>  
>  	set_bit(CONF_INPUT_DONE, &chan->conf_state);

Regards

Marcel

Re: [PATCH 2/2] Bluetooth: Define L2CAP conf continuation flag

[Andrei Emeltchenko]

Hi Marcel,

On Fri, May 18, 2012 at 09:55:00AM -0700, Marcel Holtmann wrote:
> > +/* conf req/rsp continuation flag */
> > +#define L2CAP_CONF_FLAG_CONT_CLEARED	0
> > +#define L2CAP_CONF_FLAG_CONT_SET	1
> > +
> 
> this is misleading. Even while the only defined flag is currently the
> continuation flag, it is clearly not limited to it.
> 
> I am fine with introducing L2CAP_CONF_FLAG_CONTINUATION

This looks good one.

> > -	req->flags = cpu_to_le16(0);
> > +	req->flags = cpu_to_le16(L2CAP_CONF_FLAG_CONT_CLEARED);
> 
> And as a side note, this should be __constant_cpu_to_le16(0).
>  
> > -	rsp->flags  = cpu_to_le16(0x0000);
> > +	rsp->flags  = cpu_to_le16(L2CAP_CONF_FLAG_CONT_CLEARED);
> 
> Same here, __constant_cpu_to_le16(0) is the right call.
>  
> > -	req->flags  = cpu_to_le16(0x0000);
> > +	req->flags  = cpu_to_le16(L2CAP_CONF_FLAG_CONT_CLEARED);
> 
> And here as well.
> 
> No idea on how we kept missing to change this all the time.

I will change this to the way you recommended.

> 
> >  
> >  	return ptr - data;
> >  }
> > @@ -3218,11 +3218,11 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> >  	memcpy(chan->conf_req + chan->conf_len, req->data, len);
> >  	chan->conf_len += len;
> >  
> > -	if (flags & 0x0001) {
> > +	if (flags & L2CAP_CONF_FLAG_CONT_SET) {
> >  		/* Incomplete config. Send empty response. */
> >  		l2cap_send_cmd(conn, cmd->ident, L2CAP_CONF_RSP,
> >  				l2cap_build_conf_rsp(chan, rsp,
> > -					L2CAP_CONF_SUCCESS, 0x0001), rsp);
> > +					L2CAP_CONF_SUCCESS, flags), rsp);
> 
> Here you are actually changing behavior. If flags ever has more than one
> option, you are returning the other flags as well.

I will create separate patch with this change. Currently there is only one
flag defined this is why I made such a simplifications in the patch. I
feel that using flags instead of 0x0001 looks more readable and shall be
the same.

Best regards 
Andrei Emeltchenko 

[PATCHv2 1/3] Bluetooth: Use defined link key size

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Remove magic number with defined link key size.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/hci.h      |    6 ++++--
 include/net/bluetooth/hci_core.h |    2 +-
 net/bluetooth/hci_core.c         |    2 +-
 net/bluetooth/hci_event.c        |    2 +-
 net/bluetooth/mgmt.c             |    2 +-
 5 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 66a7b57..4530e9d 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -30,6 +30,8 @@
 #define HCI_MAX_EVENT_SIZE	260
 #define HCI_MAX_FRAME_SIZE	(HCI_MAX_ACL_SIZE + 4)
 
+#define HCI_LINK_KEY_SIZE	16
+
 /* HCI dev events */
 #define HCI_DEV_REG			1
 #define HCI_DEV_UNREG			2
@@ -371,7 +373,7 @@ struct hci_cp_reject_conn_req {
 #define HCI_OP_LINK_KEY_REPLY		0x040b
 struct hci_cp_link_key_reply {
 	bdaddr_t bdaddr;
-	__u8     link_key[16];
+	__u8     link_key[HCI_LINK_KEY_SIZE];
 } __packed;
 
 #define HCI_OP_LINK_KEY_NEG_REPLY	0x040c
@@ -1048,7 +1050,7 @@ struct hci_ev_link_key_req {
 #define HCI_EV_LINK_KEY_NOTIFY		0x18
 struct hci_ev_link_key_notify {
 	bdaddr_t bdaddr;
-	__u8     link_key[16];
+	__u8     link_key[HCI_LINK_KEY_SIZE];
 	__u8     key_type;
 } __packed;
 
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 9fc7728..6c658fc 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -105,7 +105,7 @@ struct link_key {
 	struct list_head list;
 	bdaddr_t bdaddr;
 	u8 type;
-	u8 val[16];
+	u8 val[HCI_LINK_KEY_SIZE];
 	u8 pin_len;
 };
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d0a960d..4feecf3 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1291,7 +1291,7 @@ int hci_add_link_key(struct hci_dev *hdev, struct hci_conn *conn, int new_key,
 	}
 
 	bacpy(&key->bdaddr, bdaddr);
-	memcpy(key->val, val, 16);
+	memcpy(key->val, val, HCI_LINK_KEY_SIZE);
 	key->pin_len = pin_len;
 
 	if (type == HCI_LK_CHANGED_COMBINATION)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 6c2d7cc..1795c0c 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2739,7 +2739,7 @@ static inline void hci_link_key_request_evt(struct hci_dev *hdev,
 	}
 
 	bacpy(&cp.bdaddr, &ev->bdaddr);
-	memcpy(cp.link_key, key->val, 16);
+	memcpy(cp.link_key, key->val, HCI_LINK_KEY_SIZE);
 
 	hci_send_cmd(hdev, HCI_OP_LINK_KEY_REPLY, sizeof(cp), &cp);
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 6a7e926..1fd49e6 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2955,7 +2955,7 @@ int mgmt_new_link_key(struct hci_dev *hdev, struct link_key *key,
 	bacpy(&ev.key.addr.bdaddr, &key->bdaddr);
 	ev.key.addr.type = BDADDR_BREDR;
 	ev.key.type = key->type;
-	memcpy(ev.key.val, key->val, 16);
+	memcpy(ev.key.val, key->val, HCI_LINK_KEY_SIZE);
 	ev.key.pin_len = key->pin_len;
 
 	return mgmt_event(MGMT_EV_NEW_LINK_KEY, hdev, &ev, sizeof(ev), NULL);
-- 
1.7.9.5

[PATCHv2 2/3] Bluetooth: Define L2CAP conf continuation flag

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Define Continuation flag which the only flag used from Flags field
in L2CAP Configuration Request and Response.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/l2cap.h |    3 +++
 net/bluetooth/l2cap_core.c    |   10 +++++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 7bc4019..8980cbf 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -271,6 +271,9 @@ struct l2cap_conf_rsp {
 #define L2CAP_CONF_PENDING	0x0004
 #define L2CAP_CONF_EFS_REJECT	0x0005
 
+/* configuration req/rsp continuation flag */
+#define L2CAP_CONF_FLAG_CONTINUATION	0
+
 struct l2cap_conf_opt {
 	__u8       type;
 	__u8       len;
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index ae69da8..944c139 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2982,7 +2982,7 @@ done:
 	}
 
 	req->dcid  = cpu_to_le16(chan->dcid);
-	req->flags = cpu_to_le16(0);
+	req->flags = __constant_cpu_to_le16(0);
 
 	return ptr - data;
 }
@@ -3202,7 +3202,7 @@ done:
 	}
 	rsp->scid   = cpu_to_le16(chan->dcid);
 	rsp->result = cpu_to_le16(result);
-	rsp->flags  = cpu_to_le16(0x0000);
+	rsp->flags  = __constant_cpu_to_le16(0);
 
 	return ptr - data;
 }
@@ -3301,7 +3301,7 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len, voi
 	}
 
 	req->dcid   = cpu_to_le16(chan->dcid);
-	req->flags  = cpu_to_le16(0x0000);
+	req->flags  = __constant_cpu_to_le16(0);
 
 	return ptr - data;
 }
@@ -3656,7 +3656,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	memcpy(chan->conf_req + chan->conf_len, req->data, len);
 	chan->conf_len += len;
 
-	if (flags & 0x0001) {
+	if (flags & BIT(L2CAP_CONF_FLAG_CONTINUATION)) {
 		/* Incomplete config. Send empty response. */
 		l2cap_send_cmd(conn, cmd->ident, L2CAP_CONF_RSP,
 				l2cap_build_conf_rsp(chan, rsp,
@@ -3807,7 +3807,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 		goto done;
 	}
 
-	if (flags & 0x01)
+	if (flags & BIT(L2CAP_CONF_FLAG_CONTINUATION))
 		goto done;
 
 	set_bit(CONF_INPUT_DONE, &chan->conf_state);
-- 
1.7.9.5

[PATCHv2 3/3] Bluetooth: Preserve flags values

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Previous callers of l2cap_build_conf_rsp in l2cap_config_req use
flags instead of continuation flag hardcoded value. It does not change
logic and preserve future possible flags.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 944c139..cb817a0 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3660,7 +3660,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 		/* Incomplete config. Send empty response. */
 		l2cap_send_cmd(conn, cmd->ident, L2CAP_CONF_RSP,
 				l2cap_build_conf_rsp(chan, rsp,
-					L2CAP_CONF_SUCCESS, 0x0001), rsp);
+					L2CAP_CONF_SUCCESS, flags), rsp);
 		goto unlock;
 	}
 
@@ -3716,7 +3716,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 
 		l2cap_send_cmd(conn, cmd->ident, L2CAP_CONF_RSP,
 					l2cap_build_conf_rsp(chan, rsp,
-					L2CAP_CONF_SUCCESS, 0x0000), rsp);
+					L2CAP_CONF_SUCCESS, flags), rsp);
 	}
 
 unlock:
-- 
1.7.9.5

Re: [PATCH 1/2] Bluetooth: Use defined link key size

[Andrei Emeltchenko]

Hi Johan,

On Fri, May 18, 2012 at 05:40:44PM +0300, Johan Hedberg wrote:
> Hi Andrei,
> 
> On Fri, May 18, 2012, Andrei Emeltchenko wrote:
> > Remove magic number with defined link key size. This is especially
> > useful for calculation other keys which are 2 * key_size, etc that
> > makes usage of magic numbers too much.
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  include/net/bluetooth/bluetooth.h |    2 ++
> >  include/net/bluetooth/hci.h       |    4 ++--
> >  include/net/bluetooth/hci_core.h  |    2 +-
> >  net/bluetooth/hci_core.c          |    2 +-
> >  net/bluetooth/hci_event.c         |    2 +-
> >  net/bluetooth/mgmt.c              |    2 +-
> >  6 files changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index 961669b..4abedfe 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -68,6 +68,8 @@ struct bt_security {
> >  #define BT_SECURITY_MEDIUM	2
> >  #define BT_SECURITY_HIGH	3
> >  
> > +#define BT_SEC_LINK_KEY_SIZE		16
> 
> The patch seems ok'ish to me but I'd remove the _SEC part from the
> define name since it doesn't really buy you anything. 

Sent new version with a name proposed by Marcel.

> Also, I'm a bit
> confused by the commit message. What other keys are "2 * key_size"?

This is AMP key size which is calculated from link keys.

> There are no such occurrences in the patch itself so you should at least
> give some examples in the commit message.

I deleted this from the patch description.

Best regards 
Andrei Emeltchenko