* [PATCH v2 1/4] Bluetooth: Reset more state when cancelling a sync command
2021-12-03 14:58 [PATCH v2 0/4] Cancel sync commands if a TX failure occurs Benjamin Berg
@ 2021-12-03 14:58 ` Benjamin Berg
2021-12-03 14:59 ` [PATCH v2 2/4] Bluetooth: Add hci_cmd_sync_cancel to public API Benjamin Berg
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Benjamin Berg @ 2021-12-03 14:58 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Luiz Augusto von Dentz, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/4] Bluetooth: Add hci_cmd_sync_cancel to public API
2021-12-03 14:58 [PATCH v2 0/4] Cancel sync commands if a TX failure occurs Benjamin Berg
2021-12-03 14:58 ` [PATCH v2 1/4] Bluetooth: Reset more state when cancelling a sync command Benjamin Berg
@ 2021-12-03 14:59 ` Benjamin Berg
2021-12-03 14:59 ` [PATCH v2 3/4] Bluetooth: hci_core: Cancel sync command if sending a frame failed Benjamin Berg
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Benjamin Berg @ 2021-12-03 14:59 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Luiz Augusto von Dentz, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 3/4] Bluetooth: hci_core: Cancel sync command if sending a frame failed
2021-12-03 14:58 [PATCH v2 0/4] Cancel sync commands if a TX failure occurs Benjamin Berg
2021-12-03 14:58 ` [PATCH v2 1/4] Bluetooth: Reset more state when cancelling a sync command Benjamin Berg
2021-12-03 14:59 ` [PATCH v2 2/4] Bluetooth: Add hci_cmd_sync_cancel to public API Benjamin Berg
@ 2021-12-03 14:59 ` Benjamin Berg
2021-12-03 14:59 ` [PATCH v2 4/4] Bluetooth: btusb: Cancel sync commands for certain URB errors Benjamin Berg
2021-12-03 18:47 ` [PATCH v2 0/4] Cancel sync commands if a TX failure occurs Luiz Augusto von Dentz
4 siblings, 0 replies; 6+ messages in thread
From: Benjamin Berg @ 2021-12-03 14:59 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Luiz Augusto von Dentz, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 4/4] Bluetooth: btusb: Cancel sync commands for certain URB errors
2021-12-03 14:58 [PATCH v2 0/4] Cancel sync commands if a TX failure occurs Benjamin Berg
` (2 preceding siblings ...)
2021-12-03 14:59 ` [PATCH v2 3/4] Bluetooth: hci_core: Cancel sync command if sending a frame failed Benjamin Berg
@ 2021-12-03 14:59 ` Benjamin Berg
2021-12-03 18:47 ` [PATCH v2 0/4] Cancel sync commands if a TX failure occurs Luiz Augusto von Dentz
4 siblings, 0 replies; 6+ messages in thread
From: Benjamin Berg @ 2021-12-03 14:59 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Luiz Augusto von Dentz, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/4] Cancel sync commands if a TX failure occurs
2021-12-03 14:58 [PATCH v2 0/4] Cancel sync commands if a TX failure occurs Benjamin Berg
` (3 preceding siblings ...)
2021-12-03 14:59 ` [PATCH v2 4/4] Bluetooth: btusb: Cancel sync commands for certain URB errors Benjamin Berg
@ 2021-12-03 18:47 ` Luiz Augusto von Dentz
4 siblings, 0 replies; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2021-12-03 18:47 UTC (permalink / raw)
To: Benjamin Berg; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 6+ messages in thread