[PATCHv3 0/5] Rebased AMP initialization patches

[PATCHv3 0/5] Rebased AMP initialization patches

[Emeltchenko Andrei]

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

Changes:
	v3: rebased unapplied patches
	v2: added helper function for block calculation, added patches for
	checking BREDR when going to Sniff mode and complete Read Local Version
	HCI command.
	v1: Changed HCI_<block,packet>_FLOW_CTL_MODE => HCI_FLOW_CTL_MODE_<block,packet>
	RFCv1: Initial version

AMP initialization and block flow control code.

Andrei Emeltchenko (5):
  Bluetooth: Split ctrl init to BREDR and AMP parts
  Bluetooth: Initialize default flow control mode
  Bluetooth: Check for flow control mode
  Bluetooth: Clean up magic pointers
  Bluetooth: Correct packet len calculation

 include/net/bluetooth/hci.h |   11 ++++++-
 net/bluetooth/hci_core.c    |   66 ++++++++++++++++++++++++++++++++-----------
 net/bluetooth/hci_event.c   |   23 ++++++++++-----
 3 files changed, 74 insertions(+), 26 deletions(-)

-- 
1.7.4.1

[PATCHv3 1/5] Bluetooth: Split ctrl init to BREDR and AMP parts

[Emeltchenko Andrei]

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

Current controller initialization is moved tp bredr_init and new
function added amp_init to handle later AMP init sequence. Current
AMP init sequence include Reset and Read Local Version.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/hci_core.c  |   64 ++++++++++++++++++++++++++++++++------------
 net/bluetooth/hci_event.c |    3 ++
 2 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d6382db..bb089e3 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -193,33 +193,18 @@ static void hci_reset_req(struct hci_dev *hdev, unsigned long opt)
 	hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
 }
 
-static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
+static void bredr_init(struct hci_dev *hdev)
 {
 	struct hci_cp_delete_stored_link_key cp;
-	struct sk_buff *skb;
 	__le16 param;
 	__u8 flt_type;
 
-	BT_DBG("%s %ld", hdev->name, opt);
-
-	/* Driver initialization */
-
-	/* Special commands */
-	while ((skb = skb_dequeue(&hdev->driver_init))) {
-		bt_cb(skb)->pkt_type = HCI_COMMAND_PKT;
-		skb->dev = (void *) hdev;
-
-		skb_queue_tail(&hdev->cmd_q, skb);
-		queue_work(hdev->workqueue, &hdev->cmd_work);
-	}
-	skb_queue_purge(&hdev->driver_init);
-
 	/* Mandatory initialization */
 
 	/* Reset */
 	if (!test_bit(HCI_QUIRK_NO_RESET, &hdev->quirks)) {
-			set_bit(HCI_RESET, &hdev->flags);
-			hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
+		set_bit(HCI_RESET, &hdev->flags);
+		hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
 	}
 
 	/* Read Local Supported Features */
@@ -258,6 +243,49 @@ static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
 	hci_send_cmd(hdev, HCI_OP_DELETE_STORED_LINK_KEY, sizeof(cp), &cp);
 }
 
+static void amp_init(struct hci_dev *hdev)
+{
+	/* Reset */
+	hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
+
+	/* Read Local Version */
+	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
+}
+
+static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
+{
+	struct sk_buff *skb;
+
+	BT_DBG("%s %ld", hdev->name, opt);
+
+	/* Driver initialization */
+
+	/* Special commands */
+	while ((skb = skb_dequeue(&hdev->driver_init))) {
+		bt_cb(skb)->pkt_type = HCI_COMMAND_PKT;
+		skb->dev = (void *) hdev;
+
+		skb_queue_tail(&hdev->cmd_q, skb);
+		queue_work(hdev->workqueue, &hdev->cmd_work);
+	}
+	skb_queue_purge(&hdev->driver_init);
+
+	switch (hdev->dev_type) {
+	case HCI_BREDR:
+		bredr_init(hdev);
+		break;
+
+	case HCI_AMP:
+		amp_init(hdev);
+		break;
+
+	default:
+		BT_ERR("Unknown device type %d", hdev->dev_type);
+		break;
+	}
+
+}
+
 static void hci_le_init_req(struct hci_dev *hdev, unsigned long opt)
 {
 	BT_DBG("%s", hdev->name);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 4275816..ceb7979 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -556,6 +556,9 @@ static void hci_set_le_support(struct hci_dev *hdev)
 
 static void hci_setup(struct hci_dev *hdev)
 {
+	if (hdev->dev_type != HCI_BREDR)
+		return;
+
 	hci_setup_event_mask(hdev);
 
 	if (hdev->hci_ver > BLUETOOTH_VER_1_1)
-- 
1.7.4.1

[PATCHv3 2/5] Bluetooth: Initialize default flow control mode

[Emeltchenko Andrei]

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

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 include/net/bluetooth/hci.h |    4 ++++
 net/bluetooth/hci_core.c    |    4 ++++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c019b27..4ab1880 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -280,6 +280,10 @@ enum {
 #define HCI_ERROR_LOCAL_HOST_TERM	0x16
 #define HCI_ERROR_PAIRING_NOT_ALLOWED	0x18
 
+/* Flow control modes */
+#define HCI_FLOW_CTL_MODE_PACKET_BASED	0x00
+#define HCI_FLOW_CTL_MODE_BLOCK_BASED	0x01
+
 /* -----  HCI Commands ---- */
 #define HCI_OP_NOP			0x0000
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index bb089e3..884eb85 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -199,6 +199,8 @@ static void bredr_init(struct hci_dev *hdev)
 	__le16 param;
 	__u8 flt_type;
 
+	hdev->flow_ctl_mode = HCI_FLOW_CTL_MODE_PACKET_BASED;
+
 	/* Mandatory initialization */
 
 	/* Reset */
@@ -245,6 +247,8 @@ static void bredr_init(struct hci_dev *hdev)
 
 static void amp_init(struct hci_dev *hdev)
 {
+	hdev->flow_ctl_mode = HCI_FLOW_CTL_MODE_BLOCK_BASED;
+
 	/* Reset */
 	hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
 
-- 
1.7.4.1

[PATCHv3 3/5] Bluetooth: Check for flow control mode

[Emeltchenko Andrei]

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

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/hci_event.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index ceb7979..5138caf 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2271,6 +2271,11 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
 
 	BT_DBG("%s num_hndl %d", hdev->name, ev->num_hndl);
 
+	if (hdev->flow_ctl_mode != HCI_FLOW_CTL_MODE_PACKET_BASED) {
+		BT_ERR("Wrong event for mode %d", hdev->flow_ctl_mode);
+		return;
+	}
+
 	if (skb->len < ev->num_hndl * 4) {
 		BT_DBG("%s bad parameters", hdev->name);
 		return;
-- 
1.7.4.1

[PATCHv3 4/5] Bluetooth: Clean up magic pointers

[Emeltchenko Andrei]

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

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 include/net/bluetooth/hci.h |    7 ++++++-
 net/bluetooth/hci_event.c   |    8 ++++----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 4ab1880..5b2fed5 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -982,9 +982,14 @@ struct hci_ev_role_change {
 } __packed;
 
 #define HCI_EV_NUM_COMP_PKTS		0x13
+struct hci_comp_pkts_info {
+	__le16   handle;
+	__le16   count;
+} __packed;
+
 struct hci_ev_num_comp_pkts {
 	__u8     num_hndl;
-	/* variable length part */
+	struct hci_comp_pkts_info handles[0];
 } __packed;
 
 #define HCI_EV_MODE_CHANGE		0x14
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 5138caf..919e3c0 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2264,7 +2264,6 @@ static inline void hci_role_change_evt(struct hci_dev *hdev, struct sk_buff *skb
 static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_ev_num_comp_pkts *ev = (void *) skb->data;
-	__le16 *ptr;
 	int i;
 
 	skb_pull(skb, sizeof(*ev));
@@ -2281,12 +2280,13 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
 		return;
 	}
 
-	for (i = 0, ptr = (__le16 *) skb->data; i < ev->num_hndl; i++) {
+	for (i = 0; i < ev->num_hndl; i++) {
+		struct hci_comp_pkts_info *info = &ev->handles[i];
 		struct hci_conn *conn;
 		__u16  handle, count;
 
-		handle = get_unaligned_le16(ptr++);
-		count  = get_unaligned_le16(ptr++);
+		handle = __le16_to_cpu(info->handle);
+		count  = __le16_to_cpu(info->count);
 
 		conn = hci_conn_hash_lookup_handle(hdev, handle);
 		if (!conn)
-- 
1.7.4.1

Re: [PATCHv3 4/5] Bluetooth: Clean up magic pointers

[Gustavo Padovan]

Hi Andrei,

* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2011-12-19 16:31:30 +0200]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> Acked-by: Marcel Holtmann <marcel@holtmann.org>
> ---
>  include/net/bluetooth/hci.h |    7 ++++++-
>  net/bluetooth/hci_event.c   |    8 ++++----
>  2 files changed, 10 insertions(+), 5 deletions(-)

patches 1 to 4 applied, thanks.

	Gustavo

[PATCHv3 5/5] Bluetooth: Correct packet len calculation

[Emeltchenko Andrei]

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

Remove unneeded skb_pull and correct packet length calculation
removing magic number. Move BT_DBG after len check otherwise
it could possibly access wrong memory.

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

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 919e3c0..47e1476 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2266,20 +2266,19 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
 	struct hci_ev_num_comp_pkts *ev = (void *) skb->data;
 	int i;
 
-	skb_pull(skb, sizeof(*ev));
-
-	BT_DBG("%s num_hndl %d", hdev->name, ev->num_hndl);
-
 	if (hdev->flow_ctl_mode != HCI_FLOW_CTL_MODE_PACKET_BASED) {
 		BT_ERR("Wrong event for mode %d", hdev->flow_ctl_mode);
 		return;
 	}
 
-	if (skb->len < ev->num_hndl * 4) {
+	if (skb->len < sizeof(*ev) || skb->len < sizeof(*ev) +
+			ev->num_hndl * sizeof(struct hci_comp_pkts_info)) {
 		BT_DBG("%s bad parameters", hdev->name);
 		return;
 	}
 
+	BT_DBG("%s num_hndl %d", hdev->name, ev->num_hndl);
+
 	for (i = 0; i < ev->num_hndl; i++) {
 		struct hci_comp_pkts_info *info = &ev->handles[i];
 		struct hci_conn *conn;
-- 
1.7.4.1

Re: [PATCHv3 5/5] Bluetooth: Correct packet len calculation

[Marcel Holtmann]

Hi Andrei,

> Remove unneeded skb_pull and correct packet length calculation
> removing magic number. Move BT_DBG after len check otherwise
> it could possibly access wrong memory.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/hci_event.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCHv3 5/5] Bluetooth: Correct packet len calculation

[Gustavo Padovan]

Hi Andrei,

* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2011-12-19 16:31:31 +0200]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> Remove unneeded skb_pull and correct packet length calculation
> removing magic number. Move BT_DBG after len check otherwise
> it could possibly access wrong memory.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/hci_event.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 919e3c0..47e1476 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2266,20 +2266,19 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
>  	struct hci_ev_num_comp_pkts *ev = (void *) skb->data;
>  	int i;
>  
> -	skb_pull(skb, sizeof(*ev));
> -
> -	BT_DBG("%s num_hndl %d", hdev->name, ev->num_hndl);
> -
>  	if (hdev->flow_ctl_mode != HCI_FLOW_CTL_MODE_PACKET_BASED) {
>  		BT_ERR("Wrong event for mode %d", hdev->flow_ctl_mode);
>  		return;
>  	}
>  
> -	if (skb->len < ev->num_hndl * 4) {
> +	if (skb->len < sizeof(*ev) || skb->len < sizeof(*ev) +
> +			ev->num_hndl * sizeof(struct hci_comp_pkts_info)) {

I think you can remove the first part of this check.

	Gustavo

Re: [PATCHv3 5/5] Bluetooth: Correct packet len calculation

[Marcel Holtmann]

Hi Gustavo,

> > Remove unneeded skb_pull and correct packet length calculation
> > removing magic number. Move BT_DBG after len check otherwise
> > it could possibly access wrong memory.
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  net/bluetooth/hci_event.c |    9 ++++-----
> >  1 files changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 919e3c0..47e1476 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -2266,20 +2266,19 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
> >  	struct hci_ev_num_comp_pkts *ev = (void *) skb->data;
> >  	int i;
> >  
> > -	skb_pull(skb, sizeof(*ev));
> > -
> > -	BT_DBG("%s num_hndl %d", hdev->name, ev->num_hndl);
> > -
> >  	if (hdev->flow_ctl_mode != HCI_FLOW_CTL_MODE_PACKET_BASED) {
> >  		BT_ERR("Wrong event for mode %d", hdev->flow_ctl_mode);
> >  		return;
> >  	}
> >  
> > -	if (skb->len < ev->num_hndl * 4) {
> > +	if (skb->len < sizeof(*ev) || skb->len < sizeof(*ev) +
> > +			ev->num_hndl * sizeof(struct hci_comp_pkts_info)) {
> 
> I think you can remove the first part of this check.

you are accessing ev->num_handl, so you need to ensure that you have at
least ev->num_handl size of data in your SKB. So this code is correct.

Regards

Marcel

Re: [PATCHv3 5/5] Bluetooth: Correct packet len calculation

[Emeltchenko Andrei]

Hi Gustavo,

On Tue, Dec 20, 2011 at 01:02:57PM -0800, Marcel Holtmann wrote:
> > > Remove unneeded skb_pull and correct packet length calculation
> > > removing magic number. Move BT_DBG after len check otherwise
> > > it could possibly access wrong memory.
> > > 
> > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > ---
> > >  net/bluetooth/hci_event.c |    9 ++++-----
> > >  1 files changed, 4 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > index 919e3c0..47e1476 100644
> > > --- a/net/bluetooth/hci_event.c
> > > +++ b/net/bluetooth/hci_event.c
> > > @@ -2266,20 +2266,19 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
> > >  	struct hci_ev_num_comp_pkts *ev = (void *) skb->data;
> > >  	int i;
> > >  
> > > -	skb_pull(skb, sizeof(*ev));
> > > -
> > > -	BT_DBG("%s num_hndl %d", hdev->name, ev->num_hndl);
> > > -
> > >  	if (hdev->flow_ctl_mode != HCI_FLOW_CTL_MODE_PACKET_BASED) {
> > >  		BT_ERR("Wrong event for mode %d", hdev->flow_ctl_mode);
> > >  		return;
> > >  	}
> > >  
> > > -	if (skb->len < ev->num_hndl * 4) {
> > > +	if (skb->len < sizeof(*ev) || skb->len < sizeof(*ev) +
> > > +			ev->num_hndl * sizeof(struct hci_comp_pkts_info)) {
> > 
> > I think you can remove the first part of this check.
> 
> you are accessing ev->num_handl, so you need to ensure that you have at
> least ev->num_handl size of data in your SKB. So this code is correct.

BTW: I resent the remaining patch from this series. Cannot add more to
Marcel's comment above.

Best regards 
Andrei Emeltchenko