[PATCH v4 00/14] Discovery support

[PATCH v4 00/14] Discovery support

[Andre Guedes]

Hi Gustavo,

I rebased the discovery series as you asked.

Andre. 

Andre Guedes (14):
  Bluetooth: Periodic Inquiry and mgmt discovering event
  Bluetooth: Add mgmt_discovery_complete()
  Bluetooth: Check pending command in start_discovery()
  Bluetooth: Check pending commands in stop_discovery()
  Bluetooth: Create hci_do_inquiry()
  Bluetooth: Create hci_cancel_inquiry()
  Bluetooth: Handle race condition in Discovery
  Bluetooth: Prepare for full support discovery procedures
  Bluetooth: Send mgmt_discovering events
  Bluetooth: Add 'eir_len' param to mgmt_device_found()
  Bluetooth: Report LE devices
  Bluetooth: LE scan infra-structure
  Bluetooth: Support LE-Only discovery procedure
  Bluetooth: Support BR/EDR/LE discovery procedure

 include/net/bluetooth/hci.h      |   12 ++++
 include/net/bluetooth/hci_core.h |   15 ++++-
 net/bluetooth/hci_core.c         |   96 +++++++++++++++++++++++++++
 net/bluetooth/hci_event.c        |  104 ++++++++++++++++++-----------
 net/bluetooth/mgmt.c             |  135 +++++++++++++++++++++++++++++++++++---
 5 files changed, 312 insertions(+), 50 deletions(-)

-- 
1.7.5.2

[PATCH v4 01/14] Bluetooth: Periodic Inquiry and mgmt discovering event

[Andre Guedes]

By using periodic inquiry command we're not able to detect correctly
when the controller has started inquiry.

Today we have this workaround in inquiry result event handler to set
the HCI_INQUIRY flag when it sees the first inquiry result event.
This workaround isn't enough because the device may be performing
an inquiry but the HCI_INQUIRY flag is not set. For instance, if
there is no device in range, no inquiry result event is generated,
consequently, the HCI_INQUIRY flags isn't set when it should so.

We rely on HCI_INQUIRY flag to implement the discovery procedure
properly. So, as we aren't able to clear/set the HCI_INQUIRY flag in
a reliable manner, periodic inquiry events shouldn't change the
HCI_INQUIRY flag. In future, if needed, we might add a new flag (e.g.
HCI_PINQUIRY) to know if the controller is performing periodic
inquiry.

Thus, due to that issue and in order to keep compatibility with
userspace, periodic inquiry events shouldn't send mgmt discovering
events.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_event.c |   43 +++++++++++--------------------------------
 1 files changed, 11 insertions(+), 32 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 35083f2..89faf48 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -58,9 +58,9 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
 	if (status)
 		return;
 
-	if (test_and_clear_bit(HCI_INQUIRY, &hdev->flags) &&
-			test_bit(HCI_MGMT, &hdev->flags))
-		mgmt_discovering(hdev->id, 0);
+	clear_bit(HCI_INQUIRY, &hdev->flags);
+
+	mgmt_discovering(hdev->id, 0);
 
 	hci_req_complete(hdev, HCI_OP_INQUIRY_CANCEL, status);
 
@@ -76,10 +76,6 @@ static void hci_cc_exit_periodic_inq(struct hci_dev *hdev, struct sk_buff *skb)
 	if (status)
 		return;
 
-	if (test_and_clear_bit(HCI_INQUIRY, &hdev->flags) &&
-				test_bit(HCI_MGMT, &hdev->flags))
-		mgmt_discovering(hdev->id, 0);
-
 	hci_conn_check_pending(hdev);
 }
 
@@ -958,9 +954,9 @@ static inline void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
 		return;
 	}
 
-	if (!test_and_set_bit(HCI_INQUIRY, &hdev->flags) &&
-				test_bit(HCI_MGMT, &hdev->flags))
-		mgmt_discovering(hdev->id, 1);
+	set_bit(HCI_INQUIRY, &hdev->flags);
+
+	mgmt_discovering(hdev->id, 1);
 }
 
 static inline void hci_cs_create_conn(struct hci_dev *hdev, __u8 status)
@@ -1339,13 +1335,14 @@ static inline void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff
 
 	BT_DBG("%s status %d", hdev->name, status);
 
-	if (test_and_clear_bit(HCI_INQUIRY, &hdev->flags) &&
-				test_bit(HCI_MGMT, &hdev->flags))
-		mgmt_discovering(hdev->id, 0);
-
 	hci_req_complete(hdev, HCI_OP_INQUIRY, status);
 
 	hci_conn_check_pending(hdev);
+
+	if (!test_and_clear_bit(HCI_INQUIRY, &hdev->flags))
+		return;
+
+	mgmt_discovering(hdev->id, 0);
 }
 
 static inline void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *skb)
@@ -1361,12 +1358,6 @@ static inline void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *
 
 	hci_dev_lock(hdev);
 
-	if (!test_and_set_bit(HCI_INQUIRY, &hdev->flags)) {
-
-		if (test_bit(HCI_MGMT, &hdev->flags))
-			mgmt_discovering(hdev->id, 1);
-	}
-
 	for (; num_rsp; num_rsp--, info++) {
 		bacpy(&data.bdaddr, &info->bdaddr);
 		data.pscan_rep_mode	= info->pscan_rep_mode;
@@ -2359,12 +2350,6 @@ static inline void hci_inquiry_result_with_rssi_evt(struct hci_dev *hdev, struct
 
 	hci_dev_lock(hdev);
 
-	if (!test_and_set_bit(HCI_INQUIRY, &hdev->flags)) {
-
-		if (test_bit(HCI_MGMT, &hdev->flags))
-			mgmt_discovering(hdev->id, 1);
-	}
-
 	if ((skb->len - 1) / num_rsp != sizeof(struct inquiry_info_with_rssi)) {
 		struct inquiry_info_with_rssi_and_pscan_mode *info;
 		info = (void *) (skb->data + 1);
@@ -2527,12 +2512,6 @@ static inline void hci_extended_inquiry_result_evt(struct hci_dev *hdev, struct
 	if (!num_rsp)
 		return;
 
-	if (!test_and_set_bit(HCI_INQUIRY, &hdev->flags)) {
-
-		if (test_bit(HCI_MGMT, &hdev->flags))
-			mgmt_discovering(hdev->id, 1);
-	}
-
 	hci_dev_lock(hdev);
 
 	for (; num_rsp; num_rsp--, info++) {
-- 
1.7.5.2

Re: [PATCH v4 01/14] Bluetooth: Periodic Inquiry and mgmt discovering event

[Marcel Holtmann]

Hi Andre,

> By using periodic inquiry command we're not able to detect correctly
> when the controller has started inquiry.
> 
> Today we have this workaround in inquiry result event handler to set
> the HCI_INQUIRY flag when it sees the first inquiry result event.
> This workaround isn't enough because the device may be performing
> an inquiry but the HCI_INQUIRY flag is not set. For instance, if
> there is no device in range, no inquiry result event is generated,
> consequently, the HCI_INQUIRY flags isn't set when it should so.
> 
> We rely on HCI_INQUIRY flag to implement the discovery procedure
> properly. So, as we aren't able to clear/set the HCI_INQUIRY flag in
> a reliable manner, periodic inquiry events shouldn't change the
> HCI_INQUIRY flag. In future, if needed, we might add a new flag (e.g.
> HCI_PINQUIRY) to know if the controller is performing periodic
> inquiry.
> 
> Thus, due to that issue and in order to keep compatibility with
> userspace, periodic inquiry events shouldn't send mgmt discovering
> events.

I spend some time thinking about this and yes, we need to clean this
mess up right now.

So intermixing inquiry with periodic inquiry was a bad idea and we
should split this. So I think internally hci_dev needs a variable to
track if we have enabled periodic inquiry or not. So it should express
if periodic inquiry is on or off. Nothing else since we should not
bother with trying to match periodic inquiry result to inquiry results.
I would actually go this far that we should ignore inquiry result events
from periodic inquiry.

Lets start creating some hci_dev internal state flags variable to track
certain states/modes of the controller. As I said earlier, overloading a
public API/ABI is not a good idea at all.

For tracking periodic inquiry state/mode we just need to be careful and
take an inquiry result outside of start inquiry and inquiry complete as
indication that periodic inquiry is active. There is still a potential
false positive as you mentioned, but that also should not matter since
periodic inquiry is suspending itself anyway. Important is just that we
track inquiry and periodic inquiry states separately.

Another assumption is that periodic inquiry is only used by special
applications and it is essentially triggered by 3rd party daemons doing
something special for their needs. If periodic inquiry is enabled, I am
even fine failing the mgmt_start_discovery command with an error.

So in conclusion, I wanna have us tracking if the controller is
currently doing periodic inquiry. And if you wanna export that state,
then lets do it via debugfs.

Regards

Marcel

Re: [PATCH v4 01/14] Bluetooth: Periodic Inquiry and mgmt discovering event

[Andre Guedes]

Hi Marcel,

On Sep 20, 2011, at 9:23 AM, Marcel Holtmann wrote:

> Hi Andre,
> 
>> By using periodic inquiry command we're not able to detect correctly
>> when the controller has started inquiry.
>> 
>> Today we have this workaround in inquiry result event handler to set
>> the HCI_INQUIRY flag when it sees the first inquiry result event.
>> This workaround isn't enough because the device may be performing
>> an inquiry but the HCI_INQUIRY flag is not set. For instance, if
>> there is no device in range, no inquiry result event is generated,
>> consequently, the HCI_INQUIRY flags isn't set when it should so.
>> 
>> We rely on HCI_INQUIRY flag to implement the discovery procedure
>> properly. So, as we aren't able to clear/set the HCI_INQUIRY flag in
>> a reliable manner, periodic inquiry events shouldn't change the
>> HCI_INQUIRY flag. In future, if needed, we might add a new flag (e.g.
>> HCI_PINQUIRY) to know if the controller is performing periodic
>> inquiry.
>> 
>> Thus, due to that issue and in order to keep compatibility with
>> userspace, periodic inquiry events shouldn't send mgmt discovering
>> events.
> 
> I spend some time thinking about this and yes, we need to clean this
> mess up right now.
> 
> So intermixing inquiry with periodic inquiry was a bad idea and we
> should split this. So I think internally hci_dev needs a variable to
> track if we have enabled periodic inquiry or not. So it should express
> if periodic inquiry is on or off. Nothing else since we should not
> bother with trying to match periodic inquiry result to inquiry results.
> I would actually go this far that we should ignore inquiry result events
> from periodic inquiry.
> 
> Lets start creating some hci_dev internal state flags variable to track
> certain states/modes of the controller. As I said earlier, overloading a
> public API/ABI is not a good idea at all.
> 
> For tracking periodic inquiry state/mode we just need to be careful and
> take an inquiry result outside of start inquiry and inquiry complete as
> indication that periodic inquiry is active. There is still a potential
> false positive as you mentioned, but that also should not matter since
> periodic inquiry is suspending itself anyway. Important is just that we
> track inquiry and periodic inquiry states separately.
> 
> Another assumption is that periodic inquiry is only used by special
> applications and it is essentially triggered by 3rd party daemons doing
> something special for their needs. If periodic inquiry is enabled, I am
> even fine failing the mgmt_start_discovery command with an error.
> 

I agree.

> So in conclusion, I wanna have us tracking if the controller is
> currently doing periodic inquiry. And if you wanna export that state,
> then lets do it via debugfs.
> 
> Regards
> 
> Marcel
> 
> 

Andre

Re: [PATCH v4 01/14] Bluetooth: Periodic Inquiry and mgmt discovering event

[Andre Guedes]

Hi Marcel,

On Tue, Sep 20, 2011 at 9:23 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andre,
>
>> By using periodic inquiry command we're not able to detect correctly
>> when the controller has started inquiry.
>>
>> Today we have this workaround in inquiry result event handler to set
>> the HCI_INQUIRY flag when it sees the first inquiry result event.
>> This workaround isn't enough because the device may be performing
>> an inquiry but the HCI_INQUIRY flag is not set. For instance, if
>> there is no device in range, no inquiry result event is generated,
>> consequently, the HCI_INQUIRY flags isn't set when it should so.
>>
>> We rely on HCI_INQUIRY flag to implement the discovery procedure
>> properly. So, as we aren't able to clear/set the HCI_INQUIRY flag in
>> a reliable manner, periodic inquiry events shouldn't change the
>> HCI_INQUIRY flag. In future, if needed, we might add a new flag (e.g.
>> HCI_PINQUIRY) to know if the controller is performing periodic
>> inquiry.
>>
>> Thus, due to that issue and in order to keep compatibility with
>> userspace, periodic inquiry events shouldn't send mgmt discovering
>> events.
>
> I spend some time thinking about this and yes, we need to clean this
> mess up right now.
>
> So intermixing inquiry with periodic inquiry was a bad idea and we
> should split this. So I think internally hci_dev needs a variable to
> track if we have enabled periodic inquiry or not. So it should express
> if periodic inquiry is on or off. Nothing else since we should not
> bother with trying to match periodic inquiry result to inquiry results.
> I would actually go this far that we should ignore inquiry result events
> from periodic inquiry.
>
> Lets start creating some hci_dev internal state flags variable to track
> certain states/modes of the controller. As I said earlier, overloading a
> public API/ABI is not a good idea at all.
>
> For tracking periodic inquiry state/mode we just need to be careful and
> take an inquiry result outside of start inquiry and inquiry complete as
> indication that periodic inquiry is active. There is still a potential
> false positive as you mentioned, but that also should not matter since
> periodic inquiry is suspending itself anyway. Important is just that we
> track inquiry and periodic inquiry states separately.
>
> Another assumption is that periodic inquiry is only used by special
> applications and it is essentially triggered by 3rd party daemons doing
> something special for their needs. If periodic inquiry is enabled, I am
> even fine failing the mgmt_start_discovery command with an error.
>
> So in conclusion, I wanna have us tracking if the controller is
> currently doing periodic inquiry. And if you wanna export that state,
> then lets do it via debugfs.

Since main discovery patches are now pushed upstream, I'll work on
this and send patches soon.

BR,

Andre

[PATCH v4 02/14] Bluetooth: Add mgmt_discovery_complete()

[Andre Guedes]

This patch adds the mgmt_discovery_complete() function which
removes pending discovery commands and sends command complete/
status events.

This function should be called when the discovery procedure finishes.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_event.c        |    7 +++++++
 net/bluetooth/mgmt.c             |   31 +++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 5b92442..df6fa85 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -875,6 +875,7 @@ int mgmt_remote_name(u16 index, bdaddr_t *bdaddr, u8 *name);
 int mgmt_discovering(u16 index, u8 discovering);
 int mgmt_device_blocked(u16 index, bdaddr_t *bdaddr);
 int mgmt_device_unblocked(u16 index, bdaddr_t *bdaddr);
+int mgmt_discovery_complete(u16 index, u8 status);
 
 /* HCI info for socket */
 #define hci_pi(sk) ((struct hci_pinfo *) sk)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 89faf48..b44f362 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -55,6 +55,8 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
 
 	BT_DBG("%s status 0x%x", hdev->name, status);
 
+	mgmt_discovery_complete(hdev->id, status);
+
 	if (status)
 		return;
 
@@ -951,6 +953,9 @@ static inline void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
 	if (status) {
 		hci_req_complete(hdev, HCI_OP_INQUIRY, status);
 		hci_conn_check_pending(hdev);
+
+		mgmt_discovery_complete(hdev->id, status);
+
 		return;
 	}
 
@@ -1342,6 +1347,8 @@ static inline void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff
 	if (!test_and_clear_bit(HCI_INQUIRY, &hdev->flags))
 		return;
 
+	mgmt_discovery_complete(hdev->id, 0);
+
 	mgmt_discovering(hdev->id, 0);
 }
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 5a94eec..cc0c204 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2378,3 +2378,34 @@ int mgmt_device_unblocked(u16 index, bdaddr_t *bdaddr)
 	return mgmt_event(MGMT_EV_DEVICE_UNBLOCKED, index, &ev, sizeof(ev),
 						cmd ? cmd->sk : NULL);
 }
+
+int mgmt_discovery_complete(u16 index, u8 status)
+{
+	struct pending_cmd *cmd_start;
+	struct pending_cmd *cmd_stop;
+	u8 discov_status = bt_to_errno(status);
+
+	cmd_start = mgmt_pending_find(MGMT_OP_START_DISCOVERY, index);
+	if (!cmd_start)
+		return -ENOENT;
+
+	BT_DBG("hci%u status %u", index, status);
+
+	cmd_stop = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, index);
+	if (cmd_stop && status == 0)
+		discov_status = ECANCELED;
+
+	if (discov_status)
+		cmd_status(cmd_start->sk, index, MGMT_OP_START_DISCOVERY,
+								discov_status);
+	else
+		cmd_complete(cmd_start->sk, index, MGMT_OP_START_DISCOVERY,
+								NULL, 0);
+
+	mgmt_pending_remove(cmd_start);
+
+	if (cmd_stop)
+		mgmt_pending_remove(cmd_stop);
+
+	return 0;
+}
-- 
1.7.5.2

Re: [PATCH v4 02/14] Bluetooth: Add mgmt_discovery_complete()

[Marcel Holtmann]

Hi Andre,

> This patch adds the mgmt_discovery_complete() function which
> removes pending discovery commands and sends command complete/
> status events.
> 
> This function should be called when the discovery procedure finishes.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci_core.h |    1 +
>  net/bluetooth/hci_event.c        |    7 +++++++
>  net/bluetooth/mgmt.c             |   31 +++++++++++++++++++++++++++++++
>  3 files changed, 39 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 5b92442..df6fa85 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -875,6 +875,7 @@ int mgmt_remote_name(u16 index, bdaddr_t *bdaddr, u8 *name);
>  int mgmt_discovering(u16 index, u8 discovering);
>  int mgmt_device_blocked(u16 index, bdaddr_t *bdaddr);
>  int mgmt_device_unblocked(u16 index, bdaddr_t *bdaddr);
> +int mgmt_discovery_complete(u16 index, u8 status);
>  
>  /* HCI info for socket */
>  #define hci_pi(sk) ((struct hci_pinfo *) sk)
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 89faf48..b44f362 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -55,6 +55,8 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
>  
>  	BT_DBG("%s status 0x%x", hdev->name, status);
>  
> +	mgmt_discovery_complete(hdev->id, status);
> +
>  	if (status)
>  		return;
>  
> @@ -951,6 +953,9 @@ static inline void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
>  	if (status) {
>  		hci_req_complete(hdev, HCI_OP_INQUIRY, status);
>  		hci_conn_check_pending(hdev);
> +
> +		mgmt_discovery_complete(hdev->id, status);
> +
>  		return;
>  	}
>  
> @@ -1342,6 +1347,8 @@ static inline void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff
>  	if (!test_and_clear_bit(HCI_INQUIRY, &hdev->flags))
>  		return;
>  
> +	mgmt_discovery_complete(hdev->id, 0);
> +
>  	mgmt_discovering(hdev->id, 0);
>  }
>  
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 5a94eec..cc0c204 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2378,3 +2378,34 @@ int mgmt_device_unblocked(u16 index, bdaddr_t *bdaddr)
>  	return mgmt_event(MGMT_EV_DEVICE_UNBLOCKED, index, &ev, sizeof(ev),
>  						cmd ? cmd->sk : NULL);
>  }
> +
> +int mgmt_discovery_complete(u16 index, u8 status)
> +{
> +	struct pending_cmd *cmd_start;
> +	struct pending_cmd *cmd_stop;
> +	u8 discov_status = bt_to_errno(status);
> +
> +	cmd_start = mgmt_pending_find(MGMT_OP_START_DISCOVERY, index);
> +	if (!cmd_start)
> +		return -ENOENT;
> +
> +	BT_DBG("hci%u status %u", index, status);
> +
> +	cmd_stop = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, index);
> +	if (cmd_stop && status == 0)
> +		discov_status = ECANCELED;
> +
> +	if (discov_status)
> +		cmd_status(cmd_start->sk, index, MGMT_OP_START_DISCOVERY,
> +								discov_status);
> +	else
> +		cmd_complete(cmd_start->sk, index, MGMT_OP_START_DISCOVERY,
> +								NULL, 0);
> +
> +	mgmt_pending_remove(cmd_start);
> +
> +	if (cmd_stop)
> +		mgmt_pending_remove(cmd_stop);
> +

doing this via an internal flags variable to track current states seems
a bit more efficient then these lookups. It also seems a bit cleaner
from a code point of view and easy to read and understand.

Can the race condition between cmd_status and cmd_complete really happen
in reality. I am thinking that we rather should not signal POLLOUT and
return an error on write if cmd_status has not yet been sent. The
cmd_status should be always immediate anyway.

Regards

Marcel

Re: [PATCH v4 02/14] Bluetooth: Add mgmt_discovery_complete()

[Andre Guedes]

Hi Marcel,

On Sep 20, 2011, at 9:29 AM, Marcel Holtmann wrote:

> Hi Andre,
> 
>> This patch adds the mgmt_discovery_complete() function which
>> removes pending discovery commands and sends command complete/
>> status events.
>> 
>> This function should be called when the discovery procedure finishes.
>> 
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/hci_core.h |    1 +
>> net/bluetooth/hci_event.c        |    7 +++++++
>> net/bluetooth/mgmt.c             |   31 +++++++++++++++++++++++++++++++
>> 3 files changed, 39 insertions(+), 0 deletions(-)
>> 
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index 5b92442..df6fa85 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -875,6 +875,7 @@ int mgmt_remote_name(u16 index, bdaddr_t *bdaddr, u8 *name);
>> int mgmt_discovering(u16 index, u8 discovering);
>> int mgmt_device_blocked(u16 index, bdaddr_t *bdaddr);
>> int mgmt_device_unblocked(u16 index, bdaddr_t *bdaddr);
>> +int mgmt_discovery_complete(u16 index, u8 status);
>> 
>> /* HCI info for socket */
>> #define hci_pi(sk) ((struct hci_pinfo *) sk)
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 89faf48..b44f362 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -55,6 +55,8 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
>> 
>> 	BT_DBG("%s status 0x%x", hdev->name, status);
>> 
>> +	mgmt_discovery_complete(hdev->id, status);
>> +
>> 	if (status)
>> 		return;
>> 
>> @@ -951,6 +953,9 @@ static inline void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
>> 	if (status) {
>> 		hci_req_complete(hdev, HCI_OP_INQUIRY, status);
>> 		hci_conn_check_pending(hdev);
>> +
>> +		mgmt_discovery_complete(hdev->id, status);
>> +
>> 		return;
>> 	}
>> 
>> @@ -1342,6 +1347,8 @@ static inline void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff
>> 	if (!test_and_clear_bit(HCI_INQUIRY, &hdev->flags))
>> 		return;
>> 
>> +	mgmt_discovery_complete(hdev->id, 0);
>> +
>> 	mgmt_discovering(hdev->id, 0);
>> }
>> 
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index 5a94eec..cc0c204 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -2378,3 +2378,34 @@ int mgmt_device_unblocked(u16 index, bdaddr_t *bdaddr)
>> 	return mgmt_event(MGMT_EV_DEVICE_UNBLOCKED, index, &ev, sizeof(ev),
>> 						cmd ? cmd->sk : NULL);
>> }
>> +
>> +int mgmt_discovery_complete(u16 index, u8 status)
>> +{
>> +	struct pending_cmd *cmd_start;
>> +	struct pending_cmd *cmd_stop;
>> +	u8 discov_status = bt_to_errno(status);
>> +
>> +	cmd_start = mgmt_pending_find(MGMT_OP_START_DISCOVERY, index);
>> +	if (!cmd_start)
>> +		return -ENOENT;
>> +
>> +	BT_DBG("hci%u status %u", index, status);
>> +
>> +	cmd_stop = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, index);
>> +	if (cmd_stop && status == 0)
>> +		discov_status = ECANCELED;
>> +
>> +	if (discov_status)
>> +		cmd_status(cmd_start->sk, index, MGMT_OP_START_DISCOVERY,
>> +								discov_status);
>> +	else
>> +		cmd_complete(cmd_start->sk, index, MGMT_OP_START_DISCOVERY,
>> +								NULL, 0);
>> +
>> +	mgmt_pending_remove(cmd_start);
>> +
>> +	if (cmd_stop)
>> +		mgmt_pending_remove(cmd_stop);
>> +
> 
> doing this via an internal flags variable to track current states seems
> a bit more efficient then these lookups. It also seems a bit cleaner
> from a code point of view and easy to read and understand.

I see this as a tradeoff. To implement this mgmt internal flag we
would add extra code to control it and this would make the code
more complex. Moreover, this extra logic would tell us the same
information we already have by looking up discovery commands in
mgmt pending list.

About the code performance you highlighted, most of the time the
mgmt pending list have only a few elements. So, I don't see those
lookups degrading the overall code performance.

BTW, we'll need those lookups anyway since the discovery commands
must be removed from the pending list once the discovery terminates.

> 
> Can the race condition between cmd_status and cmd_complete really happen
> in reality. I am thinking that we rather should not signal POLLOUT and
> return an error on write if cmd_status has not yet been sent. The
> cmd_status should be always immediate anyway.

BR,

Andre

[PATCH v4 03/14] Bluetooth: Check pending command in start_discovery()

[Andre Guedes]

If discovery procedure is already running then EINPROGRESS command
status should be returned.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/mgmt.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index cc0c204..d8333e0 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1622,6 +1622,12 @@ static int start_discovery(struct sock *sk, u16 index)
 
 	hci_dev_lock_bh(hdev);
 
+	if (mgmt_pending_find(MGMT_OP_START_DISCOVERY, index)) {
+		err = cmd_status(sk, index, MGMT_OP_START_DISCOVERY,
+								EINPROGRESS);
+		goto failed;
+	}
+
 	cmd = mgmt_pending_add(sk, MGMT_OP_START_DISCOVERY, index, NULL, 0);
 	if (!cmd) {
 		err = -ENOMEM;
-- 
1.7.5.2

Re: [PATCH v4 03/14] Bluetooth: Check pending command in start_discovery()

[Marcel Holtmann]

Hi Andre,

> If discovery procedure is already running then EINPROGRESS command
> status should be returned.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  net/bluetooth/mgmt.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index cc0c204..d8333e0 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1622,6 +1622,12 @@ static int start_discovery(struct sock *sk, u16 index)
>  
>  	hci_dev_lock_bh(hdev);
>  
> +	if (mgmt_pending_find(MGMT_OP_START_DISCOVERY, index)) {
> +		err = cmd_status(sk, index, MGMT_OP_START_DISCOVERY,
> +								EINPROGRESS);
> +		goto failed;
> +	}
> +

the idea is correct, but instead of using mgmt_pending_find we should
have an internal flags field to track our current states. That is way
more efficient then looking for pending commands.

Regards

Marcel

Re: [PATCH v4 03/14] Bluetooth: Check pending command in start_discovery()

[Andre Guedes]

Hi Marcel,

On Sep 20, 2011, at 9:26 AM, Marcel Holtmann wrote:

> Hi Andre,
> 
>> If discovery procedure is already running then EINPROGRESS command
>> status should be returned.
>> 
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> net/bluetooth/mgmt.c |    6 ++++++
>> 1 files changed, 6 insertions(+), 0 deletions(-)
>> 
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index cc0c204..d8333e0 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -1622,6 +1622,12 @@ static int start_discovery(struct sock *sk, u16 index)
>> 
>> 	hci_dev_lock_bh(hdev);
>> 
>> +	if (mgmt_pending_find(MGMT_OP_START_DISCOVERY, index)) {
>> +		err = cmd_status(sk, index, MGMT_OP_START_DISCOVERY,
>> +								EINPROGRESS);
>> +		goto failed;
>> +	}
>> +
> 
> the idea is correct, but instead of using mgmt_pending_find we should
> have an internal flags field to track our current states. That is way
> more efficient then looking for pending commands.


See my reply about having an mgmt internal flag on patch 02/14.

Andre

[PATCH v4 04/14] Bluetooth: Check pending commands in stop_discovery()

[Andre Guedes]

This patch adds extra checks in stop_discovery().

The MGMT_OP_STOP_DISCOVERY command should be executed if the device
is running the discovery procedure. So, if there is no discovery
procedure running then EINVAL command status should be returned.

Also, if a MGMT_OP_STOP_DISCOVERY command has been already issued
then EINPROGRESS command status should returned.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/mgmt.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index d8333e0..5e1414b 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1664,6 +1664,17 @@ static int stop_discovery(struct sock *sk, u16 index)
 
 	hci_dev_lock_bh(hdev);
 
+	if (!mgmt_pending_find(MGMT_OP_START_DISCOVERY, index)) {
+		err = cmd_status(sk, index, MGMT_OP_STOP_DISCOVERY, EINVAL);
+		goto failed;
+	}
+
+	if (mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, index)) {
+		err = cmd_status(sk, index, MGMT_OP_STOP_DISCOVERY,
+								EINPROGRESS);
+		goto failed;
+	}
+
 	cmd = mgmt_pending_add(sk, MGMT_OP_STOP_DISCOVERY, index, NULL, 0);
 	if (!cmd) {
 		err = -ENOMEM;
-- 
1.7.5.2

[PATCH v4 05/14] Bluetooth: Create hci_do_inquiry()

[Andre Guedes]

This patch adds a function to hci_core to carry out inquiry.

All inquiry code from start_discovery() were replaced by a
hci_do_inquiry() call.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |    2 ++
 net/bluetooth/hci_core.c         |   17 +++++++++++++++++
 net/bluetooth/mgmt.c             |    9 +--------
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index df6fa85..b7c070b 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -914,4 +914,6 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
 void hci_le_ltk_reply(struct hci_conn *conn, u8 ltk[16]);
 void hci_le_ltk_neg_reply(struct hci_conn *conn);
 
+int hci_do_inquiry(struct hci_dev *hdev, u8 length);
+
 #endif /* __HCI_CORE_H */
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 04bc31d..ebfd963 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2405,3 +2405,20 @@ static void hci_cmd_task(unsigned long arg)
 		}
 	}
 }
+
+int hci_do_inquiry(struct hci_dev *hdev, u8 length)
+{
+	u8 lap[3] = {0x33, 0x8b, 0x9e};
+	struct hci_cp_inquiry cp;
+
+	BT_DBG("%s", hdev->name);
+
+	if (test_bit(HCI_INQUIRY, &hdev->flags))
+		return -EPERM;
+
+	memset(&cp, 0, sizeof(cp));
+	memcpy(&cp.lap, lap, 3);
+	cp.length  = length;
+
+	return hci_send_cmd(hdev, HCI_OP_INQUIRY, sizeof(cp), &cp);
+}
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 5e1414b..4f77468 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1608,8 +1608,6 @@ static int remove_remote_oob_data(struct sock *sk, u16 index,
 
 static int start_discovery(struct sock *sk, u16 index)
 {
-	u8 lap[3] = { 0x33, 0x8b, 0x9e };
-	struct hci_cp_inquiry cp;
 	struct pending_cmd *cmd;
 	struct hci_dev *hdev;
 	int err;
@@ -1634,12 +1632,7 @@ static int start_discovery(struct sock *sk, u16 index)
 		goto failed;
 	}
 
-	memset(&cp, 0, sizeof(cp));
-	memcpy(&cp.lap, lap, 3);
-	cp.length  = 0x08;
-	cp.num_rsp = 0x00;
-
-	err = hci_send_cmd(hdev, HCI_OP_INQUIRY, sizeof(cp), &cp);
+	err = hci_do_inquiry(hdev, 0x08);
 	if (err < 0)
 		mgmt_pending_remove(cmd);
 
-- 
1.7.5.2

Re: [PATCH v4 05/14] Bluetooth: Create hci_do_inquiry()

[Marcel Holtmann]

Hi Andre,

> This patch adds a function to hci_core to carry out inquiry.
> 
> All inquiry code from start_discovery() were replaced by a
> hci_do_inquiry() call.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci_core.h |    2 ++
>  net/bluetooth/hci_core.c         |   17 +++++++++++++++++
>  net/bluetooth/mgmt.c             |    9 +--------
>  3 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index df6fa85..b7c070b 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -914,4 +914,6 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
>  void hci_le_ltk_reply(struct hci_conn *conn, u8 ltk[16]);
>  void hci_le_ltk_neg_reply(struct hci_conn *conn);
>  
> +int hci_do_inquiry(struct hci_dev *hdev, u8 length);
> +
>  #endif /* __HCI_CORE_H */
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 04bc31d..ebfd963 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2405,3 +2405,20 @@ static void hci_cmd_task(unsigned long arg)
>  		}
>  	}
>  }
> +
> +int hci_do_inquiry(struct hci_dev *hdev, u8 length)
> +{
> +	u8 lap[3] = {0x33, 0x8b, 0x9e};

{ 0x33, ... 0x9e } with proper space please. Otherwise.

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

Regards

Marcel

Re: [PATCH v4 05/14] Bluetooth: Create hci_do_inquiry()

[Andre Guedes]

Hi Marcel,

On Sep 20, 2011, at 9:31 AM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>> This patch adds a function to hci_core to carry out inquiry.
>>=20
>> All inquiry code from start_discovery() were replaced by a
>> hci_do_inquiry() call.
>>=20
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/hci_core.h |    2 ++
>> net/bluetooth/hci_core.c         |   17 +++++++++++++++++
>> net/bluetooth/mgmt.c             |    9 +--------
>> 3 files changed, 20 insertions(+), 8 deletions(-)
>>=20
>> diff --git a/include/net/bluetooth/hci_core.h =
b/include/net/bluetooth/hci_core.h
>> index df6fa85..b7c070b 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -914,4 +914,6 @@ void hci_le_start_enc(struct hci_conn *conn, =
__le16 ediv, __u8 rand[8],
>> void hci_le_ltk_reply(struct hci_conn *conn, u8 ltk[16]);
>> void hci_le_ltk_neg_reply(struct hci_conn *conn);
>>=20
>> +int hci_do_inquiry(struct hci_dev *hdev, u8 length);
>> +
>> #endif /* __HCI_CORE_H */
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 04bc31d..ebfd963 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -2405,3 +2405,20 @@ static void hci_cmd_task(unsigned long arg)
>> 		}
>> 	}
>> }
>> +
>> +int hci_do_inquiry(struct hci_dev *hdev, u8 length)
>> +{
>> +	u8 lap[3] =3D {0x33, 0x8b, 0x9e};
>=20
> { 0x33, ... 0x9e } with proper space please. Otherwise.
>=20
> Acked-by: Marcel Holtmann <marcel@holtmann.org>

Sure, I'll fix it.

Andre=

[PATCH v4 06/14] Bluetooth: Create hci_cancel_inquiry()

[Andre Guedes]

This patch adds a function to hci_core to cancel an ongoing inquiry.

According to the Bluetooth spec, the inquiry cancel command should
only be issued after the inquiry command has been issued, a command
status event has been received for the inquiry command, and before
the inquiry complete event occurs.

As HCI_INQUIRY flag is only set just after an inquiry command status
event occurs and it is cleared just after an inquiry complete event
occurs, the inquiry cancel command should be issued only if HCI_INQUIRY
flag is set.

Additionally, cancel inquiry related code from stop_discovery() were
replaced by a hci_cancel_inquiry() call.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_core.c         |   10 ++++++++++
 net/bluetooth/mgmt.c             |    2 +-
 3 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index b7c070b..4a79a50 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -915,5 +915,6 @@ void hci_le_ltk_reply(struct hci_conn *conn, u8 ltk[16]);
 void hci_le_ltk_neg_reply(struct hci_conn *conn);
 
 int hci_do_inquiry(struct hci_dev *hdev, u8 length);
+int hci_cancel_inquiry(struct hci_dev *hdev);
 
 #endif /* __HCI_CORE_H */
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index ebfd963..f62b465 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2422,3 +2422,13 @@ int hci_do_inquiry(struct hci_dev *hdev, u8 length)
 
 	return hci_send_cmd(hdev, HCI_OP_INQUIRY, sizeof(cp), &cp);
 }
+
+int hci_cancel_inquiry(struct hci_dev *hdev)
+{
+	BT_DBG("%s", hdev->name);
+
+	if (!test_bit(HCI_INQUIRY, &hdev->flags))
+		return -EPERM;
+
+	return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
+}
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 4f77468..d84f242 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1674,7 +1674,7 @@ static int stop_discovery(struct sock *sk, u16 index)
 		goto failed;
 	}
 
-	err = hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
+	err = hci_cancel_inquiry(hdev);
 	if (err < 0)
 		mgmt_pending_remove(cmd);
 
-- 
1.7.5.2

Re: [PATCH v4 06/14] Bluetooth: Create hci_cancel_inquiry()

[Marcel Holtmann]

Hi Andre,

> This patch adds a function to hci_core to cancel an ongoing inquiry.
> 
> According to the Bluetooth spec, the inquiry cancel command should
> only be issued after the inquiry command has been issued, a command
> status event has been received for the inquiry command, and before
> the inquiry complete event occurs.
> 
> As HCI_INQUIRY flag is only set just after an inquiry command status
> event occurs and it is cleared just after an inquiry complete event
> occurs, the inquiry cancel command should be issued only if HCI_INQUIRY
> flag is set.
> 
> Additionally, cancel inquiry related code from stop_discovery() were
> replaced by a hci_cancel_inquiry() call.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci_core.h |    1 +
>  net/bluetooth/hci_core.c         |   10 ++++++++++
>  net/bluetooth/mgmt.c             |    2 +-
>  3 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index b7c070b..4a79a50 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -915,5 +915,6 @@ void hci_le_ltk_reply(struct hci_conn *conn, u8 ltk[16]);
>  void hci_le_ltk_neg_reply(struct hci_conn *conn);
>  
>  int hci_do_inquiry(struct hci_dev *hdev, u8 length);
> +int hci_cancel_inquiry(struct hci_dev *hdev);
>  
>  #endif /* __HCI_CORE_H */
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index ebfd963..f62b465 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2422,3 +2422,13 @@ int hci_do_inquiry(struct hci_dev *hdev, u8 length)
>  
>  	return hci_send_cmd(hdev, HCI_OP_INQUIRY, sizeof(cp), &cp);
>  }
> +
> +int hci_cancel_inquiry(struct hci_dev *hdev)
> +{
> +	BT_DBG("%s", hdev->name);
> +
> +	if (!test_bit(HCI_INQUIRY, &hdev->flags))
> +		return -EPERM;
> +
> +	return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
> +}
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 4f77468..d84f242 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1674,7 +1674,7 @@ static int stop_discovery(struct sock *sk, u16 index)
>  		goto failed;
>  	}
>  
> -	err = hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
> +	err = hci_cancel_inquiry(hdev);
>  	if (err < 0)
>  		mgmt_pending_remove(cmd);
>  

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

Regards

Marcel

[PATCH v4 07/14] Bluetooth: Handle race condition in Discovery

[Andre Guedes]

If MGMT_OP_STOP_DISCOVERY command is issued before the kernel
receives the HCI Inquiry Command Status Event from the controller
then that command will fail and the discovery procedure won't be
stopped. This situation may occur if a MGMT_OP_STOP_DISCOVERY
command is issued just after a MGMT_OP_START_DISCOVERY.

This race condition can be handled by checking for pending
MGMT_OP_STOP_DISCOVERY command in inquiry command status event
handler. If we have a pending MGMT_OP_STOP_DISCOVERY command we
cancel the ongoing discovery.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_event.c        |    3 +++
 net/bluetooth/mgmt.c             |   14 +++++++++++++-
 3 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 4a79a50..36e15cc 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -876,6 +876,7 @@ int mgmt_discovering(u16 index, u8 discovering);
 int mgmt_device_blocked(u16 index, bdaddr_t *bdaddr);
 int mgmt_device_unblocked(u16 index, bdaddr_t *bdaddr);
 int mgmt_discovery_complete(u16 index, u8 status);
+int mgmt_has_pending_stop_discov(u16 index);
 
 /* HCI info for socket */
 #define hci_pi(sk) ((struct hci_pinfo *) sk)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index b44f362..c9d641b 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -962,6 +962,9 @@ static inline void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
 	set_bit(HCI_INQUIRY, &hdev->flags);
 
 	mgmt_discovering(hdev->id, 1);
+
+	if (mgmt_has_pending_stop_discov(hdev->id))
+		hci_cancel_inquiry(hdev);
 }
 
 static inline void hci_cs_create_conn(struct hci_dev *hdev, __u8 status)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index d84f242..58cf33a 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1674,7 +1674,11 @@ static int stop_discovery(struct sock *sk, u16 index)
 		goto failed;
 	}
 
-	err = hci_cancel_inquiry(hdev);
+	if (test_bit(HCI_INQUIRY, &hdev->flags))
+		err = hci_cancel_inquiry(hdev);
+	else
+		err = 0;
+
 	if (err < 0)
 		mgmt_pending_remove(cmd);
 
@@ -2419,3 +2423,11 @@ int mgmt_discovery_complete(u16 index, u8 status)
 
 	return 0;
 }
+
+int mgmt_has_pending_stop_discov(u16 index)
+{
+	if (mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, index))
+		return 1;
+
+	return 0;
+}
-- 
1.7.5.2

Re: [PATCH v4 07/14] Bluetooth: Handle race condition in Discovery

[Marcel Holtmann]

Hi Andre,

> If MGMT_OP_STOP_DISCOVERY command is issued before the kernel
> receives the HCI Inquiry Command Status Event from the controller
> then that command will fail and the discovery procedure won't be
> stopped. This situation may occur if a MGMT_OP_STOP_DISCOVERY
> command is issued just after a MGMT_OP_START_DISCOVERY.
> 
> This race condition can be handled by checking for pending
> MGMT_OP_STOP_DISCOVERY command in inquiry command status event
> handler. If we have a pending MGMT_OP_STOP_DISCOVERY command we
> cancel the ongoing discovery.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci_core.h |    1 +
>  net/bluetooth/hci_event.c        |    3 +++
>  net/bluetooth/mgmt.c             |   14 +++++++++++++-
>  3 files changed, 17 insertions(+), 1 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 4a79a50..36e15cc 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -876,6 +876,7 @@ int mgmt_discovering(u16 index, u8 discovering);
>  int mgmt_device_blocked(u16 index, bdaddr_t *bdaddr);
>  int mgmt_device_unblocked(u16 index, bdaddr_t *bdaddr);
>  int mgmt_discovery_complete(u16 index, u8 status);
> +int mgmt_has_pending_stop_discov(u16 index);
>  
>  /* HCI info for socket */
>  #define hci_pi(sk) ((struct hci_pinfo *) sk)
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index b44f362..c9d641b 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -962,6 +962,9 @@ static inline void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
>  	set_bit(HCI_INQUIRY, &hdev->flags);
>  
>  	mgmt_discovering(hdev->id, 1);
> +
> +	if (mgmt_has_pending_stop_discov(hdev->id))
> +		hci_cancel_inquiry(hdev);
>  }
>  
>  static inline void hci_cs_create_conn(struct hci_dev *hdev, __u8 status)
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index d84f242..58cf33a 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1674,7 +1674,11 @@ static int stop_discovery(struct sock *sk, u16 index)
>  		goto failed;
>  	}
>  
> -	err = hci_cancel_inquiry(hdev);
> +	if (test_bit(HCI_INQUIRY, &hdev->flags))
> +		err = hci_cancel_inquiry(hdev);
> +	else
> +		err = 0;
> +

do we really just wanna always return success here? Even if
stop_discovery is called for a none existing discovery.

And btw. since you just changed the hci_cancel_inquiry() to return
-EPERM if the HCI_INQUIRY flag is not set you could do this simpler by
just checking the return value directly. No double check of the
HCI_INQUIRY flag.

Regards

Marcel

Re: [PATCH v4 07/14] Bluetooth: Handle race condition in Discovery

[Andre Guedes]

Hi Marcel,

On Sep 20, 2011, at 9:37 AM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>> If MGMT_OP_STOP_DISCOVERY command is issued before the kernel
>> receives the HCI Inquiry Command Status Event from the controller
>> then that command will fail and the discovery procedure won't be
>> stopped. This situation may occur if a MGMT_OP_STOP_DISCOVERY
>> command is issued just after a MGMT_OP_START_DISCOVERY.
>>=20
>> This race condition can be handled by checking for pending
>> MGMT_OP_STOP_DISCOVERY command in inquiry command status event
>> handler. If we have a pending MGMT_OP_STOP_DISCOVERY command we
>> cancel the ongoing discovery.
>>=20
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/hci_core.h |    1 +
>> net/bluetooth/hci_event.c        |    3 +++
>> net/bluetooth/mgmt.c             |   14 +++++++++++++-
>> 3 files changed, 17 insertions(+), 1 deletions(-)
>>=20
>> diff --git a/include/net/bluetooth/hci_core.h =
b/include/net/bluetooth/hci_core.h
>> index 4a79a50..36e15cc 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -876,6 +876,7 @@ int mgmt_discovering(u16 index, u8 discovering);
>> int mgmt_device_blocked(u16 index, bdaddr_t *bdaddr);
>> int mgmt_device_unblocked(u16 index, bdaddr_t *bdaddr);
>> int mgmt_discovery_complete(u16 index, u8 status);
>> +int mgmt_has_pending_stop_discov(u16 index);
>>=20
>> /* HCI info for socket */
>> #define hci_pi(sk) ((struct hci_pinfo *) sk)
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index b44f362..c9d641b 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -962,6 +962,9 @@ static inline void hci_cs_inquiry(struct hci_dev =
*hdev, __u8 status)
>> 	set_bit(HCI_INQUIRY, &hdev->flags);
>>=20
>> 	mgmt_discovering(hdev->id, 1);
>> +
>> +	if (mgmt_has_pending_stop_discov(hdev->id))
>> +		hci_cancel_inquiry(hdev);
>> }
>>=20
>> static inline void hci_cs_create_conn(struct hci_dev *hdev, __u8 =
status)
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index d84f242..58cf33a 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -1674,7 +1674,11 @@ static int stop_discovery(struct sock *sk, u16 =
index)
>> 		goto failed;
>> 	}
>>=20
>> -	err =3D hci_cancel_inquiry(hdev);
>> +	if (test_bit(HCI_INQUIRY, &hdev->flags))
>> +		err =3D hci_cancel_inquiry(hdev);
>> +	else
>> +		err =3D 0;
>> +
>=20
> do we really just wanna always return success here? Even if
> stop_discovery is called for a none existing discovery.

If stop_discovery() is called for a none existing discovery
this code isn't even reached since stop_discovery() will
return in MGMT_OP_START_DISCOVERY pending command check at=20
the beginning of stop_discovery().

If we have an ongoing discovery but the HCI_INQUIRY flag isn't
set, then we are in the race condition window. Only in that case,
we return 0 and postpone the discovery cancel operation.

> And btw. since you just changed the hci_cancel_inquiry() to return
> -EPERM if the HCI_INQUIRY flag is not set you could do this simpler by
> just checking the return value directly. No double check of the
> HCI_INQUIRY flag.

Yes, I could have done like this, but since in a further patch I'll
add the HCI_LE_SCAN flag check I've done his way here (please see
patch 13/14).

The HCI_INQUIRY check in hci_cancel_inquiry() is to make sure no
HCI_OP_CANCEL_INQUIRY command is issued unless the controller is
in inquiry mode (spec recommendations).

The HCI_INQUIRY (and HCI_LE_SCAN) check in stop_discovery() stands
for sending the right command to cancel the ongoing discovery.

Andre=

[PATCH v4 08/14] Bluetooth: Prepare for full support discovery procedures

[Andre Guedes]

This patch prepares start_discovery() to support LE-Only and BR/EDR/LE
discovery procedures (BR/EDR is already supported).

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci.h      |    1 +
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/mgmt.c             |   10 +++++++++-
 3 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index aaf79af..1e11e7f 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -202,6 +202,7 @@ enum {
 
 #define LMP_EV4		0x01
 #define LMP_EV5		0x02
+#define LMP_NO_BREDR	0x20
 #define LMP_LE		0x40
 
 #define LMP_SNIFF_SUBR	0x02
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 36e15cc..e0c9790 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -613,6 +613,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
 #define lmp_esco_capable(dev)      ((dev)->features[3] & LMP_ESCO)
 #define lmp_ssp_capable(dev)       ((dev)->features[6] & LMP_SIMPLE_PAIR)
 #define lmp_no_flush_capable(dev)  ((dev)->features[6] & LMP_NO_FLUSH)
+#define lmp_bredr_capable(dev)     (!((dev)->features[4] & LMP_NO_BREDR))
 #define lmp_le_capable(dev)        ((dev)->features[4] & LMP_LE)
 
 /* ----- Extended LMP capabilities ----- */
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 58cf33a..3a0815b 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -32,6 +32,8 @@
 #define MGMT_VERSION	0
 #define MGMT_REVISION	1
 
+#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
+
 struct pending_cmd {
 	struct list_head list;
 	__u16 opcode;
@@ -1632,7 +1634,13 @@ static int start_discovery(struct sock *sk, u16 index)
 		goto failed;
 	}
 
-	err = hci_do_inquiry(hdev, 0x08);
+	if (lmp_host_le_capable(hdev) && lmp_bredr_capable(hdev))
+		err = -ENOSYS;
+	else if (lmp_host_le_capable(hdev))
+		err = -ENOSYS;
+	else
+		err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
+
 	if (err < 0)
 		mgmt_pending_remove(cmd);
 
-- 
1.7.5.2

Re: [PATCH v4 08/14] Bluetooth: Prepare for full support discovery procedures

[Marcel Holtmann]

Hi Andre,

> This patch prepares start_discovery() to support LE-Only and BR/EDR/LE
> discovery procedures (BR/EDR is already supported).
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci.h      |    1 +
>  include/net/bluetooth/hci_core.h |    1 +
>  net/bluetooth/mgmt.c             |   10 +++++++++-
>  3 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index aaf79af..1e11e7f 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -202,6 +202,7 @@ enum {
>  
>  #define LMP_EV4		0x01
>  #define LMP_EV5		0x02
> +#define LMP_NO_BREDR	0x20
>  #define LMP_LE		0x40
>  
>  #define LMP_SNIFF_SUBR	0x02
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 36e15cc..e0c9790 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -613,6 +613,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
>  #define lmp_esco_capable(dev)      ((dev)->features[3] & LMP_ESCO)
>  #define lmp_ssp_capable(dev)       ((dev)->features[6] & LMP_SIMPLE_PAIR)
>  #define lmp_no_flush_capable(dev)  ((dev)->features[6] & LMP_NO_FLUSH)
> +#define lmp_bredr_capable(dev)     (!((dev)->features[4] & LMP_NO_BREDR))
>  #define lmp_le_capable(dev)        ((dev)->features[4] & LMP_LE)
>  
>  /* ----- Extended LMP capabilities ----- */
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 58cf33a..3a0815b 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -32,6 +32,8 @@
>  #define MGMT_VERSION	0
>  #define MGMT_REVISION	1
>  
> +#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
> +
>  struct pending_cmd {
>  	struct list_head list;
>  	__u16 opcode;
> @@ -1632,7 +1634,13 @@ static int start_discovery(struct sock *sk, u16 index)
>  		goto failed;
>  	}
>  
> -	err = hci_do_inquiry(hdev, 0x08);
> +	if (lmp_host_le_capable(hdev) && lmp_bredr_capable(hdev))
> +		err = -ENOSYS;
> +	else if (lmp_host_le_capable(hdev))
> +		err = -ENOSYS;
> +	else
> +		err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
> +

this if statement is ugly. I think you want something like this:

	if (lmp_host_le_capable()) {
		if (lmp_bredr_capable()) {
		} else {
		}
	} else
		hci_do_inquiry()

>  	if (err < 0)
>  		mgmt_pending_remove(cmd);
>  

Regards

Marcel

Re: [PATCH v4 08/14] Bluetooth: Prepare for full support discovery procedures

[Andre Guedes]

Hi Marcel,

On Sep 20, 2011, at 9:43 AM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>> This patch prepares start_discovery() to support LE-Only and =
BR/EDR/LE
>> discovery procedures (BR/EDR is already supported).
>>=20
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/hci.h      |    1 +
>> include/net/bluetooth/hci_core.h |    1 +
>> net/bluetooth/mgmt.c             |   10 +++++++++-
>> 3 files changed, 11 insertions(+), 1 deletions(-)
>>=20
>> diff --git a/include/net/bluetooth/hci.h =
b/include/net/bluetooth/hci.h
>> index aaf79af..1e11e7f 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -202,6 +202,7 @@ enum {
>>=20
>> #define LMP_EV4		0x01
>> #define LMP_EV5		0x02
>> +#define LMP_NO_BREDR	0x20
>> #define LMP_LE		0x40
>>=20
>> #define LMP_SNIFF_SUBR	0x02
>> diff --git a/include/net/bluetooth/hci_core.h =
b/include/net/bluetooth/hci_core.h
>> index 36e15cc..e0c9790 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -613,6 +613,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
>> #define lmp_esco_capable(dev)      ((dev)->features[3] & LMP_ESCO)
>> #define lmp_ssp_capable(dev)       ((dev)->features[6] & =
LMP_SIMPLE_PAIR)
>> #define lmp_no_flush_capable(dev)  ((dev)->features[6] & =
LMP_NO_FLUSH)
>> +#define lmp_bredr_capable(dev)     (!((dev)->features[4] & =
LMP_NO_BREDR))
>> #define lmp_le_capable(dev)        ((dev)->features[4] & LMP_LE)
>>=20
>> /* ----- Extended LMP capabilities ----- */
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index 58cf33a..3a0815b 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -32,6 +32,8 @@
>> #define MGMT_VERSION	0
>> #define MGMT_REVISION	1
>>=20
>> +#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
>> +
>> struct pending_cmd {
>> 	struct list_head list;
>> 	__u16 opcode;
>> @@ -1632,7 +1634,13 @@ static int start_discovery(struct sock *sk, =
u16 index)
>> 		goto failed;
>> 	}
>>=20
>> -	err =3D hci_do_inquiry(hdev, 0x08);
>> +	if (lmp_host_le_capable(hdev) && lmp_bredr_capable(hdev))
>> +		err =3D -ENOSYS;
>> +	else if (lmp_host_le_capable(hdev))
>> +		err =3D -ENOSYS;
>> +	else
>> +		err =3D hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
>> +
>=20
> this if statement is ugly. I think you want something like this:
>=20
> 	if (lmp_host_le_capable()) {
> 		if (lmp_bredr_capable()) {
> 		} else {
> 		}
> 	} else
> 		hci_do_inquiry()

Ok, I'll change this.

Andre=

[PATCH v4 09/14] Bluetooth: Send mgmt_discovering events

[Andre Guedes]

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_event.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index c9d641b..47abba4 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -897,12 +897,16 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 		return;
 
 	if (cp->enable == 0x01) {
+		mgmt_discovering(hdev->id, 1);
+
 		del_timer(&hdev->adv_timer);
 
 		hci_dev_lock(hdev);
 		hci_adv_entries_clear(hdev);
 		hci_dev_unlock(hdev);
 	} else if (cp->enable == 0x00) {
+		mgmt_discovering(hdev->id, 0);
+
 		mod_timer(&hdev->adv_timer, jiffies + ADV_CLEAR_TIMEOUT);
 	}
 }
-- 
1.7.5.2

Re: [PATCH v4 09/14] Bluetooth: Send mgmt_discovering events

[Marcel Holtmann]

Hi Andre,

> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  net/bluetooth/hci_event.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index c9d641b..47abba4 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -897,12 +897,16 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>  		return;
>  
>  	if (cp->enable == 0x01) {
> +		mgmt_discovering(hdev->id, 1);
> +
>  		del_timer(&hdev->adv_timer);
>  
>  		hci_dev_lock(hdev);
>  		hci_adv_entries_clear(hdev);
>  		hci_dev_unlock(hdev);
>  	} else if (cp->enable == 0x00) {
> +		mgmt_discovering(hdev->id, 0);
> +

do we wanna send discovering on/off for BR/EDR and then again for LE.
Don't we wanna have something like this:

	discovering on
	BR/EDR inquiry
	LE scan
	discovering off

Regards

Marcel

Re: [PATCH v4 09/14] Bluetooth: Send mgmt_discovering events

[Andre Guedes]

Hi Marcel,

On Sep 20, 2011, at 9:45 AM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> net/bluetooth/hci_event.c |    4 ++++
>> 1 files changed, 4 insertions(+), 0 deletions(-)
>>=20
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index c9d641b..47abba4 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -897,12 +897,16 @@ static void hci_cc_le_set_scan_enable(struct =
hci_dev *hdev,
>> 		return;
>>=20
>> 	if (cp->enable =3D=3D 0x01) {
>> +		mgmt_discovering(hdev->id, 1);
>> +
>> 		del_timer(&hdev->adv_timer);
>>=20
>> 		hci_dev_lock(hdev);
>> 		hci_adv_entries_clear(hdev);
>> 		hci_dev_unlock(hdev);
>> 	} else if (cp->enable =3D=3D 0x00) {
>> +		mgmt_discovering(hdev->id, 0);
>> +
>=20
> do we wanna send discovering on/off for BR/EDR and then again for LE.
> Don't we wanna have something like this:
>=20
> 	discovering on
> 	BR/EDR inquiry
> 	LE scan
> 	discovering off

Yes, we have exactly this.

Notice that at this point (patch 09/14) we don't have BR/EDR/LE
discovery supported yet. We are preparing for support LE-Only
discovery. In a further patch we implement BR/EDR/LE discovery
support and do the proper handling for mgmt_discovering event
(see patch 14/14).

BR,

Andre=

[PATCH v4 10/14] Bluetooth: Add 'eir_len' param to mgmt_device_found()

[Andre Guedes]

This patch adds a new parameter to mgmt_device_found() to inform
the length of 'eir' pointer.

EIR data from LE advertising report event doesn't have a fixed length
as EIR data from extended inquiry result event does. We needed to
change mgmt_device_found() so it copies 'eir_len' bytes instead of
HCI_MAX_EIR_LENGTH.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |    2 +-
 net/bluetooth/hci_event.c        |    9 +++++----
 net/bluetooth/mgmt.c             |    7 +++++--
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e0c9790..65a1ccf 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -871,7 +871,7 @@ int mgmt_set_local_name_complete(u16 index, u8 *name, u8 status);
 int mgmt_read_local_oob_data_reply_complete(u16 index, u8 *hash, u8 *randomizer,
 								u8 status);
 int mgmt_device_found(u16 index, bdaddr_t *bdaddr, u8 *dev_class, s8 rssi,
-								u8 *eir);
+							u8 *eir, u8 eir_len);
 int mgmt_remote_name(u16 index, bdaddr_t *bdaddr, u8 *name);
 int mgmt_discovering(u16 index, u8 discovering);
 int mgmt_device_blocked(u16 index, bdaddr_t *bdaddr);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 47abba4..f097649 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1383,7 +1383,7 @@ static inline void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *
 		data.ssp_mode		= 0x00;
 		hci_inquiry_cache_update(hdev, &data);
 		mgmt_device_found(hdev->id, &info->bdaddr, info->dev_class, 0,
-									NULL);
+								NULL, 0);
 	}
 
 	hci_dev_unlock(hdev);
@@ -2380,7 +2380,7 @@ static inline void hci_inquiry_result_with_rssi_evt(struct hci_dev *hdev, struct
 			hci_inquiry_cache_update(hdev, &data);
 			mgmt_device_found(hdev->id, &info->bdaddr,
 						info->dev_class, info->rssi,
-						NULL);
+						NULL, 0);
 		}
 	} else {
 		struct inquiry_info_with_rssi *info = (void *) (skb->data + 1);
@@ -2397,7 +2397,7 @@ static inline void hci_inquiry_result_with_rssi_evt(struct hci_dev *hdev, struct
 			hci_inquiry_cache_update(hdev, &data);
 			mgmt_device_found(hdev->id, &info->bdaddr,
 						info->dev_class, info->rssi,
-						NULL);
+						NULL, 0);
 		}
 	}
 
@@ -2539,7 +2539,8 @@ static inline void hci_extended_inquiry_result_evt(struct hci_dev *hdev, struct
 		data.ssp_mode		= 0x01;
 		hci_inquiry_cache_update(hdev, &data);
 		mgmt_device_found(hdev->id, &info->bdaddr, info->dev_class,
-						info->rssi, info->data);
+						info->rssi, info->data,
+						sizeof(info->data));
 	}
 
 	hci_dev_unlock(hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3a0815b..1eee5cc 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2339,17 +2339,20 @@ int mgmt_read_local_oob_data_reply_complete(u16 index, u8 *hash, u8 *randomizer,
 }
 
 int mgmt_device_found(u16 index, bdaddr_t *bdaddr, u8 *dev_class, s8 rssi,
-								u8 *eir)
+							u8 *eir, u8 eir_len)
 {
 	struct mgmt_ev_device_found ev;
 
+	if (eir_len > sizeof(ev.eir))
+		return -EINVAL;
+
 	memset(&ev, 0, sizeof(ev));
 
 	bacpy(&ev.bdaddr, bdaddr);
 	ev.rssi = rssi;
 
 	if (eir)
-		memcpy(ev.eir, eir, sizeof(ev.eir));
+		memcpy(ev.eir, eir, eir_len);
 
 	if (dev_class)
 		memcpy(ev.dev_class, dev_class, sizeof(ev.dev_class));
-- 
1.7.5.2

Re: [PATCH v4 10/14] Bluetooth: Add 'eir_len' param to mgmt_device_found()

[Marcel Holtmann]

Hi Andre,

> This patch adds a new parameter to mgmt_device_found() to inform
> the length of 'eir' pointer.
> 
> EIR data from LE advertising report event doesn't have a fixed length
> as EIR data from extended inquiry result event does. We needed to
> change mgmt_device_found() so it copies 'eir_len' bytes instead of
> HCI_MAX_EIR_LENGTH.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci_core.h |    2 +-
>  net/bluetooth/hci_event.c        |    9 +++++----
>  net/bluetooth/mgmt.c             |    7 +++++--
>  3 files changed, 11 insertions(+), 7 deletions(-)

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

Regards

Marcel

[PATCH v4 11/14] Bluetooth: Report LE devices

[Andre Guedes]

Devices found during LE scan should be reported to userspace through
mgmt_device_found events.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_event.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index f097649..166f8fa 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2830,6 +2830,7 @@ static inline void hci_le_adv_report_evt(struct hci_dev *hdev,
 {
 	struct hci_ev_le_advertising_info *ev;
 	u8 num_reports;
+	s8 rssi;
 
 	num_reports = skb->data[0];
 	ev = (void *) &skb->data[1];
@@ -2838,9 +2839,18 @@ static inline void hci_le_adv_report_evt(struct hci_dev *hdev,
 
 	hci_add_adv_entry(hdev, ev);
 
+	rssi = ev->data[ev->length];
+	mgmt_device_found(hdev->id, &ev->bdaddr, NULL, rssi, ev->data,
+								ev->length);
+
 	while (--num_reports) {
 		ev = (void *) (ev->data + ev->length + 1);
+
 		hci_add_adv_entry(hdev, ev);
+
+		rssi = ev->data[ev->length];
+		mgmt_device_found(hdev->id, &ev->bdaddr, NULL, rssi, ev->data,
+								ev->length);
 	}
 
 	hci_dev_unlock(hdev);
-- 
1.7.5.2

Re: [PATCH v4 11/14] Bluetooth: Report LE devices

[Marcel Holtmann]

Hi Andre,

> Devices found during LE scan should be reported to userspace through
> mgmt_device_found events.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  net/bluetooth/hci_event.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index f097649..166f8fa 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2830,6 +2830,7 @@ static inline void hci_le_adv_report_evt(struct hci_dev *hdev,
>  {
>  	struct hci_ev_le_advertising_info *ev;
>  	u8 num_reports;
> +	s8 rssi;
>  
>  	num_reports = skb->data[0];
>  	ev = (void *) &skb->data[1];
> @@ -2838,9 +2839,18 @@ static inline void hci_le_adv_report_evt(struct hci_dev *hdev,
>  
>  	hci_add_adv_entry(hdev, ev);
>  
> +	rssi = ev->data[ev->length];
> +	mgmt_device_found(hdev->id, &ev->bdaddr, NULL, rssi, ev->data,
> +								ev->length);
> +
>  	while (--num_reports) {
>  		ev = (void *) (ev->data + ev->length + 1);
> +
>  		hci_add_adv_entry(hdev, ev);
> +
> +		rssi = ev->data[ev->length];
> +		mgmt_device_found(hdev->id, &ev->bdaddr, NULL, rssi, ev->data,
> +								ev->length);
>  	}

any reason we are treating the first entry different than the potential
other ones? This code looks too complex to me. And we have to repeat the
same calls which easily leads to typos.

Regards

Marcel

Re: [PATCH v4 11/14] Bluetooth: Report LE devices

[Andre Guedes]

Hi Marcel,

On Sep 20, 2011, at 9:49 AM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>> Devices found during LE scan should be reported to userspace through
>> mgmt_device_found events.
>>=20
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> net/bluetooth/hci_event.c |   10 ++++++++++
>> 1 files changed, 10 insertions(+), 0 deletions(-)
>>=20
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index f097649..166f8fa 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -2830,6 +2830,7 @@ static inline void hci_le_adv_report_evt(struct =
hci_dev *hdev,
>> {
>> 	struct hci_ev_le_advertising_info *ev;
>> 	u8 num_reports;
>> +	s8 rssi;
>>=20
>> 	num_reports =3D skb->data[0];
>> 	ev =3D (void *) &skb->data[1];
>> @@ -2838,9 +2839,18 @@ static inline void =
hci_le_adv_report_evt(struct hci_dev *hdev,
>>=20
>> 	hci_add_adv_entry(hdev, ev);
>>=20
>> +	rssi =3D ev->data[ev->length];
>> +	mgmt_device_found(hdev->id, &ev->bdaddr, NULL, rssi, ev->data,
>> +								=
ev->length);
>> +
>> 	while (--num_reports) {
>> 		ev =3D (void *) (ev->data + ev->length + 1);
>> +
>> 		hci_add_adv_entry(hdev, ev);
>> +
>> +		rssi =3D ev->data[ev->length];
>> +		mgmt_device_found(hdev->id, &ev->bdaddr, NULL, rssi, =
ev->data,
>> +								=
ev->length);
>> 	}
>=20
> any reason we are treating the first entry different than the =
potential
> other ones? This code looks too complex to me. And we have to repeat =
the
> same calls which easily leads to typos.

There is no special reason unless it was the easiest implementation
when this function was created. But as long as new stuff are added
to this function things may become a bit ugly.

I'll do some code refactoring in hci_le_adv_report_evt() and send it
as a separated patch.

BR,

Andre=

[PATCH v4 12/14] Bluetooth: LE scan infra-structure

[Andre Guedes]

This patch adds to hci_core the infra-structure to carry out the
LE scan. Functions were created to init the LE scan and cancel
an ongoing scanning (hci_do_le_scan and hci_cancel_le_scan).

Also, the HCI_LE_SCAN flag was created to inform if the controller
is performing LE scan. The flag is set/cleared when the controller
starts/stops scanning.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci.h      |   11 ++++++
 include/net/bluetooth/hci_core.h |    5 +++
 net/bluetooth/hci_core.c         |   69 ++++++++++++++++++++++++++++++++++++++
 net/bluetooth/hci_event.c        |    4 ++
 4 files changed, 89 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 1e11e7f..7520544 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -86,6 +86,8 @@ enum {
 	HCI_DEBUG_KEYS,
 
 	HCI_RESET,
+
+	HCI_LE_SCAN,
 };
 
 /* HCI ioctl defines */
@@ -739,6 +741,15 @@ struct hci_rp_le_read_buffer_size {
 	__u8     le_max_pkt;
 } __packed;
 
+#define HCI_OP_LE_SET_SCAN_PARAM	0x200b
+struct hci_cp_le_set_scan_param {
+	__u8    type;
+	__le16  interval;
+	__le16  window;
+	__u8    own_address_type;
+	__u8    filter_policy;
+} __packed;
+
 #define HCI_OP_LE_SET_SCAN_ENABLE	0x200c
 struct hci_cp_le_set_scan_enable {
 	__u8     enable;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 65a1ccf..c6ae380 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -208,6 +208,8 @@ struct hci_dev {
 	struct list_head	adv_entries;
 	struct timer_list	adv_timer;
 
+	struct timer_list	le_scan_timer;
+
 	struct hci_dev_stats	stat;
 
 	struct sk_buff_head	driver_init;
@@ -918,5 +920,8 @@ void hci_le_ltk_neg_reply(struct hci_conn *conn);
 
 int hci_do_inquiry(struct hci_dev *hdev, u8 length);
 int hci_cancel_inquiry(struct hci_dev *hdev);
+int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
+								int timeout);
+int hci_cancel_le_scan(struct hci_dev *hdev);
 
 #endif /* __HCI_CORE_H */
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index f62b465..cd5b640 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1422,6 +1422,23 @@ int hci_add_adv_entry(struct hci_dev *hdev,
 	return 0;
 }
 
+static int le_scan(struct hci_dev *hdev, u8 enable)
+{
+	struct hci_cp_le_set_scan_enable cp;
+
+	memset(&cp, 0, sizeof(cp));
+	cp.enable = enable;
+
+	return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
+}
+
+static void le_scan_timeout(unsigned long arg)
+{
+	struct hci_dev *hdev = (void *) arg;
+
+	le_scan(hdev, 0);
+}
+
 /* Register HCI device */
 int hci_register_dev(struct hci_dev *hdev)
 {
@@ -1492,6 +1509,9 @@ int hci_register_dev(struct hci_dev *hdev)
 	setup_timer(&hdev->adv_timer, hci_clear_adv_cache,
 						(unsigned long) hdev);
 
+	setup_timer(&hdev->le_scan_timer, le_scan_timeout,
+						(unsigned long) hdev);
+
 	INIT_WORK(&hdev->power_on, hci_power_on);
 	INIT_WORK(&hdev->power_off, hci_power_off);
 	setup_timer(&hdev->off_timer, hci_auto_off, (unsigned long) hdev);
@@ -1565,6 +1585,7 @@ int hci_unregister_dev(struct hci_dev *hdev)
 
 	hci_del_off_timer(hdev);
 	del_timer(&hdev->adv_timer);
+	del_timer(&hdev->le_scan_timer);
 
 	destroy_workqueue(hdev->workqueue);
 
@@ -2432,3 +2453,51 @@ int hci_cancel_inquiry(struct hci_dev *hdev)
 
 	return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
 }
+
+static int set_le_scan_param(struct hci_dev *hdev, u8 type, u16 interval,
+								u16 window)
+{
+	struct hci_cp_le_set_scan_param cp;
+
+	memset(&cp, 0, sizeof(cp));
+	cp.type = type;
+	cp.interval = cpu_to_le16(interval);
+	cp.window = cpu_to_le16(window);
+
+	return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_PARAM, sizeof(cp), &cp);
+}
+
+int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
+								int timeout)
+{
+	int err;
+
+	BT_DBG("%s", hdev->name);
+
+	if (test_bit(HCI_LE_SCAN, &hdev->flags))
+		return -EPERM;
+
+	err = set_le_scan_param(hdev, type, interval, window);
+	if (err < 0)
+		return err;
+
+	err = le_scan(hdev, 1);
+	if (err < 0)
+		return err;
+
+	mod_timer(&hdev->le_scan_timer, jiffies + msecs_to_jiffies(timeout));
+
+	return 0;
+}
+
+int hci_cancel_le_scan(struct hci_dev *hdev)
+{
+	BT_DBG("%s", hdev->name);
+
+	if (!test_bit(HCI_LE_SCAN, &hdev->flags))
+		return -EPERM;
+
+	del_timer(&hdev->le_scan_timer);
+
+	return le_scan(hdev, 0);
+}
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 166f8fa..5bbba54 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -897,6 +897,8 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 		return;
 
 	if (cp->enable == 0x01) {
+		set_bit(HCI_LE_SCAN, &hdev->flags);
+
 		mgmt_discovering(hdev->id, 1);
 
 		del_timer(&hdev->adv_timer);
@@ -905,6 +907,8 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 		hci_adv_entries_clear(hdev);
 		hci_dev_unlock(hdev);
 	} else if (cp->enable == 0x00) {
+		clear_bit(HCI_LE_SCAN, &hdev->flags);
+
 		mgmt_discovering(hdev->id, 0);
 
 		mod_timer(&hdev->adv_timer, jiffies + ADV_CLEAR_TIMEOUT);
-- 
1.7.5.2

Re: [PATCH v4 12/14] Bluetooth: LE scan infra-structure

[Marcel Holtmann]

Hi Andre,

> This patch adds to hci_core the infra-structure to carry out the
> LE scan. Functions were created to init the LE scan and cancel
> an ongoing scanning (hci_do_le_scan and hci_cancel_le_scan).
> 
> Also, the HCI_LE_SCAN flag was created to inform if the controller
> is performing LE scan. The flag is set/cleared when the controller
> starts/stops scanning.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci.h      |   11 ++++++
>  include/net/bluetooth/hci_core.h |    5 +++
>  net/bluetooth/hci_core.c         |   69 ++++++++++++++++++++++++++++++++++++++
>  net/bluetooth/hci_event.c        |    4 ++
>  4 files changed, 89 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 1e11e7f..7520544 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -86,6 +86,8 @@ enum {
>  	HCI_DEBUG_KEYS,
>  
>  	HCI_RESET,
> +
> +	HCI_LE_SCAN,
>  };

remind me if userspace is actually needed this flag right now. Otherwise
lets hide this internally.

You (or Claudio) might have already explained this, but I forgot.

>  
>  /* HCI ioctl defines */
> @@ -739,6 +741,15 @@ struct hci_rp_le_read_buffer_size {
>  	__u8     le_max_pkt;
>  } __packed;
>  
> +#define HCI_OP_LE_SET_SCAN_PARAM	0x200b
> +struct hci_cp_le_set_scan_param {
> +	__u8    type;
> +	__le16  interval;
> +	__le16  window;
> +	__u8    own_address_type;
> +	__u8    filter_policy;
> +} __packed;
> +

Do HCI defines as separate patch is a good idea. Makes it a lot cleaner.

>  #define HCI_OP_LE_SET_SCAN_ENABLE	0x200c
>  struct hci_cp_le_set_scan_enable {
>  	__u8     enable;
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 65a1ccf..c6ae380 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -208,6 +208,8 @@ struct hci_dev {
>  	struct list_head	adv_entries;
>  	struct timer_list	adv_timer;
>  
> +	struct timer_list	le_scan_timer;
> +
>  	struct hci_dev_stats	stat;
>  
>  	struct sk_buff_head	driver_init;
> @@ -918,5 +920,8 @@ void hci_le_ltk_neg_reply(struct hci_conn *conn);
>  
>  int hci_do_inquiry(struct hci_dev *hdev, u8 length);
>  int hci_cancel_inquiry(struct hci_dev *hdev);
> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
> +								int timeout);
> +int hci_cancel_le_scan(struct hci_dev *hdev);
>  
>  #endif /* __HCI_CORE_H */
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index f62b465..cd5b640 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1422,6 +1422,23 @@ int hci_add_adv_entry(struct hci_dev *hdev,
>  	return 0;
>  }
>  
> +static int le_scan(struct hci_dev *hdev, u8 enable)
> +{
> +	struct hci_cp_le_set_scan_enable cp;
> +
> +	memset(&cp, 0, sizeof(cp));
> +	cp.enable = enable;
> +
> +	return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
> +}
> +
> +static void le_scan_timeout(unsigned long arg)
> +{
> +	struct hci_dev *hdev = (void *) arg;
> +
> +	le_scan(hdev, 0);
> +}
> +
>  /* Register HCI device */
>  int hci_register_dev(struct hci_dev *hdev)
>  {
> @@ -1492,6 +1509,9 @@ int hci_register_dev(struct hci_dev *hdev)
>  	setup_timer(&hdev->adv_timer, hci_clear_adv_cache,
>  						(unsigned long) hdev);
>  
> +	setup_timer(&hdev->le_scan_timer, le_scan_timeout,
> +						(unsigned long) hdev);
> +
>  	INIT_WORK(&hdev->power_on, hci_power_on);
>  	INIT_WORK(&hdev->power_off, hci_power_off);
>  	setup_timer(&hdev->off_timer, hci_auto_off, (unsigned long) hdev);
> @@ -1565,6 +1585,7 @@ int hci_unregister_dev(struct hci_dev *hdev)
>  
>  	hci_del_off_timer(hdev);
>  	del_timer(&hdev->adv_timer);
> +	del_timer(&hdev->le_scan_timer);
>  
>  	destroy_workqueue(hdev->workqueue);
>  
> @@ -2432,3 +2453,51 @@ int hci_cancel_inquiry(struct hci_dev *hdev)
>  
>  	return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
>  }
> +
> +static int set_le_scan_param(struct hci_dev *hdev, u8 type, u16 interval,
> +								u16 window)
> +{
> +	struct hci_cp_le_set_scan_param cp;
> +
> +	memset(&cp, 0, sizeof(cp));
> +	cp.type = type;
> +	cp.interval = cpu_to_le16(interval);
> +	cp.window = cpu_to_le16(window);
> +
> +	return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_PARAM, sizeof(cp), &cp);
> +}
> +
> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
> +								int timeout)
> +{
> +	int err;
> +
> +	BT_DBG("%s", hdev->name);
> +
> +	if (test_bit(HCI_LE_SCAN, &hdev->flags))
> +		return -EPERM;
> +
> +	err = set_le_scan_param(hdev, type, interval, window);
> +	if (err < 0)
> +		return err;
> +
> +	err = le_scan(hdev, 1);
> +	if (err < 0)
> +		return err;
> +
> +	mod_timer(&hdev->le_scan_timer, jiffies + msecs_to_jiffies(timeout));
> +
> +	return 0;
> +}
> +
> +int hci_cancel_le_scan(struct hci_dev *hdev)
> +{
> +	BT_DBG("%s", hdev->name);
> +
> +	if (!test_bit(HCI_LE_SCAN, &hdev->flags))
> +		return -EPERM;
> +
> +	del_timer(&hdev->le_scan_timer);
> +
> +	return le_scan(hdev, 0);
> +}

Don't end up with the same race condition that you just fixed with
inquiry. HCI_LE_SCAN might not be set yet since cmd_status has not yet
arrived.

> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 166f8fa..5bbba54 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -897,6 +897,8 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>  		return;
>  
>  	if (cp->enable == 0x01) {
> +		set_bit(HCI_LE_SCAN, &hdev->flags);
> +
>  		mgmt_discovering(hdev->id, 1);
>  
>  		del_timer(&hdev->adv_timer);
> @@ -905,6 +907,8 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>  		hci_adv_entries_clear(hdev);
>  		hci_dev_unlock(hdev);
>  	} else if (cp->enable == 0x00) {
> +		clear_bit(HCI_LE_SCAN, &hdev->flags);
> +
>  		mgmt_discovering(hdev->id, 0);
>  
>  		mod_timer(&hdev->adv_timer, jiffies + ADV_CLEAR_TIMEOUT);

Regards

Marcel

Re: [PATCH v4 12/14] Bluetooth: LE scan infra-structure

[Andre Guedes]

Hi Marcel,

On Sep 20, 2011, at 9:53 AM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>> This patch adds to hci_core the infra-structure to carry out the
>> LE scan. Functions were created to init the LE scan and cancel
>> an ongoing scanning (hci_do_le_scan and hci_cancel_le_scan).
>>=20
>> Also, the HCI_LE_SCAN flag was created to inform if the controller
>> is performing LE scan. The flag is set/cleared when the controller
>> starts/stops scanning.
>>=20
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/hci.h      |   11 ++++++
>> include/net/bluetooth/hci_core.h |    5 +++
>> net/bluetooth/hci_core.c         |   69 =
++++++++++++++++++++++++++++++++++++++
>> net/bluetooth/hci_event.c        |    4 ++
>> 4 files changed, 89 insertions(+), 0 deletions(-)
>>=20
>> diff --git a/include/net/bluetooth/hci.h =
b/include/net/bluetooth/hci.h
>> index 1e11e7f..7520544 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -86,6 +86,8 @@ enum {
>> 	HCI_DEBUG_KEYS,
>>=20
>> 	HCI_RESET,
>> +
>> +	HCI_LE_SCAN,
>> };
>=20
> remind me if userspace is actually needed this flag right now. =
Otherwise
> lets hide this internally.
>=20
> You (or Claudio) might have already explained this, but I forgot.

Right now, userspace doesn't need it, so we can keep it hidden
internally.

>=20
>>=20
>> /* HCI ioctl defines */
>> @@ -739,6 +741,15 @@ struct hci_rp_le_read_buffer_size {
>> 	__u8     le_max_pkt;
>> } __packed;
>>=20
>> +#define HCI_OP_LE_SET_SCAN_PARAM	0x200b
>> +struct hci_cp_le_set_scan_param {
>> +	__u8    type;
>> +	__le16  interval;
>> +	__le16  window;
>> +	__u8    own_address_type;
>> +	__u8    filter_policy;
>> +} __packed;
>> +
>=20
> Do HCI defines as separate patch is a good idea. Makes it a lot =
cleaner.

Ok, I'll do this.

>=20
>> #define HCI_OP_LE_SET_SCAN_ENABLE	0x200c
>> struct hci_cp_le_set_scan_enable {
>> 	__u8     enable;
>> diff --git a/include/net/bluetooth/hci_core.h =
b/include/net/bluetooth/hci_core.h
>> index 65a1ccf..c6ae380 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -208,6 +208,8 @@ struct hci_dev {
>> 	struct list_head	adv_entries;
>> 	struct timer_list	adv_timer;
>>=20
>> +	struct timer_list	le_scan_timer;
>> +
>> 	struct hci_dev_stats	stat;
>>=20
>> 	struct sk_buff_head	driver_init;
>> @@ -918,5 +920,8 @@ void hci_le_ltk_neg_reply(struct hci_conn *conn);
>>=20
>> int hci_do_inquiry(struct hci_dev *hdev, u8 length);
>> int hci_cancel_inquiry(struct hci_dev *hdev);
>> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 =
window,
>> +								int =
timeout);
>> +int hci_cancel_le_scan(struct hci_dev *hdev);
>>=20
>> #endif /* __HCI_CORE_H */
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index f62b465..cd5b640 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1422,6 +1422,23 @@ int hci_add_adv_entry(struct hci_dev *hdev,
>> 	return 0;
>> }
>>=20
>> +static int le_scan(struct hci_dev *hdev, u8 enable)
>> +{
>> +	struct hci_cp_le_set_scan_enable cp;
>> +
>> +	memset(&cp, 0, sizeof(cp));
>> +	cp.enable =3D enable;
>> +
>> +	return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), =
&cp);
>> +}
>> +
>> +static void le_scan_timeout(unsigned long arg)
>> +{
>> +	struct hci_dev *hdev =3D (void *) arg;
>> +
>> +	le_scan(hdev, 0);
>> +}
>> +
>> /* Register HCI device */
>> int hci_register_dev(struct hci_dev *hdev)
>> {
>> @@ -1492,6 +1509,9 @@ int hci_register_dev(struct hci_dev *hdev)
>> 	setup_timer(&hdev->adv_timer, hci_clear_adv_cache,
>> 						(unsigned long) hdev);
>>=20
>> +	setup_timer(&hdev->le_scan_timer, le_scan_timeout,
>> +						(unsigned long) hdev);
>> +
>> 	INIT_WORK(&hdev->power_on, hci_power_on);
>> 	INIT_WORK(&hdev->power_off, hci_power_off);
>> 	setup_timer(&hdev->off_timer, hci_auto_off, (unsigned long) =
hdev);
>> @@ -1565,6 +1585,7 @@ int hci_unregister_dev(struct hci_dev *hdev)
>>=20
>> 	hci_del_off_timer(hdev);
>> 	del_timer(&hdev->adv_timer);
>> +	del_timer(&hdev->le_scan_timer);
>>=20
>> 	destroy_workqueue(hdev->workqueue);
>>=20
>> @@ -2432,3 +2453,51 @@ int hci_cancel_inquiry(struct hci_dev *hdev)
>>=20
>> 	return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
>> }
>> +
>> +static int set_le_scan_param(struct hci_dev *hdev, u8 type, u16 =
interval,
>> +								u16 =
window)
>> +{
>> +	struct hci_cp_le_set_scan_param cp;
>> +
>> +	memset(&cp, 0, sizeof(cp));
>> +	cp.type =3D type;
>> +	cp.interval =3D cpu_to_le16(interval);
>> +	cp.window =3D cpu_to_le16(window);
>> +
>> +	return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_PARAM, sizeof(cp), =
&cp);
>> +}
>> +
>> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 =
window,
>> +								int =
timeout)
>> +{
>> +	int err;
>> +
>> +	BT_DBG("%s", hdev->name);
>> +
>> +	if (test_bit(HCI_LE_SCAN, &hdev->flags))
>> +		return -EPERM;
>> +
>> +	err =3D set_le_scan_param(hdev, type, interval, window);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	err =3D le_scan(hdev, 1);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	mod_timer(&hdev->le_scan_timer, jiffies + =
msecs_to_jiffies(timeout));
>> +
>> +	return 0;
>> +}
>> +
>> +int hci_cancel_le_scan(struct hci_dev *hdev)
>> +{
>> +	BT_DBG("%s", hdev->name);
>> +
>> +	if (!test_bit(HCI_LE_SCAN, &hdev->flags))
>> +		return -EPERM;
>> +
>> +	del_timer(&hdev->le_scan_timer);
>> +
>> +	return le_scan(hdev, 0);
>> +}
>=20
> Don't end up with the same race condition that you just fixed with
> inquiry. HCI_LE_SCAN might not be set yet since cmd_status has not yet
> arrived.

Sure. Actually, inquiry and le scan hci commands have different
behavior. We don't have command status event for HCI LE Set Scan
Enable Command (see spec page 1069). We only have command complete
event.

Additionally, the LE Set Scan Enable command is used to enable and
disable le scan. So, the right place to set/clear the HCI_LE_SCAN
flag is in LE Set Scan Enable command complete event handler =
(hci_cc_le_set_scan_enable).

>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 166f8fa..5bbba54 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -897,6 +897,8 @@ static void hci_cc_le_set_scan_enable(struct =
hci_dev *hdev,
>> 		return;
>>=20
>> 	if (cp->enable =3D=3D 0x01) {
>> +		set_bit(HCI_LE_SCAN, &hdev->flags);
>> +
>> 		mgmt_discovering(hdev->id, 1);
>>=20
>> 		del_timer(&hdev->adv_timer);
>> @@ -905,6 +907,8 @@ static void hci_cc_le_set_scan_enable(struct =
hci_dev *hdev,
>> 		hci_adv_entries_clear(hdev);
>> 		hci_dev_unlock(hdev);
>> 	} else if (cp->enable =3D=3D 0x00) {
>> +		clear_bit(HCI_LE_SCAN, &hdev->flags);
>> +
>> 		mgmt_discovering(hdev->id, 0);
>>=20
>> 		mod_timer(&hdev->adv_timer, jiffies + =
ADV_CLEAR_TIMEOUT);
>=20
> Regards
>=20
> Marcel
>=20
>=20

BR,

Andre=

[PATCH v4 13/14] Bluetooth: Support LE-Only discovery procedure

[Andre Guedes]

This patch adds support for LE-Only discovery procedure through
management interface.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_event.c |   16 +++++++++++++---
 net/bluetooth/mgmt.c      |   13 ++++++++++++-
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 5bbba54..0408c50 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -889,14 +889,16 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 
 	BT_DBG("%s status 0x%x", hdev->name, status);
 
-	if (status)
-		return;
-
 	cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_SCAN_ENABLE);
 	if (!cp)
 		return;
 
 	if (cp->enable == 0x01) {
+		if (status) {
+			mgmt_discovery_complete(hdev->id, status);
+			return;
+		}
+
 		set_bit(HCI_LE_SCAN, &hdev->flags);
 
 		mgmt_discovering(hdev->id, 1);
@@ -906,7 +908,15 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 		hci_dev_lock(hdev);
 		hci_adv_entries_clear(hdev);
 		hci_dev_unlock(hdev);
+
+		if (mgmt_has_pending_stop_discov(hdev->id))
+			hci_cancel_le_scan(hdev);
 	} else if (cp->enable == 0x00) {
+		mgmt_discovery_complete(hdev->id, status);
+
+		if (status)
+			return;
+
 		clear_bit(HCI_LE_SCAN, &hdev->flags);
 
 		mgmt_discovering(hdev->id, 0);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 1eee5cc..e869422 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -32,6 +32,14 @@
 #define MGMT_VERSION	0
 #define MGMT_REVISION	1
 
+/*
+ * These LE scan and inquiry parameters were chosen according to LE General
+ * Discovery Procedure specification.
+ */
+#define LE_SCAN_TYPE 0x01
+#define LE_SCAN_WIN 0x12
+#define LE_SCAN_INT 0x12
+#define LE_SCAN_TIMEOUT_LE_ONLY 10240 /* TGAP(gen_disc_scan_min) */
 #define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
 
 struct pending_cmd {
@@ -1637,7 +1645,8 @@ static int start_discovery(struct sock *sk, u16 index)
 	if (lmp_host_le_capable(hdev) && lmp_bredr_capable(hdev))
 		err = -ENOSYS;
 	else if (lmp_host_le_capable(hdev))
-		err = -ENOSYS;
+		err = hci_do_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
+					LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
 	else
 		err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
 
@@ -1684,6 +1693,8 @@ static int stop_discovery(struct sock *sk, u16 index)
 
 	if (test_bit(HCI_INQUIRY, &hdev->flags))
 		err = hci_cancel_inquiry(hdev);
+	else if (test_bit(HCI_LE_SCAN, &hdev->flags))
+		err = hci_cancel_le_scan(hdev);
 	else
 		err = 0;
 
-- 
1.7.5.2

Re: [PATCH v4 13/14] Bluetooth: Support LE-Only discovery procedure

[Marcel Holtmann]

Hi Andre,

> This patch adds support for LE-Only discovery procedure through
> management interface.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  net/bluetooth/hci_event.c |   16 +++++++++++++---
>  net/bluetooth/mgmt.c      |   13 ++++++++++++-
>  2 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 5bbba54..0408c50 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -889,14 +889,16 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>  
>  	BT_DBG("%s status 0x%x", hdev->name, status);
>  
> -	if (status)
> -		return;
> -
>  	cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_SCAN_ENABLE);
>  	if (!cp)
>  		return;
>  
>  	if (cp->enable == 0x01) {
> +		if (status) {
> +			mgmt_discovery_complete(hdev->id, status);
> +			return;
> +		}
> +
>  		set_bit(HCI_LE_SCAN, &hdev->flags);
>  
>  		mgmt_discovering(hdev->id, 1);
> @@ -906,7 +908,15 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>  		hci_dev_lock(hdev);
>  		hci_adv_entries_clear(hdev);
>  		hci_dev_unlock(hdev);
> +
> +		if (mgmt_has_pending_stop_discov(hdev->id))
> +			hci_cancel_le_scan(hdev);
>  	} else if (cp->enable == 0x00) {
> +		mgmt_discovery_complete(hdev->id, status);
> +
> +		if (status)
> +			return;
> +
>  		clear_bit(HCI_LE_SCAN, &hdev->flags);
>  
>  		mgmt_discovering(hdev->id, 0);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 1eee5cc..e869422 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -32,6 +32,14 @@
>  #define MGMT_VERSION	0
>  #define MGMT_REVISION	1
>  
> +/*
> + * These LE scan and inquiry parameters were chosen according to LE General
> + * Discovery Procedure specification.
> + */
> +#define LE_SCAN_TYPE 0x01
> +#define LE_SCAN_WIN 0x12
> +#define LE_SCAN_INT 0x12
> +#define LE_SCAN_TIMEOUT_LE_ONLY 10240 /* TGAP(gen_disc_scan_min) */

And an extra empty line here to make clear these a independent values.

>  #define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
>  
>  struct pending_cmd {
> @@ -1637,7 +1645,8 @@ static int start_discovery(struct sock *sk, u16 index)
>  	if (lmp_host_le_capable(hdev) && lmp_bredr_capable(hdev))
>  		err = -ENOSYS;
>  	else if (lmp_host_le_capable(hdev))
> -		err = -ENOSYS;
> +		err = hci_do_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
> +					LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
>  	else
>  		err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
>  
> @@ -1684,6 +1693,8 @@ static int stop_discovery(struct sock *sk, u16 index)
>  
>  	if (test_bit(HCI_INQUIRY, &hdev->flags))
>  		err = hci_cancel_inquiry(hdev);
> +	else if (test_bit(HCI_LE_SCAN, &hdev->flags))
> +		err = hci_cancel_le_scan(hdev);
>  	else
>  		err = 0;
>  

I see where you wanna go with this now. I am a bit afraid of the code
complexity with the discovery stop/start/complete etc. messages.

This is all too much done differently for 3 different possible use
cases. And I just think we need more general handling. For example:

	inquiry_started()
	inquiry_result()
	inquiry_completed()
	le_scan_started()
	le_scan_result()
	le_scan_completed()

And based on the controller capabilities, the current states etc. a
central place decides to send out which events at which time and to
allow certain commands or not.

If we do not centralize this, then I am afraid we duplicate this logic
three times.

Regards

Marcel

Re: [PATCH v4 13/14] Bluetooth: Support LE-Only discovery procedure

[Andre Guedes]

Hi Marcel,

On Sep 20, 2011, at 10:00 AM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>> This patch adds support for LE-Only discovery procedure through
>> management interface.
>>=20
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> net/bluetooth/hci_event.c |   16 +++++++++++++---
>> net/bluetooth/mgmt.c      |   13 ++++++++++++-
>> 2 files changed, 25 insertions(+), 4 deletions(-)
>>=20
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 5bbba54..0408c50 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -889,14 +889,16 @@ static void hci_cc_le_set_scan_enable(struct =
hci_dev *hdev,
>>=20
>> 	BT_DBG("%s status 0x%x", hdev->name, status);
>>=20
>> -	if (status)
>> -		return;
>> -
>> 	cp =3D hci_sent_cmd_data(hdev, HCI_OP_LE_SET_SCAN_ENABLE);
>> 	if (!cp)
>> 		return;
>>=20
>> 	if (cp->enable =3D=3D 0x01) {
>> +		if (status) {
>> +			mgmt_discovery_complete(hdev->id, status);
>> +			return;
>> +		}
>> +
>> 		set_bit(HCI_LE_SCAN, &hdev->flags);
>>=20
>> 		mgmt_discovering(hdev->id, 1);
>> @@ -906,7 +908,15 @@ static void hci_cc_le_set_scan_enable(struct =
hci_dev *hdev,
>> 		hci_dev_lock(hdev);
>> 		hci_adv_entries_clear(hdev);
>> 		hci_dev_unlock(hdev);
>> +
>> +		if (mgmt_has_pending_stop_discov(hdev->id))
>> +			hci_cancel_le_scan(hdev);
>> 	} else if (cp->enable =3D=3D 0x00) {
>> +		mgmt_discovery_complete(hdev->id, status);
>> +
>> +		if (status)
>> +			return;
>> +
>> 		clear_bit(HCI_LE_SCAN, &hdev->flags);
>>=20
>> 		mgmt_discovering(hdev->id, 0);
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index 1eee5cc..e869422 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -32,6 +32,14 @@
>> #define MGMT_VERSION	0
>> #define MGMT_REVISION	1
>>=20
>> +/*
>> + * These LE scan and inquiry parameters were chosen according to LE =
General
>> + * Discovery Procedure specification.
>> + */
>> +#define LE_SCAN_TYPE 0x01
>> +#define LE_SCAN_WIN 0x12
>> +#define LE_SCAN_INT 0x12
>> +#define LE_SCAN_TIMEOUT_LE_ONLY 10240 /* TGAP(gen_disc_scan_min) */
>=20
> And an extra empty line here to make clear these a independent values.

Ok, I'll do it.

>> #define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
>>=20
>> struct pending_cmd {
>> @@ -1637,7 +1645,8 @@ static int start_discovery(struct sock *sk, u16 =
index)
>> 	if (lmp_host_le_capable(hdev) && lmp_bredr_capable(hdev))
>> 		err =3D -ENOSYS;
>> 	else if (lmp_host_le_capable(hdev))
>> -		err =3D -ENOSYS;
>> +		err =3D hci_do_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
>> +					LE_SCAN_WIN, =
LE_SCAN_TIMEOUT_LE_ONLY);
>> 	else
>> 		err =3D hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
>>=20
>> @@ -1684,6 +1693,8 @@ static int stop_discovery(struct sock *sk, u16 =
index)
>>=20
>> 	if (test_bit(HCI_INQUIRY, &hdev->flags))
>> 		err =3D hci_cancel_inquiry(hdev);
>> +	else if (test_bit(HCI_LE_SCAN, &hdev->flags))
>> +		err =3D hci_cancel_le_scan(hdev);
>> 	else
>> 		err =3D 0;
>>=20
>=20
> I see where you wanna go with this now. I am a bit afraid of the code
> complexity with the discovery stop/start/complete etc. messages.
>=20
> This is all too much done differently for 3 different possible use
> cases. And I just think we need more general handling. For example:
>=20
> 	inquiry_started()
> 	inquiry_result()
> 	inquiry_completed()
> 	le_scan_started()
> 	le_scan_result()
> 	le_scan_completed()
>=20
> And based on the controller capabilities, the current states etc. a
> central place decides to send out which events at which time and to
> allow certain commands or not.
>=20
> If we do not centralize this, then I am afraid we duplicate this logic
> three times.

Not sure I get the idea right. Why would we repeat this logic three
times?

BTW, I don't see discovery logic too complex. We have, basically,
three main functions:

mgmt_start_discovery: based on device capabilities it initializes
the right discovery procedure.

mgmt_stop_discovery: based on the current device state it cancels
the ongoing discovery procedure.

mgmt_discovery_complete: once the discovery procedure is finished
(it doesn't matter if the discovery procedure was canceled by a
mgmt stop discovery command, a hci command failed or it finished
successfully) this function does the proper handling of mgmt
discovery pending commands and decides which event is sent to
userspace. So, we have the whole discovery procedure termination
logic centralized in this function.

BR,

Andre=

[PATCH v4 14/14] Bluetooth: Support BR/EDR/LE discovery procedure

[Andre Guedes]

This patch adds support for BR/EDR/LE discovery procedure through
management interface.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |    2 +
 net/bluetooth/hci_event.c        |   16 ++++++++++---
 net/bluetooth/mgmt.c             |   42 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index c6ae380..c5c38e3 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -880,6 +880,8 @@ int mgmt_device_blocked(u16 index, bdaddr_t *bdaddr);
 int mgmt_device_unblocked(u16 index, bdaddr_t *bdaddr);
 int mgmt_discovery_complete(u16 index, u8 status);
 int mgmt_has_pending_stop_discov(u16 index);
+int mgmt_interleaved_discovery(u16 index);
+int mgmt_is_interleaved_discovery(u16 index);
 
 /* HCI info for socket */
 #define hci_pi(sk) ((struct hci_pinfo *) sk)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 0408c50..08d5e75 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -896,12 +896,17 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 	if (cp->enable == 0x01) {
 		if (status) {
 			mgmt_discovery_complete(hdev->id, status);
+
+			if (mgmt_is_interleaved_discovery(hdev->id))
+				mgmt_discovering(hdev->id, 0);
+
 			return;
 		}
 
 		set_bit(HCI_LE_SCAN, &hdev->flags);
 
-		mgmt_discovering(hdev->id, 1);
+		if (!mgmt_is_interleaved_discovery(hdev->id))
+			mgmt_discovering(hdev->id, 1);
 
 		del_timer(&hdev->adv_timer);
 
@@ -1358,6 +1363,7 @@ static void hci_cs_le_start_enc(struct hci_dev *hdev, u8 status)
 static inline void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	__u8 status = *((__u8 *) skb->data);
+	int err;
 
 	BT_DBG("%s status %d", hdev->name, status);
 
@@ -1368,9 +1374,11 @@ static inline void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff
 	if (!test_and_clear_bit(HCI_INQUIRY, &hdev->flags))
 		return;
 
-	mgmt_discovery_complete(hdev->id, 0);
-
-	mgmt_discovering(hdev->id, 0);
+	err = mgmt_interleaved_discovery(hdev->id);
+	if (err < 0) {
+		mgmt_discovery_complete(hdev->id, 0);
+		mgmt_discovering(hdev->id, 0);
+	}
 }
 
 static inline void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *skb)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index e869422..02d05b0 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -40,7 +40,9 @@
 #define LE_SCAN_WIN 0x12
 #define LE_SCAN_INT 0x12
 #define LE_SCAN_TIMEOUT_LE_ONLY 10240 /* TGAP(gen_disc_scan_min) */
+#define LE_SCAN_TIMEOUT_BREDR_LE 5120 /* TGAP(100)/2 */
 #define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
+#define INQUIRY_LEN_BREDR_LE 0x04 /* TGAP(100)/2 */
 
 struct pending_cmd {
 	struct list_head list;
@@ -1643,7 +1645,7 @@ static int start_discovery(struct sock *sk, u16 index)
 	}
 
 	if (lmp_host_le_capable(hdev) && lmp_bredr_capable(hdev))
-		err = -ENOSYS;
+		err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR_LE);
 	else if (lmp_host_le_capable(hdev))
 		err = hci_do_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
 					LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
@@ -2453,3 +2455,41 @@ int mgmt_has_pending_stop_discov(u16 index)
 
 	return 0;
 }
+
+int mgmt_is_interleaved_discovery(u16 index)
+{
+	struct hci_dev *hdev;
+	int res = 0;
+
+	hdev = hci_dev_get(index);
+	if (!hdev)
+		return 0;
+
+	if (mgmt_pending_find(MGMT_OP_START_DISCOVERY, hdev->id) &&
+			lmp_host_le_capable(hdev) && lmp_bredr_capable(hdev))
+		res = 1;
+
+	hci_dev_put(hdev);
+
+	return res;
+}
+
+int mgmt_interleaved_discovery(u16 index)
+{
+	struct hci_dev *hdev;
+	int err;
+
+	if (!mgmt_is_interleaved_discovery(index))
+		return -EPERM;
+
+	hdev = hci_dev_get(index);
+	if (!hdev)
+		return -ENODEV;
+
+	err = hci_do_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT, LE_SCAN_WIN,
+						LE_SCAN_TIMEOUT_BREDR_LE);
+
+	hci_dev_put(hdev);
+
+	return err;
+}
-- 
1.7.5.2