[PATCH v2 0/2] Use HCI device type to determine suitability of HCI commands

[PATCH v2 0/2] Use HCI device type to determine suitability of HCI commands

[David Scherba]

BlueZ assumes that HCI devices are either BR/EDR radios, or "RAW" (via
HCI_RAW).  At present, alternate radios are handled as RAW (reference
Marcel's 943da25d9 kernel commit).  More subtlety in BlueZ is useful as
AMP support is enhanced.

These patches remove the need to treat non-BR/EDR HCI devices as "RAW."

Cheers,
David

-- 
David Scherba
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 1/2] Remove non-functional hci_devinfo calls in init_device()

[David Scherba]

---
 plugins/hciops.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index 5775cf1..3e3e172 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -122,7 +122,6 @@ static void device_devup_setup(int index)
 static void init_device(int index)
 {
 	struct hci_dev_req dr;
-	struct hci_dev_info di;
 	pid_t pid;
 	int dd, err;
 
@@ -175,13 +174,6 @@ static void init_device(int index)
 		goto fail;
 	}
 
-	if (hci_devinfo(index, &di) < 0)
-		goto fail;
-
-	if (hci_test_bit(HCI_RAW, &di.flags))
-		goto done;
-
-done:
 	hci_close_dev(dd);
 	exit(0);
 
-- 
1.7.0.2
-- 
David Scherba
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/2] Use HCI device type to gate BR/EDR-specific HCI commands

[David Scherba]

Use the hci_dev_info structure member 'type' to classify whether a HCI device
is BR/EDR, or not.  If not, gate BR/EDR-specific HCI commands.
---
 lib/hci_lib.h    |    5 +++++
 plugins/hciops.c |   20 +++++++++++++-------
 src/adapter.c    |    2 +-
 src/security.c   |    4 ++--
 4 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/lib/hci_lib.h b/lib/hci_lib.h
index b63a2a4..814672d 100644
--- a/lib/hci_lib.h
+++ b/lib/hci_lib.h
@@ -169,6 +169,11 @@ static inline int hci_test_bit(int nr, void *addr)
 	return *((uint32_t *) addr + (nr >> 5)) & (1 << (nr & 31));
 }
 
+static inline int ignore_device(struct hci_dev_info *di)
+{
+	return hci_test_bit(HCI_RAW, &di->flags) || di->type >> 4 != HCI_BREDR;
+}
+
 /* HCI filter tools */
 static inline void hci_filter_clear(struct hci_filter *f)
 {
diff --git a/plugins/hciops.c b/plugins/hciops.c
index 3e3e172..54c5bc0 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -85,7 +85,7 @@ static void device_devup_setup(int index)
 	if (hci_devinfo(index, &di) < 0)
 		return;
 
-	if (hci_test_bit(HCI_RAW, &di.flags))
+	if (ignore_device(&di))
 		return;
 
 	dd = hci_open_dev(index);
@@ -122,6 +122,7 @@ static void device_devup_setup(int index)
 static void init_device(int index)
 {
 	struct hci_dev_req dr;
+	struct hci_dev_info di;
 	pid_t pid;
 	int dd, err;
 
@@ -159,12 +160,17 @@ static void init_device(int index)
 					index, strerror(err), err);
 	}
 
-	/* Set link policy */
-	dr.dev_opt = main_opts.link_policy;
-	if (ioctl(dd, HCISETLINKPOL, (unsigned long) &dr) < 0 &&
+	/* Set link policy for BR/EDR HCI devices */
+	if (hci_devinfo(index, &di) < 0)
+		goto fail;
+
+	if (!ignore_device(&di)) {
+		dr.dev_opt = main_opts.link_policy;
+		if (ioctl(dd, HCISETLINKPOL, (unsigned long) &dr) < 0 &&
 							errno != ENETDOWN) {
-		error("Can't set link policy on hci%d: %s (%d)",
-					index, strerror(errno), errno);
+			error("Can't set link policy on hci%d: %s (%d)",
+						index, strerror(errno), errno);
+		}
 	}
 
 	/* Start HCI device */
@@ -196,7 +202,7 @@ static void device_devreg_setup(int index)
 
 	devup = hci_test_bit(HCI_UP, &di.flags);
 
-	if (!hci_test_bit(HCI_RAW, &di.flags))
+	if (ignore_device(&di))
 		manager_register_adapter(index, devup);
 }
 
diff --git a/src/adapter.c b/src/adapter.c
index af44a59..fc1e123 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2300,7 +2300,7 @@ int adapter_start(struct btd_adapter *adapter)
 	if (hci_devinfo(adapter->dev_id, &di) < 0)
 		return -errno;
 
-	if (hci_test_bit(HCI_RAW, &di.flags)) {
+	if (ignore_device(&di)) {
 		dev->ignore = 1;
 		return -1;
 	}
diff --git a/src/security.c b/src/security.c
index ca394e1..667f1f1 100644
--- a/src/security.c
+++ b/src/security.c
@@ -999,7 +999,7 @@ static gboolean io_security_event(GIOChannel *chan, GIOCondition cond,
 
 	ioctl(dev, HCIGETDEVINFO, (void *) di);
 
-	if (hci_test_bit(HCI_RAW, &di->flags))
+	if (ignore_device(di))
 		return TRUE;
 
 	switch (eh->evt) {
@@ -1185,7 +1185,7 @@ void start_security_manager(int hdev)
 	io_data[hdev].channel = chan;
 	io_data[hdev].pin_length = -1;
 
-	if (hci_test_bit(HCI_RAW, &di->flags))
+	if (ignore_device(di))
 		return;
 
 	bacpy(&cp.bdaddr, BDADDR_ANY);
-- 
1.7.0.2
-- 
David Scherba
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] Use HCI device type to gate BR/EDR-specific HCI commands

[David Scherba]

On 8/10/2010 10:15 AM, David Scherba wrote:
> -	if (!hci_test_bit(HCI_RAW,&di.flags))
> +	if (ignore_device(&di))

Johan--

This change is functionally incorrect--sorry about that.  Updating patch 
2/2.

Cheers,
David

-- 
David Scherba
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 2/2] Use HCI device type to gate BR/EDR-specific HCI commands

[David Scherba]

Use the hci_dev_info structure member 'type' to classify whether a HCI device
is BR/EDR, or not.  If not, gate BR/EDR-specific HCI commands.
---
 lib/hci_lib.h    |    5 +++++
 plugins/hciops.c |   20 +++++++++++++-------
 src/adapter.c    |    2 +-
 src/security.c   |    4 ++--
 4 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/lib/hci_lib.h b/lib/hci_lib.h
index b63a2a4..814672d 100644
--- a/lib/hci_lib.h
+++ b/lib/hci_lib.h
@@ -169,6 +169,11 @@ static inline int hci_test_bit(int nr, void *addr)
 	return *((uint32_t *) addr + (nr >> 5)) & (1 << (nr & 31));
 }
 
+static inline int ignore_device(struct hci_dev_info *di)
+{
+	return hci_test_bit(HCI_RAW, &di->flags) || di->type >> 4 != HCI_BREDR;
+}
+
 /* HCI filter tools */
 static inline void hci_filter_clear(struct hci_filter *f)
 {
diff --git a/plugins/hciops.c b/plugins/hciops.c
index 3e3e172..9c97c5a 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -85,7 +85,7 @@ static void device_devup_setup(int index)
 	if (hci_devinfo(index, &di) < 0)
 		return;
 
-	if (hci_test_bit(HCI_RAW, &di.flags))
+	if (ignore_device(&di))
 		return;
 
 	dd = hci_open_dev(index);
@@ -122,6 +122,7 @@ static void device_devup_setup(int index)
 static void init_device(int index)
 {
 	struct hci_dev_req dr;
+	struct hci_dev_info di;
 	pid_t pid;
 	int dd, err;
 
@@ -159,12 +160,17 @@ static void init_device(int index)
 					index, strerror(err), err);
 	}
 
-	/* Set link policy */
-	dr.dev_opt = main_opts.link_policy;
-	if (ioctl(dd, HCISETLINKPOL, (unsigned long) &dr) < 0 &&
+	/* Set link policy for BR/EDR HCI devices */
+	if (hci_devinfo(index, &di) < 0)
+		goto fail;
+
+	if (!ignore_device(&di)) {
+		dr.dev_opt = main_opts.link_policy;
+		if (ioctl(dd, HCISETLINKPOL, (unsigned long) &dr) < 0 &&
 							errno != ENETDOWN) {
-		error("Can't set link policy on hci%d: %s (%d)",
-					index, strerror(errno), errno);
+			error("Can't set link policy on hci%d: %s (%d)",
+						index, strerror(errno), errno);
+		}
 	}
 
 	/* Start HCI device */
@@ -196,7 +202,7 @@ static void device_devreg_setup(int index)
 
 	devup = hci_test_bit(HCI_UP, &di.flags);
 
-	if (!hci_test_bit(HCI_RAW, &di.flags))
+	if (!ignore_device(&di))
 		manager_register_adapter(index, devup);
 }
 
diff --git a/src/adapter.c b/src/adapter.c
index af44a59..fc1e123 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2300,7 +2300,7 @@ int adapter_start(struct btd_adapter *adapter)
 	if (hci_devinfo(adapter->dev_id, &di) < 0)
 		return -errno;
 
-	if (hci_test_bit(HCI_RAW, &di.flags)) {
+	if (ignore_device(&di)) {
 		dev->ignore = 1;
 		return -1;
 	}
diff --git a/src/security.c b/src/security.c
index ca394e1..667f1f1 100644
--- a/src/security.c
+++ b/src/security.c
@@ -999,7 +999,7 @@ static gboolean io_security_event(GIOChannel *chan, GIOCondition cond,
 
 	ioctl(dev, HCIGETDEVINFO, (void *) di);
 
-	if (hci_test_bit(HCI_RAW, &di->flags))
+	if (ignore_device(di))
 		return TRUE;
 
 	switch (eh->evt) {
@@ -1185,7 +1185,7 @@ void start_security_manager(int hdev)
 	io_data[hdev].channel = chan;
 	io_data[hdev].pin_length = -1;
 
-	if (hci_test_bit(HCI_RAW, &di->flags))
+	if (ignore_device(di))
 		return;
 
 	bacpy(&cp.bdaddr, BDADDR_ANY);
-- 
1.7.0.2
-- 
David Scherba
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] Use HCI device type to gate BR/EDR-specific HCI commands

[Johan Hedberg]

Hi David,

On Tue, Aug 10, 2010, David Scherba wrote:
> --- a/lib/hci_lib.h
> +++ b/lib/hci_lib.h
> @@ -169,6 +169,11 @@ static inline int hci_test_bit(int nr, void *addr)
>  	return *((uint32_t *) addr + (nr >> 5)) & (1 << (nr & 31));
>  }
>  
> +static inline int ignore_device(struct hci_dev_info *di)
> +{
> +	return hci_test_bit(HCI_RAW, &di->flags) || di->type >> 4 != HCI_BREDR;
> +}

Since this is hci_lib.h (something I didn't realize in the previous
review) the function should follow the same namespace as all other
functions in the header file, i.e. hci_*. However, does the function
really need to be exported in this public library header file, i.e.
isn't there some private header file that could be used instead?

Johan

Re: [PATCH 1/2] Remove non-functional hci_devinfo calls in init_device()

[Johan Hedberg]

Hi David,

On Tue, Aug 10, 2010, David Scherba wrote:
> ---
>  plugins/hciops.c |    8 --------
>  1 files changed, 0 insertions(+), 8 deletions(-)

This patch is now upstream. Thanks.

Johan

Re: [PATCH 2/2] Use HCI device type to gate BR/EDR-specific HCI commands

[David Scherba]

On 8/10/2010 2:16 PM, Johan Hedberg wrote:

>> +static inline int ignore_device(struct hci_dev_info *di)
>> +{
>> +	return hci_test_bit(HCI_RAW,&di->flags) || di->type>>  4 != HCI_BREDR;
>> +}
>
> Since this is hci_lib.h (something I didn't realize in the previous
> review) the function should follow the same namespace as all other
> functions in the header file, i.e. hci_*. However, does the function
> really need to be exported in this public library header file, i.e.
> isn't there some private header file that could be used instead?

It looks like src/hcid.h fits the bill, although the modified hcid.h 
will need to include the hci headers to have the appropriate defines... 
Modified patch coming up.

Thanks,
David

-- 
David Scherba
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 1/1] Use HCI device type to gate BR/EDR-specific HCI commands

[David Scherba]

Use the hci_dev_info structure member 'type' to classify whether a HCI device
is BR/EDR, or not.  If not, gate BR/EDR-specific HCI commands.
---
 plugins/hciops.c |   20 +++++++++++++-------
 src/adapter.c    |    2 +-
 src/hcid.h       |    8 ++++++++
 src/security.c   |    4 ++--
 4 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index 3e3e172..9c97c5a 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -85,7 +85,7 @@ static void device_devup_setup(int index)
 	if (hci_devinfo(index, &di) < 0)
 		return;
 
-	if (hci_test_bit(HCI_RAW, &di.flags))
+	if (ignore_device(&di))
 		return;
 
 	dd = hci_open_dev(index);
@@ -122,6 +122,7 @@ static void device_devup_setup(int index)
 static void init_device(int index)
 {
 	struct hci_dev_req dr;
+	struct hci_dev_info di;
 	pid_t pid;
 	int dd, err;
 
@@ -159,12 +160,17 @@ static void init_device(int index)
 					index, strerror(err), err);
 	}
 
-	/* Set link policy */
-	dr.dev_opt = main_opts.link_policy;
-	if (ioctl(dd, HCISETLINKPOL, (unsigned long) &dr) < 0 &&
+	/* Set link policy for BR/EDR HCI devices */
+	if (hci_devinfo(index, &di) < 0)
+		goto fail;
+
+	if (!ignore_device(&di)) {
+		dr.dev_opt = main_opts.link_policy;
+		if (ioctl(dd, HCISETLINKPOL, (unsigned long) &dr) < 0 &&
 							errno != ENETDOWN) {
-		error("Can't set link policy on hci%d: %s (%d)",
-					index, strerror(errno), errno);
+			error("Can't set link policy on hci%d: %s (%d)",
+						index, strerror(errno), errno);
+		}
 	}
 
 	/* Start HCI device */
@@ -196,7 +202,7 @@ static void device_devreg_setup(int index)
 
 	devup = hci_test_bit(HCI_UP, &di.flags);
 
-	if (!hci_test_bit(HCI_RAW, &di.flags))
+	if (!ignore_device(&di))
 		manager_register_adapter(index, devup);
 }
 
diff --git a/src/adapter.c b/src/adapter.c
index af44a59..fc1e123 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2300,7 +2300,7 @@ int adapter_start(struct btd_adapter *adapter)
 	if (hci_devinfo(adapter->dev_id, &di) < 0)
 		return -errno;
 
-	if (hci_test_bit(HCI_RAW, &di.flags)) {
+	if (ignore_device(&di)) {
 		dev->ignore = 1;
 		return -1;
 	}
diff --git a/src/hcid.h b/src/hcid.h
index 9c5f1d6..50bdb65 100644
--- a/src/hcid.h
+++ b/src/hcid.h
@@ -23,6 +23,9 @@
  *
  */
 
+#include <bluetooth/hci.h>
+#include <bluetooth/hci_lib.h>
+
 /* When all services should trust a remote device */
 #define GLOBAL_TRUST "[all]"
 
@@ -99,3 +102,8 @@ void rfkill_exit(void);
 
 void __probe_servers(const char *adapter);
 void __remove_servers(const char *adapter);
+
+static inline int ignore_device(struct hci_dev_info *di)
+{
+	return hci_test_bit(HCI_RAW, &di->flags) || di->type >> 4 != HCI_BREDR;
+}
diff --git a/src/security.c b/src/security.c
index ca394e1..667f1f1 100644
--- a/src/security.c
+++ b/src/security.c
@@ -999,7 +999,7 @@ static gboolean io_security_event(GIOChannel *chan, GIOCondition cond,
 
 	ioctl(dev, HCIGETDEVINFO, (void *) di);
 
-	if (hci_test_bit(HCI_RAW, &di->flags))
+	if (ignore_device(di))
 		return TRUE;
 
 	switch (eh->evt) {
@@ -1185,7 +1185,7 @@ void start_security_manager(int hdev)
 	io_data[hdev].channel = chan;
 	io_data[hdev].pin_length = -1;
 
-	if (hci_test_bit(HCI_RAW, &di->flags))
+	if (ignore_device(di))
 		return;
 
 	bacpy(&cp.bdaddr, BDADDR_ANY);
-- 
1.7.0.2
-- 
David Scherba
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 1/1] Use HCI device type to gate BR/EDR-specific HCI commands

[Johan Hedberg]

Hi David,

On Tue, Aug 10, 2010, David Scherba wrote:
> Use the hci_dev_info structure member 'type' to classify whether a HCI device
> is BR/EDR, or not.  If not, gate BR/EDR-specific HCI commands.
> ---
>  plugins/hciops.c |   20 +++++++++++++-------
>  src/adapter.c    |    2 +-
>  src/hcid.h       |    8 ++++++++
>  src/security.c   |    4 ++--
>  4 files changed, 24 insertions(+), 10 deletions(-)

Thanks. This one is now also pushed upstream.

Johan