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

[PATCH 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 |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index 5775cf1..b38c056 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -175,12 +175,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
-- 
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 |   23 +++++++++++++++--------
 src/adapter.c    |    3 ++-
 src/security.c   |    6 ++++--
 4 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/lib/hci_lib.h b/lib/hci_lib.h
index b63a2a4..c0786ab 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 is_bredr_hci_device_type(uint8_t type)
+{
+  return (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 b38c056..705e9dd 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -85,7 +85,8 @@ static void device_devup_setup(int index)
 	if (hci_devinfo(index, &di) < 0)
 		return;
 
-	if (hci_test_bit(HCI_RAW, &di.flags))
+	if (!is_bredr_hci_device_type(di.type) ||
+	    hci_test_bit(HCI_RAW, &di.flags))
 		return;
 
 	dd = hci_open_dev(index);
@@ -160,12 +161,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 &&
-							errno != ENETDOWN) {
-		error("Can't set link policy on hci%d: %s (%d)",
-					index, strerror(errno), errno);
+	/* Set link policy for BR/EDR HCI devices */
+	if (hci_devinfo(index, &di) < 0)
+		goto fail;
+
+	if (is_bredr_hci_device_type(di.type)) {
+		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);
+		}
 	}
 
 	/* Start HCI device */
@@ -198,7 +204,8 @@ static void device_devreg_setup(int index)
 
 	devup = hci_test_bit(HCI_UP, &di.flags);
 
-	if (!hci_test_bit(HCI_RAW, &di.flags))
+	if (is_bredr_hci_device_type(di.type) &&
+	    !hci_test_bit(HCI_RAW, &di.flags))
 		manager_register_adapter(index, devup);
 }
 
diff --git a/src/adapter.c b/src/adapter.c
index 7d0e34d..acc1068 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2314,7 +2314,8 @@ 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 (!is_bredr_hci_device_type(di.type) ||
+	    hci_test_bit(HCI_RAW, &di.flags)) {
 		dev->ignore = 1;
 		return -1;
 	}
diff --git a/src/security.c b/src/security.c
index ca394e1..6d07d86 100644
--- a/src/security.c
+++ b/src/security.c
@@ -999,7 +999,8 @@ static gboolean io_security_event(GIOChannel *chan, GIOCondition cond,
 
 	ioctl(dev, HCIGETDEVINFO, (void *) di);
 
-	if (hci_test_bit(HCI_RAW, &di->flags))
+	if (!is_bredr_hci_device_type(di->type) ||
+	    hci_test_bit(HCI_RAW, &di->flags))
 		return TRUE;
 
 	switch (eh->evt) {
@@ -1185,7 +1186,8 @@ 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 (!is_bredr_hci_device_type(di->type) ||
+	    hci_test_bit(HCI_RAW, &di->flags))
 		return;
 
 	bacpy(&cp.bdaddr, BDADDR_ANY);
-- 
1.7.0
-- 
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/2] Remove non-functional hci_devinfo calls in init_device()

[Johan Hedberg]

Hi David,

On Mon, Aug 09, 2010, David Scherba wrote:
> ---
>  plugins/hciops.c |    6 ------
>  1 files changed, 0 insertions(+), 6 deletions(-)
> 
> diff --git a/plugins/hciops.c b/plugins/hciops.c
> index 5775cf1..b38c056 100644
> --- a/plugins/hciops.c
> +++ b/plugins/hciops.c
> @@ -175,12 +175,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);

Nice catch, but that's not quite enough:

plugins/hciops.c: In function ‘init_device’:
plugins/hciops.c:178: error: label ‘done’ defined but not used
plugins/hciops.c:125: error: unused variable ‘di’

Always check that your patch compiles cleanly with ./bootstrap-configure
before submitting upstream.

Johan

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

[Johan Hedberg]

Hi David,

On Mon, Aug 09, 2010, David Scherba wrote:
> +static inline int is_bredr_hci_device_type(uint8_t type)

Could you just call this ignore_device() which combines the AMP and RAW
checks.

> +{
> +  return (type >> 4) == HCI_BREDR;
> +}

Looks like the indentation is wrong inside the function (two spaces
instead of a tab).

> -	if (hci_test_bit(HCI_RAW, &di.flags))
> +	if (!is_bredr_hci_device_type(di.type) ||
> +	    hci_test_bit(HCI_RAW, &di.flags))

No spaces+tabs mixed indentation please. The second line should be
indented by at least two tabs more than the line above it (as many tabs
as you can as long as the line doesn't go beyond 79 characters). OTOH,
the second line goes away completely if you do the ignore_device
simplification as proposed above.

> -	if (!hci_test_bit(HCI_RAW, &di.flags))
> +	if (is_bredr_hci_device_type(di.type) &&
> +	    !hci_test_bit(HCI_RAW, &di.flags))

Same here.

> -	if (hci_test_bit(HCI_RAW, &di.flags)) {
> +	if (!is_bredr_hci_device_type(di.type) ||
> +	    hci_test_bit(HCI_RAW, &di.flags)) {

And here.

> -	if (hci_test_bit(HCI_RAW, &di->flags))
> +	if (!is_bredr_hci_device_type(di->type) ||
> +	    hci_test_bit(HCI_RAW, &di->flags))

And here.

> -	if (hci_test_bit(HCI_RAW, &di->flags))
> +	if (!is_bredr_hci_device_type(di->type) ||
> +	    hci_test_bit(HCI_RAW, &di->flags))

And here.

Johan

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

[David Scherba]

On 8/9/2010 4:51 PM, Johan Hedberg wrote:

> Nice catch, but that's not quite enough:
>
> plugins/hciops.c: In function ‘init_device’:
> plugins/hciops.c:178: error: label ‘done’ defined but not used
> plugins/hciops.c:125: error: unused variable ‘di’
>
> Always check that your patch compiles cleanly with ./bootstrap-configure
> before submitting upstream.

Johan--

Thanks for the tips--I had split-up a combined patch and neglected to 
ensure clean compilation and spacing after the split :-(.

New patch versions coming your way...

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.

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