[PATCHv1 0/6] AMP/BREDR initialization patches

[PATCHv1 0/6] AMP/BREDR initialization patches

[Emeltchenko Andrei]

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

Changes:
	v1: Changed HCI_<block,packet>_FLOW_CTL_MODE => HCI_FLOW_CTL_MODE_<block,packet>
	RFCv1: Initial version

AMP initialization and block flow control code.

Resubmit "remove old code" patch as it seems that there is little
sense to copy it over.

Andrei Emeltchenko (6):
  Bluetooth: remove old code
  Bluetooth: Split ctrl init to BREDR and AMP parts
  Bluetooth: Add HCI Read Flow Control Mode function
  Bluetooth: Initialize default flow control mode
  Bluetooth: Add HCI Read Data Block Size function
  Bluetooth: Recalculate sched for HCI block flow ctrl

 include/net/bluetooth/hci.h      |   18 +++++++
 include/net/bluetooth/hci_core.h |    4 ++
 net/bluetooth/hci_core.c         |  103 +++++++++++++++++++++++++------------
 net/bluetooth/hci_event.c        |   53 +++++++++++++++++++
 4 files changed, 144 insertions(+), 34 deletions(-)

-- 
1.7.4.1

[PATCHv1 1/6] Bluetooth: remove old code

[Emeltchenko Andrei]

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

Remove old code not touched for several years.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
---
 net/bluetooth/hci_core.c |   12 ------------
 1 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index ef0423e..bc71f96 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -230,18 +230,6 @@ static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
 	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
 	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
 
-#if 0
-	/* Host buffer size */
-	{
-		struct hci_cp_host_buffer_size cp;
-		cp.acl_mtu = cpu_to_le16(HCI_MAX_ACL_SIZE);
-		cp.sco_mtu = HCI_MAX_SCO_SIZE;
-		cp.acl_max_pkt = cpu_to_le16(0xffff);
-		cp.sco_max_pkt = cpu_to_le16(0xffff);
-		hci_send_cmd(hdev, HCI_OP_HOST_BUFFER_SIZE, sizeof(cp), &cp);
-	}
-#endif
-
 	/* Read BD Address */
 	hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
 
-- 
1.7.4.1

Re: [PATCHv1 1/6] Bluetooth: remove old code

[Marcel Holtmann]

Hi Andrei,

> Remove old code not touched for several years.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> ---
>  net/bluetooth/hci_core.c |   12 ------------
>  1 files changed, 0 insertions(+), 12 deletions(-)

since you guys insist on taking this code out ;)

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

Regards

Marcel

[PATCHv1 2/6] 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 bc71f96..a0247d3 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -192,33 +192,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);
-		tasklet_schedule(&hdev->cmd_task);
-	}
-	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 */
@@ -257,6 +242,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);
+		tasklet_schedule(&hdev->cmd_task);
+	}
+	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 27b1ede..f287e1c 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -560,6 +560,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

[PATCHv1 3/6] Bluetooth: Add HCI Read Flow Control Mode function

[Emeltchenko Andrei]

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

Upstream Code Aurora function with minor trivial fixes.
Origin: git://codeaurora.org/kernel/msm.git

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

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 376c574..ee83c36 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -677,6 +677,12 @@ struct hci_rp_read_local_oob_data {
 
 #define HCI_OP_READ_INQ_RSP_TX_POWER	0x0c58
 
+#define HCI_OP_READ_FLOW_CONTROL_MODE	0x0c66
+struct hci_rp_read_flow_control_mode {
+	__u8     status;
+	__u8     mode;
+} __packed;
+
 #define HCI_OP_WRITE_LE_HOST_SUPPORTED	0x0c6d
 struct hci_cp_write_le_host_supported {
 	__u8 le;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 1795257..a7fd63a 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -170,6 +170,8 @@ struct hci_dev {
 	__u32		amp_max_flush_to;
 	__u32		amp_be_flush_to;
 
+	__u8		flow_ctl_mode;
+
 	unsigned int	auto_accept_delay;
 
 	unsigned long	quirks;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index f287e1c..b743fc3 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -720,6 +720,21 @@ static void hci_cc_read_local_ext_features(struct hci_dev *hdev,
 	hci_req_complete(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES, rp->status);
 }
 
+static void hci_cc_read_flow_control_mode(struct hci_dev *hdev,
+						struct sk_buff *skb)
+{
+	struct hci_rp_read_flow_control_mode *rp = (void *) skb->data;
+
+	BT_DBG("%s status 0x%x", hdev->name, rp->status);
+
+	if (rp->status)
+		return;
+
+	hdev->flow_ctl_mode = rp->mode;
+
+	hci_req_complete(hdev, HCI_OP_READ_FLOW_CONTROL_MODE, rp->status);
+}
+
 static void hci_cc_read_buffer_size(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_rp_read_buffer_size *rp = (void *) skb->data;
@@ -1970,6 +1985,10 @@ static inline void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *sk
 		hci_cc_write_ca_timeout(hdev, skb);
 		break;
 
+	case HCI_OP_READ_FLOW_CONTROL_MODE:
+		hci_cc_read_flow_control_mode(hdev, skb);
+		break;
+
 	case HCI_OP_READ_LOCAL_AMP_INFO:
 		hci_cc_read_local_amp_info(hdev, skb);
 		break;
-- 
1.7.4.1

[PATCHv1 4/6] Bluetooth: Initialize default flow control mode

[Emeltchenko Andrei]

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

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 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 ee83c36..0290aeb 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -271,6 +271,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 a0247d3..4a0391e 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -198,6 +198,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 */
@@ -244,6 +246,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

Re: [PATCHv1 4/6] Bluetooth: Initialize default flow control mode

[Marcel Holtmann]

Hi Andrei,

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

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

Regards

Marcel

[PATCHv1 5/6] Bluetooth: Add HCI Read Data Block Size function

[Emeltchenko Andrei]

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

Upstream Code Aurora function with minor trivial fixes.
Origin: git://codeaurora.org/kernel/msm.git

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

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 0290aeb..7d41bd7 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -741,6 +741,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 a7fd63a..869ab72 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -172,6 +172,8 @@ struct hci_dev {
 
 	__u8		flow_ctl_mode;
 
+	__u16		block_len;
+
 	unsigned int	auto_accept_delay;
 
 	unsigned long	quirks;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index b743fc3..50482a6 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -774,6 +774,33 @@ 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;
+
+	if (hdev->flow_ctl_mode == HCI_FLOW_CTL_MODE_BLOCK_BASED) {
+		hdev->acl_mtu  = __le16_to_cpu(rp->max_acl_len);
+		hdev->sco_mtu = 0;
+		hdev->block_len = __le16_to_cpu(rp->block_len);
+		/* acl_pkts indicates the number of blocks */
+		hdev->acl_pkts = __le16_to_cpu(rp->num_blocks);
+		hdev->sco_pkts = 0;
+		hdev->acl_cnt = hdev->acl_pkts;
+		hdev->sco_cnt = 0;
+	}
+
+	BT_DBG("%s acl mtu %d:%d, block len %d", hdev->name, hdev->acl_mtu,
+					hdev->acl_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);
@@ -1981,6 +2008,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

Re: [PATCHv1 5/6] Bluetooth: Add HCI Read Data Block Size function

[Marcel Holtmann]

Hi Andrei,

> Upstream Code Aurora function with minor trivial fixes.
> Origin: git://codeaurora.org/kernel/msm.git
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  include/net/bluetooth/hci.h      |    8 ++++++++
>  include/net/bluetooth/hci_core.h |    2 ++
>  net/bluetooth/hci_event.c        |   31 +++++++++++++++++++++++++++++++
>  3 files changed, 41 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 0290aeb..7d41bd7 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -741,6 +741,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 a7fd63a..869ab72 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -172,6 +172,8 @@ struct hci_dev {
>  
>  	__u8		flow_ctl_mode;
>  
> +	__u16		block_len;
> +
>  	unsigned int	auto_accept_delay;
>  
>  	unsigned long	quirks;
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index b743fc3..50482a6 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -774,6 +774,33 @@ 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;
> +
> +	if (hdev->flow_ctl_mode == HCI_FLOW_CTL_MODE_BLOCK_BASED) {
> +		hdev->acl_mtu  = __le16_to_cpu(rp->max_acl_len);
> +		hdev->sco_mtu = 0;
> +		hdev->block_len = __le16_to_cpu(rp->block_len);
> +		/* acl_pkts indicates the number of blocks */
> +		hdev->acl_pkts = __le16_to_cpu(rp->num_blocks);
> +		hdev->sco_pkts = 0;
> +		hdev->acl_cnt = hdev->acl_pkts;
> +		hdev->sco_cnt = 0;
> +	}

I am not sure we want to use the same fields that we use for packet
based flow control.

What happens if we switch between the modes? Do we re-read the size
functions or how is that suppose to work out?

We could trigger the reading of the block size or packet size buffer
information based on the success of changing the flow control mode or an
initial reading of the flow control mode.

So the buffer command is triggered from hci_cc_read_flow_control_mode
function and you just need to trigger the reading of flow control mode
from the init functions.

To this extend we might then just change the init handling anyway and
have special HCI_INIT checks in the event callbacks that trigger the
next command to send. Make it true asynchronous and be able to react on
previous read information.

Just some thoughts here. Especially we might actually wanna deal with
AMP controller in packet based mode or BR/EDR controllers in block based
mode. Even if that is just for testing purposes.

Regards

Marcel

Re: [PATCHv1 5/6] Bluetooth: Add HCI Read Data Block Size function

[Emeltchenko Andrei]

Hi Marcel,

On Wed, Nov 30, 2011 at 03:22:39PM +0100, Marcel Holtmann wrote:
> Hi Andrei,
> 
> > Upstream Code Aurora function with minor trivial fixes.
> > Origin: git://codeaurora.org/kernel/msm.git
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  include/net/bluetooth/hci.h      |    8 ++++++++
> >  include/net/bluetooth/hci_core.h |    2 ++
> >  net/bluetooth/hci_event.c        |   31 +++++++++++++++++++++++++++++++
> >  3 files changed, 41 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 0290aeb..7d41bd7 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -741,6 +741,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 a7fd63a..869ab72 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -172,6 +172,8 @@ struct hci_dev {
> >  
> >  	__u8		flow_ctl_mode;
> >  
> > +	__u16		block_len;
> > +
> >  	unsigned int	auto_accept_delay;
> >  
> >  	unsigned long	quirks;
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index b743fc3..50482a6 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -774,6 +774,33 @@ 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;
> > +
> > +	if (hdev->flow_ctl_mode == HCI_FLOW_CTL_MODE_BLOCK_BASED) {
> > +		hdev->acl_mtu  = __le16_to_cpu(rp->max_acl_len);
> > +		hdev->sco_mtu = 0;
> > +		hdev->block_len = __le16_to_cpu(rp->block_len);
> > +		/* acl_pkts indicates the number of blocks */
> > +		hdev->acl_pkts = __le16_to_cpu(rp->num_blocks);
> > +		hdev->sco_pkts = 0;
> > +		hdev->acl_cnt = hdev->acl_pkts;
> > +		hdev->sco_cnt = 0;
> > +	}
> 
> I am not sure we want to use the same fields that we use for packet
> based flow control.

I see this as a simplest way to reuse existing code.

> What happens if we switch between the modes? Do we re-read the size
> functions or how is that suppose to work out?
> 
> We could trigger the reading of the block size or packet size buffer
> information based on the success of changing the flow control mode or an
> initial reading of the flow control mode.
> 
> So the buffer command is triggered from hci_cc_read_flow_control_mode
> function and you just need to trigger the reading of flow control mode
> from the init functions.

This can be done in a separate patch.

> To this extend we might then just change the init handling anyway and
> have special HCI_INIT checks in the event callbacks that trigger the
> next command to send. Make it true asynchronous and be able to react on
> previous read information.

Maybe FLOW_CTRL_INIT ?

> Just some thoughts here. Especially we might actually wanna deal with
> AMP controller in packet based mode or BR/EDR controllers in block based
> mode. Even if that is just for testing purposes.

I think this is why we reuse existing fields.

Best regards 
Andrei Emeltchenko 

[PATCHv1 6/6] Bluetooth: Recalculate sched for HCI block flow ctrl

[Emeltchenko Andrei]

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

Upstream Code Aurora code with trivial fixes.
Origin: git://codeaurora.org/kernel/msm.git

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 4a0391e..360b44c 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2290,10 +2290,12 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
 
 	cnt = hdev->acl_cnt;
 
-	while (hdev->acl_cnt &&
+	while (hdev->acl_cnt > 0 &&
 			(chan = hci_chan_sent(hdev, ACL_LINK, &quote))) {
 		u32 priority = (skb_peek(&chan->data_q))->priority;
-		while (quote-- && (skb = skb_peek(&chan->data_q))) {
+		while (quote > 0 && (skb = skb_peek(&chan->data_q))) {
+			int blocks = 1;
+
 			BT_DBG("chan %p skb %p len %d priority %u", chan, skb,
 					skb->len, skb->priority);
 
@@ -2303,15 +2305,28 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
 
 			skb = skb_dequeue(&chan->data_q);
 
+			if (hdev->flow_ctl_mode ==
+					HCI_FLOW_CTL_MODE_BLOCK_BASED)
+				/* Calculate count of blocks used by
+				 * this packet
+				 */
+				blocks = DIV_ROUND_UP(skb->len -
+					HCI_ACL_HDR_SIZE, hdev->block_len);
+
+			if (blocks > hdev->acl_cnt)
+				return;
+
 			hci_conn_enter_active_mode(chan->conn,
 						bt_cb(skb)->force_active);
 
 			hci_send_frame(skb);
 			hdev->acl_last_tx = jiffies;
 
-			hdev->acl_cnt--;
-			chan->sent++;
-			chan->conn->sent++;
+			hdev->acl_cnt -= blocks;
+			quote -= blocks;
+
+			chan->sent += blocks;
+			chan->conn->sent += blocks;
 		}
 	}
 
-- 
1.7.4.1

Re: [PATCHv1 6/6] Bluetooth: Recalculate sched for HCI block flow ctrl

[Marcel Holtmann]

Hi Andrei,

> Upstream Code Aurora code with trivial fixes.
> Origin: git://codeaurora.org/kernel/msm.git
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/hci_core.c |   25 ++++++++++++++++++++-----
>  1 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 4a0391e..360b44c 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2290,10 +2290,12 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
>  
>  	cnt = hdev->acl_cnt;
>  
> -	while (hdev->acl_cnt &&
> +	while (hdev->acl_cnt > 0 &&
>  			(chan = hci_chan_sent(hdev, ACL_LINK, &quote))) {
>  		u32 priority = (skb_peek(&chan->data_q))->priority;
> -		while (quote-- && (skb = skb_peek(&chan->data_q))) {
> +		while (quote > 0 && (skb = skb_peek(&chan->data_q))) {
> +			int blocks = 1;
> +
>  			BT_DBG("chan %p skb %p len %d priority %u", chan, skb,
>  					skb->len, skb->priority);
>  
> @@ -2303,15 +2305,28 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
>  
>  			skb = skb_dequeue(&chan->data_q);
>  
> +			if (hdev->flow_ctl_mode ==
> +					HCI_FLOW_CTL_MODE_BLOCK_BASED)
> +				/* Calculate count of blocks used by
> +				 * this packet
> +				 */
> +				blocks = DIV_ROUND_UP(skb->len -
> +					HCI_ACL_HDR_SIZE, hdev->block_len);

would it not be more efficient to have to functions here? One
hci_sched_acl_block and one hci_sched_acl_pkt which implement the
specific details of the flow control mode. And the hci_sched_acl would
just decide which on to call once.

> +
> +			if (blocks > hdev->acl_cnt)
> +				return;
> +
>  			hci_conn_enter_active_mode(chan->conn,
>  						bt_cb(skb)->force_active);

Now an important question that comes up here. The support for the power
save mode (sniff mainly) and AMP controllers is fully pointless. We need
to actually make sure we don't try to put an AMP controller into sniff
mode since that seems like an attempt for failure.

Regards

Marcel

RE: [PATCHv1 6/6] Bluetooth: Recalculate sched for HCI block flow ctrl

[Peter Krystad]

Hi Marcel, Andrei,

> Hi Andrei,
> > >
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 4a0391e..360b44c 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -2303,15 +2305,28 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
> >
> >  			skb = skb_dequeue(&chan->data_q);
> >
> > +			if (hdev->flow_ctl_mode ==
> > +					HCI_FLOW_CTL_MODE_BLOCK_BASED)
> > +				/* Calculate count of blocks used by
> > +				 * this packet
> > +				 */
> > +				blocks = DIV_ROUND_UP(skb->len -
> > +					HCI_ACL_HDR_SIZE, hdev->block_len);
> 
> would it not be more efficient to have to functions here? One
> hci_sched_acl_block and one hci_sched_acl_pkt which implement the
> specific details of the flow control mode. And the hci_sched_acl would
> just decide which on to call once.

There has to be a check for which flow control mode is used by the device somewhere, and
this fragment is really the only difference between the two modes, in separate functions
the rest would be duplicate code.

> > +
> > +			if (blocks > hdev->acl_cnt)
> > +				return;
> > +
> >  			hci_conn_enter_active_mode(chan->conn,
> >  						bt_cb(skb)->force_active);
> 
> Now an important question that comes up here. The support for the power
> save mode (sniff mainly) and AMP controllers is fully pointless. We need
> to actually make sure we don't try to put an AMP controller into sniff
> mode since that seems like an attempt for failure.
> 
> Regards
> 
> Marcel
> 
> 

Regards,

Peter.

--Peter Krystad
  Employee of Qualcomm Innovation Center, Inc.
  Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCHv1 6/6] Bluetooth: Recalculate sched for HCI block flow ctrl

['Emeltchenko Andrei']

Hi Marcel, Peter,

On Wed, Nov 30, 2011 at 10:24:33PM -0800, Peter Krystad wrote:
> Hi Marcel, Andrei,
> 
> > Hi Andrei,
> > > >
> > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > index 4a0391e..360b44c 100644
> > > --- a/net/bluetooth/hci_core.c
> > > +++ b/net/bluetooth/hci_core.c
> > > @@ -2303,15 +2305,28 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
> > >
> > >  			skb = skb_dequeue(&chan->data_q);
> > >
> > > +			if (hdev->flow_ctl_mode ==
> > > +					HCI_FLOW_CTL_MODE_BLOCK_BASED)
> > > +				/* Calculate count of blocks used by
> > > +				 * this packet
> > > +				 */
> > > +				blocks = DIV_ROUND_UP(skb->len -
> > > +					HCI_ACL_HDR_SIZE, hdev->block_len);
> > 
> > would it not be more efficient to have to functions here? One
> > hci_sched_acl_block and one hci_sched_acl_pkt which implement the
> > specific details of the flow control mode. And the hci_sched_acl would
> > just decide which on to call once.
> 
> There has to be a check for which flow control mode is used by the device somewhere, and
> this fragment is really the only difference between the two modes, in separate functions
> the rest would be duplicate code.

I feel the same, we can still make a function like:

blocks = __get_blocks(hdev, skb);

would this be better?

> > > +
> > > +			if (blocks > hdev->acl_cnt)
> > > +				return;
> > > +
> > >  			hci_conn_enter_active_mode(chan->conn,
> > >  						bt_cb(skb)->force_active);
> > 
> > Now an important question that comes up here. The support for the power
> > save mode (sniff mainly) and AMP controllers is fully pointless. We need
> > to actually make sure we don't try to put an AMP controller into sniff
> > mode since that seems like an attempt for failure.

We can test for BREDR controller in hci_conn_enter_active_mode function.
This function is already too big.

Best regards 
Andrei Emeltchenko 

Re: [PATCHv1 6/6] Bluetooth: Recalculate sched for HCI block flow ctrl

[Marcel Holtmann]

Hi Andrei,

> > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > > index 4a0391e..360b44c 100644
> > > > --- a/net/bluetooth/hci_core.c
> > > > +++ b/net/bluetooth/hci_core.c
> > > > @@ -2303,15 +2305,28 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
> > > >
> > > >  			skb = skb_dequeue(&chan->data_q);
> > > >
> > > > +			if (hdev->flow_ctl_mode ==
> > > > +					HCI_FLOW_CTL_MODE_BLOCK_BASED)
> > > > +				/* Calculate count of blocks used by
> > > > +				 * this packet
> > > > +				 */
> > > > +				blocks = DIV_ROUND_UP(skb->len -
> > > > +					HCI_ACL_HDR_SIZE, hdev->block_len);
> > > 
> > > would it not be more efficient to have to functions here? One
> > > hci_sched_acl_block and one hci_sched_acl_pkt which implement the
> > > specific details of the flow control mode. And the hci_sched_acl would
> > > just decide which on to call once.
> > 
> > There has to be a check for which flow control mode is used by the device somewhere, and
> > this fragment is really the only difference between the two modes, in separate functions
> > the rest would be duplicate code.
> 
> I feel the same, we can still make a function like:

are we really sure here. If we have more than one SKB in the queue, I
still feel that the check here is pointless. Especially since we do
certain code changes already.

> blocks = __get_blocks(hdev, skb);
> 
> would this be better?

Lets give it a try. Can you whip up a patch for it.

> > > > +
> > > > +			if (blocks > hdev->acl_cnt)
> > > > +				return;
> > > > +
> > > >  			hci_conn_enter_active_mode(chan->conn,
> > > >  						bt_cb(skb)->force_active);
> > > 
> > > Now an important question that comes up here. The support for the power
> > > save mode (sniff mainly) and AMP controllers is fully pointless. We need
> > > to actually make sure we don't try to put an AMP controller into sniff
> > > mode since that seems like an attempt for failure.
> 
> We can test for BREDR controller in hci_conn_enter_active_mode function.
> This function is already too big.

That is what I am saying, we are running code that we know already is
only required for BR/EDR controllers. And now we have two checks here.

Actually maybe just flipping a bit and entering active before or after
might be a way here.

This is a fundamental hot path in our data processing. We need to be
smarter here.

Regards

Marcel