[PATCH 0/4] Bluetooth: Fix workqueue related issues

[PATCH 0/4] Bluetooth: Fix workqueue related issues

[Johan Hedberg]

Hi,

I was initially only going to fix the thing in the last patch of this
set but after starting to look at the code I found several other issues
and hence the rest of the patches.

The first three patches fix work queue handling. We should, whenever
possible, avoid the system-global workqueue (the schedule_*work
functions) and use hdev-specific ones instead. However, we can't use
hci_request() with the usual workqueue since that would block e.g.
hci_send_cmd from completing so a second work queue is needed.

The last patch fixes a race with setting the scan mode that I was
seeing every once in a while through a connectable or discoverable test
case of mgmt-tester failing. There are several other HCI commands with
the same potential issue (like the class of device and the local name)
but I haven't gotten around to fixing those yet.

Johan

[PATCH 1/4] Bluetooth: Add a new workqueue for hci_request operations

[Johan Hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

The hci_request function is blocking and cannot be called through the
usual per-HCI device workqueue (hdev->workqueue). While hci_request is
in progress any other work from que queue, including sending HCI
commands to the controller would be blocked and eventually cause the
hci_request call to time out.

This patch adds a second workqueue to be used by operations needing
hci_request and thereby avoiding issues with blocking other workqueue
users.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_core.c         |   11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 014a2ea..769a740 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -216,6 +216,7 @@ struct hci_dev {
 	unsigned long	le_last_tx;
 
 	struct workqueue_struct	*workqueue;
+	struct workqueue_struct	*req_workqueue;
 
 	struct work_struct	power_on;
 	struct delayed_work	power_off;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 596660d..f73907a 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1799,6 +1799,15 @@ int hci_register_dev(struct hci_dev *hdev)
 		goto err;
 	}
 
+	hdev->req_workqueue = alloc_workqueue(hdev->name,
+					      WQ_HIGHPRI | WQ_UNBOUND |
+					      WQ_MEM_RECLAIM, 1);
+	if (!hdev->req_workqueue) {
+		destroy_workqueue(hdev->workqueue);
+		error = -ENOMEM;
+		goto err;
+	}
+
 	error = hci_add_sysfs(hdev);
 	if (error < 0)
 		goto err_wqueue;
@@ -1827,6 +1836,7 @@ int hci_register_dev(struct hci_dev *hdev)
 
 err_wqueue:
 	destroy_workqueue(hdev->workqueue);
+	destroy_workqueue(hdev->req_workqueue);
 err:
 	ida_simple_remove(&hci_index_ida, hdev->id);
 	write_lock(&hci_dev_list_lock);
@@ -1880,6 +1890,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
 	hci_del_sysfs(hdev);
 
 	destroy_workqueue(hdev->workqueue);
+	destroy_workqueue(hdev->req_workqueue);
 
 	hci_dev_lock(hdev);
 	hci_blacklist_clear(hdev);
-- 
1.7.10.4

Re: [PATCH 1/4] Bluetooth: Add a new workqueue for hci_request operations

[Marcel Holtmann]

Hi Johan,

> The hci_request function is blocking and cannot be called through the
> usual per-HCI device workqueue (hdev->workqueue). While hci_request is
> in progress any other work from que queue, including sending HCI

no idea what a que queue is ;)

> commands to the controller would be blocked and eventually cause the
> hci_request call to time out.
> 
> This patch adds a second workqueue to be used by operations needing
> hci_request and thereby avoiding issues with blocking other workqueue
> users.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  include/net/bluetooth/hci_core.h |    1 +
>  net/bluetooth/hci_core.c         |   11 +++++++++++
>  2 files changed, 12 insertions(+)

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

Regards

Marcel

[PATCH 2/4] Bluetooth: Use req_workqueue for hci_request operations

[Johan Hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

This patch converts work assignment relying on hci_request() from the
system-global work queue to the per-HCI device specific work queue
(hdev->req_workqueue) intended for hci_request() related tasks.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_core.c |    5 +++--
 net/bluetooth/mgmt.c     |    4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index f73907a..545553b 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1146,7 +1146,8 @@ static void hci_power_on(struct work_struct *work)
 		return;
 
 	if (test_bit(HCI_AUTO_OFF, &hdev->dev_flags))
-		schedule_delayed_work(&hdev->power_off, HCI_AUTO_OFF_TIMEOUT);
+		queue_delayed_work(hdev->req_workqueue, &hdev->power_off,
+				   HCI_AUTO_OFF_TIMEOUT);
 
 	if (test_and_clear_bit(HCI_SETUP, &hdev->dev_flags))
 		mgmt_index_added(hdev);
@@ -1830,7 +1831,7 @@ int hci_register_dev(struct hci_dev *hdev)
 	hci_notify(hdev, HCI_DEV_REG);
 	hci_dev_hold(hdev);
 
-	schedule_work(&hdev->power_on);
+	queue_work(hdev->req_workqueue, &hdev->power_on);
 
 	return id;
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 37add53..54114ff 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -812,9 +812,9 @@ static int set_powered(struct sock *sk, struct hci_dev *hdev, void *data,
 	}
 
 	if (cp->val)
-		schedule_work(&hdev->power_on);
+		queue_work(hdev->req_workqueue, &hdev->power_on);
 	else
-		schedule_work(&hdev->power_off.work);
+		queue_work(hdev->req_workqueue, &hdev->power_off.work);
 
 	err = 0;
 
-- 
1.7.10.4

Re: [PATCH 2/4] Bluetooth: Use req_workqueue for hci_request operations

[Marcel Holtmann]

Hi Johan,

> This patch converts work assignment relying on hci_request() from the
> system-global work queue to the per-HCI device specific work queue
> (hdev->req_workqueue) intended for hci_request() related tasks.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/hci_core.c |    5 +++--
>  net/bluetooth/mgmt.c     |    4 ++--
>  2 files changed, 5 insertions(+), 4 deletions(-)

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

Regards

Marcel

[PATCH 3/4] Bluetooth: Fix using system-global workqueue when not necessary

[Johan Hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

There's a per-HCI device workqueue (hdev->workqueue) that should be used
for general per-HCI device work (except hdev->req_workqueue that's for
hci_request() related work). This patch fixes places using the
system-global work queue and makes them use the hdev->workqueue instead.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_core.c |    4 ++--
 net/bluetooth/mgmt.c     |    3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 545553b..e061b35 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1622,8 +1622,8 @@ static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
 	if (err < 0)
 		return err;
 
-	schedule_delayed_work(&hdev->le_scan_disable,
-			      msecs_to_jiffies(timeout));
+	queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
+			   msecs_to_jiffies(timeout));
 
 	return 0;
 }
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 54114ff..fc171f2 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1361,7 +1361,8 @@ static bool enable_service_cache(struct hci_dev *hdev)
 		return false;
 
 	if (!test_and_set_bit(HCI_SERVICE_CACHE, &hdev->dev_flags)) {
-		schedule_delayed_work(&hdev->service_cache, CACHE_TIMEOUT);
+		queue_delayed_work(hdev->workqueue, &hdev->service_cache,
+				   CACHE_TIMEOUT);
 		return true;
 	}
 
-- 
1.7.10.4

Re: [PATCH 3/4] Bluetooth: Fix using system-global workqueue when not necessary

[Marcel Holtmann]

Hi Johan,

> There's a per-HCI device workqueue (hdev->workqueue) that should be used
> for general per-HCI device work (except hdev->req_workqueue that's for
> hci_request() related work). This patch fixes places using the
> system-global work queue and makes them use the hdev->workqueue instead.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/hci_core.c |    4 ++--
>  net/bluetooth/mgmt.c     |    3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)

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

Regards

Marcel

Re: [PATCH 3/4] Bluetooth: Fix using system-global workqueue when not necessary

[Gustavo Padovan]

Hi Johan,

* Johan Hedberg <johan.hedberg@gmail.com> [2013-01-14 22:33:52 +0200]:

> From: Johan Hedberg <johan.hedberg@intel.com>
> 
> There's a per-HCI device workqueue (hdev->workqueue) that should be used
> for general per-HCI device work (except hdev->req_workqueue that's for
> hci_request() related work). This patch fixes places using the
> system-global work queue and makes them use the hdev->workqueue instead.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  net/bluetooth/hci_core.c |    4 ++--
>  net/bluetooth/mgmt.c     |    3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)

Patches 1 to 3 have been applied to bluetooth-next. Thanks.

	Gustavo

[PATCH 4/4] Bluetooth: Fix write_scan_enable when HCI_AUTO_OFF is set

[Johan Hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

When the HCI_AUTO_OFF flag is set the HCI device has been powered on
automatically by the kernel but through the mgmt interface it looks like
its powered off (and requires an explicit mgmt_set_powered to be
considered powered on). However, since the mgmt interface accepts
commands such as set_discoverable and set_connectable when powered off
there's no reason to avoid sending HCI commands in this state.

The hdev_is_powered() macro checks for both HCI_UP and HCI_AUTO_OFF and
is usable for mgmt commands that should fail (return NOT_POWERED) when
powered off, however for commands that should succeed testing only for
HCI_UP makes more sense.

Since the power_off delayed work is active when HCI_AUTO_OFF is set, and
can be triggered at any moment, it's also important to modify the expiry
time with mod_delayed_work() to ensure that the HCI commands are safely
completed.

The added benefit of doing these commands before an explicit
mgmt_set_powered call is that HCI commands sent in the mgmt_powered
function do not have the same kind of tracking as those triggered by a
directly related mgmt command and can therefore cause race conditions
(when e.g. set_discoverable is called quickly after calling
set_powered).

Since mgmt_powered() no longer takes care of the write_scan_enable we
also need to ensure that it's part of the hci_setup procedure and hence
the set_bredr_scan() function needs to be moved into hci_core.c from
mgmt.c.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_event.c |   17 +++++++++++++++++
 net/bluetooth/mgmt.c      |   28 ++++++++++------------------
 2 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 705078a..aaa18bd 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -573,6 +573,21 @@ static void hci_setup_event_mask(struct hci_dev *hdev)
 	}
 }
 
+static int set_bredr_scan(struct hci_dev *hdev)
+{
+	u8 scan = 0;
+
+	if (test_bit(HCI_CONNECTABLE, &hdev->dev_flags))
+		scan |= SCAN_PAGE;
+	if (test_bit(HCI_DISCOVERABLE, &hdev->dev_flags))
+		scan |= SCAN_INQUIRY;
+
+	if (!scan)
+		return 0;
+
+	return hci_send_cmd(hdev, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
+}
+
 static void bredr_setup(struct hci_dev *hdev)
 {
 	struct hci_cp_delete_stored_link_key cp;
@@ -602,6 +617,8 @@ static void bredr_setup(struct hci_dev *hdev)
 	bacpy(&cp.bdaddr, BDADDR_ANY);
 	cp.delete_all = 1;
 	hci_send_cmd(hdev, HCI_OP_DELETE_STORED_LINK_KEY, sizeof(cp), &cp);
+
+	set_bredr_scan(hdev);
 }
 
 static void le_setup(struct hci_dev *hdev)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index fc171f2..7cc2379 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -907,7 +907,7 @@ static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto failed;
 	}
 
-	if (!hdev_is_powered(hdev)) {
+	if (!test_bit(HCI_UP, &hdev->flags)) {
 		bool changed = false;
 
 		if (!!cp->val != test_bit(HCI_DISCOVERABLE, &hdev->dev_flags)) {
@@ -954,6 +954,10 @@ static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
 	else
 		cancel_delayed_work(&hdev->discov_off);
 
+	if (test_bit(HCI_AUTO_OFF, &hdev->dev_flags))
+		mod_delayed_work(hdev->req_workqueue, &hdev->power_off,
+				 HCI_AUTO_OFF_TIMEOUT);
+
 	err = hci_send_cmd(hdev, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
 	if (err < 0)
 		mgmt_pending_remove(cmd);
@@ -986,7 +990,7 @@ static int set_connectable(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	hci_dev_lock(hdev);
 
-	if (!hdev_is_powered(hdev)) {
+	if (!test_bit(HCI_UP, &hdev->flags)) {
 		bool changed = false;
 
 		if (!!cp->val != test_bit(HCI_CONNECTABLE, &hdev->dev_flags))
@@ -1027,6 +1031,10 @@ static int set_connectable(struct sock *sk, struct hci_dev *hdev, void *data,
 		goto failed;
 	}
 
+	if (test_bit(HCI_AUTO_OFF, &hdev->dev_flags))
+		mod_delayed_work(hdev->req_workqueue, &hdev->power_off,
+				 HCI_AUTO_OFF_TIMEOUT);
+
 	if (cp->val) {
 		scan = SCAN_PAGE;
 	} else {
@@ -2930,21 +2938,6 @@ static void settings_rsp(struct pending_cmd *cmd, void *data)
 	mgmt_pending_free(cmd);
 }
 
-static int set_bredr_scan(struct hci_dev *hdev)
-{
-	u8 scan = 0;
-
-	if (test_bit(HCI_CONNECTABLE, &hdev->dev_flags))
-		scan |= SCAN_PAGE;
-	if (test_bit(HCI_DISCOVERABLE, &hdev->dev_flags))
-		scan |= SCAN_INQUIRY;
-
-	if (!scan)
-		return 0;
-
-	return hci_send_cmd(hdev, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
-}
-
 int mgmt_powered(struct hci_dev *hdev, u8 powered)
 {
 	struct cmd_lookup match = { NULL, hdev };
@@ -2980,7 +2973,6 @@ int mgmt_powered(struct hci_dev *hdev, u8 powered)
 		}
 
 		if (lmp_bredr_capable(hdev)) {
-			set_bredr_scan(hdev);
 			update_class(hdev);
 			update_name(hdev, hdev->dev_name);
 			update_eir(hdev);
-- 
1.7.10.4

Re: [PATCH 4/4] Bluetooth: Fix write_scan_enable when HCI_AUTO_OFF is set

[Anderson Lizardo]

Hi Johan,

On Mon, Jan 14, 2013 at 4:33 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> [...]
> Since mgmt_powered() no longer takes care of the write_scan_enable we
> also need to ensure that it's part of the hci_setup procedure and hence
> the set_bredr_scan() function needs to be moved into hci_core.c from
> mgmt.c.

Small typo: hci_core.c -> hci_event.c

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

Re: [PATCH 0/4] Bluetooth: Fix workqueue related issues

[Mat Martineau]

Hi Johan -

On Mon, 14 Jan 2013, Johan Hedberg wrote:

> Hi,
>
> I was initially only going to fix the thing in the last patch of this
> set but after starting to look at the code I found several other issues
> and hence the rest of the patches.
>
> The first three patches fix work queue handling. We should, whenever
> possible, avoid the system-global workqueue (the schedule_*work
> functions) and use hdev-specific ones instead.

There are also multiple system workqueues beyond the default 
system_wq, like system_long_wq and system_unbound_wq.  Those may be a 
better fit and you wouldn't have to manage additional per-device 
workqueues.

> However, we can't use
> hci_request() with the usual workqueue since that would block e.g.
> hci_send_cmd from completing so a second work queue is needed.
>
> The last patch fixes a race with setting the scan mode that I was
> seeing every once in a while through a connectable or discoverable test
> case of mgmt-tester failing. There are several other HCI commands with
> the same potential issue (like the class of device and the local name)
> but I haven't gotten around to fixing those yet.

Regards,

--
Mat Martineau

Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation