* [RFCv1 1/3] Bluetooth: Add HCI Read Data Block Size function
2011-12-05 12:55 [RFCv1 0/3] Implement basic block flow control code Emeltchenko Andrei
@ 2011-12-05 12:55 ` Emeltchenko Andrei
2011-12-05 13:55 ` Marcel Holtmann
2011-12-05 12:55 ` [RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function Emeltchenko Andrei
2011-12-05 12:55 ` [RFCv1 3/3] Bluetooth: Process num completed data blocks event Emeltchenko Andrei
2 siblings, 1 reply; 12+ messages in thread
From: Emeltchenko Andrei @ 2011-12-05 12:55 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Implement block size read function. Use different variables for
packet-based and block-based flow control.
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function
2011-12-05 12:55 [RFCv1 0/3] Implement basic block flow control code Emeltchenko Andrei
2011-12-05 12:55 ` [RFCv1 1/3] Bluetooth: Add HCI Read Data Block Size function Emeltchenko Andrei
@ 2011-12-05 12:55 ` Emeltchenko Andrei
2011-12-05 13:51 ` Marcel Holtmann
2011-12-05 12:55 ` [RFCv1 3/3] Bluetooth: Process num completed data blocks event Emeltchenko Andrei
2 siblings, 1 reply; 12+ messages in thread
From: Emeltchenko Andrei @ 2011-12-05 12:55 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Simplify function and remove fourth level of indentation.
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
net/bluetooth/hci_event.c | 43 +++++++++++++++++++++++++------------------
1 files changed, 25 insertions(+), 18 deletions(-)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 273e1cb..fd7e6b5 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2288,28 +2288,35 @@ 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;
+
+ default:
+ hdev->sco_cnt += count;
+ if (hdev->sco_cnt > hdev->sco_pkts)
+ hdev->sco_cnt = hdev->sco_pkts;
+ break;
}
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function
2011-12-05 12:55 ` [RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function Emeltchenko Andrei
@ 2011-12-05 13:51 ` Marcel Holtmann
2011-12-05 15:32 ` Emeltchenko Andrei
0 siblings, 1 reply; 12+ messages in thread
From: Marcel Holtmann @ 2011-12-05 13:51 UTC (permalink / raw)
To: Emeltchenko Andrei; +Cc: linux-bluetooth
Hi Andrei,
> Simplify function and remove fourth level of indentation.
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
> net/bluetooth/hci_event.c | 43 +++++++++++++++++++++++++------------------
> 1 files changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 273e1cb..fd7e6b5 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2288,28 +2288,35 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
> count = get_unaligned_le16(ptr++);
actually we first need a cleanup of this function. This part is
horrible. It works because of our ptr cast to uint16, but we should
actually use a proper struct here. Maybe an info struct like we use for
inquiry result might be helpful here.
It also needs a check that we are in packet based flow control. If not,
we need to just ignore this event and print an error message.
> 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;
> +
> + default:
> + hdev->sco_cnt += count;
> + if (hdev->sco_cnt > hdev->sco_pkts)
> + hdev->sco_cnt = hdev->sco_pkts;
> + break;
And this should be under SCO_LINK and default should print an error.
> }
> }
>
Regards
Marcel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function
2011-12-05 13:51 ` Marcel Holtmann
@ 2011-12-05 15:32 ` Emeltchenko Andrei
2011-12-05 18:45 ` Marcel Holtmann
0 siblings, 1 reply; 12+ messages in thread
From: Emeltchenko Andrei @ 2011-12-05 15:32 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Mon, Dec 05, 2011 at 02:51:34PM +0100, Marcel Holtmann wrote:
> Hi Andrei,
>
> > Simplify function and remove fourth level of indentation.
> >
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> > net/bluetooth/hci_event.c | 43 +++++++++++++++++++++++++------------------
> > 1 files changed, 25 insertions(+), 18 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 273e1cb..fd7e6b5 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -2288,28 +2288,35 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
> > count = get_unaligned_le16(ptr++);
>
> actually we first need a cleanup of this function. This part is
> horrible. It works because of our ptr cast to uint16, but we should
> actually use a proper struct here. Maybe an info struct like we use for
> inquiry result might be helpful here.
Something like the code below?
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index e33e468..403974c 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1018,9 +1018,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 238fb65..079b94f 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2273,26 +2273,27 @@ 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;
+ struct hci_comp_pkts_info *info;
int i;
skb_pull(skb, sizeof(*ev));
BT_DBG("%s num_hndl %d", hdev->name, ev->num_hndl);
- if (skb->len < ev->num_hndl * 4) {
+ if (skb->len < ev->num_hndl * sizeof(*info)) {
BT_DBG("%s bad parameters", hdev->name);
return;
}
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_conn *conn;
__u16 handle, count;
- handle = get_unaligned_le16(ptr++);
- count = get_unaligned_le16(ptr++);
+ info = &ev->handles[i];
+ handle = __le16_to_cpu(info->handle);
+ count = __le16_to_cpu(info->count);
conn = hci_conn_hash_lookup_handle(hdev, handle);
if (!conn)
> It also needs a check that we are in packet based flow control. If not,
> we need to just ignore this event and print an error message.
will add.
>
> > 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;
> > +
> > + default:
> > + hdev->sco_cnt += count;
> > + if (hdev->sco_cnt > hdev->sco_pkts)
> > + hdev->sco_cnt = hdev->sco_pkts;
> > + break;
>
> And this should be under SCO_LINK and default should print an error.
will fix
Best regards
Andrei Emeltchenko
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function
2011-12-05 15:32 ` Emeltchenko Andrei
@ 2011-12-05 18:45 ` Marcel Holtmann
2011-12-07 8:01 ` Emeltchenko Andrei
0 siblings, 1 reply; 12+ messages in thread
From: Marcel Holtmann @ 2011-12-05 18:45 UTC (permalink / raw)
To: Emeltchenko Andrei; +Cc: linux-bluetooth
Hi Andrei,
> > > Simplify function and remove fourth level of indentation.
> > >
> > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > ---
> > > net/bluetooth/hci_event.c | 43 +++++++++++++++++++++++++------------------
> > > 1 files changed, 25 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > index 273e1cb..fd7e6b5 100644
> > > --- a/net/bluetooth/hci_event.c
> > > +++ b/net/bluetooth/hci_event.c
> > > @@ -2288,28 +2288,35 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
> > > count = get_unaligned_le16(ptr++);
> >
> > actually we first need a cleanup of this function. This part is
> > horrible. It works because of our ptr cast to uint16, but we should
> > actually use a proper struct here. Maybe an info struct like we use for
> > inquiry result might be helpful here.
>
> Something like the code below?
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index e33e468..403974c 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1018,9 +1018,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 238fb65..079b94f 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2273,26 +2273,27 @@ 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;
> + struct hci_comp_pkts_info *info;
> int i;
>
> skb_pull(skb, sizeof(*ev));
>
> BT_DBG("%s num_hndl %d", hdev->name, ev->num_hndl);
>
> - if (skb->len < ev->num_hndl * 4) {
> + if (skb->len < ev->num_hndl * sizeof(*info)) {
> BT_DBG("%s bad parameters", hdev->name);
> return;
> }
>
> 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++);
> + info = &ev->handles[i];
> + handle = __le16_to_cpu(info->handle);
> + count = __le16_to_cpu(info->count);
>
> conn = hci_conn_hash_lookup_handle(hdev, handle);
> if (!conn)
And you need to remove the skb_pull above since otherwise you are
pointing to the wrong memory location.
Regards
Marcel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function
2011-12-05 18:45 ` Marcel Holtmann
@ 2011-12-07 8:01 ` Emeltchenko Andrei
2011-12-07 8:20 ` Marcel Holtmann
0 siblings, 1 reply; 12+ messages in thread
From: Emeltchenko Andrei @ 2011-12-07 8:01 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Mon, Dec 05, 2011 at 07:45:49PM +0100, Marcel Holtmann wrote:
> Hi Andrei,
>
> > > > Simplify function and remove fourth level of indentation.
> > > >
> > > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > > ---
> > > > net/bluetooth/hci_event.c | 43 +++++++++++++++++++++++++------------------
> > > > 1 files changed, 25 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > > index 273e1cb..fd7e6b5 100644
> > > > --- a/net/bluetooth/hci_event.c
> > > > +++ b/net/bluetooth/hci_event.c
> > > > @@ -2288,28 +2288,35 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
> > > > count = get_unaligned_le16(ptr++);
> > >
> > > actually we first need a cleanup of this function. This part is
> > > horrible. It works because of our ptr cast to uint16, but we should
> > > actually use a proper struct here. Maybe an info struct like we use for
> > > inquiry result might be helpful here.
> >
> > Something like the code below?
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index e33e468..403974c 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -1018,9 +1018,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 238fb65..079b94f 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -2273,26 +2273,27 @@ 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;
> > + struct hci_comp_pkts_info *info;
> > int i;
> >
> > skb_pull(skb, sizeof(*ev));
> >
> > BT_DBG("%s num_hndl %d", hdev->name, ev->num_hndl);
> >
> > - if (skb->len < ev->num_hndl * 4) {
> > + if (skb->len < ev->num_hndl * sizeof(*info)) {
> > BT_DBG("%s bad parameters", hdev->name);
> > return;
> > }
> >
> > 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];
I was actually using "info" to eliminate magic size above, otherwise I have
to use long "sizeof(struct hci_comp_pkts_info)". This of course might be
done in a separate trivial patch.
>
> > struct hci_conn *conn;
> > __u16 handle, count;
> >
> > - handle = get_unaligned_le16(ptr++);
> > - count = get_unaligned_le16(ptr++);
> > + info = &ev->handles[i];
> > + handle = __le16_to_cpu(info->handle);
> > + count = __le16_to_cpu(info->count);
> >
> > conn = hci_conn_hash_lookup_handle(hdev, handle);
> > if (!conn)
>
> And you need to remove the skb_pull above since otherwise you are
> pointing to the wrong memory location.
Sorry, cannot get this. I assign "ev" before skb_pull, this should not be a
problem.
Best regards
Andrei Emeltchenko
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function
2011-12-07 8:01 ` Emeltchenko Andrei
@ 2011-12-07 8:20 ` Marcel Holtmann
2011-12-07 8:49 ` Emeltchenko Andrei
0 siblings, 1 reply; 12+ messages in thread
From: Marcel Holtmann @ 2011-12-07 8:20 UTC (permalink / raw)
To: Emeltchenko Andrei; +Cc: linux-bluetooth
Hi Andrei,
> > > > > Simplify function and remove fourth level of indentation.
> > > > >
> > > > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > > > ---
> > > > > net/bluetooth/hci_event.c | 43 +++++++++++++++++++++++++------------------
> > > > > 1 files changed, 25 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > > > index 273e1cb..fd7e6b5 100644
> > > > > --- a/net/bluetooth/hci_event.c
> > > > > +++ b/net/bluetooth/hci_event.c
> > > > > @@ -2288,28 +2288,35 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
> > > > > count = get_unaligned_le16(ptr++);
> > > >
> > > > actually we first need a cleanup of this function. This part is
> > > > horrible. It works because of our ptr cast to uint16, but we should
> > > > actually use a proper struct here. Maybe an info struct like we use for
> > > > inquiry result might be helpful here.
> > >
> > > Something like the code below?
> > >
> > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > > index e33e468..403974c 100644
> > > --- a/include/net/bluetooth/hci.h
> > > +++ b/include/net/bluetooth/hci.h
> > > @@ -1018,9 +1018,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 238fb65..079b94f 100644
> > > --- a/net/bluetooth/hci_event.c
> > > +++ b/net/bluetooth/hci_event.c
> > > @@ -2273,26 +2273,27 @@ 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;
> > > + struct hci_comp_pkts_info *info;
> > > int i;
> > >
> > > skb_pull(skb, sizeof(*ev));
> > >
> > > BT_DBG("%s num_hndl %d", hdev->name, ev->num_hndl);
> > >
> > > - if (skb->len < ev->num_hndl * 4) {
> > > + if (skb->len < ev->num_hndl * sizeof(*info)) {
> > > BT_DBG("%s bad parameters", hdev->name);
> > > return;
> > > }
> > >
> > > 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];
>
> I was actually using "info" to eliminate magic size above, otherwise I have
> to use long "sizeof(struct hci_comp_pkts_info)". This of course might be
> done in a separate trivial patch.
you actually lost me here. Just assign the info struct pointer and then
use it. My point was just to move the variable location localized to its
scope of usage.
> > > struct hci_conn *conn;
> > > __u16 handle, count;
> > >
> > > - handle = get_unaligned_le16(ptr++);
> > > - count = get_unaligned_le16(ptr++);
> > > + info = &ev->handles[i];
> > > + handle = __le16_to_cpu(info->handle);
> > > + count = __le16_to_cpu(info->count);
> > >
> > > conn = hci_conn_hash_lookup_handle(hdev, handle);
> > > if (!conn)
> >
> > And you need to remove the skb_pull above since otherwise you are
> > pointing to the wrong memory location.
>
> Sorry, cannot get this. I assign "ev" before skb_pull, this should not be a
> problem.
You are fully right. Nevertheless the skb_pull becomes useless now. So
just remove it.
Regards
Marcel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function
2011-12-07 8:20 ` Marcel Holtmann
@ 2011-12-07 8:49 ` Emeltchenko Andrei
0 siblings, 0 replies; 12+ messages in thread
From: Emeltchenko Andrei @ 2011-12-07 8:49 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Wed, Dec 07, 2011 at 10:20:57AM +0200, Marcel Holtmann wrote:
> Hi Andrei,
>
> > > > > > Simplify function and remove fourth level of indentation.
> > > > > >
> > > > > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > > > > ---
> > > > > > net/bluetooth/hci_event.c | 43 +++++++++++++++++++++++++------------------
> > > > > > 1 files changed, 25 insertions(+), 18 deletions(-)
> > > > > >
> > > > > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > > > > index 273e1cb..fd7e6b5 100644
> > > > > > --- a/net/bluetooth/hci_event.c
> > > > > > +++ b/net/bluetooth/hci_event.c
> > > > > > @@ -2288,28 +2288,35 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
> > > > > > count = get_unaligned_le16(ptr++);
> > > > >
> > > > > actually we first need a cleanup of this function. This part is
> > > > > horrible. It works because of our ptr cast to uint16, but we should
> > > > > actually use a proper struct here. Maybe an info struct like we use for
> > > > > inquiry result might be helpful here.
> > > >
> > > > Something like the code below?
> > > >
> > > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > > > index e33e468..403974c 100644
> > > > --- a/include/net/bluetooth/hci.h
> > > > +++ b/include/net/bluetooth/hci.h
> > > > @@ -1018,9 +1018,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 238fb65..079b94f 100644
> > > > --- a/net/bluetooth/hci_event.c
> > > > +++ b/net/bluetooth/hci_event.c
> > > > @@ -2273,26 +2273,27 @@ 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;
> > > > + struct hci_comp_pkts_info *info;
> > > > int i;
> > > >
> > > > skb_pull(skb, sizeof(*ev));
> > > >
> > > > BT_DBG("%s num_hndl %d", hdev->name, ev->num_hndl);
> > > >
> > > > - if (skb->len < ev->num_hndl * 4) {
> > > > + if (skb->len < ev->num_hndl * sizeof(*info)) {
> > > > BT_DBG("%s bad parameters", hdev->name);
> > > > return;
> > > > }
> > > >
> > > > 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];
> >
> > I was actually using "info" to eliminate magic size above, otherwise I have
> > to use long "sizeof(struct hci_comp_pkts_info)". This of course might be
> > done in a separate trivial patch.
>
> you actually lost me here. Just assign the info struct pointer and then
> use it. My point was just to move the variable location localized to its
> scope of usage.
>
> > > > struct hci_conn *conn;
> > > > __u16 handle, count;
> > > >
> > > > - handle = get_unaligned_le16(ptr++);
> > > > - count = get_unaligned_le16(ptr++);
> > > > + info = &ev->handles[i];
> > > > + handle = __le16_to_cpu(info->handle);
> > > > + count = __le16_to_cpu(info->count);
> > > >
> > > > conn = hci_conn_hash_lookup_handle(hdev, handle);
> > > > if (!conn)
> > >
> > > And you need to remove the skb_pull above since otherwise you are
> > > pointing to the wrong memory location.
> >
> > Sorry, cannot get this. I assign "ev" before skb_pull, this should not be a
> > problem.
>
> You are fully right. Nevertheless the skb_pull becomes useless now. So
> just remove it.
I will remove it in a separate patch since it is used in a packet size check.
Best regards
Andrei Emeltchenko
^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFCv1 3/3] Bluetooth: Process num completed data blocks event
2011-12-05 12:55 [RFCv1 0/3] Implement basic block flow control code Emeltchenko Andrei
2011-12-05 12:55 ` [RFCv1 1/3] Bluetooth: Add HCI Read Data Block Size function Emeltchenko Andrei
2011-12-05 12:55 ` [RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function Emeltchenko Andrei
@ 2011-12-05 12:55 ` Emeltchenko Andrei
2011-12-05 13:54 ` Marcel Holtmann
2 siblings, 1 reply; 12+ messages in thread
From: Emeltchenko Andrei @ 2011-12-05 12:55 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Adds support for Number Of Completed Data Blocks Event. Highly
modified version of Code Aurora code.
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
include/net/bluetooth/hci.h | 7 +++++
net/bluetooth/hci_event.c | 61 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 68 insertions(+), 0 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 9490c2c..0a6193e 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1149,6 +1149,13 @@ struct hci_ev_le_meta {
__u8 subevent;
} __packed;
+#define HCI_EV_NUM_COMP_BLOCKS 0x48
+struct hci_ev_num_comp_blocks {
+ __le16 num_blocks;
+ __u8 num_hndl;
+ /* variable length part */
+} __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 fd7e6b5..26e4d38 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2325,6 +2325,63 @@ 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;
+ __le16 *ptr;
+ int i;
+
+ skb_pull(skb, sizeof(*ev));
+
+ BT_DBG("%s num_blocks %d num_hndl %d", hdev->name, ev->num_blocks,
+ ev->num_hndl);
+
+ if (skb->len < ev->num_hndl * 6) {
+ BT_DBG("%s bad parameters", hdev->name);
+ return;
+ }
+
+ if (hdev->dev_type != HCI_BREDR)
+ return;
+
+ tasklet_disable(&hdev->tx_task);
+
+ for (i = 0, ptr = (__le16 *) skb->data; i < ev->num_hndl; i++) {
+ struct hci_conn *conn;
+ __u16 handle, block_count;
+
+ handle = get_unaligned_le16(ptr++);
+
+ /* Skip packet count*/
+ ptr++;
+
+ block_count = get_unaligned_le16(ptr++);
+
+ 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("Not implemented for type %d", conn->type);
+ 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;
@@ -3264,6 +3321,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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [RFCv1 3/3] Bluetooth: Process num completed data blocks event
2011-12-05 12:55 ` [RFCv1 3/3] Bluetooth: Process num completed data blocks event Emeltchenko Andrei
@ 2011-12-05 13:54 ` Marcel Holtmann
0 siblings, 0 replies; 12+ messages in thread
From: Marcel Holtmann @ 2011-12-05 13:54 UTC (permalink / raw)
To: Emeltchenko Andrei; +Cc: linux-bluetooth
Hi Andrei,
> Adds support for Number Of Completed Data Blocks Event. Highly
> modified version of Code Aurora code.
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
> include/net/bluetooth/hci.h | 7 +++++
> net/bluetooth/hci_event.c | 61 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 68 insertions(+), 0 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 9490c2c..0a6193e 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1149,6 +1149,13 @@ struct hci_ev_le_meta {
> __u8 subevent;
> } __packed;
>
> +#define HCI_EV_NUM_COMP_BLOCKS 0x48
> +struct hci_ev_num_comp_blocks {
> + __le16 num_blocks;
> + __u8 num_hndl;
> + /* variable length part */
> +} __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 fd7e6b5..26e4d38 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2325,6 +2325,63 @@ 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;
> + __le16 *ptr;
> + int i;
> +
> + skb_pull(skb, sizeof(*ev));
> +
> + BT_DBG("%s num_blocks %d num_hndl %d", hdev->name, ev->num_blocks,
> + ev->num_hndl);
> +
> + if (skb->len < ev->num_hndl * 6) {
> + BT_DBG("%s bad parameters", hdev->name);
> + return;
> + }
> +
> + if (hdev->dev_type != HCI_BREDR)
> + return;
This check is bogus. Check for current flow control mode and not the
controller type.
> + tasklet_disable(&hdev->tx_task);
> +
> + for (i = 0, ptr = (__le16 *) skb->data; i < ev->num_hndl; i++) {
> + struct hci_conn *conn;
> + __u16 handle, block_count;
> +
> + handle = get_unaligned_le16(ptr++);
> +
> + /* Skip packet count*/
> + ptr++;
> +
> + block_count = get_unaligned_le16(ptr++);
I think the info struct here would also make this part a lot easier to
read.
> +
> + 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("Not implemented for type %d", conn->type);
> + break;
> + }
> + }
> +
> + tasklet_schedule(&hdev->tx_task);
> +
> + tasklet_enable(&hdev->tx_task);
> +}
The sooner we move everything over to processing within a workqueue, the
better. I am not even sure why just disabling the tx_task tasklet is
enough to not lock hci_dev. It is too long ago. The last time I touched
this code was in 2007.
Regards
Marcel
^ permalink raw reply [flat|nested] 12+ messages in thread