[PATCHv4 0/5] Trivial fixes for emulating AMP HCI

[PATCHv4 0/5] Trivial fixes for emulating AMP HCI

[Emeltchenko Andrei]

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

Changes:
	v4: Added 2 more patches making sure that BREDR is the 1st device in hci_dev_list;
	added AMP definitions form Bluez patch sent by Peter Krystad <pkrystad@codeaurora.org>.
	For VHCI set also amp_status.
	v3: Changed parameter description
        v2: Taken Marcel's comments about parameter name & permissions.

Those trivial patches help to emulate AMP HCI controller
currently used for A2MP protocol verification.

Andrei Emeltchenko (5):
  Bluetooth: Define AMP controller statuses
  Bluetooth: Allow to set AMP type for virtual HCI
  Bluetooth: Move scope of kernel parameter enable_hs
  Bluetooth: Do not set HCI_RAW when HS enabled
  Bluetooth: Assure BREDR HCI device first in the list

 drivers/bluetooth/hci_vhci.c  |   10 ++++++++++
 include/net/bluetooth/hci.h   |   11 +++++++++++
 include/net/bluetooth/l2cap.h |    1 -
 net/bluetooth/hci_core.c      |   11 ++++++++---
 4 files changed, 29 insertions(+), 4 deletions(-)

-- 
1.7.4.1

[PATCHv4 1/5] Bluetooth: Define AMP controller statuses

[Emeltchenko Andrei]

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

AMP status codes copied from Bluez patch sent by Peter Krystad
<pkrystad@codeaurora.org>.

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

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 139ce2a..e79ed67 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -56,6 +56,15 @@
 #define HCI_BREDR	0x00
 #define HCI_AMP		0x01
 
+/* AMP controller status */
+#define AMP_CTRL_POWERED_DOWN			0x00
+#define AMP_CTRL_BLUETOOTH_ONLY			0x01
+#define AMP_CTRL_NO_CAPACITY			0x02
+#define AMP_CTRL_LOW_CAPACITY			0x03
+#define AMP_CTRL_MEDIUM_CAPACITY		0x04
+#define AMP_CTRL_HIGH_CAPACITY			0x05
+#define AMP_CTRL_FULL_CAPACITY			0x06
+
 /* HCI device quirks */
 enum {
 	HCI_QUIRK_NO_RESET,
-- 
1.7.4.1

[PATCHv4 2/5] Bluetooth: Allow to set AMP type for virtual HCI

[Emeltchenko Andrei]

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

Type can be changed during re-opening device /dev/vhci.

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

diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
index 2e302a1..ac08adc 100644
--- a/drivers/bluetooth/hci_vhci.c
+++ b/drivers/bluetooth/hci_vhci.c
@@ -41,6 +41,8 @@
 
 #define VERSION "1.3"
 
+static bool amp;
+
 struct vhci_data {
 	struct hci_dev *hdev;
 
@@ -239,6 +241,11 @@ static int vhci_open(struct inode *inode, struct file *file)
 	hdev->bus = HCI_VIRTUAL;
 	hdev->driver_data = data;
 
+	if (amp) {
+		hdev->dev_type = HCI_AMP;
+		hdev->amp_status = AMP_CTRL_BLUETOOTH_ONLY;
+	}
+
 	hdev->open     = vhci_open_dev;
 	hdev->close    = vhci_close_dev;
 	hdev->flush    = vhci_flush;
@@ -303,6 +310,9 @@ static void __exit vhci_exit(void)
 module_init(vhci_init);
 module_exit(vhci_exit);
 
+module_param(amp, bool, 0644);
+MODULE_PARM_DESC(amp, "Create AMP controller device");
+
 MODULE_AUTHOR("Marcel Holtmann <marcel@holtmann.org>");
 MODULE_DESCRIPTION("Bluetooth virtual HCI driver ver " VERSION);
 MODULE_VERSION(VERSION);
-- 
1.7.4.1

[PATCHv4 3/5] Bluetooth: Move scope of kernel parameter enable_hs

[Emeltchenko Andrei]

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

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 include/net/bluetooth/hci.h   |    2 ++
 include/net/bluetooth/l2cap.h |    1 -
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index e79ed67..8496028 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1340,4 +1340,6 @@ struct hci_inquiry_req {
 };
 #define IREQ_CACHE_FLUSH 0x0001
 
+extern int enable_hs;
+
 #endif /* __HCI_H */
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 9280bff..7299df8 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -789,7 +789,6 @@ static inline __u8 __ctrl_size(struct l2cap_chan *chan)
 }
 
 extern int disable_ertm;
-extern int enable_hs;
 
 int l2cap_init_sockets(void);
 void l2cap_cleanup_sockets(void);
-- 
1.7.4.1

[PATCHv4 4/5] Bluetooth: Do not set HCI_RAW when HS enabled

[Emeltchenko Andrei]

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

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/hci_core.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index fb3feeb..cf18f6d 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -521,8 +521,9 @@ int hci_dev_open(__u16 dev)
 	if (test_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks))
 		set_bit(HCI_RAW, &hdev->flags);
 
-	/* Treat all non BR/EDR controllers as raw devices for now */
-	if (hdev->dev_type != HCI_BREDR)
+	/* Treat all non BR/EDR controllers as raw devices if
+	   enable_hs is not set */
+	if (hdev->dev_type != HCI_BREDR && !enable_hs)
 		set_bit(HCI_RAW, &hdev->flags);
 
 	if (hdev->open(hdev)) {
-- 
1.7.4.1

[PATCHv4 5/5] Bluetooth: Assure BREDR HCI device first in the list

[Emeltchenko Andrei]

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

Using different list_add to make sure that BR/EDR HCI device is the
first device in the hci_dev_list. This is needed for e.g. A2MP Discover
Command which requires that "entry for Controller ID 0x00 (Primary
BR/EDR controller) shall always be sent and shall be the first entry
in the Controller List.
Also output from hciconfig looks nicer :-)

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

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

Re: [PATCHv4 2/5] Bluetooth: Allow to set AMP type for virtual HCI

[Marcel Holtmann]

Hi Andrei,

> Type can be changed during re-opening device /dev/vhci.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  drivers/bluetooth/hci_vhci.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
> index 2e302a1..ac08adc 100644
> --- a/drivers/bluetooth/hci_vhci.c
> +++ b/drivers/bluetooth/hci_vhci.c
> @@ -41,6 +41,8 @@
>  
>  #define VERSION "1.3"
>  
> +static bool amp;
> +
>  struct vhci_data {
>  	struct hci_dev *hdev;
>  
> @@ -239,6 +241,11 @@ static int vhci_open(struct inode *inode, struct file *file)
>  	hdev->bus = HCI_VIRTUAL;
>  	hdev->driver_data = data;
>  
> +	if (amp) {
> +		hdev->dev_type = HCI_AMP;
> +		hdev->amp_status = AMP_CTRL_BLUETOOTH_ONLY;
> +	}
> +

I do not like doing this. You are not squeezing two patches into one.
Please keep them separate. I already acked the previous one.

Regards

Marcel

Re: [PATCHv4 1/5] Bluetooth: Define AMP controller statuses

[Marcel Holtmann]

Hi Andrei,

> AMP status codes copied from Bluez patch sent by Peter Krystad
> <pkrystad@codeaurora.org>.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  include/net/bluetooth/hci.h |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 139ce2a..e79ed67 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -56,6 +56,15 @@
>  #define HCI_BREDR	0x00
>  #define HCI_AMP		0x01
>  
> +/* AMP controller status */
> +#define AMP_CTRL_POWERED_DOWN			0x00
> +#define AMP_CTRL_BLUETOOTH_ONLY			0x01
> +#define AMP_CTRL_NO_CAPACITY			0x02
> +#define AMP_CTRL_LOW_CAPACITY			0x03
> +#define AMP_CTRL_MEDIUM_CAPACITY		0x04
> +#define AMP_CTRL_HIGH_CAPACITY			0x05
> +#define AMP_CTRL_FULL_CAPACITY			0x06
> +

is hci.h really the right place for these? It is not HCI specific
per-se. It is A2MP detail. And as mentioned earlier, I do not believe we
should do it like this.

I think we need to expose some sort of functionality that lets the AMP
drivers handle this dynamically.

Regards

Marcel

Re: [PATCHv4 2/5] Bluetooth: Allow to set AMP type for virtual HCI

[Emeltchenko Andrei]

Hi Marcel,

On Wed, Nov 16, 2011 at 02:48:24PM +0900, Marcel Holtmann wrote:
> Hi Andrei,
> 
> > Type can be changed during re-opening device /dev/vhci.
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  drivers/bluetooth/hci_vhci.c |   10 ++++++++++
> >  1 files changed, 10 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
> > index 2e302a1..ac08adc 100644
> > --- a/drivers/bluetooth/hci_vhci.c
> > +++ b/drivers/bluetooth/hci_vhci.c
> > @@ -41,6 +41,8 @@
> >  
> >  #define VERSION "1.3"
> >  
> > +static bool amp;
> > +
> >  struct vhci_data {
> >  	struct hci_dev *hdev;
> >  
> > @@ -239,6 +241,11 @@ static int vhci_open(struct inode *inode, struct file *file)
> >  	hdev->bus = HCI_VIRTUAL;
> >  	hdev->driver_data = data;
> >  
> > +	if (amp) {
> > +		hdev->dev_type = HCI_AMP;
> > +		hdev->amp_status = AMP_CTRL_BLUETOOTH_ONLY;
> > +	}
> > +
> 
> I do not like doing this. You are not squeezing two patches into one.
> Please keep them separate. I already acked the previous one.

OK I will resend as 2 patches. Because I changed the patch I have removed
your ack.

Best regards 
Andrei Emeltchenko 

Re: [PATCHv4 1/5] Bluetooth: Define AMP controller statuses

[Emeltchenko Andrei]

Hi Marcel,

On Wed, Nov 16, 2011 at 02:50:10PM +0900, Marcel Holtmann wrote:
> Hi Andrei,
> 
> > AMP status codes copied from Bluez patch sent by Peter Krystad
> > <pkrystad@codeaurora.org>.
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  include/net/bluetooth/hci.h |    9 +++++++++
> >  1 files changed, 9 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 139ce2a..e79ed67 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -56,6 +56,15 @@
> >  #define HCI_BREDR	0x00
> >  #define HCI_AMP		0x01
> >  
> > +/* AMP controller status */
> > +#define AMP_CTRL_POWERED_DOWN			0x00
> > +#define AMP_CTRL_BLUETOOTH_ONLY			0x01
> > +#define AMP_CTRL_NO_CAPACITY			0x02
> > +#define AMP_CTRL_LOW_CAPACITY			0x03
> > +#define AMP_CTRL_MEDIUM_CAPACITY		0x04
> > +#define AMP_CTRL_HIGH_CAPACITY			0x05
> > +#define AMP_CTRL_FULL_CAPACITY			0x06
> > +
> 
> is hci.h really the right place for these? It is not HCI specific
> per-se. It is A2MP detail. And as mentioned earlier, I do not believe we
> should do it like this.

I believe that it is HCI device specific since hci_dev structure is
accountable for BR/EDR and AMP controllers and we currently keep
controller-specific information in hci_dev.

Those defines indicate AMP controller status like powered or not.

What would be the better place?

include/net/bluetooth/hci_core.h
include/net/bluetooth/amp.h
include/net/bluetooth/a2mp.h


> I think we need to expose some sort of functionality that lets the AMP
> drivers handle this dynamically.

This status and other AMP parameters would be normally returned when
"read local amp info" HCI command.

Best regards 
Andrei Emeltchenko 

Re: [PATCHv4 1/5] Bluetooth: Define AMP controller statuses

[Marcel Holtmann]

Hi Andrei,

> > > AMP status codes copied from Bluez patch sent by Peter Krystad
> > > <pkrystad@codeaurora.org>.
> > > 
> > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > ---
> > >  include/net/bluetooth/hci.h |    9 +++++++++
> > >  1 files changed, 9 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > > index 139ce2a..e79ed67 100644
> > > --- a/include/net/bluetooth/hci.h
> > > +++ b/include/net/bluetooth/hci.h
> > > @@ -56,6 +56,15 @@
> > >  #define HCI_BREDR	0x00
> > >  #define HCI_AMP		0x01
> > >  
> > > +/* AMP controller status */
> > > +#define AMP_CTRL_POWERED_DOWN			0x00
> > > +#define AMP_CTRL_BLUETOOTH_ONLY			0x01
> > > +#define AMP_CTRL_NO_CAPACITY			0x02
> > > +#define AMP_CTRL_LOW_CAPACITY			0x03
> > > +#define AMP_CTRL_MEDIUM_CAPACITY		0x04
> > > +#define AMP_CTRL_HIGH_CAPACITY			0x05
> > > +#define AMP_CTRL_FULL_CAPACITY			0x06
> > > +
> > 
> > is hci.h really the right place for these? It is not HCI specific
> > per-se. It is A2MP detail. And as mentioned earlier, I do not believe we
> > should do it like this.
> 
> I believe that it is HCI device specific since hci_dev structure is
> accountable for BR/EDR and AMP controllers and we currently keep
> controller-specific information in hci_dev.
> 
> Those defines indicate AMP controller status like powered or not.
> 
> What would be the better place?
> 
> include/net/bluetooth/hci_core.h
> include/net/bluetooth/amp.h
> include/net/bluetooth/a2mp.h
> 
> 
> > I think we need to expose some sort of functionality that lets the AMP
> > drivers handle this dynamically.
> 
> This status and other AMP parameters would be normally returned when
> "read local amp info" HCI command.

if these information come from the AMP controller, then this is clearly
an A2MP specific detail. There is no point in storing them in hci_dev at
all.

I think these definition can stay local in net/bluetooth/a2mp.c for now.

Regards

Marcel

Re: [PATCHv4 1/5] Bluetooth: Define AMP controller statuses

[Emeltchenko Andrei]

Hi Marcel,

On Wed, Nov 16, 2011 at 06:16:27PM +0900, Marcel Holtmann wrote:
> Hi Andrei,
> 
> > > > AMP status codes copied from Bluez patch sent by Peter Krystad
> > > > <pkrystad@codeaurora.org>.
> > > > 
> > > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > > ---
> > > >  include/net/bluetooth/hci.h |    9 +++++++++
> > > >  1 files changed, 9 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > > > index 139ce2a..e79ed67 100644
> > > > --- a/include/net/bluetooth/hci.h
> > > > +++ b/include/net/bluetooth/hci.h
> > > > @@ -56,6 +56,15 @@
> > > >  #define HCI_BREDR	0x00
> > > >  #define HCI_AMP		0x01
> > > >  
> > > > +/* AMP controller status */
> > > > +#define AMP_CTRL_POWERED_DOWN			0x00
> > > > +#define AMP_CTRL_BLUETOOTH_ONLY			0x01
> > > > +#define AMP_CTRL_NO_CAPACITY			0x02
> > > > +#define AMP_CTRL_LOW_CAPACITY			0x03
> > > > +#define AMP_CTRL_MEDIUM_CAPACITY		0x04
> > > > +#define AMP_CTRL_HIGH_CAPACITY			0x05
> > > > +#define AMP_CTRL_FULL_CAPACITY			0x06
> > > > +
> > > 
> > > is hci.h really the right place for these? It is not HCI specific
> > > per-se. It is A2MP detail. And as mentioned earlier, I do not believe we
> > > should do it like this.
> > 
> > I believe that it is HCI device specific since hci_dev structure is
> > accountable for BR/EDR and AMP controllers and we currently keep
> > controller-specific information in hci_dev.
> > 
> > Those defines indicate AMP controller status like powered or not.
> > 
> > What would be the better place?
> > 
> > include/net/bluetooth/hci_core.h
> > include/net/bluetooth/amp.h
> > include/net/bluetooth/a2mp.h
> > 
> > 
> > > I think we need to expose some sort of functionality that lets the AMP
> > > drivers handle this dynamically.
> > 
> > This status and other AMP parameters would be normally returned when
> > "read local amp info" HCI command.
> 
> if these information come from the AMP controller, then this is clearly
> an A2MP specific detail. There is no point in storing them in hci_dev at
> all.

I thought that A2MP specific details are details related to A2MP
connection. Information about status of AMP controller does not depend on
A2MP connection.

> I think these definition can stay local in net/bluetooth/a2mp.c for now.

maybe a2mp.h then? As we use existing HCI command infrastructure to handle
AMP specific HCI commands we might need to source the header.

Best regards 
Andrei Emeltchenko 

Re: [PATCHv4 1/5] Bluetooth: Define AMP controller statuses

[Marcel Holtmann]

Hi Andrei,

> > > > > AMP status codes copied from Bluez patch sent by Peter Krystad
> > > > > <pkrystad@codeaurora.org>.
> > > > > 
> > > > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > > > ---
> > > > >  include/net/bluetooth/hci.h |    9 +++++++++
> > > > >  1 files changed, 9 insertions(+), 0 deletions(-)
> > > > > 
> > > > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > > > > index 139ce2a..e79ed67 100644
> > > > > --- a/include/net/bluetooth/hci.h
> > > > > +++ b/include/net/bluetooth/hci.h
> > > > > @@ -56,6 +56,15 @@
> > > > >  #define HCI_BREDR	0x00
> > > > >  #define HCI_AMP		0x01
> > > > >  
> > > > > +/* AMP controller status */
> > > > > +#define AMP_CTRL_POWERED_DOWN			0x00
> > > > > +#define AMP_CTRL_BLUETOOTH_ONLY			0x01
> > > > > +#define AMP_CTRL_NO_CAPACITY			0x02
> > > > > +#define AMP_CTRL_LOW_CAPACITY			0x03
> > > > > +#define AMP_CTRL_MEDIUM_CAPACITY		0x04
> > > > > +#define AMP_CTRL_HIGH_CAPACITY			0x05
> > > > > +#define AMP_CTRL_FULL_CAPACITY			0x06
> > > > > +
> > > > 
> > > > is hci.h really the right place for these? It is not HCI specific
> > > > per-se. It is A2MP detail. And as mentioned earlier, I do not believe we
> > > > should do it like this.
> > > 
> > > I believe that it is HCI device specific since hci_dev structure is
> > > accountable for BR/EDR and AMP controllers and we currently keep
> > > controller-specific information in hci_dev.
> > > 
> > > Those defines indicate AMP controller status like powered or not.
> > > 
> > > What would be the better place?
> > > 
> > > include/net/bluetooth/hci_core.h
> > > include/net/bluetooth/amp.h
> > > include/net/bluetooth/a2mp.h
> > > 
> > > 
> > > > I think we need to expose some sort of functionality that lets the AMP
> > > > drivers handle this dynamically.
> > > 
> > > This status and other AMP parameters would be normally returned when
> > > "read local amp info" HCI command.
> > 
> > if these information come from the AMP controller, then this is clearly
> > an A2MP specific detail. There is no point in storing them in hci_dev at
> > all.
> 
> I thought that A2MP specific details are details related to A2MP
> connection. Information about status of AMP controller does not depend on
> A2MP connection.

the only part using this information will be A2MP and it has to read it
from the controller fresh almost every time. So yes, it belongs there
and not inside HCI. At least that is how I read the AMP controller
specification and how it is suppose to be used.

> > I think these definition can stay local in net/bluetooth/a2mp.c for now.
> 
> maybe a2mp.h then? As we use existing HCI command infrastructure to handle
> AMP specific HCI commands we might need to source the header.

As I said, leave it local for now. We can always move them around later.

Regards

Marcel

Re: [PATCHv4 5/5] Bluetooth: Assure BREDR HCI device first in the list

[Gustavo Padovan]

Hi Andrei,

* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2011-11-15 16:15:47 +0200]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> Using different list_add to make sure that BR/EDR HCI device is the
> first device in the hci_dev_list. This is needed for e.g. A2MP Discover
> Command which requires that "entry for Controller ID 0x00 (Primary
> BR/EDR controller) shall always be sent and shall be the first entry
> in the Controller List.
> Also output from hciconfig looks nicer :-)
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/hci_core.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)

Applied, thanks.

	Gustavo

Re: [PATCHv4 5/5] Bluetooth: Assure BREDR HCI device first in the list

[Gustavo Padovan]

* Gustavo Padovan <padovan@profusion.mobi> [2011-11-16 18:37:10 -0200]:

> Hi Andrei,
> 
> * Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2011-11-15 16:15:47 +0200]:
> 
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > 
> > Using different list_add to make sure that BR/EDR HCI device is the
> > first device in the hci_dev_list. This is needed for e.g. A2MP Discover
> > Command which requires that "entry for Controller ID 0x00 (Primary
> > BR/EDR controller) shall always be sent and shall be the first entry
> > in the Controller List.
> > Also output from hciconfig looks nicer :-)
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  net/bluetooth/hci_core.c |    6 +++++-
> >  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> Applied, thanks.

Actually I'm reverting it as we have some discussion around this patch flowing
in another thread.

	Gustavo

RE: [PATCHv4 1/5] Bluetooth: Define AMP controller statuses

[Peter Krystad]

Hi Andrei and Marcel,

> > > > > > AMP status codes copied from Bluez patch sent by Peter Krystad
> > > > > > <pkrystad@codeaurora.org>.
> > > > > >
> > > > > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > > > > ---
> > > > > >  include/net/bluetooth/hci.h |    9 +++++++++
> > > > > >  1 files changed, 9 insertions(+), 0 deletions(-)
> > > > > >
> > > > > > diff --git a/include/net/bluetooth/hci.h
> b/include/net/bluetooth/hci.h
> > > > > > index 139ce2a..e79ed67 100644
> > > > > > --- a/include/net/bluetooth/hci.h
> > > > > > +++ b/include/net/bluetooth/hci.h
> > > > > > @@ -56,6 +56,15 @@
> > > > > >  #define HCI_BREDR	0x00
> > > > > >  #define HCI_AMP		0x01
> > > > > >
> > > > > > +/* AMP controller status */
> > > > > > +#define AMP_CTRL_POWERED_DOWN			0x00
> > > > > > +#define AMP_CTRL_BLUETOOTH_ONLY			0x01
> > > > > > +#define AMP_CTRL_NO_CAPACITY			0x02
> > > > > > +#define AMP_CTRL_LOW_CAPACITY			0x03
> > > > > > +#define AMP_CTRL_MEDIUM_CAPACITY		0x04
> > > > > > +#define AMP_CTRL_HIGH_CAPACITY			0x05
> > > > > > +#define AMP_CTRL_FULL_CAPACITY			0x06
> > > > > > +
> > > > >
> > > > > is hci.h really the right place for these? It is not HCI specific
> > > > > per-se. It is A2MP detail. And as mentioned earlier, I do not believe
> we
> > > > > should do it like this.
> > > >
> > > > I believe that it is HCI device specific since hci_dev structure is
> > > > accountable for BR/EDR and AMP controllers and we currently keep
> > > > controller-specific information in hci_dev.
> > > >
> > > > Those defines indicate AMP controller status like powered or not.
> > > >
> > > > What would be the better place?
> > > >
> > > > include/net/bluetooth/hci_core.h
> > > > include/net/bluetooth/amp.h
> > > > include/net/bluetooth/a2mp.h
> > > >
> > > >
> > > > > I think we need to expose some sort of functionality that lets the
> AMP
> > > > > drivers handle this dynamically.
> > > >
> > > > This status and other AMP parameters would be normally returned when
> > > > "read local amp info" HCI command.
> > >
> > > if these information come from the AMP controller, then this is clearly
> > > an A2MP specific detail. There is no point in storing them in hci_dev at
> > > all.
> >
> > I thought that A2MP specific details are details related to A2MP
> > connection. Information about status of AMP controller does not depend on
> > A2MP connection.
> 
> the only part using this information will be A2MP and it has to read it
> from the controller fresh almost every time. So yes, it belongs there
> and not inside HCI. At least that is how I read the AMP controller
> specification and how it is suppose to be used.

This status is updated by the controller via unsolicited AMP status event, there
is no reason to read it other than during init. I agree with Andrei that storing
it in hci_dev makes sense, as this structure represents state of a local HCI 
controller. AMP Manager data structures are all related to A2MP connections, not
devices. AMP Manager does need to know when the status changes, because it has
connections to service when this status happens.
 
> > > I think these definition can stay local in net/bluetooth/a2mp.c for now.
> >
> > maybe a2mp.h then? As we use existing HCI command infrastructure to handle
> > AMP specific HCI commands we might need to source the header.
> 
> As I said, leave it local for now. We can always move them around later.
> 
> Regards
> 
> Marcel

Regards,

Peter.