[PATCH v2 0/9] LE-Only discovery procedure support

[PATCH v2 0/9] LE-Only discovery procedure support

[Andre Guedes]

Hi all,

This version implements Marcel's and Johan's comments from
the previous version.

BR,

Andre Guedes

Andre Guedes (9):
  Bluetooth: Add dev_flags to struct hci_dev
  Bluetooth: LE Set Scan Parameter Command
  Bluetooth: Add helper functions to send LE scan commands
  Bluetooth: Add structs to implement LE scan
  Bluetooth: LE scan infra-structure
  Bluetooth: Add LE scan functions to hci_core
  Bluetooth: Add 'eir_len' param to mgmt_device_found()
  Bluetooth: Report LE devices
  Bluetooth: Support LE-Only discovery procedure

 include/net/bluetooth/hci.h      |   18 +++++++
 include/net/bluetooth/hci_core.h |   19 +++++++-
 net/bluetooth/hci_core.c         |   94 ++++++++++++++++++++++++++++++++++++++
 net/bluetooth/hci_event.c        |   61 ++++++++++++++++++++++---
 net/bluetooth/mgmt.c             |   39 ++++++++++++++--
 5 files changed, 218 insertions(+), 13 deletions(-)

-- 
1.7.7.1

[PATCH v2 1/9] Bluetooth: Add dev_flags to struct hci_dev

[Andre Guedes]

This patch adds the dev_flags field to struct hci_dev. This new
flags variable should be used to define flags related to BR/EDR
and/or LE controller itself. It should be used to define flags
which represents states from the controller. The dev_flags is
cleared in case the controller sends a Reset Command Complete
Event to the host.

Also, this patch adds the HCI_LE_SCAN flag which was created to
track if the controller is performing LE scan or not. The flag
is set/cleared when the controller starts/stops scanning.

This is an initial effort to stop using hdev->flags to define
internal flags since it is exported to userspace by an ioctl.

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

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 376c574..c949a67 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -88,6 +88,14 @@ enum {
 	HCI_RESET,
 };
 
+/*
+ * BR/EDR and/or LE controller flags: the flags defined here should represent
+ * states from the controller.
+ */
+enum {
+	HCI_LE_SCAN,
+};
+
 /* HCI ioctl defines */
 #define HCIDEVUP	_IOW('H', 201, int)
 #define HCIDEVDOWN	_IOW('H', 202, int)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 1795257..b2d7514 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -250,6 +250,8 @@ struct hci_dev {
 
 	struct module		*owner;
 
+	unsigned long		dev_flags;
+
 	int (*open)(struct hci_dev *hdev);
 	int (*close)(struct hci_dev *hdev);
 	int (*flush)(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index ef0423e..dcbe1d2 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1458,6 +1458,7 @@ int hci_register_dev(struct hci_dev *hdev)
 	spin_lock_init(&hdev->lock);
 
 	hdev->flags = 0;
+	hdev->dev_flags = 0;
 	hdev->pkt_type  = (HCI_DM1 | HCI_DH1 | HCI_HV1);
 	hdev->esco_type = (ESCO_HV1);
 	hdev->link_mode = (HCI_LM_ACCEPT);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index dfe6fbc..9f74879 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -194,6 +194,8 @@ static void hci_cc_reset(struct hci_dev *hdev, struct sk_buff *skb)
 	clear_bit(HCI_RESET, &hdev->flags);
 
 	hci_req_complete(hdev, HCI_OP_RESET, status);
+
+	hdev->dev_flags = 0;
 }
 
 static void hci_cc_write_local_name(struct hci_dev *hdev, struct sk_buff *skb)
@@ -960,12 +962,16 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 		return;
 
 	if (cp->enable == 0x01) {
+		set_bit(HCI_LE_SCAN, &hdev->dev_flags);
+
 		del_timer(&hdev->adv_timer);
 
 		hci_dev_lock(hdev);
 		hci_adv_entries_clear(hdev);
 		hci_dev_unlock(hdev);
 	} else if (cp->enable == 0x00) {
+		clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
+
 		mod_timer(&hdev->adv_timer, jiffies + ADV_CLEAR_TIMEOUT);
 	}
 }
-- 
1.7.7.1

Re: [PATCH v2 1/9] Bluetooth: Add dev_flags to struct hci_dev

[Marcel Holtmann]

Hi Andre,

> This patch adds the dev_flags field to struct hci_dev. This new
> flags variable should be used to define flags related to BR/EDR
> and/or LE controller itself. It should be used to define flags
> which represents states from the controller. The dev_flags is
> cleared in case the controller sends a Reset Command Complete
> Event to the host.
> 
> Also, this patch adds the HCI_LE_SCAN flag which was created to
> track if the controller is performing LE scan or not. The flag
> is set/cleared when the controller starts/stops scanning.
> 
> This is an initial effort to stop using hdev->flags to define
> internal flags since it is exported to userspace by an ioctl.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci.h      |    8 ++++++++
>  include/net/bluetooth/hci_core.h |    2 ++
>  net/bluetooth/hci_core.c         |    1 +
>  net/bluetooth/hci_event.c        |    6 ++++++
>  4 files changed, 17 insertions(+), 0 deletions(-)

looks good to me.

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

Regards

Marcel

[PATCH v2 2/9] Bluetooth: LE Set Scan Parameter Command

[Andre Guedes]

This patch adds the parameter struct and the command complete event
handler to the LE Set Scan Parameter HCI command.

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

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c949a67..b7c6452 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -776,6 +776,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/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 9f74879..845b26b 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -946,6 +946,13 @@ static void hci_cc_read_local_oob_data_reply(struct hci_dev *hdev,
 	hci_dev_unlock(hdev);
 }
 
+static void hci_cc_le_set_scan_param(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	__u8 status = *((__u8 *) skb->data);
+
+	BT_DBG("%s status 0x%x", hdev->name, status);
+}
+
 static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 					struct sk_buff *skb)
 {
@@ -2021,6 +2028,10 @@ static inline void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *sk
 		hci_cc_user_confirm_neg_reply(hdev, skb);
 		break;
 
+	case HCI_OP_LE_SET_SCAN_PARAM:
+		hci_cc_le_set_scan_param(hdev, skb);
+		break;
+
 	case HCI_OP_LE_SET_SCAN_ENABLE:
 		hci_cc_le_set_scan_enable(hdev, skb);
 		break;
-- 
1.7.7.1

Re: [PATCH v2 2/9] Bluetooth: LE Set Scan Parameter Command

[Marcel Holtmann]

Hi Andre,

> This patch adds the parameter struct and the command complete event
> handler to the LE Set Scan Parameter HCI command.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci.h |    9 +++++++++
>  net/bluetooth/hci_event.c   |   11 +++++++++++
>  2 files changed, 20 insertions(+), 0 deletions(-)

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

Regards

Marcel

Re: [PATCH v2 2/9] Bluetooth: LE Set Scan Parameter Command

[Gustavo Padovan]

Hi Andre,

* Andre Guedes <andre.guedes@openbossa.org> [2011-11-25 20:53:39 -0300]:

> This patch adds the parameter struct and the command complete event
> handler to the LE Set Scan Parameter HCI command.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci.h |    9 +++++++++
>  net/bluetooth/hci_event.c   |   11 +++++++++++
>  2 files changed, 20 insertions(+), 0 deletions(-)

Patches 1 and 2 were applied, thanks.

	Gustavo

[PATCH v2 3/9] Bluetooth: Add helper functions to send LE scan commands

[Andre Guedes]

This patch creates two helper functions to send LE Set Scan
Parameters and LE Set Scan Enable commands.

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index dcbe1d2..28ef2ac 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -94,6 +94,29 @@ static void hci_notify(struct hci_dev *hdev, int event)
 	atomic_notifier_call_chain(&hci_notifier, event, hdev);
 }
 
+static int send_le_scan_param_cmd(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);
+}
+
+static int send_le_scan_enable_cmd(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);
+}
+
 /* ---- HCI requests ---- */
 
 void hci_req_complete(struct hci_dev *hdev, __u16 cmd, int result)
-- 
1.7.7.1

Re: [PATCH v2 3/9] Bluetooth: Add helper functions to send LE scan commands

[Marcel Holtmann]

Hi Andre,

> This patch creates two helper functions to send LE Set Scan
> Parameters and LE Set Scan Enable commands.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  net/bluetooth/hci_core.c |   23 +++++++++++++++++++++++
>  1 files changed, 23 insertions(+), 0 deletions(-)

I trust you that these are really needed this way.

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

Regards

Marcel

[PATCH v2 4/9] Bluetooth: Add structs to implement LE scan

[Andre Guedes]

This patch adds to hci_dev the structs needed to implement the
LE scan infra-structure.

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index b2d7514..a48c699 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -121,6 +121,13 @@ struct adv_entry {
 	u8 bdaddr_type;
 };
 
+struct le_scan_params {
+	u8 type;
+	u16 interval;
+	u16 window;
+	int timeout;
+};
+
 #define NUM_REASSEMBLY 4
 struct hci_dev {
 	struct list_head list;
@@ -252,6 +259,10 @@ struct hci_dev {
 
 	unsigned long		dev_flags;
 
+	struct work_struct	le_scan;
+	struct le_scan_params	le_scan_params;
+	struct timer_list	le_scan_timer;
+
 	int (*open)(struct hci_dev *hdev);
 	int (*close)(struct hci_dev *hdev);
 	int (*flush)(struct hci_dev *hdev);
-- 
1.7.7.1

[PATCH v2 5/9] Bluetooth: LE scan infra-structure

[Andre Guedes]

This patch does the proper init of the structs required to carry
out LE scan and implement the LE scan work.

The LE scan work sends the commands (Set LE Scan Parameters and Set
LE Scan Enable) to the controller in order to start LE scanning. If
commands were executed successfully the le_scan_timer is set to
disable the ongoing scanning after some amount of time.

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

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index b7c6452..6e2b88f 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -130,6 +130,7 @@ enum {
 #define HCI_IDLE_TIMEOUT	(6000)	/* 6 seconds */
 #define HCI_INIT_TIMEOUT	(10000)	/* 10 seconds */
 #define HCI_CMD_TIMEOUT		(1000)	/* 1 seconds */
+#define HCI_LE_SCAN_TIMEOUT	(3000)	/* 3 seconds */
 
 /* HCI data types */
 #define HCI_COMMAND_PKT		0x01
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 28ef2ac..8e96e3b 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -981,6 +981,33 @@ static void hci_power_on(struct work_struct *work)
 		mgmt_index_added(hdev);
 }
 
+static void le_scan_req(struct hci_dev *hdev, unsigned long opt)
+{
+	struct le_scan_params *params = (void *) opt;
+
+	if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
+		return;
+
+	send_le_scan_param_cmd(hdev, params->type, params->interval,
+							params->window);
+	send_le_scan_enable_cmd(hdev, 1);
+}
+
+static void hci_le_scan(struct work_struct *work)
+{
+	struct hci_dev *hdev = container_of(work, struct hci_dev, le_scan);
+	struct le_scan_params *params = &hdev->le_scan_params;
+	int err;
+
+	err = hci_request(hdev, le_scan_req, (unsigned long) params,
+					msecs_to_jiffies(HCI_LE_SCAN_TIMEOUT));
+	if (err < 0)
+		return;
+
+	mod_timer(&hdev->le_scan_timer, jiffies +
+					msecs_to_jiffies(params->timeout));
+}
+
 static void hci_power_off(struct work_struct *work)
 {
 	struct hci_dev *hdev = container_of(work, struct hci_dev,
@@ -1447,6 +1474,13 @@ int hci_add_adv_entry(struct hci_dev *hdev,
 	return 0;
 }
 
+static void le_scan_timeout(unsigned long arg)
+{
+	struct hci_dev *hdev = (void *) arg;
+
+	send_le_scan_enable_cmd(hdev, 0);
+}
+
 /* Register HCI device */
 int hci_register_dev(struct hci_dev *hdev)
 {
@@ -1500,6 +1534,8 @@ int hci_register_dev(struct hci_dev *hdev)
 	skb_queue_head_init(&hdev->raw_q);
 
 	setup_timer(&hdev->cmd_timer, hci_cmd_timer, (unsigned long) hdev);
+	setup_timer(&hdev->le_scan_timer, le_scan_timeout,
+							(unsigned long) hdev);
 
 	for (i = 0; i < NUM_REASSEMBLY; i++)
 		hdev->reassembly[i] = NULL;
@@ -1526,6 +1562,7 @@ int hci_register_dev(struct hci_dev *hdev)
 						(unsigned long) hdev);
 
 	INIT_WORK(&hdev->power_on, hci_power_on);
+	INIT_WORK(&hdev->le_scan, hci_le_scan);
 	INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
 
 	INIT_DELAYED_WORK(&hdev->discov_off, hci_discov_off);
@@ -1611,6 +1648,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
 	hci_del_sysfs(hdev);
 
 	del_timer(&hdev->adv_timer);
+	del_timer(&hdev->le_scan_timer);
 
 	destroy_workqueue(hdev->workqueue);
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 845b26b..ea09c11 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -951,6 +951,9 @@ static void hci_cc_le_set_scan_param(struct hci_dev *hdev, struct sk_buff *skb)
 	__u8 status = *((__u8 *) skb->data);
 
 	BT_DBG("%s status 0x%x", hdev->name, status);
+
+	if (status)
+		hci_req_complete(hdev, HCI_OP_LE_SET_SCAN_PARAM, status);
 }
 
 static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
@@ -969,6 +972,8 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 		return;
 
 	if (cp->enable == 0x01) {
+		hci_req_complete(hdev, HCI_OP_LE_SET_SCAN_ENABLE, status);
+
 		set_bit(HCI_LE_SCAN, &hdev->dev_flags);
 
 		del_timer(&hdev->adv_timer);
-- 
1.7.7.1

Re: [PATCH v2 5/9] Bluetooth: LE scan infra-structure

[Marcel Holtmann]

Hi Andre,

> This patch does the proper init of the structs required to carry
> out LE scan and implement the LE scan work.
> 
> The LE scan work sends the commands (Set LE Scan Parameters and Set
> LE Scan Enable) to the controller in order to start LE scanning. If
> commands were executed successfully the le_scan_timer is set to
> disable the ongoing scanning after some amount of time.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci.h |    1 +
>  net/bluetooth/hci_core.c    |   38 ++++++++++++++++++++++++++++++++++++++
>  net/bluetooth/hci_event.c   |    5 +++++
>  3 files changed, 44 insertions(+), 0 deletions(-)

combine patch 4 and 5 since the split is weird.

> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index b7c6452..6e2b88f 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -130,6 +130,7 @@ enum {
>  #define HCI_IDLE_TIMEOUT	(6000)	/* 6 seconds */
>  #define HCI_INIT_TIMEOUT	(10000)	/* 10 seconds */
>  #define HCI_CMD_TIMEOUT		(1000)	/* 1 seconds */
> +#define HCI_LE_SCAN_TIMEOUT	(3000)	/* 3 seconds */
>  
>  /* HCI data types */
>  #define HCI_COMMAND_PKT		0x01
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 28ef2ac..8e96e3b 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -981,6 +981,33 @@ static void hci_power_on(struct work_struct *work)
>  		mgmt_index_added(hdev);
>  }
>  
> +static void le_scan_req(struct hci_dev *hdev, unsigned long opt)
> +{
> +	struct le_scan_params *params = (void *) opt;
> +
> +	if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
> +		return;
> +
> +	send_le_scan_param_cmd(hdev, params->type, params->interval,
> +							params->window);
> +	send_le_scan_enable_cmd(hdev, 1);
> +}
> +
> +static void hci_le_scan(struct work_struct *work)
> +{
> +	struct hci_dev *hdev = container_of(work, struct hci_dev, le_scan);
> +	struct le_scan_params *params = &hdev->le_scan_params;
> +	int err;
> +
> +	err = hci_request(hdev, le_scan_req, (unsigned long) params,
> +					msecs_to_jiffies(HCI_LE_SCAN_TIMEOUT));
> +	if (err < 0)
> +		return;
> +
> +	mod_timer(&hdev->le_scan_timer, jiffies +
> +					msecs_to_jiffies(params->timeout));
> +}
> +
>  static void hci_power_off(struct work_struct *work)
>  {
>  	struct hci_dev *hdev = container_of(work, struct hci_dev,
> @@ -1447,6 +1474,13 @@ int hci_add_adv_entry(struct hci_dev *hdev,
>  	return 0;
>  }
>  
> +static void le_scan_timeout(unsigned long arg)
> +{
> +	struct hci_dev *hdev = (void *) arg;
> +
> +	send_le_scan_enable_cmd(hdev, 0);
> +}
> +
>  /* Register HCI device */
>  int hci_register_dev(struct hci_dev *hdev)
>  {
> @@ -1500,6 +1534,8 @@ int hci_register_dev(struct hci_dev *hdev)
>  	skb_queue_head_init(&hdev->raw_q);
>  
>  	setup_timer(&hdev->cmd_timer, hci_cmd_timer, (unsigned long) hdev);
> +	setup_timer(&hdev->le_scan_timer, le_scan_timeout,
> +							(unsigned long) hdev);
>  
>  	for (i = 0; i < NUM_REASSEMBLY; i++)
>  		hdev->reassembly[i] = NULL;
> @@ -1526,6 +1562,7 @@ int hci_register_dev(struct hci_dev *hdev)
>  						(unsigned long) hdev);
>  
>  	INIT_WORK(&hdev->power_on, hci_power_on);
> +	INIT_WORK(&hdev->le_scan, hci_le_scan);
>  	INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
>  
>  	INIT_DELAYED_WORK(&hdev->discov_off, hci_discov_off);

I am beginning to question how many work structs we really want here.
And more important that we need to ensure that they are scheduled from
the main workqueue that we have per HCI controller.

And while at this, we might wanna actually move away from tasklets
finally so that we actually can sleep and can reduce the number of work
structs here. I am open for ideas.

Regards

Marcel

Re: [PATCH v2 5/9] Bluetooth: LE scan infra-structure

[Andre Guedes]

Hi Marcel,

On Nov 28, 2011, at 1:24 PM, Marcel Holtmann wrote:

> Hi Andre,
> 
>> This patch does the proper init of the structs required to carry
>> out LE scan and implement the LE scan work.
>> 
>> The LE scan work sends the commands (Set LE Scan Parameters and Set
>> LE Scan Enable) to the controller in order to start LE scanning. If
>> commands were executed successfully the le_scan_timer is set to
>> disable the ongoing scanning after some amount of time.
>> 
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/hci.h |    1 +
>> net/bluetooth/hci_core.c    |   38 ++++++++++++++++++++++++++++++++++++++
>> net/bluetooth/hci_event.c   |    5 +++++
>> 3 files changed, 44 insertions(+), 0 deletions(-)
> 
> combine patch 4 and 5 since the split is weird.

Ok.

> 
>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>> index b7c6452..6e2b88f 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -130,6 +130,7 @@ enum {
>> #define HCI_IDLE_TIMEOUT	(6000)	/* 6 seconds */
>> #define HCI_INIT_TIMEOUT	(10000)	/* 10 seconds */
>> #define HCI_CMD_TIMEOUT		(1000)	/* 1 seconds */
>> +#define HCI_LE_SCAN_TIMEOUT	(3000)	/* 3 seconds */
>> 
>> /* HCI data types */
>> #define HCI_COMMAND_PKT		0x01
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 28ef2ac..8e96e3b 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -981,6 +981,33 @@ static void hci_power_on(struct work_struct *work)
>> 		mgmt_index_added(hdev);
>> }
>> 
>> +static void le_scan_req(struct hci_dev *hdev, unsigned long opt)
>> +{
>> +	struct le_scan_params *params = (void *) opt;
>> +
>> +	if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
>> +		return;
>> +
>> +	send_le_scan_param_cmd(hdev, params->type, params->interval,
>> +							params->window);
>> +	send_le_scan_enable_cmd(hdev, 1);
>> +}
>> +
>> +static void hci_le_scan(struct work_struct *work)
>> +{
>> +	struct hci_dev *hdev = container_of(work, struct hci_dev, le_scan);
>> +	struct le_scan_params *params = &hdev->le_scan_params;
>> +	int err;
>> +
>> +	err = hci_request(hdev, le_scan_req, (unsigned long) params,
>> +					msecs_to_jiffies(HCI_LE_SCAN_TIMEOUT));
>> +	if (err < 0)
>> +		return;
>> +
>> +	mod_timer(&hdev->le_scan_timer, jiffies +
>> +					msecs_to_jiffies(params->timeout));
>> +}
>> +
>> static void hci_power_off(struct work_struct *work)
>> {
>> 	struct hci_dev *hdev = container_of(work, struct hci_dev,
>> @@ -1447,6 +1474,13 @@ int hci_add_adv_entry(struct hci_dev *hdev,
>> 	return 0;
>> }
>> 
>> +static void le_scan_timeout(unsigned long arg)
>> +{
>> +	struct hci_dev *hdev = (void *) arg;
>> +
>> +	send_le_scan_enable_cmd(hdev, 0);
>> +}
>> +
>> /* Register HCI device */
>> int hci_register_dev(struct hci_dev *hdev)
>> {
>> @@ -1500,6 +1534,8 @@ int hci_register_dev(struct hci_dev *hdev)
>> 	skb_queue_head_init(&hdev->raw_q);
>> 
>> 	setup_timer(&hdev->cmd_timer, hci_cmd_timer, (unsigned long) hdev);
>> +	setup_timer(&hdev->le_scan_timer, le_scan_timeout,
>> +							(unsigned long) hdev);
>> 
>> 	for (i = 0; i < NUM_REASSEMBLY; i++)
>> 		hdev->reassembly[i] = NULL;
>> @@ -1526,6 +1562,7 @@ int hci_register_dev(struct hci_dev *hdev)
>> 						(unsigned long) hdev);
>> 
>> 	INIT_WORK(&hdev->power_on, hci_power_on);
>> +	INIT_WORK(&hdev->le_scan, hci_le_scan);
>> 	INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
>> 
>> 	INIT_DELAYED_WORK(&hdev->discov_off, hci_discov_off);
> 
> I am beginning to question how many work structs we really want here.
> And more important that we need to ensure that they are scheduled from
> the main workqueue that we have per HCI controller.
> 
> And while at this, we might wanna actually move away from tasklets
> finally so that we actually can sleep and can reduce the number of work
> structs here. I am open for ideas.
> 
> Regards
> 
> Marcel
> 
> 

Andre

Query on Media Interface "RegisterPlayer" and Dbus signal "TrackChanged"

[Jaganath]

Hi,

I have come across a scenario related to Media Interface meant for AVRCP 1.3 
features.

Kindly please provide your opinions on this.

If the Media Interface "RegisterPlayer" is called before the  headset 
connection, then player gets registered successfully and all the
Vendor dependent commands are handled properly. And in case of any change in 
the track, through the "TrackChanged" Dbus signal
shall be sent with information about "Change of track" and "Start of track". 
This scenario will work because we have registered for the
callback  "state_changed"(audio/avrcp.c) through "avrcp_register_player" 
(audio/avrcp.c) API. When connection is ongoing "state_changed"
callback shall be invoked with state as "AVCTP_STATE_CONNECTING" where in 
"player->session" will be initialized.


However if the "RegisterPlayer" is called after the headset connection, then 
we will not be able to send the "TrackChanged" signal with
information of "Change of track" and "Start of track". Since the headset is 
already connected,"state_changed" callback will not be
invoked, hence in the avrcp_player_event" API "player->session" is NULL.

Is it not necessary to handle the above the mentioned scenario?

Thanks and Regards
Jaganath
 

Re: Query on Media Interface "RegisterPlayer" and Dbus signal "TrackChanged"

[Luiz Augusto von Dentz]

Hi Jaganath,

On Fri, Dec 2, 2011 at 12:02 PM, Jaganath <jaganath.k@samsung.com> wrote:
> Hi,
>
> I have come across a scenario related to Media Interface meant for AVRCP 1.3
> features.
>
> Kindly please provide your opinions on this.
>
> If the Media Interface "RegisterPlayer" is called before the  headset
> connection, then player gets registered successfully and all the
> Vendor dependent commands are handled properly. And in case of any change in
> the track, through the "TrackChanged" Dbus signal
> shall be sent with information about "Change of track" and "Start of track".
> This scenario will work because we have registered for the
> callback  "state_changed"(audio/avrcp.c) through "avrcp_register_player"
> (audio/avrcp.c) API. When connection is ongoing "state_changed"
> callback shall be invoked with state as "AVCTP_STATE_CONNECTING" where in
> "player->session" will be initialized.
>
>
> However if the "RegisterPlayer" is called after the headset connection, then
> we will not be able to send the "TrackChanged" signal with
> information of "Change of track" and "Start of track". Since the headset is
> already connected,"state_changed" callback will not be
> invoked, hence in the avrcp_player_event" API "player->session" is NULL.
>
> Is it not necessary to handle the above the mentioned scenario?

Yes that is probably a valid and common scenario, so we probably need
to change the code to update the session when this happen but
apparently there is no way to notify the remote device about this
since the concept of players and EVENT_AVAILABLE_PLAYERS_CHANGED was
introduced only on 1.4. So for 1.3 we might have to pretend we have a
player without any metadata and as soon as one is registered we
assigned it to the active session and notify the metadata has changed
via events.

-- 
Luiz Augusto von Dentz

Re: Query on Media Interface "RegisterPlayer" and Dbus signal "TrackChanged"

[sathish]

  On Friday 02 December 2011 04:37 PM, Luiz Augusto von Dentz wrote:
> Hi Jaganath,
>
> On Fri, Dec 2, 2011 at 12:02 PM, Jaganath<jaganath.k@samsung.com>  wrote:
>> Hi,
>>
>> I have come across a scenario related to Media Interface meant for AVRCP 1.3
>> features.
>>
>> Kindly please provide your opinions on this.
>>
>> If the Media Interface "RegisterPlayer" is called before the  headset
>> connection, then player gets registered successfully and all the
>> Vendor dependent commands are handled properly. And in case of any change in
>> the track, through the "TrackChanged" Dbus signal
>> shall be sent with information about "Change of track" and "Start of track".
>> This scenario will work because we have registered for the
>> callback  "state_changed"(audio/avrcp.c) through "avrcp_register_player"
>> (audio/avrcp.c) API. When connection is ongoing "state_changed"
>> callback shall be invoked with state as "AVCTP_STATE_CONNECTING" where in
>> "player->session" will be initialized.
>>
>>
>> However if the "RegisterPlayer" is called after the headset connection, then
>> we will not be able to send the "TrackChanged" signal with
>> information of "Change of track" and "Start of track". Since the headset is
>> already connected,"state_changed" callback will not be
>> invoked, hence in the avrcp_player_event" API "player->session" is NULL.
>>
>> Is it not necessary to handle the above the mentioned scenario?
> Yes that is probably a valid and common scenario, so we probably need
> to change the code to update the session when this happen but
> apparently there is no way to notify the remote device about this
> since the concept of players and EVENT_AVAILABLE_PLAYERS_CHANGED was
> introduced only on 1.4. So for 1.3 we might have to pretend we have a
> player without any metadata and as soon as one is registered we
> assigned it to the active session and notify the metadata has changed
> via events.
>
Hi,
     Please provide the information about how to use mpris 2.0 spec , 
can u please provide me detail of platform that register player worked 
successfully. I am also working on getting these information .

Thanks & regards ,
sathish N

-- 
Thanks&  Regards,
Sathish N

Re: Query on Media Interface "RegisterPlayer" and Dbus signal "TrackChanged"

[Chethan T N]

Hi,

--------------------------------------------------
From: "sathish" <sathish.n@globaledgesoft.com>
Sent: Monday, December 05, 2011 6:33 PM
To: "Luiz Augusto von Dentz" <luiz.dentz@gmail.com>
Cc: "Jaganath" <jaganath.k@samsung.com>; <linux-bluetooth@vger.kernel.org>
Subject: Re: Query on Media Interface "RegisterPlayer" and Dbus signal 
"TrackChanged"

>  On Friday 02 December 2011 04:37 PM, Luiz Augusto von Dentz wrote:
>> Hi Jaganath,
>>
>> On Fri, Dec 2, 2011 at 12:02 PM, Jaganath<jaganath.k@samsung.com>  wrote:
>>> Hi,
>>>
>>> I have come across a scenario related to Media Interface meant for AVRCP 
>>> 1.3
>>> features.
>>>
>>> Kindly please provide your opinions on this.
>>>
>>> If the Media Interface "RegisterPlayer" is called before the  headset
>>> connection, then player gets registered successfully and all the
>>> Vendor dependent commands are handled properly. And in case of any 
>>> change in
>>> the track, through the "TrackChanged" Dbus signal
>>> shall be sent with information about "Change of track" and "Start of 
>>> track".
>>> This scenario will work because we have registered for the
>>> callback  "state_changed"(audio/avrcp.c) through "avrcp_register_player"
>>> (audio/avrcp.c) API. When connection is ongoing "state_changed"
>>> callback shall be invoked with state as "AVCTP_STATE_CONNECTING" where 
>>> in
>>> "player->session" will be initialized.
>>>
>>>
>>> However if the "RegisterPlayer" is called after the headset connection, 
>>> then
>>> we will not be able to send the "TrackChanged" signal with
>>> information of "Change of track" and "Start of track". Since the headset 
>>> is
>>> already connected,"state_changed" callback will not be
>>> invoked, hence in the avrcp_player_event" API "player->session" is NULL.
>>>
>>> Is it not necessary to handle the above the mentioned scenario?
>> Yes that is probably a valid and common scenario, so we probably need
>> to change the code to update the session when this happen but
>> apparently there is no way to notify the remote device about this
>> since the concept of players and EVENT_AVAILABLE_PLAYERS_CHANGED was
>> introduced only on 1.4. So for 1.3 we might have to pretend we have a
>> player without any metadata and as soon as one is registered we
>> assigned it to the active session and notify the metadata has changed
>> via events.
>>
> Hi,
>     Please provide the information about how to use mpris 2.0 spec , can u 
> please provide me detail of platform that register player worked 
> successfully. I am also working on getting these information .
>
> Thanks & regards ,
> sathish N
>
> -- 
> Thanks&  Regards,
> Sathish N
>
    You can directly make use of DBus methods specified in the media-api.txt 
for
registering/unregistering the player, also you can send the Dbus signals 
using the
PropertyChanged for the player settings changes and TrackChanged for 
metadata
attributes changes.

Write a simple test application which makes uses of the Dbus methods and 
signals
provided in the media-api.txt.

To implement the above methods need not to follow the specification of 
mpris2.0 spec.

Thanks and Regards
Chethan 

[PATCH v2 6/9] Bluetooth: Add LE scan functions to hci_core

[Andre Guedes]

This patch adds to hci_core functions to init LE scan and cancel
an ongoing scanning (hci_do_le_scan and hci_cancel_le_scan).

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a48c699..db137ca 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -998,5 +998,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 8e96e3b..1e5d9db 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2678,5 +2678,37 @@ int hci_cancel_inquiry(struct hci_dev *hdev)
 	return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
 }
 
+int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
+								int timeout)
+{
+	struct le_scan_params *params = &hdev->le_scan_params;
+
+	if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
+		return -EINPROGRESS;
+
+	BT_DBG("%s", hdev->name);
+
+	params->type = type;
+	params->interval = interval;
+	params->window = window;
+	params->timeout = timeout;
+
+	queue_work(hdev->workqueue, &hdev->le_scan);
+
+	return 0;
+}
+
+int hci_cancel_le_scan(struct hci_dev *hdev)
+{
+	if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
+		return -EPERM;
+
+	BT_DBG("%s", hdev->name);
+
+	del_timer(&hdev->le_scan_timer);
+
+	return send_le_scan_enable_cmd(hdev, 0);
+}
+
 module_param(enable_hs, bool, 0644);
 MODULE_PARM_DESC(enable_hs, "Enable High Speed");
-- 
1.7.7.1

Re: [PATCH v2 6/9] Bluetooth: Add LE scan functions to hci_core

[Marcel Holtmann]

Hi Andre,

> This patch adds to hci_core functions to init LE scan and cancel
> an ongoing scanning (hci_do_le_scan and hci_cancel_le_scan).
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci_core.h |    3 +++
>  net/bluetooth/hci_core.c         |   32 ++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index a48c699..db137ca 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -998,5 +998,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 8e96e3b..1e5d9db 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2678,5 +2678,37 @@ int hci_cancel_inquiry(struct hci_dev *hdev)
>  	return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
>  }
>  
> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
> +								int timeout)
> +{
> +	struct le_scan_params *params = &hdev->le_scan_params;
> +
> +	if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
> +		return -EINPROGRESS;
> +
> +	BT_DBG("%s", hdev->name);
> +
> +	params->type = type;
> +	params->interval = interval;
> +	params->window = window;
> +	params->timeout = timeout;
> +
> +	queue_work(hdev->workqueue, &hdev->le_scan);

so you are using the controller workqueue already. That is good. However
if the send command are already processed in a workqueue, we could just
sleep for their results. No need for hci_req_complete handling that we
are using for ioctl based triggers. We could have a lot simpler
hci_request handling from within the workqueue.

> +
> +	return 0;
> +}
> +
> +int hci_cancel_le_scan(struct hci_dev *hdev)
> +{
> +	if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
> +		return -EPERM;
> +
> +	BT_DBG("%s", hdev->name);
> +
> +	del_timer(&hdev->le_scan_timer);
> +
> +	return send_le_scan_enable_cmd(hdev, 0);
> +}
> +

Don't you need to clear out the work struct as well? In case that one is
still running? Meaning cancel gets called quickly after starting the
scan. The window might be small, but this is a race condition.

Regards

Marcel

Re: [PATCH v2 6/9] Bluetooth: Add LE scan functions to hci_core

[Andre Guedes]

Hi Marcel,

On Nov 28, 2011, at 1:28 PM, Marcel Holtmann wrote:

> Hi Andre,
>=20
>> This patch adds to hci_core functions to init LE scan and cancel
>> an ongoing scanning (hci_do_le_scan and hci_cancel_le_scan).
>>=20
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/hci_core.h |    3 +++
>> net/bluetooth/hci_core.c         |   32 =
++++++++++++++++++++++++++++++++
>> 2 files changed, 35 insertions(+), 0 deletions(-)
>>=20
>> diff --git a/include/net/bluetooth/hci_core.h =
b/include/net/bluetooth/hci_core.h
>> index a48c699..db137ca 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -998,5 +998,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 8e96e3b..1e5d9db 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -2678,5 +2678,37 @@ int hci_cancel_inquiry(struct hci_dev *hdev)
>> 	return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
>> }
>>=20
>> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 =
window,
>> +								int =
timeout)
>> +{
>> +	struct le_scan_params *params =3D &hdev->le_scan_params;
>> +
>> +	if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
>> +		return -EINPROGRESS;
>> +
>> +	BT_DBG("%s", hdev->name);
>> +
>> +	params->type =3D type;
>> +	params->interval =3D interval;
>> +	params->window =3D window;
>> +	params->timeout =3D timeout;
>> +
>> +	queue_work(hdev->workqueue, &hdev->le_scan);
>=20
> so you are using the controller workqueue already. That is good. =
However
> if the send command are already processed in a workqueue, we could =
just
> sleep for their results. No need for hci_req_complete handling that we
> are using for ioctl based triggers. We could have a lot simpler
> hci_request handling from within the workqueue.

Ok, I'll replace hci_request by another simpler mechanism.

>> +
>> +	return 0;
>> +}
>> +
>> +int hci_cancel_le_scan(struct hci_dev *hdev)
>> +{
>> +	if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
>> +		return -EPERM;
>> +
>> +	BT_DBG("%s", hdev->name);
>> +
>> +	del_timer(&hdev->le_scan_timer);
>> +
>> +	return send_le_scan_enable_cmd(hdev, 0);
>> +}
>> +
>=20
> Don't you need to clear out the work struct as well? In case that one =
is
> still running? Meaning cancel gets called quickly after starting the
> scan. The window might be small, but this is a race condition.


If we want to be able to cancel le scan during this small window we
should cancel it (if it didn't start yet) or wait until "le scan" work
finishes. To achieve that we can use cancel_work_sync(), but it
blocks. So, we'll need another work to handle this.

This is a bit tricky actually. Since hdev->workqueue is single thread,
the "cancel le scan" work will always run after "le scan" work. So, if
we enqueue "cancel le scan" work in hdev->workqueue we won't be able
to cancel the "le scan" work if it is not started yet.

Do you think we should enqueue "cancel le scan" work in the system-wide
workqueue so we have the chance to cancel "le scan" work before it
starts?

BR,

Andre

[PATCH v2 7/9] 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>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 include/net/bluetooth/hci_core.h |    3 ++-
 net/bluetooth/hci_event.c        |    9 +++++----
 net/bluetooth/mgmt.c             |    8 ++++++--
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index db137ca..a4ac427 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -951,7 +951,8 @@ int mgmt_set_local_name_complete(struct hci_dev *hdev, u8 *name, u8 status);
 int mgmt_read_local_oob_data_reply_complete(struct hci_dev *hdev, u8 *hash,
 						u8 *randomizer, u8 status);
 int mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
-				u8 addr_type, u8 *dev_class, s8 rssi, u8 *eir);
+					u8 addr_type, u8 *dev_class, s8 rssi,
+					u8 *eir,  u8 eir_len);
 int mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 *name);
 int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status);
 int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index ea09c11..865fdf6 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1460,7 +1460,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, &info->bdaddr, ACL_LINK, 0x00,
-						info->dev_class, 0, NULL);
+						info->dev_class, 0, NULL, 0);
 	}
 
 	hci_dev_unlock(hdev);
@@ -2474,7 +2474,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, &info->bdaddr, ACL_LINK, 0x00,
 						info->dev_class, info->rssi,
-						NULL);
+						NULL, 0);
 		}
 	} else {
 		struct inquiry_info_with_rssi *info = (void *) (skb->data + 1);
@@ -2491,7 +2491,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, &info->bdaddr, ACL_LINK, 0x00,
 						info->dev_class, info->rssi,
-						NULL);
+						NULL, 0);
 		}
 	}
 
@@ -2633,7 +2633,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, &info->bdaddr, ACL_LINK, 0x00,
-				info->dev_class, info->rssi, info->data);
+					info->dev_class, info->rssi,
+					info->data, sizeof(info->data));
 	}
 
 	hci_dev_unlock(hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index c06a05c..6a74955 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2590,10 +2590,14 @@ int mgmt_read_local_oob_data_reply_complete(struct hci_dev *hdev, u8 *hash,
 }
 
 int mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
-				u8 addr_type, u8 *dev_class, s8 rssi, u8 *eir)
+					u8 addr_type, u8 *dev_class, s8 rssi,
+					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.addr.bdaddr, bdaddr);
@@ -2601,7 +2605,7 @@ int mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 	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.7.1

RE: [PATCH v2 7/9] Bluetooth: Add 'eir_len' param to mgmt_device_found()

[Ganir, Chen]

Andre,

> -----Original Message-----
> From: linux-bluetooth-owner@vger.kernel.org [mailto:linux-bluetooth-
> owner@vger.kernel.org] On Behalf Of Andre Guedes
> Sent: Saturday, November 26, 2011 1:54 AM
> To: linux-bluetooth@vger.kernel.org
> Subject: [PATCH v2 7/9] Bluetooth: Add 'eir_len' param to
> mgmt_device_found()
> 
> 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>
> Acked-by: Marcel Holtmann <marcel@holtmann.org>
> ---
>  include/net/bluetooth/hci_core.h |    3 ++-
>  net/bluetooth/hci_event.c        |    9 +++++----
>  net/bluetooth/mgmt.c             |    8 ++++++--
>  3 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h
> b/include/net/bluetooth/hci_core.h
> index db137ca..a4ac427 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -951,7 +951,8 @@ int mgmt_set_local_name_complete(struct hci_dev
> *hdev, u8 *name, u8 status);
>  int mgmt_read_local_oob_data_reply_complete(struct hci_dev *hdev, u8
> *hash,
>  						u8 *randomizer, u8 status);
>  int mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8
> link_type,
> -				u8 addr_type, u8 *dev_class, s8 rssi, u8 *eir);
> +					u8 addr_type, u8 *dev_class, s8 rssi,
> +					u8 *eir,  u8 eir_len);
>  int mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8
> *name);
>  int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status);
>  int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index ea09c11..865fdf6 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1460,7 +1460,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, &info->bdaddr, ACL_LINK, 0x00,
> -						info->dev_class, 0, NULL);
> +						info->dev_class, 0, NULL, 0);
>  	}
> 
>  	hci_dev_unlock(hdev);
> @@ -2474,7 +2474,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, &info->bdaddr, ACL_LINK,
> 0x00,
>  						info->dev_class, info->rssi,
> -						NULL);
> +						NULL, 0);
>  		}
>  	} else {
>  		struct inquiry_info_with_rssi *info = (void *) (skb->data +
> 1);
> @@ -2491,7 +2491,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, &info->bdaddr, ACL_LINK,
> 0x00,
>  						info->dev_class, info->rssi,
> -						NULL);
> +						NULL, 0);
>  		}
>  	}
> 
> @@ -2633,7 +2633,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, &info->bdaddr, ACL_LINK, 0x00,
> -				info->dev_class, info->rssi, info->data);
> +					info->dev_class, info->rssi,
> +					info->data, sizeof(info->data));
>  	}
> 
>  	hci_dev_unlock(hdev);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index c06a05c..6a74955 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2590,10 +2590,14 @@ int
> mgmt_read_local_oob_data_reply_complete(struct hci_dev *hdev, u8 *hash,
>  }
> 
>  int mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8
> link_type,
> -				u8 addr_type, u8 *dev_class, s8 rssi, u8 *eir)
> +					u8 addr_type, u8 *dev_class, s8 rssi,
> +					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.addr.bdaddr, bdaddr);
> @@ -2601,7 +2605,7 @@ int mgmt_device_found(struct hci_dev *hdev,
> bdaddr_t *bdaddr, u8 link_type,
>  	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.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Why do we really need this ? The GAP Spec clearly defines a fixed advertising size of 31 octets (Vol3, Part C, Section 11). Instead of reporting how much we got (may be other than 31 if the peer device does not conform to the spec as required), we should make sure that BlueZ will always report 31 octets, and make sure that the device found event always sends a buffer of 31 octets.

Thanks,
Chen Ganir

Re: [PATCH v2 7/9] Bluetooth: Add 'eir_len' param to mgmt_device_found()

[Anderson Lizardo]

Hi Chen,

On Sun, Nov 27, 2011 at 2:37 AM, Ganir, Chen <chen.ganir@ti.com> wrote:
> Why do we really need this ? The GAP Spec clearly defines a fixed advertising size of 31 octets (Vol3, Part C, Section 11). Instead of reporting how much we got (may be other than 31 if the peer device does not conform to the spec as required), we should make sure that BlueZ will always report 31 octets, and make sure that the device found event always sends a buffer of 31 octets.

These functions are also used for BR/EDR, where EIR data is 240 bytes.
If I remember correctly, it has already been discussed on this list
about dropping this length, and the decision was against dropping it,
otherwise we would send always 240 bytes containing mostly zeroes (for
LE case).

Also note these items from the AD section:

"If the Length field is set to zero, then the Data field has zero
octets. This shall
only occur to allow an early termination of the Advertising or Scan Response
data."

and:

"Only the significant part of the Advertising or Scan Response data needs to be
sent over the air."

So a device sending less than 31 bytes of AD is compliant as long as
the last AD length field is zero. The receiving controller is not
required to fill the remaining AD with zeroes before sending it to the
host.

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH v2 7/9] Bluetooth: Add 'eir_len' param to mgmt_device_found()

[Andre Guedes]

Hi Chen,

On Nov 28, 2011, at 8:08 AM, Anderson Lizardo wrote:

> Hi Chen,
> 
> On Sun, Nov 27, 2011 at 2:37 AM, Ganir, Chen <chen.ganir@ti.com> wrote:
>> Why do we really need this ? The GAP Spec clearly defines a fixed advertising size of 31 octets (Vol3, Part C, Section 11). Instead of reporting how much we got (may be other than 31 if the peer device does not conform to the spec as required), we should make sure that BlueZ will always report 31 octets, and make sure that the device found event always sends a buffer of 31 octets.
> 
> These functions are also used for BR/EDR, where EIR data is 240 bytes.
> If I remember correctly, it has already been discussed on this list
> about dropping this length, and the decision was against dropping it,
> otherwise we would send always 240 bytes containing mostly zeroes (for
> LE case).
> 
> Also note these items from the AD section:
> 
> "If the Length field is set to zero, then the Data field has zero
> octets. This shall
> only occur to allow an early termination of the Advertising or Scan Response
> data."
> 
> and:
> 
> "Only the significant part of the Advertising or Scan Response data needs to be
> sent over the air."
> 
> So a device sending less than 31 bytes of AD is compliant as long as
> the last AD length field is zero. The receiving controller is not
> required to fill the remaining AD with zeroes before sending it to the
> host.

I guess Lizardo already answered your question. Further info about this
issue please take a look at the early discussion we had here in the ML.

Subject: [PATCH v2 11/16] Bluetooth: Add 'eir_len' param to mgmt_device_found()

BR,

Andre

[PATCH v2 8/9] 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>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/hci_event.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 865fdf6..b09b5cc 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2925,6 +2925,7 @@ static inline void hci_le_adv_report_evt(struct hci_dev *hdev,
 {
 	u8 num_reports = skb->data[0];
 	void *ptr = &skb->data[1];
+	s8 rssi;
 
 	hci_dev_lock(hdev);
 
@@ -2933,6 +2934,10 @@ 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, &ev->bdaddr, LE_LINK, ev->bdaddr_type,
+					NULL, rssi, ev->data, ev->length);
+
 		ptr += sizeof(*ev) + ev->length + 1;
 	}
 
-- 
1.7.7.1

[PATCH v2 9/9] 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 |   25 ++++++++++++++++++++++---
 net/bluetooth/mgmt.c      |   31 ++++++++++++++++++++++++++++---
 2 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index b09b5cc..1a51381 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -964,9 +964,6 @@ 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;
@@ -974,17 +971,39 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 	if (cp->enable == 0x01) {
 		hci_req_complete(hdev, HCI_OP_LE_SET_SCAN_ENABLE, status);
 
+		if (status) {
+			hci_dev_lock(hdev);
+			mgmt_start_discovery_failed(hdev, status);
+			hci_dev_unlock(hdev);
+			return;
+		}
+
 		set_bit(HCI_LE_SCAN, &hdev->dev_flags);
 
 		del_timer(&hdev->adv_timer);
 
 		hci_dev_lock(hdev);
+
 		hci_adv_entries_clear(hdev);
+
+		mgmt_discovering(hdev, 1);
+
 		hci_dev_unlock(hdev);
 	} else if (cp->enable == 0x00) {
+		if (status) {
+			hci_dev_lock(hdev);
+			mgmt_stop_discovery_failed(hdev, status);
+			hci_dev_unlock(hdev);
+			return;
+		}
+
 		clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
 
 		mod_timer(&hdev->adv_timer, jiffies + ADV_CLEAR_TIMEOUT);
+
+		hci_dev_lock(hdev);
+		mgmt_discovering(hdev, 0);
+		hci_dev_unlock(hdev);
 	}
 }
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 6a74955..e7e224a 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -33,7 +33,16 @@
 #define MGMT_VERSION	0
 #define MGMT_REVISION	1
 
-#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
+/*
+ * 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 {
 	struct list_head list;
@@ -1824,6 +1833,7 @@ static int start_discovery(struct sock *sk, u16 index,
 						unsigned char *data, u16 len)
 {
 	struct mgmt_cp_start_discovery *cp = (void *) data;
+	unsigned long discov_type = cp->type;
 	struct pending_cmd *cmd;
 	struct hci_dev *hdev;
 	int err;
@@ -1853,7 +1863,16 @@ static int start_discovery(struct sock *sk, u16 index,
 		goto failed;
 	}
 
-	err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
+	if (test_bit(MGMT_ADDR_BREDR, &discov_type))
+		err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
+	else if (test_bit(MGMT_ADDR_LE_PUBLIC, &discov_type) &&
+				test_bit(MGMT_ADDR_LE_RANDOM, &discov_type))
+		err =  hci_do_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
+					LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
+	else
+		err = cmd_status(sk, index, MGMT_OP_START_DISCOVERY,
+						MGMT_STATUS_NOT_SUPPORTED);
+
 	if (err < 0)
 		mgmt_pending_remove(cmd);
 
@@ -1885,7 +1904,13 @@ 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 if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
+		err = hci_cancel_le_scan(hdev);
+	else
+		err = -EPERM;
+
 	if (err < 0)
 		mgmt_pending_remove(cmd);
 
-- 
1.7.7.1

RE: [PATCH v2 9/9] Bluetooth: Support LE-Only discovery procedure

[Ganir, Chen]

Andre, 

> -----Original Message-----
> From: linux-bluetooth-owner@vger.kernel.org [mailto:linux-bluetooth-
> owner@vger.kernel.org] On Behalf Of Andre Guedes
> Sent: Saturday, November 26, 2011 1:54 AM
> To: linux-bluetooth@vger.kernel.org
> Subject: [PATCH v2 9/9] Bluetooth: Support LE-Only discovery procedure
> 
> 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 |   25 ++++++++++++++++++++++---
>  net/bluetooth/mgmt.c      |   31 ++++++++++++++++++++++++++++---
>  2 files changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index b09b5cc..1a51381 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -964,9 +964,6 @@ 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;
> @@ -974,17 +971,39 @@ static void hci_cc_le_set_scan_enable(struct
> hci_dev *hdev,
>  	if (cp->enable == 0x01) {
>  		hci_req_complete(hdev, HCI_OP_LE_SET_SCAN_ENABLE, status);
> 
> +		if (status) {
> +			hci_dev_lock(hdev);
> +			mgmt_start_discovery_failed(hdev, status);
> +			hci_dev_unlock(hdev);
> +			return;
> +		}
> +
>  		set_bit(HCI_LE_SCAN, &hdev->dev_flags);
> 
>  		del_timer(&hdev->adv_timer);
> 
>  		hci_dev_lock(hdev);
> +
>  		hci_adv_entries_clear(hdev);
> +
> +		mgmt_discovering(hdev, 1);
> +
>  		hci_dev_unlock(hdev);
>  	} else if (cp->enable == 0x00) {
> +		if (status) {
> +			hci_dev_lock(hdev);
> +			mgmt_stop_discovery_failed(hdev, status);
> +			hci_dev_unlock(hdev);
> +			return;
> +		}
> +
>  		clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
> 
>  		mod_timer(&hdev->adv_timer, jiffies + ADV_CLEAR_TIMEOUT);
> +
> +		hci_dev_lock(hdev);
> +		mgmt_discovering(hdev, 0);
> +		hci_dev_unlock(hdev);
>  	}
>  }
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 6a74955..e7e224a 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -33,7 +33,16 @@
>  #define MGMT_VERSION	0
>  #define MGMT_REVISION	1
> 
> -#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
> +/*
> + * 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 {
>  	struct list_head list;
> @@ -1824,6 +1833,7 @@ static int start_discovery(struct sock *sk, u16
> index,
>  						unsigned char *data, u16 len)
>  {
>  	struct mgmt_cp_start_discovery *cp = (void *) data;
> +	unsigned long discov_type = cp->type;
>  	struct pending_cmd *cmd;
>  	struct hci_dev *hdev;
>  	int err;
> @@ -1853,7 +1863,16 @@ static int start_discovery(struct sock *sk, u16
> index,
>  		goto failed;
>  	}
> 
> -	err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
> +	if (test_bit(MGMT_ADDR_BREDR, &discov_type))
> +		err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
> +	else if (test_bit(MGMT_ADDR_LE_PUBLIC, &discov_type) &&
> +				test_bit(MGMT_ADDR_LE_RANDOM, &discov_type))
> +		err =  hci_do_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
> +					LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
> +	else
> +		err = cmd_status(sk, index, MGMT_OP_START_DISCOVERY,
> +						MGMT_STATUS_NOT_SUPPORTED);
> +
>  	if (err < 0)
>  		mgmt_pending_remove(cmd);
> 
> @@ -1885,7 +1904,13 @@ 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 if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
> +		err = hci_cancel_le_scan(hdev);
> +	else
> +		err = -EPERM;
> +
>  	if (err < 0)
>  		mgmt_pending_remove(cmd);
> 
> --
> 1.7.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Do we really need this kind of functionality ? The GAP spec defines how a single mode/dual mode devices should search for devices. In addition, the kernel has all the knowledge required to decide which of the GAP spec device discovery procedures to use, so why bother the bluetoothd with decisions of such kind ? Why create an option for mistake ? If a device is BR/EDR only, mgmt_start_discovery will start discovery of BR/EDR devices. If it is a single mode LE device, the mgmt_start_discovery will start a discovery for LE devices only. If the device is dual mode (BR/EDR/LE) then the mgmt_start_discovery command will do the interleaved scanning as the spec defines. Adding such complexity and allowing the selection of scan methods breaks the meaning of the spec. Why do we need to allow the user to scan for LE devices while in BR/EDR only mode, and return an error ? Why should the user even be aware of such an option ? I thought the whole idea behind the mgmtops (as opposite to hciops) was to encapsulate some logic into basic operations and procedures, and prevent the 1:1 reflection of hci commands. In this case, device discovery is opaque to the user - it will discover the devices as the spec defines, without any irrelevant errors and without too much understanding from the upper layers.

Thanks,
Chen Ganir.

Re: [PATCH v2 9/9] Bluetooth: Support LE-Only discovery procedure

[Andre Guedes]

Hi Chen,

On Nov 27, 2011, at 3:44 AM, Ganir, Chen wrote:

> Do we really need this kind of functionality ? The GAP spec defines how a single mode/dual mode devices should search for devices. In addition, the kernel has all the knowledge required to decide which of the GAP spec device discovery procedures to use, so why bother the bluetoothd with decisions of such kind ? Why create an option for mistake ? If a device is BR/EDR only, mgmt_start_discovery will start discovery of BR/EDR devices. If it is a single mode LE device, the mgmt_start_discovery will start a discovery for LE devices only. If the device is dual mode (BR/EDR/LE) then the mgmt_start_discovery command will do the interleaved scanning as the spec defines. Adding such complexity and allowing the selection of scan methods breaks the meaning of the spec. Why do we need to allow the user to scan for LE devices while in BR/EDR only mode, and return an error ? Why should the user even be aware of such an option ? I thought the whole idea behind the mgmtops (as opposite to hciops) was to encapsulate some logic into basic operations and procedures, and prevent the 1:1 reflection of hci commands. In this case, device discovery is opaque to the user - it will discover the devices as the spec defines, without any irrelevant errors and without too much understanding from the upper layers.

I'm not sure what were key reasons for this API change, but this is the
way Start Discovery API was designed. I guess Johan or Marcel can answer
your question about why the user would like to set the discovery procedure.

BR,

Andre

Re: [PATCH v2 9/9] Bluetooth: Support LE-Only discovery procedure

[Johan Hedberg]

Hi Chen,

> Do we really need this kind of functionality?

Firstly you should realize that this is not about functionality exposed
to the user or applications, but functionality exposed to bluetoothd.
The fact that we have something in the mgmt interface doesn't mean that
it'll automatically be exposed in the D-Bus interface or bluetoothd
internal APIs.

As for this particular case (allowing bluetoothd to restrict device
discovery to LE or BR/EDR) the reason is the concern that when doing
discovery for a specific profile which is only applicable for BR/EDR or
LE, you really don't want to waste the users time doing the "other"
discovery which will not yield any valuable results. Examples of this
could be discovering devices to send a file over Object Push (BR/EDR
only) or when running a application for a LE profile which utilizes
features only available though an LE radio.

I'm not completely sure the need for this will be strong enough for
exposing it in the D-Bus API, but not having it in the kernel interface
(mgmt) from the start makes it a lot harder to fix if we do end up
needing it later (as opposed to the D-Bus interface which would "only"
mean doing a new major BlueZ version).

Johan

RE: [PATCH v2 9/9] Bluetooth: Support LE-Only discovery procedure

[Ganir, Chen]

Johan, 

> -----Original Message-----
> From: Johan Hedberg [mailto:johan.hedberg@gmail.com]
> Sent: Tuesday, November 29, 2011 11:15 AM
> To: Ganir, Chen
> Cc: Andre Guedes; linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH v2 9/9] Bluetooth: Support LE-Only discovery
> procedure
> 
> Hi Chen,
> 
> > Do we really need this kind of functionality?
> 
> Firstly you should realize that this is not about functionality exposed
> to the user or applications, but functionality exposed to bluetoothd.
> The fact that we have something in the mgmt interface doesn't mean that
> it'll automatically be exposed in the D-Bus interface or bluetoothd
> internal APIs.
> 
> As for this particular case (allowing bluetoothd to restrict device
> discovery to LE or BR/EDR) the reason is the concern that when doing
> discovery for a specific profile which is only applicable for BR/EDR or
> LE, you really don't want to waste the users time doing the "other"
> discovery which will not yield any valuable results. Examples of this
> could be discovering devices to send a file over Object Push (BR/EDR
> only) or when running a application for a LE profile which utilizes
> features only available though an LE radio.
> 
> I'm not completely sure the need for this will be strong enough for
> exposing it in the D-Bus API, but not having it in the kernel interface
> (mgmt) from the start makes it a lot harder to fix if we do end up
> needing it later (as opposed to the D-Bus interface which would "only"
> mean doing a new major BlueZ version).
> 
> Johan

If this is the case, and we know that for now, we have no need for this, why introduce more complexity ? How will the current GAP device search work ? How will interleaved scanning work ? Will it be triggered from the bluetoothd ? Will it be handled by the kernel ?

Chen.

Re: [PATCH v2 9/9] Bluetooth: Support LE-Only discovery procedure

[Johan Hedberg]

Hi Chen,

On Wed, Nov 30, 2011, Ganir, Chen wrote:
> > Firstly you should realize that this is not about functionality exposed
> > to the user or applications, but functionality exposed to bluetoothd.
> > The fact that we have something in the mgmt interface doesn't mean that
> > it'll automatically be exposed in the D-Bus interface or bluetoothd
> > internal APIs.
> > 
> > As for this particular case (allowing bluetoothd to restrict device
> > discovery to LE or BR/EDR) the reason is the concern that when doing
> > discovery for a specific profile which is only applicable for BR/EDR or
> > LE, you really don't want to waste the users time doing the "other"
> > discovery which will not yield any valuable results. Examples of this
> > could be discovering devices to send a file over Object Push (BR/EDR
> > only) or when running a application for a LE profile which utilizes
> > features only available though an LE radio.
> > 
> > I'm not completely sure the need for this will be strong enough for
> > exposing it in the D-Bus API, but not having it in the kernel interface
> > (mgmt) from the start makes it a lot harder to fix if we do end up
> > needing it later (as opposed to the D-Bus interface which would "only"
> > mean doing a new major BlueZ version).
> > 
> > Johan
> 
> If this is the case, and we know that for now, we have no need for
> this, why introduce more complexity?

I really fail to see what you can consider complex about a single extra
parameter to start_discovery.

> How will the current GAP device search work?

Just like it has worked so far.

> How will interleaved scanning work?

Just like it would work without the extra parameter. It gets triggered
when user-space (bluetoothd) says it wants LE + BR/EDR discovery.

> Will it be triggered from the bluetoothd?

Yes, just like it would without the extra parameter.

> Will it be handled by the kernel ?

Yes, just like it would without the extra parameter.

Johan

RE: [PATCH v2 9/9] Bluetooth: Support LE-Only discovery procedure

[Ganir, Chen]

Johan, 

> -----Original Message-----
> From: Johan Hedberg [mailto:johan.hedberg@gmail.com]
> Sent: Wednesday, November 30, 2011 1:27 PM
> To: Ganir, Chen
> Cc: Andre Guedes; linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH v2 9/9] Bluetooth: Support LE-Only discovery
> procedure
> 
> Hi Chen,
> 
> On Wed, Nov 30, 2011, Ganir, Chen wrote:
> > > Firstly you should realize that this is not about functionality
> exposed
> > > to the user or applications, but functionality exposed to
> bluetoothd.
> > > The fact that we have something in the mgmt interface doesn't mean
> that
> > > it'll automatically be exposed in the D-Bus interface or bluetoothd
> > > internal APIs.
> > >
> > > As for this particular case (allowing bluetoothd to restrict device
> > > discovery to LE or BR/EDR) the reason is the concern that when
> doing
> > > discovery for a specific profile which is only applicable for
> BR/EDR or
> > > LE, you really don't want to waste the users time doing the "other"
> > > discovery which will not yield any valuable results. Examples of
> this
> > > could be discovering devices to send a file over Object Push
> (BR/EDR
> > > only) or when running a application for a LE profile which utilizes
> > > features only available though an LE radio.
> > >
> > > I'm not completely sure the need for this will be strong enough for
> > > exposing it in the D-Bus API, but not having it in the kernel
> interface
> > > (mgmt) from the start makes it a lot harder to fix if we do end up
> > > needing it later (as opposed to the D-Bus interface which would
> "only"
> > > mean doing a new major BlueZ version).
> > >
> > > Johan
> >
> > If this is the case, and we know that for now, we have no need for
> > this, why introduce more complexity?
> 
> I really fail to see what you can consider complex about a single extra
> parameter to start_discovery.
> 
> > How will the current GAP device search work?
> 
> Just like it has worked so far.
> 
> > How will interleaved scanning work?
> 
> Just like it would work without the extra parameter. It gets triggered
> when user-space (bluetoothd) says it wants LE + BR/EDR discovery.
> 
> > Will it be triggered from the bluetoothd?
> 
> Yes, just like it would without the extra parameter.
> 
> > Will it be handled by the kernel ?
> 
> Yes, just like it would without the extra parameter.
> 
> Johan

What I meant to ask was who is responsible for asking for dual mode or single mode scan now that the parameters were added ? Does the bluetoothd now needs to know if the controller and host support LE, so it can start a BR/EDR/LE scan ? or will the bluetoothd always ask for BR/EDR/LE scan, and the kernel will execute the supported search according to the LMP flags ?

Chen.

Re: [PATCH v2 9/9] Bluetooth: Support LE-Only discovery procedure

[Johan Hedberg]

Hi Chen,

On Wed, Nov 30, 2011, Ganir, Chen wrote:
> What I meant to ask was who is responsible for asking for dual mode or
> single mode scan now that the parameters were added? Does the
> bluetoothd now needs to know if the controller and host support LE, so
> it can start a BR/EDR/LE scan? or will the bluetoothd always ask for
> BR/EDR/LE scan, and the kernel will execute the supported search
> according to the LMP flags?

The latter (in fact that's what bluetoothd is already now doing; it's
just that the necessary bits on the kernel side aren't in place yet).

Johan