[PATCH v2 0/4] Cancel sync commands if a TX failure occurs

[PATCH v2 0/4] Cancel sync commands if a TX failure occurs

[Benjamin Berg]

It was reported that userspace could hang for 10s after resuming due to
btusb hitting the internal timeout when sending the firmware.

In this case, the bluetooth dongle disappeared right after resume due to
the thinkpad_acpi rfkill being blocked. This causes the USB device to
disappear, however the bluetooth stack does not handle the
corresponding ENODEV errors and instead waits for a timeout to happen.

To avoid blocking everything for such a long time, the synchronous
command has to finish immediately after an ENODEV error occurs. This
requires further error handling, which this patchset adds by building
on top of the existing cancellation infrastructure for synchronous
commands.

Note that this just adds basic error handling in order to quickly abort
the initialization routine in case the device disappears at that time.
Additional error handling such as calling hci_reset_dev might be
sensible in some cases.

Benjamin Berg (4):
  Bluetooth: Reset more state when cancelling a sync command
  Bluetooth: Add hci_cmd_sync_cancel to public API
  Bluetooth: hci_core: Cancel sync command if sending a frame failed
  Bluetooth: btusb: Cancel sync commands for certain URB errors

 drivers/bluetooth/btusb.c        | 11 +++++++++--
 include/net/bluetooth/hci_sync.h |  1 +
 net/bluetooth/hci_core.c         | 14 +++++++++++---
 net/bluetooth/hci_request.c      | 13 +------------
 net/bluetooth/hci_request.h      |  1 -
 net/bluetooth/hci_sync.c         | 17 +++++++++++++++++
 6 files changed, 39 insertions(+), 18 deletions(-)

-- 
2.33.1

[PATCH v2 1/4] Bluetooth: Reset more state when cancelling a sync command

[Benjamin Berg]

From: Benjamin Berg <bberg@redhat.com>

Resetting the timers and cmd_cnt means that we assume the device will be
in a good state after the sync command finishes. Without this a chain of
synchronous commands might get stuck if one of them is cancelled.

Signed-off-by: Benjamin Berg <bberg@redhat.com>
---
 net/bluetooth/hci_request.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 8b3205e4b23e..58d640a31bde 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -118,6 +118,11 @@ void hci_req_sync_cancel(struct hci_dev *hdev, int err)
 	if (hdev->req_status == HCI_REQ_PEND) {
 		hdev->req_result = err;
 		hdev->req_status = HCI_REQ_CANCELED;
+
+		cancel_delayed_work_sync(&hdev->cmd_timer);
+		cancel_delayed_work_sync(&hdev->ncmd_timer);
+		atomic_set(&hdev->cmd_cnt, 1);
+
 		wake_up_interruptible(&hdev->req_wait_q);
 	}
 }
-- 
2.33.1

[PATCH v2 2/4] Bluetooth: Add hci_cmd_sync_cancel to public API

[Benjamin Berg]

From: Benjamin Berg <bberg@redhat.com>

After transfer errors it makes sense to cancel an ongoing synchronous
command that cannot complete anymore. To permit this, export the old
hci_req_sync_cancel function as hci_cmd_sync_cancel in the API.

Signed-off-by: Benjamin Berg <bberg@redhat.com>
---
 include/net/bluetooth/hci_sync.h |  1 +
 net/bluetooth/hci_request.c      | 18 +-----------------
 net/bluetooth/hci_request.h      |  1 -
 net/bluetooth/hci_sync.c         | 17 +++++++++++++++++
 4 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h
index 0336c1bc5d25..f4034bf8f1ce 100644
--- a/include/net/bluetooth/hci_sync.h
+++ b/include/net/bluetooth/hci_sync.h
@@ -37,6 +37,7 @@ int __hci_cmd_sync_status_sk(struct hci_dev *hdev, u16 opcode, u32 plen,
 
 void hci_cmd_sync_init(struct hci_dev *hdev);
 void hci_cmd_sync_clear(struct hci_dev *hdev);
+void hci_cmd_sync_cancel(struct hci_dev *hdev, int err);
 
 int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
 		       void *data, hci_cmd_sync_work_destroy_t destroy);
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 58d640a31bde..a2607db44404 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -111,22 +111,6 @@ void hci_req_sync_complete(struct hci_dev *hdev, u8 result, u16 opcode,
 	}
 }
 
-void hci_req_sync_cancel(struct hci_dev *hdev, int err)
-{
-	bt_dev_dbg(hdev, "err 0x%2.2x", err);
-
-	if (hdev->req_status == HCI_REQ_PEND) {
-		hdev->req_result = err;
-		hdev->req_status = HCI_REQ_CANCELED;
-
-		cancel_delayed_work_sync(&hdev->cmd_timer);
-		cancel_delayed_work_sync(&hdev->ncmd_timer);
-		atomic_set(&hdev->cmd_cnt, 1);
-
-		wake_up_interruptible(&hdev->req_wait_q);
-	}
-}
-
 /* Execute request and wait for completion. */
 int __hci_req_sync(struct hci_dev *hdev, int (*func)(struct hci_request *req,
 						     unsigned long opt),
@@ -2708,7 +2692,7 @@ void hci_request_setup(struct hci_dev *hdev)
 
 void hci_request_cancel_all(struct hci_dev *hdev)
 {
-	hci_req_sync_cancel(hdev, ENODEV);
+	hci_cmd_sync_cancel(hdev, ENODEV);
 
 	cancel_work_sync(&hdev->discov_update);
 	cancel_work_sync(&hdev->scan_update);
diff --git a/net/bluetooth/hci_request.h b/net/bluetooth/hci_request.h
index 5f8e8846ec74..8d39e9416861 100644
--- a/net/bluetooth/hci_request.h
+++ b/net/bluetooth/hci_request.h
@@ -64,7 +64,6 @@ int hci_req_sync(struct hci_dev *hdev, int (*req)(struct hci_request *req,
 int __hci_req_sync(struct hci_dev *hdev, int (*func)(struct hci_request *req,
 						     unsigned long opt),
 		   unsigned long opt, u32 timeout, u8 *hci_status);
-void hci_req_sync_cancel(struct hci_dev *hdev, int err);
 
 struct sk_buff *hci_prepare_cmd(struct hci_dev *hdev, u16 opcode, u32 plen,
 				const void *param);
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index ad86caf41f91..7ac6c170ec49 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -335,6 +335,23 @@ void hci_cmd_sync_clear(struct hci_dev *hdev)
 	}
 }
 
+void hci_cmd_sync_cancel(struct hci_dev *hdev, int err)
+{
+	bt_dev_dbg(hdev, "err 0x%2.2x", err);
+
+	if (hdev->req_status == HCI_REQ_PEND) {
+		hdev->req_result = err;
+		hdev->req_status = HCI_REQ_CANCELED;
+
+		cancel_delayed_work_sync(&hdev->cmd_timer);
+		cancel_delayed_work_sync(&hdev->ncmd_timer);
+		atomic_set(&hdev->cmd_cnt, 1);
+
+		wake_up_interruptible(&hdev->req_wait_q);
+	}
+}
+EXPORT_SYMBOL(hci_cmd_sync_cancel);
+
 int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
 		       void *data, hci_cmd_sync_work_destroy_t destroy)
 {
-- 
2.33.1

[PATCH v2 3/4] Bluetooth: hci_core: Cancel sync command if sending a frame failed

[Benjamin Berg]

From: Benjamin Berg <bberg@redhat.com>

If sending a frame failed any sync command associated with it will never
be completed. As such, cancel any such command immediately to avoid
timing out.

Signed-off-by: Benjamin Berg <bberg@redhat.com>

---

v2:
 * Pass error up and cancel sync command from hci_cmd_work
---
 net/bluetooth/hci_core.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index fdc0dcf8ee36..5cadecd31f66 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2906,7 +2906,7 @@ int hci_unregister_cb(struct hci_cb *cb)
 }
 EXPORT_SYMBOL(hci_unregister_cb);
 
-static void hci_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
+static int hci_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	int err;
 
@@ -2929,14 +2929,17 @@ static void hci_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 
 	if (!test_bit(HCI_RUNNING, &hdev->flags)) {
 		kfree_skb(skb);
-		return;
+		return -EINVAL;
 	}
 
 	err = hdev->send(hdev, skb);
 	if (err < 0) {
 		bt_dev_err(hdev, "sending frame failed (%d)", err);
 		kfree_skb(skb);
+		return err;
 	}
+
+	return 0;
 }
 
 /* Send HCI command */
@@ -3843,10 +3846,15 @@ static void hci_cmd_work(struct work_struct *work)
 
 		hdev->sent_cmd = skb_clone(skb, GFP_KERNEL);
 		if (hdev->sent_cmd) {
+			int res;
 			if (hci_req_status_pend(hdev))
 				hci_dev_set_flag(hdev, HCI_CMD_PENDING);
 			atomic_dec(&hdev->cmd_cnt);
-			hci_send_frame(hdev, skb);
+
+			res = hci_send_frame(hdev, skb);
+			if (res < 0)
+				hci_cmd_sync_cancel(hdev, -res);
+
 			if (test_bit(HCI_RESET, &hdev->flags))
 				cancel_delayed_work(&hdev->cmd_timer);
 			else
-- 
2.33.1

[PATCH v2 4/4] Bluetooth: btusb: Cancel sync commands for certain URB errors

[Benjamin Berg]

From: Benjamin Berg <bberg@redhat.com>

Cancel sync commands when transmission of URBs fail. This is done for
both failures to send a command URB and also when the interrupt URB used
to retrieve a response fails.

This approach is sufficient to quickly deal with certain errors such as
a device being disconnected while synchronous commands are done during
initialization.

Signed-off-by: Benjamin Berg <bberg@redhat.com>

---

v2:
 * Do not call handler in btusb_isoc_tx_complete.
 * Only call handler for command SKBs in btusb_tx_complete
---
 drivers/bluetooth/btusb.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index ab169fc673ea..d9067a8fad60 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -933,6 +933,8 @@ static void btusb_intr_complete(struct urb *urb)
 		if (err != -EPERM && err != -ENODEV)
 			bt_dev_err(hdev, "urb %p failed to resubmit (%d)",
 				   urb, -err);
+		if (err != -EPERM)
+			hci_cmd_sync_cancel(hdev, -err);
 		usb_unanchor_urb(urb);
 	}
 }
@@ -976,6 +978,8 @@ static int btusb_submit_intr_urb(struct hci_dev *hdev, gfp_t mem_flags)
 		if (err != -EPERM && err != -ENODEV)
 			bt_dev_err(hdev, "urb %p submission failed (%d)",
 				   urb, -err);
+		if (err != -EPERM)
+			hci_cmd_sync_cancel(hdev, -err);
 		usb_unanchor_urb(urb);
 	}
 
@@ -1331,10 +1335,13 @@ static void btusb_tx_complete(struct urb *urb)
 	if (!test_bit(HCI_RUNNING, &hdev->flags))
 		goto done;
 
-	if (!urb->status)
+	if (!urb->status) {
 		hdev->stat.byte_tx += urb->transfer_buffer_length;
-	else
+	} else {
+		if (hci_skb_pkt_type(skb) == HCI_COMMAND_PKT)
+			hci_cmd_sync_cancel(hdev, -urb->status);
 		hdev->stat.err_tx++;
+	}
 
 done:
 	spin_lock_irqsave(&data->txlock, flags);
-- 
2.33.1

Re: [PATCH v2 0/4] Cancel sync commands if a TX failure occurs

[Luiz Augusto von Dentz]

Hi Benjamin,

On Fri, Dec 3, 2021 at 6:59 AM Benjamin Berg <benjamin@sipsolutions.net> wrote:
>
> It was reported that userspace could hang for 10s after resuming due to
> btusb hitting the internal timeout when sending the firmware.
>
> In this case, the bluetooth dongle disappeared right after resume due to
> the thinkpad_acpi rfkill being blocked. This causes the USB device to
> disappear, however the bluetooth stack does not handle the
> corresponding ENODEV errors and instead waits for a timeout to happen.
>
> To avoid blocking everything for such a long time, the synchronous
> command has to finish immediately after an ENODEV error occurs. This
> requires further error handling, which this patchset adds by building
> on top of the existing cancellation infrastructure for synchronous
> commands.
>
> Note that this just adds basic error handling in order to quickly abort
> the initialization routine in case the device disappears at that time.
> Additional error handling such as calling hci_reset_dev might be
> sensible in some cases.
>
> Benjamin Berg (4):
>   Bluetooth: Reset more state when cancelling a sync command
>   Bluetooth: Add hci_cmd_sync_cancel to public API
>   Bluetooth: hci_core: Cancel sync command if sending a frame failed
>   Bluetooth: btusb: Cancel sync commands for certain URB errors
>
>  drivers/bluetooth/btusb.c        | 11 +++++++++--
>  include/net/bluetooth/hci_sync.h |  1 +
>  net/bluetooth/hci_core.c         | 14 +++++++++++---
>  net/bluetooth/hci_request.c      | 13 +------------
>  net/bluetooth/hci_request.h      |  1 -
>  net/bluetooth/hci_sync.c         | 17 +++++++++++++++++
>  6 files changed, 39 insertions(+), 18 deletions(-)
>
> --
> 2.33.1

Applied, thanks.

-- 
Luiz Augusto von Dentz