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
From: Benjamin Berg <[email protected]>
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 <[email protected]>
---
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
From: Benjamin Berg <[email protected]>
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 <[email protected]>
---
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
From: Benjamin Berg <[email protected]>
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 <[email protected]>
---
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
Hi Benjamin,
On Fri, Dec 3, 2021 at 6:59 AM Benjamin Berg <[email protected]> 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