[PATCH 0/7] MGMT Start Discovery command LE-Only Support

[PATCH 0/7] MGMT Start Discovery command LE-Only Support

[Andre Guedes]

Hi all,

I've split discovery patches series into smaller series so we keep easier
to review and apply these patches. This first patch series adds LE-Only
discovery support to MGMT Start Discovery command.

About the comments from previous series, hci_request/hci_req_complete
mechanism was replaced by a simpler one where we just wait for the commands
results to go ahead with LE scanning.

Coming patch series are:
   * Stop Discovery LE-Only support
   * Interleaved discovery support to MGMT Discovery commands

BR,

Andre

Andre Guedes (7):
  Bluetooth: Add 'eir_len' param to mgmt_device_found()
  Bluetooth: Report LE devices
  Bluetooth: LE scan should send MGMT Discovering events
  Bluetooth: Add helper functions to send LE scan commands
  Bluetooth: LE scan infra-structure
  Bluetooth: Add hci_do_le_scan() to hci_core
  Bluetooth: MGMT start discovery LE-Only support

 include/net/bluetooth/hci.h      |    1 +
 include/net/bluetooth/hci_core.h |   18 ++++++-
 net/bluetooth/hci_core.c         |  104 ++++++++++++++++++++++++++++++++++++++
 net/bluetooth/hci_event.c        |   37 +++++++++++---
 net/bluetooth/mgmt.c             |   31 ++++++++++--
 5 files changed, 179 insertions(+), 12 deletions(-)

-- 
1.7.8

[PATCH 1/7] 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 e34cd71..4db9a2f 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -947,7 +947,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 35cb56e..4ad7bf3 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1501,7 +1501,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);
@@ -2526,7 +2526,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);
@@ -2543,7 +2543,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);
 		}
 	}
 
@@ -2685,7 +2685,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 7a23f21..72c4b35 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2662,10 +2662,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);
@@ -2673,7 +2677,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.8

[PATCH 2/7] 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 4ad7bf3..3d45b05 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2992,6 +2992,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);
 
@@ -3000,6 +3001,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.8

[PATCH 3/7] Bluetooth: LE scan should send MGMT Discovering events

[Andre Guedes]

Send MGMT Discovering events once LE scan starts/stops so the
userspace can track when local adapters are discovering LE devices.

This way, we also keep the same behavior of inquiry which sends MGMT
Discovering events once inquiry starts/stops even if it is triggered
by an external tool (e.g. hcitool).

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

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 3d45b05..1f74b54 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1020,12 +1020,20 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 		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) {
 		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);
 	}
 }
 
-- 
1.7.8

[PATCH 4/7] 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>
Acked-by: Marcel Holtmann <marcel@holtmann.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 ce3727e..7923efc 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.8

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

[Andre Guedes]

This patch adds to hci_core an infra-structure to scan LE devices.

The LE scan is implemented using a work_struct which is enqueued
on hdev->workqueue. The LE scan work sends commands (Set LE Scan
Parameters and Set LE Scan Enable) to the controller and waits for
its results. If commands were executed successfully a 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 +
 include/net/bluetooth/hci_core.h |   13 ++++++++
 net/bluetooth/hci_core.c         |   60 ++++++++++++++++++++++++++++++++++++++
 net/bluetooth/hci_event.c        |   15 +++++++--
 4 files changed, 86 insertions(+), 3 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 67ad984..e419e1c 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -274,6 +274,7 @@ enum {
 
 /* ---- HCI Error Codes ---- */
 #define HCI_ERROR_AUTH_FAILURE		0x05
+#define HCI_ERROR_TIMEOUT		0x08
 #define HCI_ERROR_REJ_BAD_ADDR		0x0f
 #define HCI_ERROR_REMOTE_USER_TERM	0x13
 #define HCI_ERROR_LOCAL_HOST_TERM	0x16
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 4db9a2f..0ef3c7c 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;
@@ -254,6 +261,12 @@ struct hci_dev {
 
 	unsigned long		dev_flags;
 
+	struct work_struct	le_scan_wk;
+	struct le_scan_params	le_scan_params;
+	u8			le_scan_result;
+	wait_queue_head_t	le_scan_wait_q;
+	struct timer_list	le_scan_timer;
+
 	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 7923efc..56fcc59 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -620,6 +620,9 @@ static int hci_dev_do_close(struct hci_dev *hdev)
 	if (test_and_clear_bit(HCI_AUTO_OFF, &hdev->flags))
 		cancel_delayed_work(&hdev->power_off);
 
+	cancel_work_sync(&hdev->le_scan_wk);
+	del_timer_sync(&hdev->le_scan_timer);
+
 	hci_dev_lock_bh(hdev);
 	inquiry_cache_flush(hdev);
 	hci_conn_hash_flush(hdev);
@@ -1435,6 +1438,58 @@ int hci_add_adv_entry(struct hci_dev *hdev,
 	return 0;
 }
 
+static void le_scan_work(struct work_struct *work)
+{
+	struct hci_dev *hdev = container_of(work, struct hci_dev, le_scan_wk);
+	struct le_scan_params *params = &hdev->le_scan_params;
+	long timeout = msecs_to_jiffies(3000);
+	DECLARE_WAITQUEUE(wait, current);
+
+	BT_DBG("%s", hdev->name);
+
+	add_wait_queue(&hdev->le_scan_wait_q, &wait);
+
+	/* Send LE Set Scan Parameter command and wait for the result */
+	hdev->le_scan_result = HCI_ERROR_TIMEOUT;
+	send_le_scan_param_cmd(hdev, params->type, params->interval,
+							params->window);
+
+	schedule_timeout_uninterruptible(timeout);
+	if (hdev->le_scan_result)
+		goto failed;
+
+	/* Send LE Set Scan Enable command and wait for the result */
+	hdev->le_scan_result = HCI_ERROR_TIMEOUT;
+	send_le_scan_enable_cmd(hdev, 1);
+
+	schedule_timeout_uninterruptible(timeout);
+	if (hdev->le_scan_result)
+		goto failed;
+
+	remove_wait_queue(&hdev->le_scan_wait_q, &wait);
+
+	mod_timer(&hdev->le_scan_timer, jiffies +
+					msecs_to_jiffies(params->timeout));
+
+	return;
+
+failed:
+	remove_wait_queue(&hdev->le_scan_wait_q, &wait);
+
+	hci_dev_lock_bh(hdev);
+	mgmt_start_discovery_failed(hdev, hdev->le_scan_result);
+	hci_dev_unlock_bh(hdev);
+}
+
+static void le_scan_timeout(unsigned long arg)
+{
+	struct hci_dev *hdev = (void *) arg;
+
+	BT_DBG("%s", hdev->name);
+
+	send_le_scan_enable_cmd(hdev, 0);
+}
+
 /* Register HCI device */
 int hci_register_dev(struct hci_dev *hdev)
 {
@@ -1522,6 +1577,11 @@ int hci_register_dev(struct hci_dev *hdev)
 
 	atomic_set(&hdev->promisc, 0);
 
+	INIT_WORK(&hdev->le_scan_wk, le_scan_work);
+	init_waitqueue_head(&hdev->le_scan_wait_q);
+	setup_timer(&hdev->le_scan_timer, le_scan_timeout,
+							(unsigned long) hdev);
+
 	write_unlock_bh(&hci_dev_list_lock);
 
 	hdev->workqueue = create_singlethread_workqueue(hdev->name);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 1f74b54..232505b 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -997,6 +997,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);
+
+	hdev->le_scan_result = status;
+	wake_up(&hdev->le_scan_wait_q);
 }
 
 static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
@@ -1007,14 +1010,17 @@ 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) {
+		hdev->le_scan_result = status;
+		wake_up(&hdev->le_scan_wait_q);
+
+		if (status)
+			return;
+
 		set_bit(HCI_LE_SCAN, &hdev->dev_flags);
 
 		del_timer(&hdev->adv_timer);
@@ -1027,6 +1033,9 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 
 		hci_dev_unlock(hdev);
 	} else if (cp->enable == 0x00) {
+		if (status)
+			return;
+
 		clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
 
 		mod_timer(&hdev->adv_timer, jiffies + ADV_CLEAR_TIMEOUT);
-- 
1.7.8

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

[Marcel Holtmann]

Hi Andre,

> This patch adds to hci_core an infra-structure to scan LE devices.
> 
> The LE scan is implemented using a work_struct which is enqueued
> on hdev->workqueue. The LE scan work sends commands (Set LE Scan
> Parameters and Set LE Scan Enable) to the controller and waits for
> its results. If commands were executed successfully a timer is set
> to disable the ongoing scanning after some amount of time.

so I rather hold off on these until we get the tasklet removal patches
merged. The mgmt processing will then also be done in process context
and we can just sleep. This should make code like this a lot simpler.

Regards

Marcel

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

[Mat Martineau]

Marcel & Andre -

On Wed, 14 Dec 2011, Marcel Holtmann wrote:

> Hi Andre,
>
>> This patch adds to hci_core an infra-structure to scan LE devices.
>>
>> The LE scan is implemented using a work_struct which is enqueued
>> on hdev->workqueue. The LE scan work sends commands (Set LE Scan
>> Parameters and Set LE Scan Enable) to the controller and waits for
>> its results. If commands were executed successfully a timer is set
>> to disable the ongoing scanning after some amount of time.
>
> so I rather hold off on these until we get the tasklet removal patches
> merged. The mgmt processing will then also be done in process context
> and we can just sleep. This should make code like this a lot simpler.


While execution on a workqueue can sleep, it's not a good idea to 
block for a long time like this patch does.  A single-threaded 
workqueue (like the hdev workqueue) will not run the next scheduled 
work until the current work function returns.  If code executing in a 
workqueue suspends execution by using a wait queue, like 
le_scan_workqueue(), then all other pending work is blocked until 
le_scan_workqueue() returns.

It might be better to think of workqueues as having similar 
restrictions to tasklets, except you can use GFP_KERNEL when 
allocating and can block while acquiring locks.


In getting rid of tasklets, I think we also need to not use timers 
(which also execute in softirq context), and use the delayed_work 
calls instead.  That way we can assume all net/bluetooth code runs in 
process context, except for calls up from the driver layer.


--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

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

[Andre Guedes]

Hi Mat,

On Dec 15, 2011, at 4:25 PM, Mat Martineau wrote:

> 
> Marcel & Andre -
> 
> On Wed, 14 Dec 2011, Marcel Holtmann wrote:
> 
>> Hi Andre,
>> 
>>> This patch adds to hci_core an infra-structure to scan LE devices.
>>> 
>>> The LE scan is implemented using a work_struct which is enqueued
>>> on hdev->workqueue. The LE scan work sends commands (Set LE Scan
>>> Parameters and Set LE Scan Enable) to the controller and waits for
>>> its results. If commands were executed successfully a timer is set
>>> to disable the ongoing scanning after some amount of time.
>> 
>> so I rather hold off on these until we get the tasklet removal patches
>> merged. The mgmt processing will then also be done in process context
>> and we can just sleep. This should make code like this a lot simpler.
> 
> 
> While execution on a workqueue can sleep, it's not a good idea to block for a long time like this patch does.  A single-threaded workqueue (like the hdev workqueue) will not run the next scheduled work until the current work function returns.  If code executing in a workqueue suspends execution by using a wait queue, like le_scan_workqueue(), then all other pending work is blocked until le_scan_workqueue() returns.

Yes, I'm aware of it. This is why we have the schedule_timeout(). Most
of the time, we sleep for a short time since the controller sends the
command result event just after we send the command. If the controller
takes too long or simply doesn't send the command event we timeout
and abort the le scan work. This way we don't starve other works
enqueued on hdev->workqueue.

Do you mean "block for a long time" the time we wait for the command
result or the timeout?

BR,

Andre

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

[Mat Martineau]

On Thu, 15 Dec 2011, Andre Guedes wrote:

> Hi Mat,
>
> On Dec 15, 2011, at 4:25 PM, Mat Martineau wrote:
>
>>
>> Marcel & Andre -
>>
>> On Wed, 14 Dec 2011, Marcel Holtmann wrote:
>>
>>> Hi Andre,
>>>
>>>> This patch adds to hci_core an infra-structure to scan LE devices.
>>>>
>>>> The LE scan is implemented using a work_struct which is enqueued
>>>> on hdev->workqueue. The LE scan work sends commands (Set LE Scan
>>>> Parameters and Set LE Scan Enable) to the controller and waits for
>>>> its results. If commands were executed successfully a timer is set
>>>> to disable the ongoing scanning after some amount of time.
>>>
>>> so I rather hold off on these until we get the tasklet removal patches
>>> merged. The mgmt processing will then also be done in process context
>>> and we can just sleep. This should make code like this a lot simpler.
>>
>>
>> While execution on a workqueue can sleep, it's not a good idea to 
>> block for a long time like this patch does.  A single-threaded 
>> workqueue (like the hdev workqueue) will not run the next scheduled 
>> work until the current work function returns.  If code executing in 
>> a workqueue suspends execution by using a wait queue, like 
>> le_scan_workqueue(), then all other pending work is blocked until 
>> le_scan_workqueue() returns.
>
> Yes, I'm aware of it. This is why we have the schedule_timeout(). Most
> of the time, we sleep for a short time since the controller sends the
> command result event just after we send the command. If the controller
> takes too long or simply doesn't send the command event we timeout
> and abort the le scan work. This way we don't starve other works
> enqueued on hdev->workqueue.

I did some work to verify what I'm asserting about workqueue 
serialization in a single-threaded workqueue, and I still think I'm on 
the right track for our current use of workqueues.

My initial understanding was based on what I had read in Linux Device 
Drivers (3rd ed), which dates back to 2.6.10.  Workqueues have since 
been updated to use a thread pool (http://lwn.net/Articles/355700/), 
but our hdev workqueue is still serialized.  From that LWN article: 
"Code which requires that workqueue tasks be executed in the order in 
which they are submitted can use a WQ_UNBOUND workqueue with 
max_active set to one."  Look at process_one_work() in workqueue.c, 
the submitted work is run in the middle ("f(work);"), and the next 
work is not started until cwq_dec_nr_in_flight() is called at the end 
-- after f(work) returns.

So, using schedule_timeout() doesn't help, unless we switch away 
from the single threaded workqueue.  I'll reply to David's email on 
that point.

> Do you mean "block for a long time" the time we wait for the command
> result or the timeout?

Both.

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

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

[David Herrmann]

Hi Mat

On Thu, Dec 15, 2011 at 8:25 PM, Mat Martineau <mathewm@codeaurora.org> wro=
te:
>
> Marcel & Andre -
>
>
> On Wed, 14 Dec 2011, Marcel Holtmann wrote:
>
>> Hi Andre,
>>
>>> This patch adds to hci_core an infra-structure to scan LE devices.
>>>
>>> The LE scan is implemented using a work_struct which is enqueued
>>> on hdev->workqueue. The LE scan work sends commands (Set LE Scan
>>> Parameters and Set LE Scan Enable) to the controller and waits for
>>> its results. If commands were executed successfully a timer is set
>>> to disable the ongoing scanning after some amount of time.
>>
>>
>> so I rather hold off on these until we get the tasklet removal patches
>> merged. The mgmt processing will then also be done in process context
>> and we can just sleep. This should make code like this a lot simpler.
>
>
>
> While execution on a workqueue can sleep, it's not a good idea to block f=
or
> a long time like this patch does. =A0A single-threaded workqueue (like th=
e
> hdev workqueue) will not run the next scheduled work until the current wo=
rk
> function returns. =A0If code executing in a workqueue suspends execution =
by
> using a wait queue, like le_scan_workqueue(), then all other pending work=
 is
> blocked until le_scan_workqueue() returns.

Why do we use a single-threaded workqueue anyway? Why don't we switch
to a non-reentrant workqueue? Otherwise, we are just blocking the
whole hdev workqueue because we are too lazy to implement proper
locking between work-structs that depend on each other.

> It might be better to think of workqueues as having similar restrictions =
to
> tasklets, except you can use GFP_KERNEL when allocating and can block whi=
le
> acquiring locks.

That sounds like a lot of work with almost no benefit. If we start the
transition from tasklets to workqueues I think we should do it
properly so we do not require a single-threaded workqueue.

> In getting rid of tasklets, I think we also need to not use timers (which
> also execute in softirq context), and use the delayed_work calls instead.
> =A0That way we can assume all net/bluetooth code runs in process context,
> except for calls up from the driver layer.

Are there currently any pending patches? I've tried converting the
tasklets to workqueues myself but I always ended up with several
race-conditions. I haven't found a clean and easy way to fix them,
yet. And it's also a quite frustrating work.

Cheers
David

Does it make sense to have the hdev workqueue serialized?

[Mat Martineau]

On Thu, 15 Dec 2011, David Herrmann wrote:

> Hi Mat
>
> On Thu, Dec 15, 2011 at 8:25 PM, Mat Martineau <mathewm@codeaurora.org> wrote:
>>
>> Marcel & Andre -
>>
>>
>> On Wed, 14 Dec 2011, Marcel Holtmann wrote:
>>
>>> Hi Andre,
>>>
>>>> This patch adds to hci_core an infra-structure to scan LE devices.
>>>>
>>>> The LE scan is implemented using a work_struct which is enqueued
>>>> on hdev->workqueue. The LE scan work sends commands (Set LE Scan
>>>> Parameters and Set LE Scan Enable) to the controller and waits for
>>>> its results. If commands were executed successfully a timer is set
>>>> to disable the ongoing scanning after some amount of time.
>>>
>>>
>>> so I rather hold off on these until we get the tasklet removal patches
>>> merged. The mgmt processing will then also be done in process context
>>> and we can just sleep. This should make code like this a lot simpler.
>>
>>
>>
>> While execution on a workqueue can sleep, it's not a good idea to block for
>> a long time like this patch does.  A single-threaded workqueue (like the
>> hdev workqueue) will not run the next scheduled work until the current work
>> function returns.  If code executing in a workqueue suspends execution by
>> using a wait queue, like le_scan_workqueue(), then all other pending work is
>> blocked until le_scan_workqueue() returns.
>
> Why do we use a single-threaded workqueue anyway? Why don't we switch
> to a non-reentrant workqueue? Otherwise, we are just blocking the
> whole hdev workqueue because we are too lazy to implement proper
> locking between work-structs that depend on each other.

Before 2.6.36, creating a workqueue would create a dedicated thread 
per processor (or just one thread for a single threaded workqueue). 
I think I've seen Marcel comment that we didn't have enough work to 
justify the extra resources for multiple threads.

Since 2.6.36, there are dynamic thread pools for each processor that 
do not depend on the number of workqueues.  Threads are instead 
allocated based on the amount of concurrent work present in the 
system.  See http://lwn.net/Articles/403891/

>> It might be better to think of workqueues as having similar restrictions to
>> tasklets, except you can use GFP_KERNEL when allocating and can block while
>> acquiring locks.
>
> That sounds like a lot of work with almost no benefit. If we start the
> transition from tasklets to workqueues I think we should do it
> properly so we do not require a single-threaded workqueue.

The benefit would be in having no need to keep track of which context 
functions are executing in.  It's been a big headache with the ERTM 
and AMP changes, and there is a bunch of code that could work better 
in process context if it didn't have to also handle calls from 
tasklets.

That said, after learning more about how workqueues are implemented 
now, it may be worthwhile to change the "use one single-threaded 
workqueue for everything" assumption.  alloc_workqueue() has a 
max_active parameter, and it is possible to have many work items 
running concurrently.  Some of those threads could be suspended, like 
Andre does in his patch.  Workqueues created with the old 
create_workqueue() or create_singlethread_workqueue() have max_active 
== 1, which enforces serialization on each processor.


So there are two big questions:  Do we want to keep pushing everything 
on the hdev workqueue, since workqueues are not as heavyweight as they 
used to be?  And does it make sense to keep our workqueues serialized?


Advantages of serialization:
  * An HCI device is serialized by the transport anyway, so it makes 
sense to match that model.
  * Ordering is maintained.  The order of incoming HCI events may queue 
work in a particular order and need to assume the work will be 
executed in that order.
  * Simplicity.
  * No lock contention between multiple workers.

Advantages of not serializing:
  * Takes advantage of SMP
  * Workers can block without affecting the rest of the queue, enabling 
workers to be long-lived and use state on the thread stack instead of 
complicated lists of pending operations and dynamic allocation.
  * We need to have proper locking to deal with user processes anyway, 
so why not allow more concurrency internally.
  * Some work can proceed while waiting for locks in other workers.
  * Can use WQ_HIGHPRI to keep tx/rx data moving even when workers are 
waiting for locks


I think what's called for is a hybrid approach that serializes where 
necessary, but uses multiple workqueues.  How about this:

  * Use the serialized hdev workqueue for rx/tx only.  This could use 
WQ_HIGHPRI to help performance.
  * Have a serialized workqueue for each L2CAP channel to handle 
per-channel timeouts.
  * Have a global, non-serialized workqueue for stuff like sysfs and 
mgmt to use.


Does that sound workable?


>> In getting rid of tasklets, I think we also need to not use timers (which
>> also execute in softirq context), and use the delayed_work calls instead.
>>  That way we can assume all net/bluetooth code runs in process context,
>> except for calls up from the driver layer.
>
> Are there currently any pending patches? I've tried converting the
> tasklets to workqueues myself but I always ended up with several
> race-conditions. I haven't found a clean and easy way to fix them,
> yet. And it's also a quite frustrating work.

Gustavo's working on it, starting from Marcel's patch.  I think it's 
this one: http://www.spinics.net/lists/linux-bluetooth/msg06892.html



--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Changes to workqueues (was: Does it make sense to have the hdev workqueue serialized?)

[Mat Martineau]

Sorry - I forgot to update my subject line after writing the actual 
email and refining my proposal.

On Fri, 16 Dec 2011, Mat Martineau wrote:

> On Thu, 15 Dec 2011, David Herrmann wrote:
>
>> Hi Mat
>> 
>> On Thu, Dec 15, 2011 at 8:25 PM, Mat Martineau <mathewm@codeaurora.org> 
>> wrote:
>>> 
>>> Marcel & Andre -
>>> 
>>> 
>>> On Wed, 14 Dec 2011, Marcel Holtmann wrote:
>>> 
>>>> Hi Andre,
>>>> 
>>>>> This patch adds to hci_core an infra-structure to scan LE devices.
>>>>> 
>>>>> The LE scan is implemented using a work_struct which is enqueued
>>>>> on hdev->workqueue. The LE scan work sends commands (Set LE Scan
>>>>> Parameters and Set LE Scan Enable) to the controller and waits for
>>>>> its results. If commands were executed successfully a timer is set
>>>>> to disable the ongoing scanning after some amount of time.
>>>> 
>>>> 
>>>> so I rather hold off on these until we get the tasklet removal patches
>>>> merged. The mgmt processing will then also be done in process context
>>>> and we can just sleep. This should make code like this a lot simpler.
>>> 
>>> 
>>> 
>>> While execution on a workqueue can sleep, it's not a good idea to 
>>> block for a long time like this patch does.  A single-threaded 
>>> workqueue (like the hdev workqueue) will not run the next 
>>> scheduled work until the current work function returns.  If code 
>>> executing in a workqueue suspends execution by using a wait queue, 
>>> like le_scan_workqueue(), then all other pending work is blocked 
>>> until le_scan_workqueue() returns.
>> 
>> Why do we use a single-threaded workqueue anyway? Why don't we switch
>> to a non-reentrant workqueue? Otherwise, we are just blocking the
>> whole hdev workqueue because we are too lazy to implement proper
>> locking between work-structs that depend on each other.
>
> Before 2.6.36, creating a workqueue would create a dedicated thread per 
> processor (or just one thread for a single threaded workqueue). I think I've 
> seen Marcel comment that we didn't have enough work to justify the extra 
> resources for multiple threads.
>
> Since 2.6.36, there are dynamic thread pools for each processor that do not 
> depend on the number of workqueues.  Threads are instead allocated based on 
> the amount of concurrent work present in the system.  See 
> http://lwn.net/Articles/403891/
>
>>> It might be better to think of workqueues as having similar restrictions 
>>> to
>>> tasklets, except you can use GFP_KERNEL when allocating and can block 
>>> while
>>> acquiring locks.
>> 
>> That sounds like a lot of work with almost no benefit. If we start the
>> transition from tasklets to workqueues I think we should do it
>> properly so we do not require a single-threaded workqueue.
>
> The benefit would be in having no need to keep track of which context 
> functions are executing in.  It's been a big headache with the ERTM and AMP 
> changes, and there is a bunch of code that could work better in process 
> context if it didn't have to also handle calls from tasklets.
>
> That said, after learning more about how workqueues are implemented now, it 
> may be worthwhile to change the "use one single-threaded workqueue for 
> everything" assumption.  alloc_workqueue() has a max_active parameter, and it 
> is possible to have many work items running concurrently.  Some of those 
> threads could be suspended, like Andre does in his patch.  Workqueues created 
> with the old create_workqueue() or create_singlethread_workqueue() have 
> max_active == 1, which enforces serialization on each processor.
>
>
> So there are two big questions:  Do we want to keep pushing everything on the 
> hdev workqueue, since workqueues are not as heavyweight as they used to be? 
> And does it make sense to keep our workqueues serialized?
>
>
> Advantages of serialization:
> * An HCI device is serialized by the transport anyway, so it makes sense to 
> match that model.
> * Ordering is maintained.  The order of incoming HCI events may queue work 
> in a particular order and need to assume the work will be executed in that 
> order.
> * Simplicity.
> * No lock contention between multiple workers.
>
> Advantages of not serializing:
> * Takes advantage of SMP
> * Workers can block without affecting the rest of the queue, enabling 
> workers to be long-lived and use state on the thread stack instead of 
> complicated lists of pending operations and dynamic allocation.
> * We need to have proper locking to deal with user processes anyway, so why 
> not allow more concurrency internally.
> * Some work can proceed while waiting for locks in other workers.
> * Can use WQ_HIGHPRI to keep tx/rx data moving even when workers are waiting 
> for locks
>
>
> I think what's called for is a hybrid approach that serializes where 
> necessary, but uses multiple workqueues.  How about this:
>
> * Use the serialized hdev workqueue for rx/tx only.  This could use 
> WQ_HIGHPRI to help performance.
> * Have a serialized workqueue for each L2CAP channel to handle per-channel 
> timeouts.
> * Have a global, non-serialized workqueue for stuff like sysfs and mgmt to 
> use.
>
>
> Does that sound workable?
>
>
>>> In getting rid of tasklets, I think we also need to not use timers (which
>>> also execute in softirq context), and use the delayed_work calls instead.
>>>  That way we can assume all net/bluetooth code runs in process context,
>>> except for calls up from the driver layer.
>> 
>> Are there currently any pending patches? I've tried converting the
>> tasklets to workqueues myself but I always ended up with several
>> race-conditions. I haven't found a clean and easy way to fix them,
>> yet. And it's also a quite frustrating work.
>
> Gustavo's working on it, starting from Marcel's patch.  I think it's this 
> one: http://www.spinics.net/lists/linux-bluetooth/msg06892.html




--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: Does it make sense to have the hdev workqueue serialized?

[Gustavo Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2011-12-16 11:20:21 -0800]:

>=20
>=20
> On Thu, 15 Dec 2011, David Herrmann wrote:
>=20
> >Hi Mat
> >
> >On Thu, Dec 15, 2011 at 8:25 PM, Mat Martineau <mathewm@codeaurora.org> =
wrote:
> >>
> >>Marcel & Andre -
> >>
> >>
> >>On Wed, 14 Dec 2011, Marcel Holtmann wrote:
> >>
> >>>Hi Andre,
> >>>
> >>>>This patch adds to hci_core an infra-structure to scan LE devices.
> >>>>
> >>>>The LE scan is implemented using a work_struct which is enqueued
> >>>>on hdev->workqueue. The LE scan work sends commands (Set LE Scan
> >>>>Parameters and Set LE Scan Enable) to the controller and waits for
> >>>>its results. If commands were executed successfully a timer is set
> >>>>to disable the ongoing scanning after some amount of time.
> >>>
> >>>
> >>>so I rather hold off on these until we get the tasklet removal patches
> >>>merged. The mgmt processing will then also be done in process context
> >>>and we can just sleep. This should make code like this a lot simpler.
> >>
> >>
> >>
> >>While execution on a workqueue can sleep, it's not a good idea to block=
 for
> >>a long time like this patch does. =A0A single-threaded workqueue (like =
the
> >>hdev workqueue) will not run the next scheduled work until the current =
work
> >>function returns. =A0If code executing in a workqueue suspends executio=
n by
> >>using a wait queue, like le_scan_workqueue(), then all other pending wo=
rk is
> >>blocked until le_scan_workqueue() returns.
> >
> >Why do we use a single-threaded workqueue anyway? Why don't we switch
> >to a non-reentrant workqueue? Otherwise, we are just blocking the
> >whole hdev workqueue because we are too lazy to implement proper
> >locking between work-structs that depend on each other.
>=20
> Before 2.6.36, creating a workqueue would create a dedicated thread
> per processor (or just one thread for a single threaded workqueue).
> I think I've seen Marcel comment that we didn't have enough work to
> justify the extra resources for multiple threads.
>=20
> Since 2.6.36, there are dynamic thread pools for each processor that
> do not depend on the number of workqueues.  Threads are instead
> allocated based on the amount of concurrent work present in the
> system.  See http://lwn.net/Articles/403891/
>=20
> >>It might be better to think of workqueues as having similar restriction=
s to
> >>tasklets, except you can use GFP_KERNEL when allocating and can block w=
hile
> >>acquiring locks.
> >
> >That sounds like a lot of work with almost no benefit. If we start the
> >transition from tasklets to workqueues I think we should do it
> >properly so we do not require a single-threaded workqueue.
>=20
> The benefit would be in having no need to keep track of which
> context functions are executing in.  It's been a big headache with
> the ERTM and AMP changes, and there is a bunch of code that could
> work better in process context if it didn't have to also handle
> calls from tasklets.
>=20
> That said, after learning more about how workqueues are implemented
> now, it may be worthwhile to change the "use one single-threaded
> workqueue for everything" assumption.  alloc_workqueue() has a
> max_active parameter, and it is possible to have many work items
> running concurrently.  Some of those threads could be suspended,
> like Andre does in his patch.  Workqueues created with the old
> create_workqueue() or create_singlethread_workqueue() have
> max_active =3D=3D 1, which enforces serialization on each processor.

I didn't know alloc_workqueue() either, I think its a good idea go for it.

>=20
>=20
> So there are two big questions:  Do we want to keep pushing
> everything on the hdev workqueue, since workqueues are not as
> heavyweight as they used to be?  And does it make sense to keep our
> workqueues serialized?
>=20
>=20
> Advantages of serialization:
>  * An HCI device is serialized by the transport anyway, so it makes
> sense to match that model.
>  * Ordering is maintained.  The order of incoming HCI events may
> queue work in a particular order and need to assume the work will be
> executed in that order.
>  * Simplicity.
>  * No lock contention between multiple workers.
>=20
> Advantages of not serializing:
>  * Takes advantage of SMP
>  * Workers can block without affecting the rest of the queue,
> enabling workers to be long-lived and use state on the thread stack
> instead of complicated lists of pending operations and dynamic
> allocation.
>  * We need to have proper locking to deal with user processes
> anyway, so why not allow more concurrency internally.
>  * Some work can proceed while waiting for locks in other workers.
>  * Can use WQ_HIGHPRI to keep tx/rx data moving even when workers
> are waiting for locks
>=20
>=20
> I think what's called for is a hybrid approach that serializes where
> necessary, but uses multiple workqueues.  How about this:
>=20
>  * Use the serialized hdev workqueue for rx/tx only.  This could use
> WQ_HIGHPRI to help performance.

I agree with this one.

>  * Have a serialized workqueue for each L2CAP channel to handle
> per-channel timeouts.

Isn't it too much? maybe one workqueue per l2cap_conn is better.

>  * Have a global, non-serialized workqueue for stuff like sysfs and
> mgmt to use.

Some of the workqueue usage we have today might go away after the workqueue
change patches, we can check later if such workqueue will be really needed.

>=20
>=20
> Does that sound workable?
>=20
>=20
> >>In getting rid of tasklets, I think we also need to not use timers (whi=
ch
> >>also execute in softirq context), and use the delayed_work calls instea=
d.
> >>=A0That way we can assume all net/bluetooth code runs in process contex=
t,
> >>except for calls up from the driver layer.
> >
> >Are there currently any pending patches? I've tried converting the
> >tasklets to workqueues myself but I always ended up with several
> >race-conditions. I haven't found a clean and easy way to fix them,
> >yet. And it's also a quite frustrating work.
>=20
> Gustavo's working on it, starting from Marcel's patch.  I think it's
> this one: http://www.spinics.net/lists/linux-bluetooth/msg06892.html

As I said on the other e-mail, code is here:

http://git.kernel.org/?p=3Dlinux/kernel/git/padovan/bluetooth-testing.git

Comments are welcome!

	Gustavo

Re: Does it make sense to have the hdev workqueue serialized?

[Mat Martineau]

On Fri, 16 Dec 2011, Gustavo Padovan wrote:

> Hi Mat,
>
> * Mat Martineau <mathewm@codeaurora.org> [2011-12-16 11:20:21 -0800]:
>
>>
>>
>> On Thu, 15 Dec 2011, David Herrmann wrote:
>>
>>> Hi Mat
>>>
>>> On Thu, Dec 15, 2011 at 8:25 PM, Mat Martineau <mathewm@codeaurora.org> wrote:
>>>>
>>>> Marcel & Andre -
>>>>
>>>>
>>>> On Wed, 14 Dec 2011, Marcel Holtmann wrote:
>>>>
>>>>> Hi Andre,
>>>>>
>>>>>> This patch adds to hci_core an infra-structure to scan LE devices.
>>>>>>
>>>>>> The LE scan is implemented using a work_struct which is enqueued
>>>>>> on hdev->workqueue. The LE scan work sends commands (Set LE Scan
>>>>>> Parameters and Set LE Scan Enable) to the controller and waits for
>>>>>> its results. If commands were executed successfully a timer is set
>>>>>> to disable the ongoing scanning after some amount of time.
>>>>>
>>>>>
>>>>> so I rather hold off on these until we get the tasklet removal patches
>>>>> merged. The mgmt processing will then also be done in process context
>>>>> and we can just sleep. This should make code like this a lot simpler.
>>>>
>>>>
>>>>
>>>> While execution on a workqueue can sleep, it's not a good idea to block for
>>>> a long time like this patch does.  A single-threaded workqueue (like the
>>>> hdev workqueue) will not run the next scheduled work until the current work
>>>> function returns.  If code executing in a workqueue suspends execution by
>>>> using a wait queue, like le_scan_workqueue(), then all other pending work is
>>>> blocked until le_scan_workqueue() returns.
>>>
>>> Why do we use a single-threaded workqueue anyway? Why don't we switch
>>> to a non-reentrant workqueue? Otherwise, we are just blocking the
>>> whole hdev workqueue because we are too lazy to implement proper
>>> locking between work-structs that depend on each other.
>>
>> Before 2.6.36, creating a workqueue would create a dedicated thread
>> per processor (or just one thread for a single threaded workqueue).
>> I think I've seen Marcel comment that we didn't have enough work to
>> justify the extra resources for multiple threads.
>>
>> Since 2.6.36, there are dynamic thread pools for each processor that
>> do not depend on the number of workqueues.  Threads are instead
>> allocated based on the amount of concurrent work present in the
>> system.  See http://lwn.net/Articles/403891/
>>
>>>> It might be better to think of workqueues as having similar restrictions to
>>>> tasklets, except you can use GFP_KERNEL when allocating and can block while
>>>> acquiring locks.
>>>
>>> That sounds like a lot of work with almost no benefit. If we start the
>>> transition from tasklets to workqueues I think we should do it
>>> properly so we do not require a single-threaded workqueue.
>>
>> The benefit would be in having no need to keep track of which
>> context functions are executing in.  It's been a big headache with
>> the ERTM and AMP changes, and there is a bunch of code that could
>> work better in process context if it didn't have to also handle
>> calls from tasklets.
>>
>> That said, after learning more about how workqueues are implemented
>> now, it may be worthwhile to change the "use one single-threaded
>> workqueue for everything" assumption.  alloc_workqueue() has a
>> max_active parameter, and it is possible to have many work items
>> running concurrently.  Some of those threads could be suspended,
>> like Andre does in his patch.  Workqueues created with the old
>> create_workqueue() or create_singlethread_workqueue() have
>> max_active == 1, which enforces serialization on each processor.
>
> I didn't know alloc_workqueue() either, I think its a good idea go for it.
>
>>
>>
>> So there are two big questions:  Do we want to keep pushing
>> everything on the hdev workqueue, since workqueues are not as
>> heavyweight as they used to be?  And does it make sense to keep our
>> workqueues serialized?
>>
>>
>> Advantages of serialization:
>>  * An HCI device is serialized by the transport anyway, so it makes
>> sense to match that model.
>>  * Ordering is maintained.  The order of incoming HCI events may
>> queue work in a particular order and need to assume the work will be
>> executed in that order.
>>  * Simplicity.
>>  * No lock contention between multiple workers.
>>
>> Advantages of not serializing:
>>  * Takes advantage of SMP
>>  * Workers can block without affecting the rest of the queue,
>> enabling workers to be long-lived and use state on the thread stack
>> instead of complicated lists of pending operations and dynamic
>> allocation.
>>  * We need to have proper locking to deal with user processes
>> anyway, so why not allow more concurrency internally.
>>  * Some work can proceed while waiting for locks in other workers.
>>  * Can use WQ_HIGHPRI to keep tx/rx data moving even when workers
>> are waiting for locks
>>
>>
>> I think what's called for is a hybrid approach that serializes where
>> necessary, but uses multiple workqueues.  How about this:
>>
>>  * Use the serialized hdev workqueue for rx/tx only.  This could use
>> WQ_HIGHPRI to help performance.
>
> I agree with this one.
>
>>  * Have a serialized workqueue for each L2CAP channel to handle
>> per-channel timeouts.
>
> Isn't it too much? maybe one workqueue per l2cap_conn is better.

l2cap_conn does make more sense.

>>  * Have a global, non-serialized workqueue for stuff like sysfs and
>> mgmt to use.
>
> Some of the workqueue usage we have today might go away after the workqueue
> change patches, we can check later if such workqueue will be really needed.

I was thinking it would be useful for jobs like Andre has (a 
longer-running thread that blocks at different stages), but one of the 
system workqueues (like system_long_wq) would work fine for that.  No 
need for a special Bluetooth queue.

A long-running thread like this could even be useful for the AMP 
manager work that's going on - AMP manager state could be built up in 
a thread instead of building an async state machine.

>
>>
>>
>> Does that sound workable?
>>
>>
>>>> In getting rid of tasklets, I think we also need to not use timers (which
>>>> also execute in softirq context), and use the delayed_work calls instead.
>>>>  That way we can assume all net/bluetooth code runs in process context,
>>>> except for calls up from the driver layer.
>>>
>>> Are there currently any pending patches? I've tried converting the
>>> tasklets to workqueues myself but I always ended up with several
>>> race-conditions. I haven't found a clean and easy way to fix them,
>>> yet. And it's also a quite frustrating work.
>>
>> Gustavo's working on it, starting from Marcel's patch.  I think it's
>> this one: http://www.spinics.net/lists/linux-bluetooth/msg06892.html
>
> As I said on the other e-mail, code is here:
>
> http://git.kernel.org/?p=linux/kernel/git/padovan/bluetooth-testing.git
>
> Comments are welcome!

Thanks for the pointer to bluetooth-testing - I hadn't checked on it 
in a while!  I will look over your changes.


--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: Does it make sense to have the hdev workqueue serialized?

[Marcel Holtmann]

Hi Mat,

> The benefit would be in having no need to keep track of which context 
> functions are executing in.  It's been a big headache with the ERTM 
> and AMP changes, and there is a bunch of code that could work better 
> in process context if it didn't have to also handle calls from 
> tasklets.
> 
> That said, after learning more about how workqueues are implemented 
> now, it may be worthwhile to change the "use one single-threaded 
> workqueue for everything" assumption.  alloc_workqueue() has a 
> max_active parameter, and it is possible to have many work items 
> running concurrently.  Some of those threads could be suspended, like 
> Andre does in his patch.  Workqueues created with the old 
> create_workqueue() or create_singlethread_workqueue() have max_active 
> == 1, which enforces serialization on each processor.
> 
> 
> So there are two big questions:  Do we want to keep pushing everything 
> on the hdev workqueue, since workqueues are not as heavyweight as they 
> used to be?  And does it make sense to keep our workqueues serialized?
> 
> 
> Advantages of serialization:
>   * An HCI device is serialized by the transport anyway, so it makes 
> sense to match that model.
>   * Ordering is maintained.  The order of incoming HCI events may queue 
> work in a particular order and need to assume the work will be 
> executed in that order.
>   * Simplicity.
>   * No lock contention between multiple workers.
> 
> Advantages of not serializing:
>   * Takes advantage of SMP
>   * Workers can block without affecting the rest of the queue, enabling 
> workers to be long-lived and use state on the thread stack instead of 
> complicated lists of pending operations and dynamic allocation.
>   * We need to have proper locking to deal with user processes anyway, 
> so why not allow more concurrency internally.
>   * Some work can proceed while waiting for locks in other workers.
>   * Can use WQ_HIGHPRI to keep tx/rx data moving even when workers are 
> waiting for locks
> 
> 
> I think what's called for is a hybrid approach that serializes where 
> necessary, but uses multiple workqueues.  How about this:
> 
>   * Use the serialized hdev workqueue for rx/tx only.  This could use 
> WQ_HIGHPRI to help performance.

I think this is what we have to do to ensure that our event, cmd and ACL
and also SCO processing is not affected by anything else.

>   * Have a serialized workqueue for each L2CAP channel to handle 
> per-channel timeouts.

Why per channel? Wouldn't be one per ACL connection by enough?

>   * Have a global, non-serialized workqueue for stuff like sysfs and 
> mgmt to use.

And yes, one global workqueue for random tasks we have to do seems like
a good idea as well.

Regards

Marcel

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

[Gustavo Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2011-12-15 11:25:49 -0800]:

> 
> Marcel & Andre -
> 
> On Wed, 14 Dec 2011, Marcel Holtmann wrote:
> 
> >Hi Andre,
> >
> >>This patch adds to hci_core an infra-structure to scan LE devices.
> >>
> >>The LE scan is implemented using a work_struct which is enqueued
> >>on hdev->workqueue. The LE scan work sends commands (Set LE Scan
> >>Parameters and Set LE Scan Enable) to the controller and waits for
> >>its results. If commands were executed successfully a timer is set
> >>to disable the ongoing scanning after some amount of time.
> >
> >so I rather hold off on these until we get the tasklet removal patches
> >merged. The mgmt processing will then also be done in process context
> >and we can just sleep. This should make code like this a lot simpler.
> 
> 
> While execution on a workqueue can sleep, it's not a good idea to
> block for a long time like this patch does.  A single-threaded
> workqueue (like the hdev workqueue) will not run the next scheduled
> work until the current work function returns.  If code executing in
> a workqueue suspends execution by using a wait queue, like
> le_scan_workqueue(), then all other pending work is blocked until
> le_scan_workqueue() returns.

Yes, we know this issue, I'll be looking to this issue quite soon.
Btw, having a single-threaded is not the real problem if we use
create_workqueue() instead but in system with only one CPU we will have the
same problem.

> 
> It might be better to think of workqueues as having similar
> restrictions to tasklets, except you can use GFP_KERNEL when
> allocating and can block while acquiring locks.
> 
> 
> In getting rid of tasklets, I think we also need to not use timers
> (which also execute in softirq context), and use the delayed_work
> calls instead.  That way we can assume all net/bluetooth code runs
> in process context, except for calls up from the driver layer.

I'm taking care of that as well, timers are a problem if we run the rest of
the code in process context.

If you wanna take a look to the work I've been doing I pushing it to this
tree:
http://git.kernel.org/?p=linux/kernel/git/padovan/bluetooth-testing.git

This is in an unstable state, I still see some locking issues like
deadlocks in the code, but I plan to move fast and merge this into
bluetooth-next soon.

Comments are welcome!

	Gustavo

[PATCH 6/7] Bluetooth: Add hci_do_le_scan() to hci_core

[Andre Guedes]

This patch adds to hci_core the hci_do_le_scan function which should
be used to scan LE devices.

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 0ef3c7c..eab90b8 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1008,5 +1008,7 @@ 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);
 
 #endif /* __HCI_CORE_H */
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 56fcc59..9b659af 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2688,5 +2688,26 @@ 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;
+
+	BT_DBG("%s", hdev->name);
+
+	if (work_busy(&hdev->le_scan_wk) ||
+				test_bit(HCI_LE_SCAN, &hdev->dev_flags))
+		return -EINPROGRESS;
+
+	params->type = type;
+	params->interval = interval;
+	params->window = window;
+	params->timeout = timeout;
+
+	queue_work(hdev->workqueue, &hdev->le_scan_wk);
+
+	return 0;
+}
+
 module_param(enable_hs, bool, 0644);
 MODULE_PARM_DESC(enable_hs, "Enable High Speed");
-- 
1.7.8

[PATCH 7/7] Bluetooth: MGMT start discovery LE-Only support

[Andre Guedes]

This patch adds LE-Only discovery procedure support to MGMT Start
Discovery command.

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

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 72c4b35..ae114f6 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;
@@ -1863,6 +1872,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;
@@ -1892,7 +1902,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);
 
-- 
1.7.8