[RFC 1/3] Bluetooth: Use queue in the device list

[RFC 1/3] Bluetooth: Use queue in the device list

[Emeltchenko Andrei]

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

Use queue instead of stack discipline for device list. When processing
dev_list with list_for_each* devices will be prosessed in order they
were added (Usually BR/EDR first and AMP later).

Also output from hciconfig looks nicer :-)

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index cf18f6d..aee76fd 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1452,7 +1452,7 @@ int hci_register_dev(struct hci_dev *hdev)
 
 	sprintf(hdev->name, "hci%d", id);
 	hdev->id = id;
-	list_add(&hdev->list, head);
+	list_add_tail(&hdev->list, head);
 
 	atomic_set(&hdev->refcnt, 1);
 	spin_lock_init(&hdev->lock);
-- 
1.7.4.1

[RFC 2/3] 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>
---
 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 aee76fd..d1eef7c 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -228,18 +228,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: [RFC 2/3] Bluetooth: remove old code

[Gustavo Padovan]

* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2011-11-16 17:30:21 +0200]:

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

In despite of this being an RFC I'm taking this patch. Applied, thanks.

	Gustavo

Re: [RFC 2/3] Bluetooth: remove old code

[Marcel Holtmann]

Hi Gustavo,

> * Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2011-11-16 17:30:21 +0200]:
> 
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > 
> > Remove old code not touched for several years.
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  net/bluetooth/hci_core.c |   12 ------------
> >  1 files changed, 0 insertions(+), 12 deletions(-)
> 
> In despite of this being an RFC I'm taking this patch. Applied, thanks.

I wish you didn't sine I left that code in as a reminder that eventually
we might need to get this working for host-to-host-controller flow
control.

Regards

Marcel

[RFC 3/3] Bluetooth: Add AMP initialization

[Emeltchenko Andrei]

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

Define init sequence for AMP controller. Code based on Code Aurora
sequence.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/hci_core.c  |   58 ++++++++++++++++++++++++++++----------------
 net/bluetooth/hci_event.c |    2 +-
 2 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d1eef7c..290acb2 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -219,40 +219,56 @@ static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
 			hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
 	}
 
-	/* Read Local Supported Features */
-	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
-
 	/* Read Local Version */
 	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
 
 	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
 	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
 
-	/* Read BD Address */
-	hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
+	switch (hdev->dev_type) {
+	case HCI_BREDR:
+		/* BR-EDR specific initialization */
+		/* Read Local Supported Features */
+		hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
+
+		/* Read BD Address */
+		hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
 
-	/* Read Class of Device */
-	hci_send_cmd(hdev, HCI_OP_READ_CLASS_OF_DEV, 0, NULL);
+		/* Read Class of Device */
+		hci_send_cmd(hdev, HCI_OP_READ_CLASS_OF_DEV, 0, NULL);
 
-	/* Read Local Name */
-	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_NAME, 0, NULL);
+		/* Read Local Name */
+		hci_send_cmd(hdev, HCI_OP_READ_LOCAL_NAME, 0, NULL);
 
-	/* Read Voice Setting */
-	hci_send_cmd(hdev, HCI_OP_READ_VOICE_SETTING, 0, NULL);
+		/* Read Voice Setting */
+		hci_send_cmd(hdev, HCI_OP_READ_VOICE_SETTING, 0, NULL);
 
-	/* Optional initialization */
+		/* Optional initialization */
+		/* Clear Event Filters */
+		flt_type = HCI_FLT_CLEAR_ALL;
+		hci_send_cmd(hdev, HCI_OP_SET_EVENT_FLT, 1, &flt_type);
 
-	/* Clear Event Filters */
-	flt_type = HCI_FLT_CLEAR_ALL;
-	hci_send_cmd(hdev, HCI_OP_SET_EVENT_FLT, 1, &flt_type);
+		/* Connection accept timeout ~20 secs */
+		param = cpu_to_le16(0x7d00);
+		hci_send_cmd(hdev, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);
 
-	/* Connection accept timeout ~20 secs */
-	param = cpu_to_le16(0x7d00);
-	hci_send_cmd(hdev, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);
+		bacpy(&cp.bdaddr, BDADDR_ANY);
+		cp.delete_all = 1;
+		hci_send_cmd(hdev, HCI_OP_DELETE_STORED_LINK_KEY,
+							sizeof(cp), &cp);
+		break;
+
+	case HCI_AMP:
+		/* AMP initialization */
+		/* Read AMP Info */
+		hci_send_cmd(hdev, HCI_OP_READ_LOCAL_AMP_INFO, 0, NULL);
+		break;
+
+	default:
+		BT_ERR("Unknown device type %d", hdev->dev_type);
+		break;
+	}
 
-	bacpy(&cp.bdaddr, BDADDR_ANY);
-	cp.delete_all = 1;
-	hci_send_cmd(hdev, HCI_OP_DELETE_STORED_LINK_KEY, sizeof(cp), &cp);
 }
 
 static void hci_le_init_req(struct hci_dev *hdev, unsigned long opt)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index a89cf1f..ad31eb4 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -603,7 +603,7 @@ static void hci_cc_read_local_version(struct hci_dev *hdev, struct sk_buff *skb)
 					hdev->manufacturer,
 					hdev->hci_ver, hdev->hci_rev);
 
-	if (test_bit(HCI_INIT, &hdev->flags))
+	if (hdev->dev_type == HCI_BREDR && test_bit(HCI_INIT, &hdev->flags))
 		hci_setup(hdev);
 }
 
-- 
1.7.4.1

Re: [RFC 3/3] Bluetooth: Add AMP initialization

[Marcel Holtmann]

Hi Andrei,

> Define init sequence for AMP controller. Code based on Code Aurora
> sequence.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/hci_core.c  |   58 ++++++++++++++++++++++++++++----------------
>  net/bluetooth/hci_event.c |    2 +-
>  2 files changed, 38 insertions(+), 22 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index d1eef7c..290acb2 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -219,40 +219,56 @@ static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
>  			hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
>  	}
>  
> -	/* Read Local Supported Features */
> -	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
> -
>  	/* Read Local Version */
>  	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
>  
>  	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
>  	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
>  
> -	/* Read BD Address */
> -	hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
> +	switch (hdev->dev_type) {
> +	case HCI_BREDR:

stop right here and first split this out into proper helper functions.
Making this part longer and more convoluted is not going to work well in
the long run.
 
> -	if (test_bit(HCI_INIT, &hdev->flags))
> +	if (hdev->dev_type == HCI_BREDR && test_bit(HCI_INIT, &hdev->flags))
>  		hci_setup(hdev);
>  }
>  

This is ugly. Why don't we set HCI_INIT for AMP controllers as well?

Regards

Marcel

Re: [RFC 3/3] Bluetooth: Add AMP initialization

[Emeltchenko Andrei]

Hi Marcel,

On Thu, Nov 17, 2011 at 06:49:44AM +0900, Marcel Holtmann wrote:
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index d1eef7c..290acb2 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -219,40 +219,56 @@ static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
> >  			hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
> >  	}
> >  
> > -	/* Read Local Supported Features */
> > -	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
> > -
> >  	/* Read Local Version */
> >  	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
> >  
> >  	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
> >  	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
> >  
> > -	/* Read BD Address */
> > -	hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
> > +	switch (hdev->dev_type) {
> > +	case HCI_BREDR:
> 
> stop right here and first split this out into proper helper functions.
> Making this part longer and more convoluted is not going to work well in
> the long run.

Would following functions work:

__hci_common_init
__hci_bredr_init
__hci_amp_init

> > -	if (test_bit(HCI_INIT, &hdev->flags))
> > +	if (hdev->dev_type == HCI_BREDR && test_bit(HCI_INIT, &hdev->flags))
> >  		hci_setup(hdev);
> >  }
> This is ugly. Why don't we set HCI_INIT for AMP controllers as well?

The purpose of the hunk is to avoid hci_setup for other controllers than
BR/EDR. Maybe later on we will come up with something like amp_setup for
AMP controllers.

Maybe I have to create special patch for it?

Best regards 
Andrei Emeltchenko 

Re: [RFC 3/3] Bluetooth: Add AMP initialization

[Marcel Holtmann]

Hi Andrei,

> > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > index d1eef7c..290acb2 100644
> > > --- a/net/bluetooth/hci_core.c
> > > +++ b/net/bluetooth/hci_core.c
> > > @@ -219,40 +219,56 @@ static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
> > >  			hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
> > >  	}
> > >  
> > > -	/* Read Local Supported Features */
> > > -	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
> > > -
> > >  	/* Read Local Version */
> > >  	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
> > >  
> > >  	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
> > >  	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
> > >  
> > > -	/* Read BD Address */
> > > -	hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
> > > +	switch (hdev->dev_type) {
> > > +	case HCI_BREDR:
> > 
> > stop right here and first split this out into proper helper functions.
> > Making this part longer and more convoluted is not going to work well in
> > the long run.
> 
> Would following functions work:
> 
> __hci_common_init
> __hci_bredr_init
> __hci_amp_init

I would not bother with common init. I think there is nothing really
common between AMP and BR/EDR/LE controllers.

So just split it into bredr_init and amp_init.

> > > -	if (test_bit(HCI_INIT, &hdev->flags))
> > > +	if (hdev->dev_type == HCI_BREDR && test_bit(HCI_INIT, &hdev->flags))
> > >  		hci_setup(hdev);
> > >  }
> > This is ugly. Why don't we set HCI_INIT for AMP controllers as well?
> 
> The purpose of the hunk is to avoid hci_setup for other controllers than
> BR/EDR. Maybe later on we will come up with something like amp_setup for
> AMP controllers.
> 
> Maybe I have to create special patch for it?

I like to not see a temporary hack and just init the AMP controller.
This needs to be done anyway. So lets just do it.

Regards

Marcel

RE: [RFC 3/3] Bluetooth: Add AMP initialization

[Peter Krystad]

Hi Andrei & Marcel,

> Hi Andrei,
> 
> > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > > index d1eef7c..290acb2 100644
> > > > --- a/net/bluetooth/hci_core.c
> > > > +++ b/net/bluetooth/hci_core.c
> > > > @@ -219,40 +219,56 @@ static void hci_init_req(struct hci_dev *hdev,
> unsigned long opt)
> > > >  			hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
> > > >  	}
> > > >
> > > > -	/* Read Local Supported Features */
> > > > -	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
> > > > -
> > > >  	/* Read Local Version */
> > > >  	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
> > > >
> > > >  	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
> > > >  	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
> > > >
> > > > -	/* Read BD Address */
> > > > -	hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
> > > > +	switch (hdev->dev_type) {
> > > > +	case HCI_BREDR:
> > >
> > > stop right here and first split this out into proper helper functions.
> > > Making this part longer and more convoluted is not going to work well in
> > > the long run.
> >
> > Would following functions work:
> >
> > __hci_common_init
> > __hci_bredr_init
> > __hci_amp_init
> 
> I would not bother with common init. I think there is nothing really
> common between AMP and BR/EDR/LE controllers.

The commands quoted above are the common part of init between BR-EDR and AMP, plus 
there will be a few additional common commands when block-based flow control is added.
Makes sense to me to split the controller-specific sequences into two helper funcs 
called at this switch statement.

> So just split it into bredr_init and amp_init.
> 
> > > > -	if (test_bit(HCI_INIT, &hdev->flags))
> > > > +	if (hdev->dev_type == HCI_BREDR && test_bit(HCI_INIT, &hdev-
> >flags))
> > > >  		hci_setup(hdev);
> > > >  }
> > > This is ugly. Why don't we set HCI_INIT for AMP controllers as well?
> >
> > The purpose of the hunk is to avoid hci_setup for other controllers than
> > BR/EDR. Maybe later on we will come up with something like amp_setup for
> > AMP controllers.
> >
> > Maybe I have to create special patch for it?
> 
> I like to not see a temporary hack and just init the AMP controller.
> This needs to be done anyway. So lets just do it.
> 
> Regards
> 
> Marcel
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards,

Peter.

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

RE: [RFC 3/3] Bluetooth: Add AMP initialization

[Marcel Holtmann]

Hi Peter,

> > > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > > > index d1eef7c..290acb2 100644
> > > > > --- a/net/bluetooth/hci_core.c
> > > > > +++ b/net/bluetooth/hci_core.c
> > > > > @@ -219,40 +219,56 @@ static void hci_init_req(struct hci_dev *hdev,
> > unsigned long opt)
> > > > >  			hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
> > > > >  	}
> > > > >
> > > > > -	/* Read Local Supported Features */
> > > > > -	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
> > > > > -
> > > > >  	/* Read Local Version */
> > > > >  	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
> > > > >
> > > > >  	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
> > > > >  	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
> > > > >
> > > > > -	/* Read BD Address */
> > > > > -	hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
> > > > > +	switch (hdev->dev_type) {
> > > > > +	case HCI_BREDR:
> > > >
> > > > stop right here and first split this out into proper helper functions.
> > > > Making this part longer and more convoluted is not going to work well in
> > > > the long run.
> > >
> > > Would following functions work:
> > >
> > > __hci_common_init
> > > __hci_bredr_init
> > > __hci_amp_init
> > 
> > I would not bother with common init. I think there is nothing really
> > common between AMP and BR/EDR/LE controllers.
> 
> The commands quoted above are the common part of init between BR-EDR and AMP, plus 
> there will be a few additional common commands when block-based flow control is added.
> Makes sense to me to split the controller-specific sequences into two helper funcs 
> called at this switch statement.

with our current approach on handling this, I rather copy hci_send_cmd
than making this code more complicated.

However since we are talking about controller init. Can someone please
first propose a list of how we init AMP vs BR/EDR/LE controllers instead
of just dumping code here. I think it needs some clear strategy on how
to do it.

Regards

Marcel

Re: [RFC 3/3] Bluetooth: Add AMP initialization

['Emeltchenko Andrei']

Hi Marcel,

On Fri, Nov 18, 2011 at 06:14:13AM +0100, Marcel Holtmann wrote:
> Hi Peter,
> 
> > > > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > > > > index d1eef7c..290acb2 100644
> > > > > > --- a/net/bluetooth/hci_core.c
> > > > > > +++ b/net/bluetooth/hci_core.c
> > > > > > @@ -219,40 +219,56 @@ static void hci_init_req(struct hci_dev *hdev,
> > > unsigned long opt)
> > > > > >  			hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
> > > > > >  	}
> > > > > >
> > > > > > -	/* Read Local Supported Features */
> > > > > > -	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
> > > > > > -
> > > > > >  	/* Read Local Version */
> > > > > >  	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
> > > > > >
> > > > > >  	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
> > > > > >  	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
> > > > > >
> > > > > > -	/* Read BD Address */
> > > > > > -	hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
> > > > > > +	switch (hdev->dev_type) {
> > > > > > +	case HCI_BREDR:
> > > > >
> > > > > stop right here and first split this out into proper helper functions.
> > > > > Making this part longer and more convoluted is not going to work well in
> > > > > the long run.
> > > >
> > > > Would following functions work:
> > > >
> > > > __hci_common_init
> > > > __hci_bredr_init
> > > > __hci_amp_init
> > > 
> > > I would not bother with common init. I think there is nothing really
> > > common between AMP and BR/EDR/LE controllers.
> > 
> > The commands quoted above are the common part of init between BR-EDR and AMP, plus 
> > there will be a few additional common commands when block-based flow control is added.
> > Makes sense to me to split the controller-specific sequences into two helper funcs 
> > called at this switch statement.
> 
> with our current approach on handling this, I rather copy hci_send_cmd
> than making this code more complicated.
> 
> However since we are talking about controller init. Can someone please
> first propose a list of how we init AMP vs BR/EDR/LE controllers instead
> of just dumping code here. I think it needs some clear strategy on how
> to do it.

AMP controller is initialized following way in Code Aurora code:

Common part:

<------8<--------------------------------------------------
|  /* 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);
|  }
|
|  /* Read Local Version */
|  hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
|
|  /* Read HCI Flow Control Mode */
|  hci_send_cmd(hdev, HCI_OP_READ_FLOW_CONTROL_MODE, 0, NULL);
|
|  /* Read Buffer Size (ACL mtu, max pkt, etc.) */
|  hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
|
|  /* Read Data Block Size (ACL mtu, max pkt, etc.) */
|  hci_send_cmd(hdev, HCI_OP_READ_DATA_BLOCK_SIZE, 0, NULL);
<------8<------------------------------------------------------

AMP - specific part:

<------8<---------------------------------------------------
|  /* AMP initialization */
|  /* Connection accept timeout ~5 secs */
|  param = cpu_to_le16(0x1f40);
|  hci_send_cmd(hdev, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);
|
|  /* Read AMP Info */
|  hci_send_cmd(hdev, HCI_OP_READ_LOCAL_AMP_INFO, 0, NULL);
<------8<---------------------------------------------------

Source:
git://codeaurora.org/kernel/msm.git

Best regards 
Andrei Emeltchenko 

Re: [RFC 3/3] Bluetooth: Add AMP initialization

[Marcel Holtmann]

Hi Andrei,

> > > > > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > > > > > index d1eef7c..290acb2 100644
> > > > > > > --- a/net/bluetooth/hci_core.c
> > > > > > > +++ b/net/bluetooth/hci_core.c
> > > > > > > @@ -219,40 +219,56 @@ static void hci_init_req(struct hci_dev *hdev,
> > > > unsigned long opt)
> > > > > > >  			hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
> > > > > > >  	}
> > > > > > >
> > > > > > > -	/* Read Local Supported Features */
> > > > > > > -	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
> > > > > > > -
> > > > > > >  	/* Read Local Version */
> > > > > > >  	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
> > > > > > >
> > > > > > >  	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
> > > > > > >  	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
> > > > > > >
> > > > > > > -	/* Read BD Address */
> > > > > > > -	hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
> > > > > > > +	switch (hdev->dev_type) {
> > > > > > > +	case HCI_BREDR:
> > > > > >
> > > > > > stop right here and first split this out into proper helper functions.
> > > > > > Making this part longer and more convoluted is not going to work well in
> > > > > > the long run.
> > > > >
> > > > > Would following functions work:
> > > > >
> > > > > __hci_common_init
> > > > > __hci_bredr_init
> > > > > __hci_amp_init
> > > > 
> > > > I would not bother with common init. I think there is nothing really
> > > > common between AMP and BR/EDR/LE controllers.
> > > 
> > > The commands quoted above are the common part of init between BR-EDR and AMP, plus 
> > > there will be a few additional common commands when block-based flow control is added.
> > > Makes sense to me to split the controller-specific sequences into two helper funcs 
> > > called at this switch statement.
> > 
> > with our current approach on handling this, I rather copy hci_send_cmd
> > than making this code more complicated.
> > 
> > However since we are talking about controller init. Can someone please
> > first propose a list of how we init AMP vs BR/EDR/LE controllers instead
> > of just dumping code here. I think it needs some clear strategy on how
> > to do it.
> 
> AMP controller is initialized following way in Code Aurora code:
> 
> Common part:
> 
> <------8<--------------------------------------------------
> |  /* 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);
> |  }

this check for QUIRK_NO_RESET is pointless. That is just there for old
Bluetooth 1.0b controllers. Or broken hardware. Let's assume the AMP
controllers are fine and just issue OP_RESET.

> |  /* Read Local Version */
> |  hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
> |
> |  /* Read HCI Flow Control Mode */
> |  hci_send_cmd(hdev, HCI_OP_READ_FLOW_CONTROL_MODE, 0, NULL);
> |
> |  /* Read Buffer Size (ACL mtu, max pkt, etc.) */
> |  hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
> |
> |  /* Read Data Block Size (ACL mtu, max pkt, etc.) */
> |  hci_send_cmd(hdev, HCI_OP_READ_DATA_BLOCK_SIZE, 0, NULL);
> <------8<------------------------------------------------------

This is not a generic sequence. We will not be reading flow control or
data block size commands. It will heavily fail on older controllers.

Regards

Marcel

Re: [RFC 3/3] Bluetooth: Add AMP initialization

['Emeltchenko Andrei']

Hi Marcel,

On Fri, Nov 18, 2011 at 05:23:01PM +0100, Marcel Holtmann wrote:
> > > 
> > > However since we are talking about controller init. Can someone please
> > > first propose a list of how we init AMP vs BR/EDR/LE controllers instead
> > > of just dumping code here. I think it needs some clear strategy on how
> > > to do it.
> > 
> > AMP controller is initialized following way in Code Aurora code:
> > 
> > Common part:
> > 
> > <------8<--------------------------------------------------
> > |  /* 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);
> > |  }
> 
> this check for QUIRK_NO_RESET is pointless. That is just there for old
> Bluetooth 1.0b controllers. Or broken hardware. Let's assume the AMP
> controllers are fine and just issue OP_RESET.
> 
> > |  /* Read Local Version */
> > |  hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
> > |
> > |  /* Read HCI Flow Control Mode */
> > |  hci_send_cmd(hdev, HCI_OP_READ_FLOW_CONTROL_MODE, 0, NULL);
> > |
> > |  /* Read Buffer Size (ACL mtu, max pkt, etc.) */
> > |  hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
> > |
> > |  /* Read Data Block Size (ACL mtu, max pkt, etc.) */
> > |  hci_send_cmd(hdev, HCI_OP_READ_DATA_BLOCK_SIZE, 0, NULL);
> > <------8<------------------------------------------------------
> 
> This is not a generic sequence. We will not be reading flow control or
> data block size commands. It will heavily fail on older controllers.

What about following code (basically BR/EDR initialization is the same as
now and only amp_init is new). I use now virtual AMP device so any init is
good, I need Read Version and Read AMP info commands.

static void bredr_init(struct hci_dev *hdev)
{
	struct hci_cp_delete_stored_link_key cp;
	__le16 param;
	__u8 flt_type;

	/* 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);
	}

	/* Read Local Supported Features */
	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);

	/* Read Local Version */
	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);

	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);

	/* Read BD Address */
	hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);

	/* Read Class of Device */
	hci_send_cmd(hdev, HCI_OP_READ_CLASS_OF_DEV, 0, NULL);

	/* Read Local Name */
	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_NAME, 0, NULL);

	/* Read Voice Setting */
	hci_send_cmd(hdev, HCI_OP_READ_VOICE_SETTING, 0, NULL);

	/* Optional initialization */
	/* Clear Event Filters */
	flt_type = HCI_FLT_CLEAR_ALL;
	hci_send_cmd(hdev, HCI_OP_SET_EVENT_FLT, 1, &flt_type);

	/* Connection accept timeout ~20 secs */
	param = cpu_to_le16(0x7d00);
	hci_send_cmd(hdev, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);

	bacpy(&cp.bdaddr, BDADDR_ANY);
	cp.delete_all = 1;
	hci_send_cmd(hdev, HCI_OP_DELETE_STORED_LINK_KEY, sizeof(cp), &cp);
}

static void amp_init(struct hci_dev *hdev)
{
	/* Mandatory initialization */
	/* Reset */
	hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);

	/* Read Local Version */
	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);

	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);

	/* Read AMP Info */
	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_AMP_INFO, 0, NULL);
}

Best regards 
Andrei Emeltchenko 

Re: [RFC 3/3] Bluetooth: Add AMP initialization

[Marcel Holtmann]

Hi Andrei,

> > > > However since we are talking about controller init. Can someone please
> > > > first propose a list of how we init AMP vs BR/EDR/LE controllers instead
> > > > of just dumping code here. I think it needs some clear strategy on how
> > > > to do it.
> > > 
> > > AMP controller is initialized following way in Code Aurora code:
> > > 
> > > Common part:
> > > 
> > > <------8<--------------------------------------------------
> > > |  /* 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);
> > > |  }
> > 
> > this check for QUIRK_NO_RESET is pointless. That is just there for old
> > Bluetooth 1.0b controllers. Or broken hardware. Let's assume the AMP
> > controllers are fine and just issue OP_RESET.
> > 
> > > |  /* Read Local Version */
> > > |  hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
> > > |
> > > |  /* Read HCI Flow Control Mode */
> > > |  hci_send_cmd(hdev, HCI_OP_READ_FLOW_CONTROL_MODE, 0, NULL);
> > > |
> > > |  /* Read Buffer Size (ACL mtu, max pkt, etc.) */
> > > |  hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
> > > |
> > > |  /* Read Data Block Size (ACL mtu, max pkt, etc.) */
> > > |  hci_send_cmd(hdev, HCI_OP_READ_DATA_BLOCK_SIZE, 0, NULL);
> > > <------8<------------------------------------------------------
> > 
> > This is not a generic sequence. We will not be reading flow control or
> > data block size commands. It will heavily fail on older controllers.
> 
> What about following code (basically BR/EDR initialization is the same as
> now and only amp_init is new). I use now virtual AMP device so any init is
> good, I need Read Version and Read AMP info commands.
> 
> static void bredr_init(struct hci_dev *hdev)
> {
> 	struct hci_cp_delete_stored_link_key cp;
> 	__le16 param;
> 	__u8 flt_type;
> 
> 	/* 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);
> 	}
> 
> 	/* Read Local Supported Features */
> 	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
> 
> 	/* Read Local Version */
> 	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
> 
> 	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
> 	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
> 
> 	/* Read BD Address */
> 	hci_send_cmd(hdev, HCI_OP_READ_BD_ADDR, 0, NULL);
> 
> 	/* Read Class of Device */
> 	hci_send_cmd(hdev, HCI_OP_READ_CLASS_OF_DEV, 0, NULL);
> 
> 	/* Read Local Name */
> 	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_NAME, 0, NULL);
> 
> 	/* Read Voice Setting */
> 	hci_send_cmd(hdev, HCI_OP_READ_VOICE_SETTING, 0, NULL);
> 
> 	/* Optional initialization */
> 	/* Clear Event Filters */
> 	flt_type = HCI_FLT_CLEAR_ALL;
> 	hci_send_cmd(hdev, HCI_OP_SET_EVENT_FLT, 1, &flt_type);
> 
> 	/* Connection accept timeout ~20 secs */
> 	param = cpu_to_le16(0x7d00);
> 	hci_send_cmd(hdev, HCI_OP_WRITE_CA_TIMEOUT, 2, &param);
> 
> 	bacpy(&cp.bdaddr, BDADDR_ANY);
> 	cp.delete_all = 1;
> 	hci_send_cmd(hdev, HCI_OP_DELETE_STORED_LINK_KEY, sizeof(cp), &cp);
> }
> 
> static void amp_init(struct hci_dev *hdev)
> {
> 	/* Mandatory initialization */
> 	/* Reset */
> 	hci_send_cmd(hdev, HCI_OP_RESET, 0, NULL);
> 
> 	/* Read Local Version */
> 	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
> 
> 	/* Read Buffer Size (ACL mtu, max pkt, etc.) */
> 	hci_send_cmd(hdev, HCI_OP_READ_BUFFER_SIZE, 0, NULL);
> 
> 	/* Read AMP Info */
> 	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_AMP_INFO, 0, NULL);
> }

actually I think the READ_AMP_INFO is wrong here. It should be only
executed based on an AMP_Get_Info_Request during a connection setup
procedure.

And even the READ_BUFFER_SIZE is the wrong command since by default the
AMP controller is in block based flow control mode. So you would need to
read the flow control mode first actually.

Regards

Marcel

Re: [RFC 1/3] Bluetooth: Use queue in the device list

[Marcel Holtmann]

Hi Andrei,

> Use queue instead of stack discipline for device list. When processing
> dev_list with list_for_each* devices will be prosessed in order they
> were added (Usually BR/EDR first and AMP later).
> 
> Also output from hciconfig looks nicer :-)
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/hci_core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

not that this matters much, but I am fine with it.

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

Regards

Marcel

Re: [RFC 1/3] Bluetooth: Use queue in the device list

[Gustavo Padovan]

Hi Andrei,

* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2011-11-16 17:30:20 +0200]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> Use queue instead of stack discipline for device list. When processing
> dev_list with list_for_each* devices will be prosessed in order they
> were added (Usually BR/EDR first and AMP later).
> 
> Also output from hciconfig looks nicer :-)
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/hci_core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index cf18f6d..aee76fd 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1452,7 +1452,7 @@ int hci_register_dev(struct hci_dev *hdev)
>  
>  	sprintf(hdev->name, "hci%d", id);
>  	hdev->id = id;
> -	list_add(&hdev->list, head);
> +	list_add_tail(&hdev->list, head);
>  
>  	atomic_set(&hdev->refcnt, 1);
>  	spin_lock_init(&hdev->lock);

Applied, thanks.

	Gustavo