2011-12-08 12:53:00

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCHv1 0/6] Implement basic HCI block-based flow control

From: Andrei Emeltchenko <[email protected]>

Changes:
PATCHv1: added check for packet len before accessing it.
RFCv2: taken Marcel's comments: added check for flow control,
removed magic pointers and numbers, added SCO type check.

Simplify current handling of number of completed packets.
Adds basic HCI commands and event handlers related to data block
flow control.

Andrei Emeltchenko (6):
Bluetooth: Add HCI Read Data Block Size function
Bluetooth: Simplify num_comp_pkts_evt function
Bluetooth: Check for flow control mode
Bluetooth: Clean up magic pointers
Bluetooth: Correct packet len calculation
Bluetooth: Process num completed data blocks event

include/net/bluetooth/hci.h | 28 +++++++-
include/net/bluetooth/hci_core.h | 5 +
net/bluetooth/hci_event.c | 151 +++++++++++++++++++++++++++++++-------
3 files changed, 157 insertions(+), 27 deletions(-)

--
1.7.4.1



2011-12-18 23:55:12

by Gustavo Padovan

[permalink] [raw]
Subject: Re: [PATCHv1 3/6] Bluetooth: Check for flow control mode

Hi Andrei,

* Gustavo Padovan <[email protected]> [2011-12-18 21:47:35 -0200]:

> Hi Andrei,
>
> * Emeltchenko Andrei <[email protected]> [2011-12-08 14:53:03 +0200]:
>
> > From: Andrei Emeltchenko <[email protected]>
> >
> > Signed-off-by: Andrei Emeltchenko <[email protected]>
> > Acked-by: Marcel Holtmann <[email protected]>
> > ---
> > net/bluetooth/hci_event.c | 5 +++++
> > 1 files changed, 5 insertions(+), 0 deletions(-)
>
> Found the new ones! This one is applied, and the others need rebase against
> bluetooth-next. Thanks.

net/bluetooth/hci_event.c: In function ‘hci_num_comp_pkts_evt’:
net/bluetooth/hci_event.c:2263:29: error: ‘HCI_FLOW_CTL_MODE_PACKET_BASED’
undeclared (first use in this function)

I remove this patch, please fix this.

Gustavo

2011-12-18 23:47:35

by Gustavo Padovan

[permalink] [raw]
Subject: Re: [PATCHv1 3/6] Bluetooth: Check for flow control mode

Hi Andrei,

* Emeltchenko Andrei <[email protected]> [2011-12-08 14:53:03 +0200]:

> From: Andrei Emeltchenko <[email protected]>
>
> Signed-off-by: Andrei Emeltchenko <[email protected]>
> Acked-by: Marcel Holtmann <[email protected]>
> ---
> net/bluetooth/hci_event.c | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)

Found the new ones! This one is applied, and the others need rebase against
bluetooth-next. Thanks.

Gustavo

2011-12-08 14:56:00

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCHv1 0/6] Implement basic HCI block-based flow control

Hi Gustavo,

> Changes:
> PATCHv1: added check for packet len before accessing it.
> RFCv2: taken Marcel's comments: added check for flow control,
> removed magic pointers and numbers, added SCO type check.
>
> Simplify current handling of number of completed packets.
> Adds basic HCI commands and event handlers related to data block
> flow control.
>
> Andrei Emeltchenko (6):
> Bluetooth: Add HCI Read Data Block Size function
> Bluetooth: Simplify num_comp_pkts_evt function
> Bluetooth: Check for flow control mode
> Bluetooth: Clean up magic pointers
> Bluetooth: Correct packet len calculation
> Bluetooth: Process num completed data blocks event
>
> include/net/bluetooth/hci.h | 28 +++++++-
> include/net/bluetooth/hci_core.h | 5 +
> net/bluetooth/hci_event.c | 151 +++++++++++++++++++++++++++++++-------
> 3 files changed, 157 insertions(+), 27 deletions(-)

this whole set is fine now. So ACK.

Regards

Marcel



2011-12-08 14:55:13

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCHv1 6/6] Bluetooth: Process num completed data blocks event

Hi Andrei,

> Adds support for Number Of Completed Data Blocks Event. Highly
> modified version of Code Aurora code.
>
> Signed-off-by: Andrei Emeltchenko <[email protected]>
> ---
> include/net/bluetooth/hci.h | 13 +++++++++
> net/bluetooth/hci_event.c | 58 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 71 insertions(+), 0 deletions(-)

Acked-by: Marcel Holtmann <[email protected]>

Regards

Marcel



2011-12-08 14:54:43

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCHv1 5/6] Bluetooth: Correct packet len calculation

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 <[email protected]>
> ---
> net/bluetooth/hci_event.c | 9 ++++-----
> 1 files changed, 4 insertions(+), 5 deletions(-)

Acked-by: Marcel Holtmann <[email protected]>

Regards

Marcel



2011-12-08 12:53:04

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCHv1 4/6] Bluetooth: Clean up magic pointers

From: Andrei Emeltchenko <[email protected]>

Signed-off-by: Andrei Emeltchenko <[email protected]>
Acked-by: Marcel Holtmann <[email protected]>
---
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 9490c2c..b9c0087 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -981,9 +981,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 a06cc60..670903b 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2266,7 +2266,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));
@@ -2285,12 +2284,13 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s

tasklet_disable(&hdev->tx_task);

- 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


2011-12-08 12:53:06

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCHv1 6/6] Bluetooth: Process num completed data blocks event

From: Andrei Emeltchenko <[email protected]>

Adds support for Number Of Completed Data Blocks Event. Highly
modified version of Code Aurora code.

Signed-off-by: Andrei Emeltchenko <[email protected]>
---
include/net/bluetooth/hci.h | 13 +++++++++
net/bluetooth/hci_event.c | 58 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index b9c0087..6846f3e 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1154,6 +1154,19 @@ struct hci_ev_le_meta {
__u8 subevent;
} __packed;

+#define HCI_EV_NUM_COMP_BLOCKS 0x48
+struct hci_comp_blocks_info {
+ __le16 handle;
+ __le16 pkts;
+ __le16 blocks;
+} __packed;
+
+struct hci_ev_num_comp_blocks {
+ __le16 num_blocks;
+ __u8 num_hndl;
+ struct hci_comp_blocks_info handles[0];
+} __packed;
+
/* Low energy meta events */
#define HCI_EV_LE_CONN_COMPLETE 0x01
struct hci_ev_le_conn_complete {
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index c96d580..403ddd5 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2333,6 +2333,60 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
tasklet_enable(&hdev->tx_task);
}

+static inline void hci_num_comp_blocks_evt(struct hci_dev *hdev,
+ struct sk_buff *skb)
+{
+ struct hci_ev_num_comp_blocks *ev = (void *) skb->data;
+ int i;
+
+ if (hdev->flow_ctl_mode != HCI_FLOW_CTL_MODE_BLOCK_BASED) {
+ BT_ERR("Wrong event for mode %d", hdev->flow_ctl_mode);
+ return;
+ }
+
+ if (skb->len < sizeof(*ev) || skb->len < sizeof(*ev) +
+ ev->num_hndl * sizeof(struct hci_comp_blocks_info)) {
+ BT_DBG("%s bad parameters", hdev->name);
+ return;
+ }
+
+ BT_DBG("%s num_blocks %d num_hndl %d", hdev->name, ev->num_blocks,
+ ev->num_hndl);
+
+ tasklet_disable(&hdev->tx_task);
+
+ for (i = 0; i < ev->num_hndl; i++) {
+ struct hci_comp_blocks_info *info = &ev->handles[i];
+ struct hci_conn *conn;
+ __u16 handle, block_count;
+
+ handle = __le16_to_cpu(info->handle);
+ block_count = __le16_to_cpu(info->blocks);
+
+ conn = hci_conn_hash_lookup_handle(hdev, handle);
+ if (!conn)
+ continue;
+
+ conn->sent -= block_count;
+
+ switch (conn->type) {
+ case ACL_LINK:
+ hdev->block_cnt += block_count;
+ if (hdev->block_cnt > hdev->num_blocks)
+ hdev->block_cnt = hdev->num_blocks;
+ break;
+
+ default:
+ BT_ERR("Unknown type %d conn %p", conn->type, conn);
+ break;
+ }
+ }
+
+ tasklet_schedule(&hdev->tx_task);
+
+ tasklet_enable(&hdev->tx_task);
+}
+
static inline void hci_mode_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
{
struct hci_ev_mode_change *ev = (void *) skb->data;
@@ -3272,6 +3326,10 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
hci_remote_oob_data_request_evt(hdev, skb);
break;

+ case HCI_EV_NUM_COMP_BLOCKS:
+ hci_num_comp_blocks_evt(hdev, skb);
+ break;
+
default:
BT_DBG("%s event 0x%x", hdev->name, event);
break;
--
1.7.4.1


2011-12-08 12:53:05

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCHv1 5/6] Bluetooth: Correct packet len calculation

From: Andrei Emeltchenko <[email protected]>

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 <[email protected]>
---
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 670903b..c96d580 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2268,20 +2268,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);
+
tasklet_disable(&hdev->tx_task);

for (i = 0; i < ev->num_hndl; i++) {
--
1.7.4.1


2011-12-08 12:53:02

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCHv1 2/6] Bluetooth: Simplify num_comp_pkts_evt function

From: Andrei Emeltchenko <[email protected]>

Simplify function and remove fourth level of indentation.

Signed-off-by: Andrei Emeltchenko <[email protected]>
Acked-by: Marcel Holtmann <[email protected]>
---
net/bluetooth/hci_event.c | 47 +++++++++++++++++++++++++++-----------------
1 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 273e1cb..493e08b 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2288,28 +2288,39 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
count = get_unaligned_le16(ptr++);

conn = hci_conn_hash_lookup_handle(hdev, handle);
- if (conn) {
- conn->sent -= count;
-
- if (conn->type == ACL_LINK) {
+ if (!conn)
+ continue;
+
+ conn->sent -= count;
+
+ switch (conn->type) {
+ case ACL_LINK:
+ hdev->acl_cnt += count;
+ if (hdev->acl_cnt > hdev->acl_pkts)
+ hdev->acl_cnt = hdev->acl_pkts;
+ break;
+
+ case LE_LINK:
+ if (hdev->le_pkts) {
+ hdev->le_cnt += count;
+ if (hdev->le_cnt > hdev->le_pkts)
+ hdev->le_cnt = hdev->le_pkts;
+ } else {
hdev->acl_cnt += count;
if (hdev->acl_cnt > hdev->acl_pkts)
hdev->acl_cnt = hdev->acl_pkts;
- } else if (conn->type == LE_LINK) {
- if (hdev->le_pkts) {
- hdev->le_cnt += count;
- if (hdev->le_cnt > hdev->le_pkts)
- hdev->le_cnt = hdev->le_pkts;
- } else {
- hdev->acl_cnt += count;
- if (hdev->acl_cnt > hdev->acl_pkts)
- hdev->acl_cnt = hdev->acl_pkts;
- }
- } else {
- hdev->sco_cnt += count;
- if (hdev->sco_cnt > hdev->sco_pkts)
- hdev->sco_cnt = hdev->sco_pkts;
}
+ break;
+
+ case SCO_LINK:
+ hdev->sco_cnt += count;
+ if (hdev->sco_cnt > hdev->sco_pkts)
+ hdev->sco_cnt = hdev->sco_pkts;
+ break;
+
+ default:
+ BT_ERR("Unknown type %d conn %p", conn->type, conn);
+ break;
}
}

--
1.7.4.1


2011-12-08 12:53:03

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCHv1 3/6] Bluetooth: Check for flow control mode

From: Andrei Emeltchenko <[email protected]>

Signed-off-by: Andrei Emeltchenko <[email protected]>
Acked-by: Marcel Holtmann <[email protected]>
---
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 493e08b..a06cc60 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2273,6 +2273,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


2011-12-08 12:53:01

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCHv1 1/6] Bluetooth: Add HCI Read Data Block Size function

From: Andrei Emeltchenko <[email protected]>

Implement block size read function. Use different variables for
packet-based and block-based flow control.

Signed-off-by: Andrei Emeltchenko <[email protected]>
Acked-by: Marcel Holtmann <[email protected]>
---
include/net/bluetooth/hci.h | 8 ++++++++
include/net/bluetooth/hci_core.h | 5 +++++
net/bluetooth/hci_event.c | 26 ++++++++++++++++++++++++++
3 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 12649d0..9490c2c 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -749,6 +749,14 @@ struct hci_rp_read_bd_addr {
bdaddr_t bdaddr;
} __packed;

+#define HCI_OP_READ_DATA_BLOCK_SIZE 0x100a
+struct hci_rp_read_data_block_size {
+ __u8 status;
+ __le16 max_acl_len;
+ __le16 block_len;
+ __le16 num_blocks;
+} __packed;
+
#define HCI_OP_WRITE_PAGE_SCAN_ACTIVITY 0x0c1c
struct hci_cp_write_page_scan_activity {
__le16 interval;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e34cd71..4c61161 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -188,6 +188,11 @@ struct hci_dev {
unsigned int sco_pkts;
unsigned int le_pkts;

+ __u16 block_len;
+ __u16 block_mtu;
+ __u16 num_blocks;
+ __u16 block_cnt;
+
unsigned long acl_last_tx;
unsigned long sco_last_tx;
unsigned long le_last_tx;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index afd4731..273e1cb 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -776,6 +776,28 @@ static void hci_cc_read_bd_addr(struct hci_dev *hdev, struct sk_buff *skb)
hci_req_complete(hdev, HCI_OP_READ_BD_ADDR, rp->status);
}

+static void hci_cc_read_data_block_size(struct hci_dev *hdev,
+ struct sk_buff *skb)
+{
+ struct hci_rp_read_data_block_size *rp = (void *) skb->data;
+
+ BT_DBG("%s status 0x%x", hdev->name, rp->status);
+
+ if (rp->status)
+ return;
+
+ hdev->block_mtu = __le16_to_cpu(rp->max_acl_len);
+ hdev->block_len = __le16_to_cpu(rp->block_len);
+ hdev->num_blocks = __le16_to_cpu(rp->num_blocks);
+
+ hdev->block_cnt = hdev->num_blocks;
+
+ BT_DBG("%s blk mtu %d cnt %d len %d", hdev->name, hdev->block_mtu,
+ hdev->block_cnt, hdev->block_len);
+
+ hci_req_complete(hdev, HCI_OP_READ_DATA_BLOCK_SIZE, rp->status);
+}
+
static void hci_cc_write_ca_timeout(struct hci_dev *hdev, struct sk_buff *skb)
{
__u8 status = *((__u8 *) skb->data);
@@ -2031,6 +2053,10 @@ static inline void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *sk
hci_cc_read_bd_addr(hdev, skb);
break;

+ case HCI_OP_READ_DATA_BLOCK_SIZE:
+ hci_cc_read_data_block_size(hdev, skb);
+ break;
+
case HCI_OP_WRITE_CA_TIMEOUT:
hci_cc_write_ca_timeout(hdev, skb);
break;
--
1.7.4.1