[PATCH 1/3] Bluetooth: hci_vhci: Fix calling hci_{suspend,resume}_dev

[PATCH 1/3] Bluetooth: hci_vhci: Fix calling hci_{suspend,resume}_dev

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Defer calls to hci_{suspend,resume}_dev to work so it doesn't block the
processing of the events.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 drivers/bluetooth/hci_vhci.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
index 5fd91106e853..56c6b22be10b 100644
--- a/drivers/bluetooth/hci_vhci.c
+++ b/drivers/bluetooth/hci_vhci.c
@@ -38,6 +38,7 @@ struct vhci_data {
 
 	struct mutex open_mutex;
 	struct delayed_work open_timeout;
+	struct work_struct suspend_work;
 
 	bool suspended;
 	bool wakeup;
@@ -114,6 +115,17 @@ static ssize_t force_suspend_read(struct file *file, char __user *user_buf,
 	return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
 }
 
+static void vhci_suspend_work(struct work_struct *work)
+{
+	struct vhci_data *data = container_of(work, struct vhci_data,
+					      suspend_work);
+
+	if (data->suspended)
+		hci_suspend_dev(data->hdev);
+	else
+		hci_resume_dev(data->hdev);
+}
+
 static ssize_t force_suspend_write(struct file *file,
 				   const char __user *user_buf,
 				   size_t count, loff_t *ppos)
@@ -129,16 +141,10 @@ static ssize_t force_suspend_write(struct file *file,
 	if (data->suspended == enable)
 		return -EALREADY;
 
-	if (enable)
-		err = hci_suspend_dev(data->hdev);
-	else
-		err = hci_resume_dev(data->hdev);
-
-	if (err)
-		return err;
-
 	data->suspended = enable;
 
+	schedule_work(&data->suspend_work);
+
 	return count;
 }
 
@@ -442,6 +448,7 @@ static int vhci_open(struct inode *inode, struct file *file)
 
 	mutex_init(&data->open_mutex);
 	INIT_DELAYED_WORK(&data->open_timeout, vhci_open_timeout);
+	INIT_WORK(&data->suspend_work, vhci_suspend_work);
 
 	file->private_data = data;
 	nonseekable_open(inode, file);
@@ -457,6 +464,7 @@ static int vhci_release(struct inode *inode, struct file *file)
 	struct hci_dev *hdev;
 
 	cancel_delayed_work_sync(&data->open_timeout);
+	flush_work(&data->suspend_work);
 
 	hdev = data->hdev;
 
-- 
2.31.1

[PATCH 2/3] Bluetooth: Fix handling of SUSPEND_DISCONNECTING

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

When SUSPEND_DISCONNECTING bit is set that means Disconnect is pending
but the code was evaluating if the list is empty before calling
hci_conn_del which does the actual cleanup and remove the connection
from the list thus the bit is never cleared causing the suspend
procedure to always timeout when there are connections to be
disconnected:

Suspend/Resume - Success 5 (Pairing - Legacy) - waiting done
  Set the system into Suspend via force_suspend
= mgmt-tester: Suspend/Resume - Success 5 (Pairing -..   17:03:13.200458
= mgmt-tester: Set the system into Suspend via force_suspend    17:03:13.205812
< HCI Command: Write Scan E.. (0x03|0x001a) plen 1  #122 [hci0] 17:03:13.213561
        Scan enable: No Scans (0x00)
> HCI Event: Command Complete (0x0e) plen 4         #123 [hci0] 17:03:13.214710
      Write Scan Enable (0x03|0x001a) ncmd 1
        Status: Success (0x00)
< HCI Command: Disconnect (0x01|0x0006) plen 3      #124 [hci0] 17:03:13.215830
        Handle: 42
        Reason: Remote Device Terminated due to Power Off (0x15)
> HCI Event: Command Status (0x0f) plen 4           #125 [hci0] 17:03:13.216602
      Disconnect (0x01|0x0006) ncmd 1
        Status: Success (0x00)
> HCI Event: Disconnect Complete (0x05) plen 4      #126 [hci0] 17:03:13.217342
        Status: Success (0x00)
        Handle: 42
        Reason: Remote Device Terminated due to Power Off (0x15)
@ MGMT Event: Device Disconn.. (0x000c) plen 8  {0x0002} [hci0] 17:03:13.217688
        BR/EDR Address: 00:AA:01:01:00:00 (Intel Corporation)
        Reason: Connection terminated by local host for suspend (0x05)
@ MGMT Event: Device Disconn.. (0x000c) plen 8  {0x0001} [hci0] 17:03:13.217688
        BR/EDR Address: 00:AA:01:01:00:00 (Intel Corporation)
        Reason: Connection terminated by local host for suspend (0x05)
Suspend/Resume - Success 5 (Pairing - Legacy) - test timed out
= mgmt-tester: Suspend/Resume - Success 5 (Pairing -..   17:03:13.939317
Suspend/Resume - Success 5 (Pairing - Legacy) - teardown
= mgmt-tester: Suspend/Resume - Success 5 (Pairing -..   17:03:13.947267
[   13.284291] Bluetooth: hci0: Timed out waiting for suspend events
[   13.287324] Bluetooth: hci0: Suspend timeout bit: 6

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/hci_event.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 7d0db1ca1248..3cba2bbefcd6 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2987,14 +2987,6 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	hci_disconn_cfm(conn, ev->reason);
 
-	/* The suspend notifier is waiting for all devices to disconnect so
-	 * clear the bit from pending tasks and inform the wait queue.
-	 */
-	if (list_empty(&hdev->conn_hash.list) &&
-	    test_and_clear_bit(SUSPEND_DISCONNECTING, hdev->suspend_tasks)) {
-		wake_up(&hdev->suspend_wait_q);
-	}
-
 	/* Re-enable advertising if necessary, since it might
 	 * have been disabled by the connection. From the
 	 * HCI_LE_Set_Advertise_Enable command description in
@@ -3012,6 +3004,14 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	hci_conn_del(conn);
 
+	/* The suspend notifier is waiting for all devices to disconnect so
+	 * clear the bit from pending tasks and inform the wait queue.
+	 */
+	if (list_empty(&hdev->conn_hash.list) &&
+	    test_and_clear_bit(SUSPEND_DISCONNECTING, hdev->suspend_tasks)) {
+		wake_up(&hdev->suspend_wait_q);
+	}
+
 unlock:
 	hci_dev_unlock(hdev);
 }
-- 
2.31.1

[PATCH 3/3] Bluetooth: Fix wake up suspend_wait_q prematurely

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

suspend_req_complete shall only attempt to wake up if there no tasks
left otherwise the WAKE_COND will evaluate to false causing a premature
timeout when in fact the tasks are still in progress.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/hci_request.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 92611bfc0b9e..209f4fe17237 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -1092,17 +1092,24 @@ static void suspend_req_complete(struct hci_dev *hdev, u8 status, u16 opcode)
 {
 	bt_dev_dbg(hdev, "Request complete opcode=0x%x, status=0x%x", opcode,
 		   status);
-	if (test_bit(SUSPEND_SCAN_ENABLE, hdev->suspend_tasks) ||
-	    test_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks)) {
-		clear_bit(SUSPEND_SCAN_ENABLE, hdev->suspend_tasks);
-		clear_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks);
+
+	if (status) {
 		wake_up(&hdev->suspend_wait_q);
+		return;
 	}
 
-	if (test_bit(SUSPEND_SET_ADV_FILTER, hdev->suspend_tasks)) {
-		clear_bit(SUSPEND_SET_ADV_FILTER, hdev->suspend_tasks);
+	/* Clear all tasks that could be initiated using suspend_req_complete as
+	 * callback.
+	 */
+	clear_bit(SUSPEND_PAUSE_DISCOVERY, hdev->suspend_tasks);
+	clear_bit(SUSPEND_PAUSE_ADVERTISING, hdev->suspend_tasks);
+	clear_bit(SUSPEND_SCAN_ENABLE, hdev->suspend_tasks);
+	clear_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks);
+	clear_bit(SUSPEND_SET_ADV_FILTER, hdev->suspend_tasks);
+
+	/* Wake up only if there are no tasks left */
+	if (!hdev->suspend_tasks)
 		wake_up(&hdev->suspend_wait_q);
-	}
 }
 
 static void hci_req_prepare_adv_monitor_suspend(struct hci_request *req,
-- 
2.31.1

Re: [PATCH 2/3] Bluetooth: Fix handling of SUSPEND_DISCONNECTING

[Marcel Holtmann]

Hi Luiz,

> When SUSPEND_DISCONNECTING bit is set that means Disconnect is pending
> but the code was evaluating if the list is empty before calling
> hci_conn_del which does the actual cleanup and remove the connection
> from the list thus the bit is never cleared causing the suspend
> procedure to always timeout when there are connections to be
> disconnected:
> 
> Suspend/Resume - Success 5 (Pairing - Legacy) - waiting done
>  Set the system into Suspend via force_suspend
> = mgmt-tester: Suspend/Resume - Success 5 (Pairing -..   17:03:13.200458
> = mgmt-tester: Set the system into Suspend via force_suspend    17:03:13.205812
> < HCI Command: Write Scan E.. (0x03|0x001a) plen 1  #122 [hci0] 17:03:13.213561
>        Scan enable: No Scans (0x00)
>> HCI Event: Command Complete (0x0e) plen 4         #123 [hci0] 17:03:13.214710
>      Write Scan Enable (0x03|0x001a) ncmd 1
>        Status: Success (0x00)
> < HCI Command: Disconnect (0x01|0x0006) plen 3      #124 [hci0] 17:03:13.215830
>        Handle: 42
>        Reason: Remote Device Terminated due to Power Off (0x15)
>> HCI Event: Command Status (0x0f) plen 4           #125 [hci0] 17:03:13.216602
>      Disconnect (0x01|0x0006) ncmd 1
>        Status: Success (0x00)
>> HCI Event: Disconnect Complete (0x05) plen 4      #126 [hci0] 17:03:13.217342
>        Status: Success (0x00)
>        Handle: 42
>        Reason: Remote Device Terminated due to Power Off (0x15)
> @ MGMT Event: Device Disconn.. (0x000c) plen 8  {0x0002} [hci0] 17:03:13.217688
>        BR/EDR Address: 00:AA:01:01:00:00 (Intel Corporation)
>        Reason: Connection terminated by local host for suspend (0x05)
> @ MGMT Event: Device Disconn.. (0x000c) plen 8  {0x0001} [hci0] 17:03:13.217688
>        BR/EDR Address: 00:AA:01:01:00:00 (Intel Corporation)
>        Reason: Connection terminated by local host for suspend (0x05)
> Suspend/Resume - Success 5 (Pairing - Legacy) - test timed out
> = mgmt-tester: Suspend/Resume - Success 5 (Pairing -..   17:03:13.939317
> Suspend/Resume - Success 5 (Pairing - Legacy) - teardown
> = mgmt-tester: Suspend/Resume - Success 5 (Pairing -..   17:03:13.947267
> [   13.284291] Bluetooth: hci0: Timed out waiting for suspend events
> [   13.287324] Bluetooth: hci0: Suspend timeout bit: 6
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> net/bluetooth/hci_event.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH 3/3] Bluetooth: Fix wake up suspend_wait_q prematurely

[Marcel Holtmann]

Hi Luiz,

> suspend_req_complete shall only attempt to wake up if there no tasks
> left otherwise the WAKE_COND will evaluate to false causing a premature
> timeout when in fact the tasks are still in progress.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> net/bluetooth/hci_request.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH 1/3] Bluetooth: hci_vhci: Fix calling hci_{suspend,resume}_dev

[Marcel Holtmann]

Hi Luiz,

> Defer calls to hci_{suspend,resume}_dev to work so it doesn't block the
> processing of the events.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> drivers/bluetooth/hci_vhci.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel