[RFCv1 0/3] Implement basic block flow control code

[RFCv1 0/3] Implement basic block flow control code

[Emeltchenko Andrei]

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

Adds basic HCI commands and event handlers related to data block
flow control.

Andrei Emeltchenko (3):
  Bluetooth: Add HCI Read Data Block Size function
  Bluetooth: Simplify num_comp_pkts_evt function
  Bluetooth: Process num completed data blocks event

 include/net/bluetooth/hci.h      |   15 +++++
 include/net/bluetooth/hci_core.h |    5 ++
 net/bluetooth/hci_event.c        |  130 ++++++++++++++++++++++++++++++++-----
 3 files changed, 132 insertions(+), 18 deletions(-)

-- 
1.7.4.1

[RFCv1 1/3] Bluetooth: Add HCI Read Data Block Size function

[Emeltchenko Andrei]

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

[RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function

[Emeltchenko Andrei]

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

[RFCv1 3/3] Bluetooth: Process num completed data blocks event

[Emeltchenko Andrei]

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

Re: [RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function

[Marcel Holtmann]

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

Re: [RFCv1 3/3] Bluetooth: Process num completed data blocks event

[Marcel Holtmann]

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

Re: [RFCv1 1/3] Bluetooth: Add HCI Read Data Block Size function

[Marcel Holtmann]

Hi Andrei,

> 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(-)

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

Regards

Marcel

Re: [RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function

[Emeltchenko Andrei]

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 

Re: [RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function

[Marcel Holtmann]

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

Re: [RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function

[Emeltchenko Andrei]

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 

Re: [RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function

[Marcel Holtmann]

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

Re: [RFCv1 2/3] Bluetooth: Simplify num_comp_pkts_evt function

[Emeltchenko Andrei]

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